| Message ID | 20220607090549.2345795-9-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a panel API to set orientation properly | expand |
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
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)