Panel orientation property should be set before drm_dev_register().
Mediatek drm driver calls drm_dev_register() in .bind(). However, most
panels sets orientation property relatively late, mostly in .get_modes()
callback, since this is when they are able to get the connector and
binds the orientation property to it, though the value should be known
when the panel is probed.

Let the drm driver check if the remote end point is a panel and if it
contains the orientation property. If it does, set it before
drm_dev_register() is called.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
v4->v5:
- use the new function in v5.
- don't use drm_of_find_panel_or_bridge().
---
 drivers/gpu/drm/mediatek/mtk_dsi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index d9f10a33e6fa..998b1237e193 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -185,6 +185,7 @@ struct mtk_dsi {
 	struct drm_encoder encoder;
 	struct drm_bridge bridge;
 	struct drm_bridge *next_bridge;
+	struct drm_panel *panel;
 	struct drm_connector *connector;
 	struct phy *phy;
 
@@ -822,6 +823,10 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
 		ret = PTR_ERR(dsi->connector);
 		goto err_cleanup_encoder;
 	}
+
+	/* Read panel orientation */
+	drm_connector_set_orientation_from_panel(dsi->connector, dsi->panel);
+
 	drm_connector_attach_encoder(dsi->connector, &dsi->encoder);
 
 	return 0;
@@ -836,6 +841,16 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
 	int ret;
 	struct drm_device *drm = data;
 	struct mtk_dsi *dsi = dev_get_drvdata(dev);
+	struct device_node *panel_node;
+
+	/* Get panel if existed */
+	panel_node = of_graph_get_remote_node(dev->of_node, 0, 0);
+	if (panel_node) {
+		dsi->panel = of_drm_find_panel(panel_node);
+		if (IS_ERR(dsi->panel))
+			dsi->panel = NULL;
+		of_node_put(panel_node);
+	}
 
 	ret = mtk_dsi_encoder_init(drm, dsi);
 	if (ret)
-- 
2.36.1.255.ge46751e96f-goog

Hi,

On Tue, Jun 7, 2022 at 2:06 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Panel orientation property should be set before drm_dev_register().
> Mediatek drm driver calls drm_dev_register() in .bind(). However, most
> panels sets orientation property relatively late, mostly in .get_modes()
> callback, since this is when they are able to get the connector and
> binds the orientation property to it, though the value should be known
> when the panel is probed.
>
> Let the drm driver check if the remote end point is a panel and if it
> contains the orientation property. If it does, set it before
> drm_dev_register() is called.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
> v4->v5:
> - use the new function in v5.
> - don't use drm_of_find_panel_or_bridge().
> ---
>  drivers/gpu/drm/mediatek/mtk_dsi.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index d9f10a33e6fa..998b1237e193 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -185,6 +185,7 @@ struct mtk_dsi {
>         struct drm_encoder encoder;
>         struct drm_bridge bridge;
>         struct drm_bridge *next_bridge;
> +       struct drm_panel *panel;
>         struct drm_connector *connector;
>         struct phy *phy;
>
> @@ -822,6 +823,10 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
>                 ret = PTR_ERR(dsi->connector);
>                 goto err_cleanup_encoder;
>         }
> +
> +       /* Read panel orientation */
> +       drm_connector_set_orientation_from_panel(dsi->connector, dsi->panel);
> +
>         drm_connector_attach_encoder(dsi->connector, &dsi->encoder);
>
>         return 0;
> @@ -836,6 +841,16 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
>         int ret;
>         struct drm_device *drm = data;
>         struct mtk_dsi *dsi = dev_get_drvdata(dev);
> +       struct device_node *panel_node;
> +
> +       /* Get panel if existed */
> +       panel_node = of_graph_get_remote_node(dev->of_node, 0, 0);
> +       if (panel_node) {
> +               dsi->panel = of_drm_find_panel(panel_node);
> +               if (IS_ERR(dsi->panel))
> +                       dsi->panel = NULL;
> +               of_node_put(panel_node);
> +       }

While the above works, it feels like we could do better. What about this?

* We add _some_ way to determine if a bridge is actually a
panel_bridge. If nothing else maybe this could be
drm_bridge_is_panel() and it could just check if bridge.funcs ==
panel_bridge_bridge_funcs

* In drm_bridge_connector_init(), when we're looping through all the
bridges we find the panel_bridge if it's there.

* At the end of drm_bridge_connector_init() if we found a panel_bridge
then we call a function like drm_panel_bridge_set_orientation().


Then you can fully get rid of the mediatek patch, right? The above
will only work if you're using a panel_bridge / bridge_connector, but
that's "the future" anyway and we want to encourage people to
transition to that.

-Doug

On Tue, Jun 7, 2022 at 11:39 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jun 7, 2022 at 2:06 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > Panel orientation property should be set before drm_dev_register().
> > Mediatek drm driver calls drm_dev_register() in .bind(). However, most
> > panels sets orientation property relatively late, mostly in .get_modes()
> > callback, since this is when they are able to get the connector and
> > binds the orientation property to it, though the value should be known
> > when the panel is probed.
> >
> > Let the drm driver check if the remote end point is a panel and if it
> > contains the orientation property. If it does, set it before
> > drm_dev_register() is called.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > ---
> > v4->v5:
> > - use the new function in v5.
> > - don't use drm_of_find_panel_or_bridge().
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index d9f10a33e6fa..998b1237e193 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -185,6 +185,7 @@ struct mtk_dsi {
> >         struct drm_encoder encoder;
> >         struct drm_bridge bridge;
> >         struct drm_bridge *next_bridge;
> > +       struct drm_panel *panel;
> >         struct drm_connector *connector;
> >         struct phy *phy;
> >
> > @@ -822,6 +823,10 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
> >                 ret = PTR_ERR(dsi->connector);
> >                 goto err_cleanup_encoder;
> >         }
> > +
> > +       /* Read panel orientation */
> > +       drm_connector_set_orientation_from_panel(dsi->connector, dsi->panel);
> > +
> >         drm_connector_attach_encoder(dsi->connector, &dsi->encoder);
> >
> >         return 0;
> > @@ -836,6 +841,16 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> >         int ret;
> >         struct drm_device *drm = data;
> >         struct mtk_dsi *dsi = dev_get_drvdata(dev);
> > +       struct device_node *panel_node;
> > +
> > +       /* Get panel if existed */
> > +       panel_node = of_graph_get_remote_node(dev->of_node, 0, 0);
> > +       if (panel_node) {
> > +               dsi->panel = of_drm_find_panel(panel_node);
> > +               if (IS_ERR(dsi->panel))
> > +                       dsi->panel = NULL;
> > +               of_node_put(panel_node);
> > +       }
>
> While the above works, it feels like we could do better. What about this?
>
> * We add _some_ way to determine if a bridge is actually a
> panel_bridge. If nothing else maybe this could be
> drm_bridge_is_panel() and it could just check if bridge.funcs ==
> panel_bridge_bridge_funcs
>
> * In drm_bridge_connector_init(), when we're looping through all the
> bridges we find the panel_bridge if it's there.
>
> * At the end of drm_bridge_connector_init() if we found a panel_bridge
> then we call a function like drm_panel_bridge_set_orientation().
>
>
> Then you can fully get rid of the mediatek patch, right? The above
> will only work if you're using a panel_bridge / bridge_connector, but
> that's "the future" anyway and we want to encourage people to
> transition to that.
>
This also works with the mtk dsi, and it's more generic. I'll drop
this patch and go with this implementation in the next version. Thanks
for the suggestion.

> -Doug