Re: [RFC 3/4] drm: tegra: use the Common Display Framework

Mark Zhang
2013-01-30T06:50:12Z

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:
> 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 <acourbot@nvidia.com>
> ---
[...]
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 741b5dc..5e63c56 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -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;
> @@ -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"... :)

> + struct display_entity_notifier display_notifier;
> };
>
[...]
> +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);
> +
> + break;
> +
> + case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
> + if (!output->output || output->output != entity)
> + break;
> +
> + dev_dbg(output->dev, "disconnecting panel\n");
> + 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.

Only when you have "display_entity_get", use "display_entity_put" to
release.

> +
> + break;
> +
> + default:
> + dev_dbg(output->dev, "unhandled display event\n");
> + break;
> + }
> +
> + return 0;
> +}
> +
[...]
> int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
> {
> int connector, encoder, err;
> @@ -250,6 +341,23 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
>
> output->encoder.possible_crtcs = 0x3;
>
> + /* 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...).

> + output->this.ops.video = &tegra_output_video_ops;
> + err = display_entity_register(&output->this);
> + if (err) {
> + dev_err(output->dev, "cannot register display entity\n");
> + return err;
> + }
> +
> + /* 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".

> + 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);
>