| Message ID | 20190826152649.13820-5-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
Hi Inki, On Mon, 26 Aug 2019 17:26:32 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > The encoder->enable() can't report errors and is expected to always > succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error > path (as currently done) we'll have unbalanced get/put calls when > encoder->disable() is called. > > The situation is not ideal since drm_panel_{prepare,enable}() can > theoretically return an error (even if in practice I don't think any > panel driver does that). Putting a WARN_ON() is the best we can do, > unfortunately. Note that -ENOSYS is actually a valid case, it just > means the panel driver does not implement the hook. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Did you have a chance to look at this patch 4 and 5 of this series? I'd really like to get those 2 patches merged. Thanks, Boris > --- > Changes in v2: > * New patch > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index 8e655ae1fb0c..c555cecfe1f5 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1387,8 +1387,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) > > if (dsi->panel) { > ret = drm_panel_prepare(dsi->panel); > - if (ret < 0) > - goto err_put_sync; > + WARN_ON(ret && ret != -ENOSYS); > } else { > drm_bridge_pre_enable(dsi->out_bridge); > } > @@ -1398,22 +1397,13 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) > > if (dsi->panel) { > ret = drm_panel_enable(dsi->panel); > - if (ret < 0) > - goto err_display_disable; > + WARN_ON(ret && ret != -ENOSYS); > } else { > drm_bridge_enable(dsi->out_bridge); > } > > dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE; > return; > - > -err_display_disable: > - exynos_dsi_set_display_enable(dsi, false); > - drm_panel_unprepare(dsi->panel); > - > -err_put_sync: > - dsi->state &= ~DSIM_STATE_ENABLED; > - pm_runtime_put(dsi->dev); > } > > static void exynos_dsi_disable(struct drm_encoder *encoder)
On 26.08.2019 17:26, Boris Brezillon wrote: > The encoder->enable() can't report errors and is expected to always > succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error > path (as currently done) we'll have unbalanced get/put calls when > encoder->disable() is called. True > > The situation is not ideal since drm_panel_{prepare,enable}() can > theoretically return an error (even if in practice I don't think any > panel driver does that). So why do you want to fix it at all, if you think return value is always 0 :) ? git grep -p -A30 '_prepare' drivers/gpu/drm/panel/ shows that many of them can return errors. > Putting a WARN_ON() is the best we can do, > unfortunately. I guess optimally we should use DRM_MODE_LINK_STATUS_BAD, but I am not sure how exactly it should be handled. > Note that -ENOSYS is actually a valid case, it just > means the panel driver does not implement the hook. It would be good then to fix it in panel framework, ie without hook drm_panel_* function should return 0, ENOSYS makes no sense here. Regards Andrzej > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > Changes in v2: > * New patch > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index 8e655ae1fb0c..c555cecfe1f5 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1387,8 +1387,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) > > if (dsi->panel) { > ret = drm_panel_prepare(dsi->panel); > - if (ret < 0) > - goto err_put_sync; > + WARN_ON(ret && ret != -ENOSYS); > } else { > drm_bridge_pre_enable(dsi->out_bridge); > } > @@ -1398,22 +1397,13 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) > > if (dsi->panel) { > ret = drm_panel_enable(dsi->panel); > - if (ret < 0) > - goto err_display_disable; > + WARN_ON(ret && ret != -ENOSYS); > } else { > drm_bridge_enable(dsi->out_bridge); > } > > dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE; > return; > - > -err_display_disable: > - exynos_dsi_set_display_enable(dsi, false); > - drm_panel_unprepare(dsi->panel); > - > -err_put_sync: > - dsi->state &= ~DSIM_STATE_ENABLED; > - pm_runtime_put(dsi->dev); > } > > static void exynos_dsi_disable(struct drm_encoder *encoder)
On Fri, 11 Oct 2019 15:54:53 +0200 Andrzej Hajda <a.hajda@samsung.com> wrote: > On 26.08.2019 17:26, Boris Brezillon wrote: > > The encoder->enable() can't report errors and is expected to always > > succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error > > path (as currently done) we'll have unbalanced get/put calls when > > encoder->disable() is called. > > > True > > > > > > The situation is not ideal since drm_panel_{prepare,enable}() can > > theoretically return an error (even if in practice I don't think any > > panel driver does that). > > > So why do you want to fix it at all, if you think return value is always > 0 :) ? > > > git grep -p -A30 '_prepare' drivers/gpu/drm/panel/ shows that many of > them can return errors. Then I was wrong :-). > > > > Putting a WARN_ON() is the best we can do, > > unfortunately. > > > I guess optimally we should use DRM_MODE_LINK_STATUS_BAD, but I am not > sure how exactly it should be handled. You mean call drm_connector_set_link_status_property(DRM_MODE_LINK_STATUS_BAD) ? > > > > Note that -ENOSYS is actually a valid case, it just > > means the panel driver does not implement the hook. > > > It would be good then to fix it in panel framework, ie without hook > drm_panel_* function should return 0, ENOSYS makes no sense here. I'm fine with that. Thierry, Sam, any opinion?
Hi Boris/Andrzej. > > > > > > > > Note that -ENOSYS is actually a valid case, it just > > > means the panel driver does not implement the hook. > > > > > > It would be good then to fix it in panel framework, ie without hook > > drm_panel_* function should return 0, ENOSYS makes no sense here. > > I'm fine with that. Thierry, Sam, any opinion? Agreed, I have following patch in my panel patch queue: drm/drm_panel: no error when no callback The callbacks in drm_panel_funcs are optional, so do not return an error just because no callback is assigned. Signed-off-by: Sam Ravnborg <sam@ravnborg.org> If I get time this weekend I will rebase/test and send the set of patches out. Sam
On Sat, 12 Oct 2019 09:15:26 +0200 Sam Ravnborg <sam@ravnborg.org> wrote: > Hi Boris/Andrzej. > > > > > > > > > > > > > Note that -ENOSYS is actually a valid case, it just > > > > means the panel driver does not implement the hook. > > > > > > > > > It would be good then to fix it in panel framework, ie without hook > > > drm_panel_* function should return 0, ENOSYS makes no sense here. > > > > I'm fine with that. Thierry, Sam, any opinion? > > Agreed, I have following patch in my panel patch queue: > > drm/drm_panel: no error when no callback > > The callbacks in drm_panel_funcs are optional, so do not > return an error just because no callback is assigned. > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > If I get time this weekend I will rebase/test and send the > set of patches out. Any progress on that? Can I rebase/send it for you if you don't have time?
On Fri, 11 Oct 2019 15:54:53 +0200 Andrzej Hajda <a.hajda@samsung.com> wrote: > On 26.08.2019 17:26, Boris Brezillon wrote: > > The encoder->enable() can't report errors and is expected to always > > succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error > > path (as currently done) we'll have unbalanced get/put calls when > > encoder->disable() is called. > > > True I just realized this is actually not the case, because the DSIM_STATE_ENABLED flag is cleared in the error path, and exynos_dsi_disable() bails out early when DSIM_STATE_ENABLED is not set.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 8e655ae1fb0c..c555cecfe1f5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1387,8 +1387,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) if (dsi->panel) { ret = drm_panel_prepare(dsi->panel); - if (ret < 0) - goto err_put_sync; + WARN_ON(ret && ret != -ENOSYS); } else { drm_bridge_pre_enable(dsi->out_bridge); } @@ -1398,22 +1397,13 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) if (dsi->panel) { ret = drm_panel_enable(dsi->panel); - if (ret < 0) - goto err_display_disable; + WARN_ON(ret && ret != -ENOSYS); } else { drm_bridge_enable(dsi->out_bridge); } dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE; return; - -err_display_disable: - exynos_dsi_set_display_enable(dsi, false); - drm_panel_unprepare(dsi->panel); - -err_put_sync: - dsi->state &= ~DSIM_STATE_ENABLED; - pm_runtime_put(dsi->dev); } static void exynos_dsi_disable(struct drm_encoder *encoder)
The encoder->enable() can't report errors and is expected to always succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error path (as currently done) we'll have unbalanced get/put calls when encoder->disable() is called. The situation is not ideal since drm_panel_{prepare,enable}() can theoretically return an error (even if in practice I don't think any panel driver does that). Putting a WARN_ON() is the best we can do, unfortunately. Note that -ENOSYS is actually a valid case, it just means the panel driver does not implement the hook. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- Changes in v2: * New patch --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)