Use the Common Display Framework in tegra-drm

Add support for the Chunghwa CLAA101WA01A display panel.

Signed-off-by Alexandre Courbot <[email protected]>

---

drivers/video/display/panel-claa101wa01a.c new file +10 -0

+
+struct panel_claa101 {
+	struct display_entity entity;
+	struct regulator *vdd_pnl;
+	struct regulator *vdd_bl;
+	/* Enable GPIOs */
+	int pnl_enable;
+	int bl_enable;
+};
+

drivers/video/display/panel-claa101wa01a.c new file +28 -0

+
+	switch (state) {
+	case DISPLAY_ENTITY_STATE_OFF:
+	case DISPLAY_ENTITY_STATE_STANDBY:
+		if (entity->source)
+			display_entity_set_stream(entity->source,
+						 DISPLAY_ENTITY_STREAM_STOPPED);
+
+		/* TODO error checking? */
+		gpio_set_value_cansleep(panel->bl_enable, 0);
+		usleep_range(10000, 10000);
+		regulator_disable(panel->vdd_bl);
+		usleep_range(200000, 200000);
+		gpio_set_value_cansleep(panel->pnl_enable, 0);
+		regulator_disable(panel->vdd_pnl);
+		break;
+
+	case DISPLAY_ENTITY_STATE_ON:
+		regulator_enable(panel->vdd_pnl);
+		gpio_set_value_cansleep(panel->pnl_enable, 1);
+		usleep_range(200000, 200000);
+		regulator_enable(panel->vdd_bl);
+		usleep_range(10000, 10000);
+		gpio_set_value_cansleep(panel->bl_enable, 1);
+
+		if (entity->source)
+			display_entity_set_stream(entity->source,
+					      DISPLAY_ENTITY_STREAM_CONTINUOUS);

(quoted diff) +2 -0

+	return 0;
+}

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+#else
+#endif
+
+static const struct dev_pm_ops panel_claa101_dev_pm_ops = {

drivers/video/display/panel-claa101wa01a.c new file +12 -0

+#endif
+#ifdef CONFIG_OF
+		.of_match_table	= of_match_ptr(panel_claa101_of_match),
+#endif
+	},
+};
+
+module_platform_driver(panel_claa101_driver);
+
+MODULE_AUTHOR("Alexandre Courbot <[email protected]>");
+MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel");
+MODULE_LICENSE("GPL");

drivers/video/display/panel-claa101wa01a.c line 44

Quoted 3 lines

drivers/video/display/panel-claa101wa01a.c new file +3 -0

+	/* OFF and STANDBY are equivalent to us */
+	if (state = DISPLAY_ENTITY_STATE_STANDBY)
+		state = DISPLAY_ENTITY_STATE_OFF;

Do we need this? The "switch" below handles the same thing already.

drivers/video/display/panel-claa101wa01a.c line 82

Quoted 4 lines

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you.

drivers/video/display/panel-claa101wa01a.c line 183

Quoted 5 lines

drivers/video/display/panel-claa101wa01a.c new file +5 -0

+static struct of_device_id panel_claa101_of_match[] = {
+	{ .compatible = "chunghwa,claa101wa01a", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);

What does this mean? Why we need this?

On 01/30/2013 04:20 PM, Mark Zhang wrote:

Quoted 9 lines

drivers/video/display/panel-claa101wa01a.c line 82

Quoted 4 lines

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you.

I explained this in the cover letter - I'm not sure we want to have a dependency on DRM here, as CDF entities could also be connected to other subsystems. That's something we need to figure out. But yes, ultimately this should be the place where EDID is retrieved.

* PGP Signed by an unknown key

Quoted 1 line · Alex Courbot

On 01/30/2013 04:20 PM, Mark Zhang wrote:

[...]

Quoted 10 lines

Quoted 9 lines

drivers/video/display/panel-claa101wa01a.c line 82

Quoted 4 lines

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you.

Quoted 4 lines

I explained this in the cover letter - I'm not sure we want to have a dependency on DRM here, as CDF entities could also be connected to other subsystems. That's something we need to figure out. But yes, ultimately this should be the place where EDID is retrieved.

I already said this in another thread. I think we should only be using the CDF .get_modes() for static modes that cannot be obtained from EDID. Thinking about it, I'm not quite sure why EDID would be needed for this kind of panel anyway. Ventana probably has it because it keeps us from having to hardcode things, but if we provide drivers for the panel anyway, we can just as well hard-code the supported mode(s) in those drivers.

What I'm trying to say is that the existence of EDID is board-specific, so boards other than Ventana may not have an EDID EEPROM. Or perhaps this particular display has the EEPROM integrated? Even in that case, some boards may use this panel and simply not connect the EEPROM to an I2C controller.

Thierry

* Unknown Key
* 0x7F3EB3A1

I already said this in another thread. I think we should only be using the CDF .get_modes() for static modes that cannot be obtained from EDID. Thinking about it, I'm not quite sure why EDID would be needed for this kind of panel anyway. Ventana probably has it because it keeps us from having to hardcode things, but if we provide drivers for the panel anyway, we can just as well hard-code the supported mode(s) in those drivers.

What I'm trying to say is that the existence of EDID is board-specific, so boards other than Ventana may not have an EDID EEPROM. Or perhaps this particular display has the EEPROM integrated? Even in that case, some boards may use this panel and simply not connect the EEPROM to an I2C controller.

drivers/video/display/panel-claa101wa01a.c line 82

Quoted 6 lines · Alexandre Courbot

Add support for the Chunghwa CLAA101WA01A display panel.

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you.

Quoted 12 lines · Mark Zhang

drivers/video/display/panel-claa101wa01a.c line 82

Quoted 6 lines · Alexandre Courbot

Add support for the Chunghwa CLAA101WA01A display panel.

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you.

DDC access is a property of the display controller, not the panel itself. The panel might be hooked up to a display controller's DDC/I2C channel as the target, but it isn't the host/controller of the DDC/I2C channel. As such, placing the nvidia,ddc property into the display controller node makes sense.

Re: the other discussion in this thread: Probably the simplest is if tegradrm/other-CDF-users do something like:

1) If DDC I2C channel available for this display channel, query DDC.

2) If not, or perhaps also if that fails, query the panel driver for the mode list.

That would cover all bases very simply.

Quoted 6 lines

DDC access is a property of the display controller, not the panel itself. The panel might be hooked up to a display controller's DDC/I2C channel as the target, but it isn't the host/controller of the DDC/I2C channel. As such, placing the nvidia,ddc property into the display controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. So I think it's reasonable to put nvidia,ddc property into the display controller node. But the video mode info in EDID which be fetched via DDC is the property of the panel, so this info should be provided by panel driver. Actually display controller cares about the video modes, not the way to get these info. So I think it's better to do the whole work like this:

1) display controller relied on CDF to call "display_entity_get_modes"
2) panel driver calls the function which is hooked to display controller to get the video modes via DDC.
3) if the video modes can't be fetched via DDC, panel driver provides the info(either by hard-coded or defined in DT) anyway

Just like I said above, display controller should not query DDC directly. Since CDF already defines these for us, display controller should call CDF's functions, like: display_entity_get_modes, display_entity_get_size... I think this is the key difference I wanna talk about. And also in this way, display controller doesn't need to care about this 2 steps logics, just call "display_entity_get_modes" is OK.

Quoted 15 lines · Mark Zhang

Quoted 6 lines

DDC access is a property of the display controller, not the panel itself. The panel might be hooked up to a display controller's DDC/I2C channel as the target, but it isn't the host/controller of the DDC/I2C channel. As such, placing the nvidia,ddc property into the display controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. So I think it's reasonable to put nvidia,ddc property into the display controller node. But the video mode info in EDID which be fetched via DDC is the property of the panel, so this info should be provided by panel driver. Actually display controller cares about the video modes, not the way to get these info. So I think it's better to do the whole work like this:

Quoted 5 lines

1) display controller relied on CDF to call "display_entity_get_modes"
2) panel driver calls the function which is hooked to display controller to get the video modes via DDC.
3) if the video modes can't be fetched via DDC, panel driver provides the info(either by hard-coded or defined in DT) anyway

This would require a callback dedicated to this in display_entity and would break its simple design.

Quoted 7 lines

Just like I said above, display controller should not query DDC directly. Since CDF already defines these for us, display controller should call CDF's functions, like: display_entity_get_modes, display_entity_get_size... I think this is the key difference I wanna talk about. And also in this way, display controller doesn't need to care about this 2 steps logics, just call "display_entity_get_modes" is OK.

Well the fact is that it *is* the display controller that queries DDC directly. The panel is just a bus here, and the EDID EEPROM could perfectly work without it. If you model what you described with DT nodes, I'm afraid you will end up with something both complex (with DC node referencing panel node and back again for the EDID bus) and that does not picture reality accurately.

Alex.

Display controller don't know whether the panel has EDID EEPROM but the panel driver knows. So why we need to make display controller queries EDID blindly? Since panel driver knows about all "video modes/panel size" stuffs, why not we let it handle all these things?

Quoted 4 lines · Mark Zhang

Display controller don't know whether the panel has EDID EEPROM but the panel driver knows. So why we need to make display controller queries EDID blindly? Since panel driver knows about all "video modes/panel size" stuffs, why not we let it handle all these things?

The DC actually does know whether the connected panel supports EDID - in this case, the output node will have a property for the DDC bus, e.g.

        nvidia,ddc-i2c-bus = <&lcd_ddc>;

If no DDC bus is specified or if transfer fails, the DC driver can still query the panel driver for hardcoded modes.

Also passing a function pointer to get_modes() does not relief the DC driver from probing for the DDC bus. You will still have to handle both conditions (DDC bus present or not), the check will just be moved into your callback function.

Alex.

Quoted 10 lines · Alexandre Courbot

Quoted 4 lines · Mark Zhang

Display controller don't know whether the panel has EDID EEPROM but the panel driver knows. So why we need to make display controller queries EDID blindly? Since panel driver knows about all "video modes/panel size" stuffs, why not we let it handle all these things?

The DC actually does know whether the connected panel supports EDID - in this case, the output node will have a property for the DDC bus, e.g.

Quoted 4 lines

        nvidia,ddc-i2c-bus = <&lcd_ddc>;

If no DDC bus is specified or if transfer fails, the DC driver can still query the panel driver for hardcoded modes.

Strictly speaking, according to my understanding, "nvidia,ddc-i2c-bus" means tegra has a hardware ddc channel, this doesn't mean we can get the EDID from this DDC surely.

Quoted 14 lines · Stephen Warren

Quoted 12 lines · Mark Zhang

drivers/video/display/panel-claa101wa01a.c line 82

Quoted 6 lines · Alexandre Courbot

Add support for the Chunghwa CLAA101WA01A display panel.

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you.

Quoted 5 lines

DDC access is a property of the display controller, not the panel itself. The panel might be hooked up to a display controller's DDC/I2C channel as the target, but it isn't the host/controller of the DDC/I2C channel. As such, placing the nvidia,ddc property into the display controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. So I think it's reasonable to put nvidia,ddc property into the display controller node. But the video mode info in EDID which be fetched via DDC is the property of the panel, so this info should be provided by panel driver.

Quoted 21 lines · Mark Zhang

Quoted 14 lines · Stephen Warren

Quoted 12 lines · Mark Zhang

drivers/video/display/panel-claa101wa01a.c line 82

Quoted 6 lines · Alexandre Courbot

Add support for the Chunghwa CLAA101WA01A display panel.

drivers/video/display/panel-claa101wa01a.c new file +4 -0

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you.

Quoted 5 lines

DDC access is a property of the display controller, not the panel itself. The panel might be hooked up to a display controller's DDC/I2C channel as the target, but it isn't the host/controller of the DDC/I2C channel. As such, placing the nvidia,ddc property into the display controller node makes sense.

Quoted 5 lines

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. So I think it's reasonable to put nvidia,ddc property into the display controller node. But the video mode info in EDID which be fetched via DDC is the property of the panel, so this info should be provided by panel driver.

No, that makes absolutely no sense at all in the EDID case.

By the same argument, we'd need a panel driver for every external monitor which implemented EDID, just to transfer the EDID results from the display controller's DDC channel into the panel driver and back into the display controller code, which wants the mode list.

Again, if the mode list is coming from DDC, the display controller should retrieve it in exactly the same way it retrieves it for any external monitor - by direct control of the DDC channel to read the EDID. The only time it makes sense for the panel driver to get involved in supplying the mode list is when there's no EDID, so the list must be hard-coded into the driver.

So this looks like a reasonable binding to me. The one issue is that it's very generic, and if we go this route, we'll probably end up with tens or hundreds of identical or extremely similar simple bindings, and associated drivers.

We can avoid this instead by defining something like a "simple-lcd" binding, and associated driver, that will work for perhaps 90%, 95%, 99%, even 100%(?) of panels.

Just like the above, but with:

compatible="simple-panel", "chunghwa,claa101wa01a"

instead, and the driver binding to "simple-panel" rather than "chunghwa,claa101wa01a".

The driver can assume that a specific set of supplies (and perhaps GPIOs) is always present, and that the /sequence/ of manipulating those is fixed. This will avoid the need for anything like the power sequences code. If a particular panel doesn't fit those assumptions, including the exact sequence of manipulations for each state transition (which should be documented in the binding) then it can get a custom driver, this also avoiding having to define custom sequences in DT.

Things that might be parameterized/optional:

* Perhaps some GPIOs aren't always present.
* If some regulators aren't SW-controllable, DT should still provide a fixed/dummy regulator so the driver doesn't care.

* Wait times between regulator/GPIO/... manipulation could be specified in DT.
* For panels without EDID, CDF DT bindings can provide the list of supported modes, otherwise we assume that the display controller that drives the panel has been told how to access the EDID, just like it would for an "external" display.

Quoted 8 lines · Stephen Warren

So this looks like a reasonable binding to me. The one issue is that it's very generic, and if we go this route, we'll probably end up with tens or hundreds of identical or extremely similar simple bindings, and associated drivers.

We can avoid this instead by defining something like a "simple-lcd" binding, and associated driver, that will work for perhaps 90%, 95%, 99%, even 100%(?) of panels.

That seems totally doable indeed. Actually the right way to do this might be by extending the simple DPI panel driver Laurent included in his patchset.

Quoted 6 lines

Just like the above, but with:

compatible="simple-panel", "chunghwa,claa101wa01a"

instead, and the driver binding to "simple-panel" rather than "chunghwa,claa101wa01a".

Just out of curiosity, why don't we rather define

compatible="chunghwa,claa101wa01a", "simple-panel"

in that order? I thought DT compatible strings should go from more to less specific. The device would still bind to "simple-panel" if no more specific driver exists.

Quoted 8 lines

The driver can assume that a specific set of supplies (and perhaps GPIOs) is always present, and that the /sequence/ of manipulating those is fixed. This will avoid the need for anything like the power sequences code. If a particular panel doesn't fit those assumptions, including the exact sequence of manipulations for each state transition (which should be documented in the binding) then it can get a custom driver, this also avoiding having to define custom sequences in DT.

Quoted 5 lines

Things that might be parameterized/optional:

* Perhaps some GPIOs aren't always present.
* If some regulators aren't SW-controllable, DT should still provide a fixed/dummy regulator so the driver doesn't care.

How about making all regulators and GPIO optional in the driver?

Add support for the Chunghwa CLAA101WA01A display panel.

Make the tegra-drm driver use the Common Display Framework, letting it control the panel state according to the DPMS status.

A "nvidia,panel" property is added to the output node of the Tegra DC that references the panel connected to a given output.

Signed-off-by Alexandre Courbot <[email protected]>

---

drivers/gpu/drm/tegra/drm.h +1 -0

index 741b5dc..5e63c56 100644
@@ -17,6 +17,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fixed.h>
+#include <video/display.h>
 
 struct tegra_framebuffer {
 	struct drm_framebuffer base;

drivers/gpu/drm/tegra/drm.h +1 -0

+	struct display_entity_notifier display_notifier;
 };
 

drivers/gpu/drm/tegra/drm.h +22 -0

+static int display_notify_callback(struct display_entity_notifier *notifier,
+				   struct display_entity *entity, int event)
+{
+	struct tegra_output *output = display_notifier_to_output(notifier);
+	struct device_node *pnode;
+
+	switch (event) {
+	case DISPLAY_ENTITY_NOTIFIER_CONNECT:
+		if (output->output)
+			break;
+
+		pnode = of_parse_phandle(output->of_node, "nvidia,panel", 0);
+		if (!pnode)
+			break;
+
+		if (entity->dev && entity->dev->of_node = pnode) {
+			dev_dbg(output->dev, "connecting panel\n");
+			output->output = display_entity_get(entity);
+			display_entity_connect(&output->this, output->output);
+		}
+		of_node_put(pnode);
+

drivers/gpu/drm/tegra/drm.h +11 -0

+
+		break;
+
+	default:
+		dev_dbg(output->dev, "unhandled display event\n");
+		break;
+	}
+
+	return 0;
+}
+

drivers/gpu/drm/tegra/drm.h +11 -0

+	output->display_notifier.notify = display_notify_callback;
+	err = display_entity_register_notifier(&output->display_notifier);
+	if (err)
+		return err;
+
 	return 0;
 
 free_hpd:

@@ -260,6 +368,12 @@ free_hpd:
 
 int tegra_output_exit(struct tegra_output *output)
 {
+	if (output->output)
+		display_entity_put(output->output);
+
+	display_entity_unregister_notifier(&output->display_notifier);
+	display_entity_unregister(&output->this);
+
 	if (gpio_is_valid(output->hpd_gpio)) {
 		free_irq(output->hpd_irq, output);
 		gpio_free(output->hpd_gpio);

drivers/gpu/drm/tegra/drm.h struct tegra_output { · line 152

Quoted 6 lines

drivers/gpu/drm/tegra/drm.h +2 -0

@@ -147,6 +148,9 @@ struct tegra_output {
 
  	struct drm_encoder encoder;
  	struct drm_connector connector;
+	struct display_entity this;
+	struct display_entity *output;

Could you pick up a somewhat meaningful name? You know, there are too many variables with name "drm/connector/output/encoder"... :)

Quoted 14 lines

drivers/gpu/drm/tegra/output.c +14 -0

+		if (entity->dev && entity->dev->of_node = pnode) {
+			dev_dbg(output->dev, "connecting panel\n");
+			output->output = display_entity_get(entity);
+			display_entity_connect(&output->this, output->output);
+		}
+		of_node_put(pnode);
+
+		break;
+
+	case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
+		if (!output->output || output->output != entity)
+			break;
+
+		dev_dbg(output->dev, "disconnecting panel\n");

drivers/gpu/drm/tegra/output.c int tegra_output_parse_dt(struct tegra_output *output) · line 247

Quoted 3 lines

drivers/gpu/drm/tegra/output.c +3 -0

+		display_entity_disconnect(&output->this, output->output);
+		output->output = NULL;
+		display_entity_put(&output->this);

No "display_entity_get" for "output->this", so I don't think we need "display_entity_put" here. If you register this entity with "release" callback and you wanna release "output->this", call the "release" function manually.

drivers/gpu/drm/tegra/output.c int tegra_output_init(struct drm_device *drm, struct tegra_output *output) · line 346

Quoted 3 lines

drivers/gpu/drm/tegra/output.c +3 -0

+	/* register display entity */
+	memset(&output->this, 0, sizeof(output->this));
+	output->this.dev = drm->dev;

Use "output->dev" here. Actually the device you wanna register it to display entity is the "encoder"(in drm terms), not "drm->dev". If we use "drm->dev" here, we will have all same device for all encoders(HDMI, DSI...).

drivers/gpu/drm/tegra/output.c int tegra_output_init(struct drm_device *drm, struct tegra_output *output) · line 355

Quoted 2 lines

drivers/gpu/drm/tegra/output.c +2 -0

+	/* register display notifier */
+	output->display_notifier.dev = NULL;

Set "display_notifier.dev" to NULL makes we have to compare with every display entity, just like what you do in "display_notify_callback":

entity->dev && entity->dev->of_node = pnode

So can we get the "struct device *" of panel here? Seems currently the "of" framework doesn't allow "device_node -> device".

Quoted 9 lines · Mark Zhang

drivers/gpu/drm/tegra/drm.h struct tegra_output { · line 152

Quoted 6 lines

drivers/gpu/drm/tegra/drm.h +2 -0

@@ -147,6 +148,9 @@ struct tegra_output {
 
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+	struct display_entity this;
+	struct display_entity *output;

Could you pick up a somewhat meaningful name? You know, there are too many variables with name "drm/connector/output/encoder"... :)

Well, it's supposed to be abstract. From the CDF point of view it could be anything besides a panel. I know this makes it an output of an output, but I can't think of anything better right now.

Quoted 6 lines

Quoted 2 lines

Could you pick up a somewhat meaningful name? You know, there are too many variables with name "drm/connector/output/encoder"... :)

Well, it's supposed to be abstract. From the CDF point of view it could be anything besides a panel. I know this makes it an output of an output, but I can't think of anything better right now.

How about renaming "this" to stream to match with what the output is in CDF speak.

And the output's output is the panel, right? Why not just call it that? Even if it isn't directly connected to a panel entity but has indeed a whole pipeline in between, for tegra-drm it is still a panel.

On 01/30/2013 03:50 PM, Mark Zhang wrote:

Quoted 9 lines

drivers/gpu/drm/tegra/output.c int tegra_output_init(struct drm_device *drm, struct tegra_output *output) · line 355

Quoted 3 lines

drivers/gpu/drm/tegra/output.c +2 -0

+    /* register display notifier */
+    output->display_notifier.dev = NULL;

Set "display_notifier.dev" to NULL makes we have to compare with every display entity, just like what you do in "display_notify_callback":

entity->dev && entity->dev->of_node = pnode

Quoted 2 lines

So can we get the "struct device *" of panel here? Seems currently the "of" framework doesn't allow "device_node -> device".

Nope. AFAICT the device might not be instanciated at this point. We become aware of it for the first time in the callback function. We also don't want to defer probing until the panel is parsed first, since the panel might also depend on resources of the display device.

Thanks,
Alex.

This series leverages the (still work-in-progress) Common Display Framework to add panel support to the tegra-drm driver. It also adds a driver for the CLAA101WA01A panel used on the Ventana board.

The CDF is a moving target but Tegra needs some sort of display framework and even in its current state the CDF seems to be the best candidate. Besides, by using the CDF from now on we hope to provide useful feedback to Laurent and the other CDF designers.

The changes to tegra-drm are rather minimal. Panels are referenced from Tegra DC output nodes through the "nvidia,panel" property. This property is looked up when a display connect notification is received in order to see if it points to the connected display entity. If it does, the entity is then used for output.

The DPMS states are then propagated to the output entity, which is then supposed to call back into the set_stream() hook in order to enable/disable the output stream as needed.

Although the overall design seems to work ok, a few specific issues need to be addressed:

1) The CDF has a get_modes() hook, but this is already implemented by tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook, but Tegra's implementation uses DRM functions to retrieve the EDID and CDF should, AFAIK, remain DRM-agnostic.

2) There is currently no panel/backlight interaction, e.g. backlight status is controlled through FB events, independantly from the panel state. It could make sense to have the panel DT node reference the backlight and control it as part of its own power on/off sequence. Right now however, a backlight device cannot ignore FB events.

3) Probably related to 2), now the backlight's power controls are part of the panel driver, because the pwm-backlight driver cannot control the power regulator and enable GPIO. This means that the backlight power is not turned off when its brightness is set to 0 through sysfs. Once again this speaks in favor of having stronger panel/backlight interaction: for instance, the panel driver could reference the backlight and hijack its update_status() op to replace it by one that does the correct power sequencing before calling the original function. This would require some extra infrastructure though. Another possibility would be to have a dedicated backlight driver for each panel, with its own "compatible" string.

Thanks *a lot* for taking care of this Alexandre! From a quick look at the patches they seem generally fine. I'll go over them in a bit more detail though.

Quoted 4 lines

1) The CDF has a get_modes() hook, but this is already implemented by tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook, but Tegra's implementation uses DRM functions to retrieve the EDID and CDF should, AFAIK, remain DRM-agnostic.

Maybe a good option would be to just not implement get_modes() if the same information can be retrieved via EDID. That is, the DRM driver could just go and fetch EDID when the nvidia,ddc-i2c-bus is available (or parse the nvidia,edid blob) and only rely on CDF otherwise.

So for Ventana the only reason why we need CDF is basically the power sequencing, right?

I definitely think that we should aim for correct panel and backlight interaction. Perhaps this could work by looking up the real backlight via it's phandle and have the CDF driver use the backlight API to disable or enable the backlight as part of the power sequencing.

I'm not sure what you mean by "cannot ignore FB events"? Can you provide a concrete problematic use-case?

Hijacking .update_status() sounds a bit risky. But perhaps you could wrap the real backlight in a CDF backlight to receive notifications. Obviously you'd get two backlight devices in sysfs, but that turn out not to be a problem.

Quoted 3 lines · Thierry Reding

Thanks *a lot* for taking care of this Alexandre! From a quick look at the patches they seem generally fine. I'll go over them in a bit more detail though.

Glad you like it better than my previous attempts at controlling Tegra's panels and backlights. ;)

Quoted 5 lines

Quoted 4 lines

1) The CDF has a get_modes() hook, but this is already implemented by tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook, but Tegra's implementation uses DRM functions to retrieve the EDID and CDF should, AFAIK, remain DRM-agnostic.

Quoted 4 lines

Maybe a good option would be to just not implement get_modes() if the same information can be retrieved via EDID. That is, the DRM driver could just go and fetch EDID when the nvidia,ddc-i2c-bus is available (or parse the nvidia,edid blob) and only rely on CDF otherwise.

Since EDID information is per-panel I'd intuitively say it should be provided by the panel driver.