| Message ID | 20190804201637.1240-15-sam@ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: panel related updates | expand |
Hi Sam, Thank you for the patch. On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote: > Many panel drivers duplicate logic to prevent prepare to be called > for a panel that is already prepared. > Likewise for enable. > > Implement this logic in drm_panel so the individual drivers > no longer needs this. > A panel is considered prepared/enabled only if the prepare/enable call > succeeds. > For disable/unprepare it is unconditionally considered > disabled/unprepared. > > This allows calls to prepare/enable again, even if there were > some issue disabling a regulator or similar during disable/unprepare. Is this the right place to handle this ? Shouldn't the upper layers ensure than enable/disable and prepare/unprepare are correcty balanced, and not called multiple times ? Adding enabled and prepared state to drm_panel not only doesn't align well with atomic state handling, but also would hide issues in upper layers that should really be fixed there. > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- > include/drm/drm_panel.h | 21 ++++++++++++ > 2 files changed, 75 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index da19d5b4a2f4..0853764040de 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); > */ > int drm_panel_prepare(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->prepare) > - return panel->funcs->prepare(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (panel->prepared) > + return 0; > + > + if (panel->funcs && panel->funcs->prepare) > + ret = panel->funcs->prepare(panel); > + > + if (ret >= 0) > + panel->prepared = true; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_prepare); > > @@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); > */ > int drm_panel_enable(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->enable) > - return panel->funcs->enable(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (panel->enabled) > + return 0; > + > + if (panel->funcs && panel->funcs->enable) > + ret = panel->funcs->enable(panel); > + > + if (ret >= 0) > + panel->enabled = true; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_enable); > > @@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); > */ > int drm_panel_disable(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->disable) > - return panel->funcs->disable(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (!panel->enabled) > + return 0; > + > + if (panel->funcs && panel->funcs->disable) > + ret = panel->funcs->disable(panel); > + > + panel->enabled = false; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_disable); > > @@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); > */ > int drm_panel_unprepare(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->unprepare) > - return panel->funcs->unprepare(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (!panel->prepared) > + return 0; > + > + if (panel->funcs && panel->funcs->unprepare) > + ret = panel->funcs->unprepare(panel); > + > + panel->prepared = false; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_unprepare); > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 624bd15ecfab..7493500fc9bd 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -65,6 +65,9 @@ struct drm_panel_funcs { > * @prepare: > * > * Turn on panel and perform set up. > + * When the panel is successfully prepared the prepare() function > + * will not be called again until the panel has been unprepared. > + * > */ > int (*prepare)(struct drm_panel *panel); > > @@ -72,6 +75,8 @@ struct drm_panel_funcs { > * @enable: > * > * Enable panel (turn on back light, etc.). > + * When the panel is successfully enabled the enable() function > + * will not be called again until the panel has been disabled. > */ > int (*enable)(struct drm_panel *panel); > > @@ -79,6 +84,7 @@ struct drm_panel_funcs { > * @disable: > * > * Disable panel (turn off back light, etc.). > + * If the panel is already disabled the disable() function is not called. > */ > int (*disable)(struct drm_panel *panel); > > @@ -86,6 +92,7 @@ struct drm_panel_funcs { > * @unprepare: > * > * Turn off panel. > + * If the panel is already unprepared the unprepare() function is not called. > */ > int (*unprepare)(struct drm_panel *panel); > > @@ -145,6 +152,20 @@ struct drm_panel { > * Panel entry in registry. > */ > struct list_head list; > + > + /** > + * @prepared: > + * > + * Set to true when the panel is successfully prepared. > + */ > + bool prepared; > + > + /** > + * @enabled: > + * > + * Set to true when the panel is successfully enabled. > + */ > + bool enabled; > }; > > void drm_panel_init(struct drm_panel *panel);
On 2019/08/05, Laurent Pinchart wrote: > Hi Sam, > > Thank you for the patch. > > On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote: > > Many panel drivers duplicate logic to prevent prepare to be called > > for a panel that is already prepared. > > Likewise for enable. > > > > Implement this logic in drm_panel so the individual drivers > > no longer needs this. > > A panel is considered prepared/enabled only if the prepare/enable call > > succeeds. > > For disable/unprepare it is unconditionally considered > > disabled/unprepared. > > > > This allows calls to prepare/enable again, even if there were > > some issue disabling a regulator or similar during disable/unprepare. > > Is this the right place to handle this ? Shouldn't the upper layers > ensure than enable/disable and prepare/unprepare are correcty balanced, > and not called multiple times ? Adding enabled and prepared state to > drm_panel not only doesn't align well with atomic state handling, but > also would hide issues in upper layers that should really be fixed > there. > Fully agreed. Mistakes happen - hiding them, by returning "success" does not sound like a wise approach. HTH Emil
Hi Laurent. > > On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote: > > Many panel drivers duplicate logic to prevent prepare to be called > > for a panel that is already prepared. > > Likewise for enable. > > > > Implement this logic in drm_panel so the individual drivers > > no longer needs this. > > A panel is considered prepared/enabled only if the prepare/enable call > > succeeds. > > For disable/unprepare it is unconditionally considered > > disabled/unprepared. > > > > This allows calls to prepare/enable again, even if there were > > some issue disabling a regulator or similar during disable/unprepare. > > Is this the right place to handle this ? Shouldn't the upper layers > ensure than enable/disable and prepare/unprepare are correcty balanced, > and not called multiple times ? Adding enabled and prepared state to > drm_panel not only doesn't align well with atomic state handling, but > also would hide issues in upper layers that should really be fixed > there. The main rationale behind starting on this was that ~15 panel drivers already implements logic to prevent the prepare/enable/disable/unprepare functions to be called out of order. $ cd drivers/gpu/drm/panel/; git grep enabled | grep bool | wc -l Several of the panel drivers also implements a mipi_dsi_driver.shutdown() or platform_driver.shutdown(). To the best of my knowledge we cannot guarantee that the upper layers have done the proper disable()/unprepare() dance before a shutdown. So the flags exists to allow the driver to unconditionally call disable() / unprepare() in the shutdown methods. Same goes for *_driver.remove() One improvement could be to detect if the panel is prepare() when upper layers call enable() and warn/error in this situation. With the current implementation this is not checked at all. Likewise for unprepare() (require it was never enabled or disable() was caled first) I claim the check exists for the benefit of .remove and .shutdown, so we could also check if prepare() or enable() is called twice. Adding logic to call prepare() automagically would hide probems in upper layers and this was only briefly considered - and discarded as hiding bugs. So to sum up: - Moving the checks from drivers to the core is a good thing - The core shall check that a panel is prepared when enable is called and error out if not (or warn). - The core shall check that a panel is disabled when unprepare is called and error out if not (or warn). The core shall check if prepare() and enable() is called out of order. The patch needs to be extended to cover the last three points. Laurent / Emil / Thierry - agree/comments? Note: Did a quick round to see if could spot any wrong use of drm_panel_* functions. Most looked good, but then I did not do a throughly check. bridge/analogix/analogix_dp_core.c looks fishy. Looks like analogix_dp_prepare_panel() is a nop the way it is called. I did not look too much on this, maybe I am wrong. Sam > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- > > include/drm/drm_panel.h | 21 ++++++++++++ > > 2 files changed, 75 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > index da19d5b4a2f4..0853764040de 100644 > > --- a/drivers/gpu/drm/drm_panel.c > > +++ b/drivers/gpu/drm/drm_panel.c > > @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); > > */ > > int drm_panel_prepare(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->prepare) > > - return panel->funcs->prepare(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (panel->prepared) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->prepare) > > + ret = panel->funcs->prepare(panel); > > + > > + if (ret >= 0) > > + panel->prepared = true; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_prepare); > > > > @@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); > > */ > > int drm_panel_enable(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->enable) > > - return panel->funcs->enable(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (panel->enabled) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->enable) > > + ret = panel->funcs->enable(panel); > > + > > + if (ret >= 0) > > + panel->enabled = true; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_enable); > > > > @@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); > > */ > > int drm_panel_disable(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->disable) > > - return panel->funcs->disable(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (!panel->enabled) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->disable) > > + ret = panel->funcs->disable(panel); > > + > > + panel->enabled = false; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_disable); > > > > @@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); > > */ > > int drm_panel_unprepare(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->unprepare) > > - return panel->funcs->unprepare(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (!panel->prepared) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->unprepare) > > + ret = panel->funcs->unprepare(panel); > > + > > + panel->prepared = false; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_unprepare); > > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > index 624bd15ecfab..7493500fc9bd 100644 > > --- a/include/drm/drm_panel.h > > +++ b/include/drm/drm_panel.h > > @@ -65,6 +65,9 @@ struct drm_panel_funcs { > > * @prepare: > > * > > * Turn on panel and perform set up. > > + * When the panel is successfully prepared the prepare() function > > + * will not be called again until the panel has been unprepared. > > + * > > */ > > int (*prepare)(struct drm_panel *panel); > > > > @@ -72,6 +75,8 @@ struct drm_panel_funcs { > > * @enable: > > * > > * Enable panel (turn on back light, etc.). > > + * When the panel is successfully enabled the enable() function > > + * will not be called again until the panel has been disabled. > > */ > > int (*enable)(struct drm_panel *panel); > > > > @@ -79,6 +84,7 @@ struct drm_panel_funcs { > > * @disable: > > * > > * Disable panel (turn off back light, etc.). > > + * If the panel is already disabled the disable() function is not called. > > */ > > int (*disable)(struct drm_panel *panel); > > > > @@ -86,6 +92,7 @@ struct drm_panel_funcs { > > * @unprepare: > > * > > * Turn off panel. > > + * If the panel is already unprepared the unprepare() function is not called. > > */ > > int (*unprepare)(struct drm_panel *panel); > > > > @@ -145,6 +152,20 @@ struct drm_panel { > > * Panel entry in registry. > > */ > > struct list_head list; > > + > > + /** > > + * @prepared: > > + * > > + * Set to true when the panel is successfully prepared. > > + */ > > + bool prepared; > > + > > + /** > > + * @enabled: > > + * > > + * Set to true when the panel is successfully enabled. > > + */ > > + bool enabled; > > }; > > > > void drm_panel_init(struct drm_panel *panel); > > -- > Regards, > > Laurent Pinchart
On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote: > Many panel drivers duplicate logic to prevent prepare to be called > for a panel that is already prepared. > Likewise for enable. > > Implement this logic in drm_panel so the individual drivers > no longer needs this. > A panel is considered prepared/enabled only if the prepare/enable call > succeeds. > For disable/unprepare it is unconditionally considered > disabled/unprepared. Hi Sam, I did a similar thing a few years ago [1]. IIRC it was well received and just needed some nits cleaned up. Unfortunately I lost interest^W^W switched to other things and dropped it. So thanks for picking this back up! Fast forward to today, I still think it's a good idea but I want to make sure this won't negatively interact with the self refresh helpers. With the helpers in place, it's possible to call disable consecutively (ie: once to enter self refresh and again to actually shut down). I did a quick pass and it looks like this patch might break that behavior, so you might want to take that into account. Sean [1]- https://patchwork.freedesktop.org/series/30712/ > > This allows calls to prepare/enable again, even if there were > some issue disabling a regulator or similar during disable/unprepare. > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- > include/drm/drm_panel.h | 21 ++++++++++++ > 2 files changed, 75 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index da19d5b4a2f4..0853764040de 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); > */ > int drm_panel_prepare(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->prepare) > - return panel->funcs->prepare(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (panel->prepared) > + return 0; > + > + if (panel->funcs && panel->funcs->prepare) > + ret = panel->funcs->prepare(panel); > + > + if (ret >= 0) > + panel->prepared = true; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_prepare); > > @@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); > */ > int drm_panel_enable(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->enable) > - return panel->funcs->enable(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (panel->enabled) > + return 0; > + > + if (panel->funcs && panel->funcs->enable) > + ret = panel->funcs->enable(panel); > + > + if (ret >= 0) > + panel->enabled = true; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_enable); > > @@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); > */ > int drm_panel_disable(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->disable) > - return panel->funcs->disable(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (!panel->enabled) > + return 0; > + > + if (panel->funcs && panel->funcs->disable) > + ret = panel->funcs->disable(panel); > + > + panel->enabled = false; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_disable); > > @@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); > */ > int drm_panel_unprepare(struct drm_panel *panel) > { > - if (panel && panel->funcs && panel->funcs->unprepare) > - return panel->funcs->unprepare(panel); > + int ret = -ENOSYS; > > - return panel ? -ENOSYS : -EINVAL; > + if (!panel) > + return -EINVAL; > + > + if (!panel->prepared) > + return 0; > + > + if (panel->funcs && panel->funcs->unprepare) > + ret = panel->funcs->unprepare(panel); > + > + panel->prepared = false; > + > + return ret; > } > EXPORT_SYMBOL(drm_panel_unprepare); > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 624bd15ecfab..7493500fc9bd 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -65,6 +65,9 @@ struct drm_panel_funcs { > * @prepare: > * > * Turn on panel and perform set up. > + * When the panel is successfully prepared the prepare() function > + * will not be called again until the panel has been unprepared. > + * > */ > int (*prepare)(struct drm_panel *panel); > > @@ -72,6 +75,8 @@ struct drm_panel_funcs { > * @enable: > * > * Enable panel (turn on back light, etc.). > + * When the panel is successfully enabled the enable() function > + * will not be called again until the panel has been disabled. > */ > int (*enable)(struct drm_panel *panel); > > @@ -79,6 +84,7 @@ struct drm_panel_funcs { > * @disable: > * > * Disable panel (turn off back light, etc.). > + * If the panel is already disabled the disable() function is not called. > */ > int (*disable)(struct drm_panel *panel); > > @@ -86,6 +92,7 @@ struct drm_panel_funcs { > * @unprepare: > * > * Turn off panel. > + * If the panel is already unprepared the unprepare() function is not called. > */ > int (*unprepare)(struct drm_panel *panel); > > @@ -145,6 +152,20 @@ struct drm_panel { > * Panel entry in registry. > */ > struct list_head list; > + > + /** > + * @prepared: > + * > + * Set to true when the panel is successfully prepared. > + */ > + bool prepared; > + > + /** > + * @enabled: > + * > + * Set to true when the panel is successfully enabled. > + */ > + bool enabled; > }; > > void drm_panel_init(struct drm_panel *panel); > -- > 2.20.1 >
Hi Sean, On Mon, Aug 05, 2019 at 01:01:20PM -0400, Sean Paul wrote: > On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote: > > Many panel drivers duplicate logic to prevent prepare to be called > > for a panel that is already prepared. > > Likewise for enable. > > > > Implement this logic in drm_panel so the individual drivers > > no longer needs this. > > A panel is considered prepared/enabled only if the prepare/enable call > > succeeds. > > For disable/unprepare it is unconditionally considered > > disabled/unprepared. > > Hi Sam, > I did a similar thing a few years ago [1]. IIRC it was well received and just > needed some nits cleaned up. Unfortunately I lost interest^W^W switched to other > things and dropped it. So thanks for picking this back up! > > Fast forward to today, I still think it's a good idea but I want to make sure > this won't negatively interact with the self refresh helpers. With the helpers > in place, it's possible to call disable consecutively (ie: once to enter self > refresh and again to actually shut down). I did a quick pass and it looks like > this patch might break that behavior, so you might want to take that into > account. Is this semantics documented somewhere ? The documentation of the panel disable operation is pretty terse, and we need to explicitly state who of the caller and callee needs to track the state. > [1]- https://patchwork.freedesktop.org/series/30712/ > > > This allows calls to prepare/enable again, even if there were > > some issue disabling a regulator or similar during disable/unprepare. > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- > > include/drm/drm_panel.h | 21 ++++++++++++ > > 2 files changed, 75 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > index da19d5b4a2f4..0853764040de 100644 > > --- a/drivers/gpu/drm/drm_panel.c > > +++ b/drivers/gpu/drm/drm_panel.c > > @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); > > */ > > int drm_panel_prepare(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->prepare) > > - return panel->funcs->prepare(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (panel->prepared) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->prepare) > > + ret = panel->funcs->prepare(panel); > > + > > + if (ret >= 0) > > + panel->prepared = true; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_prepare); > > > > @@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); > > */ > > int drm_panel_enable(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->enable) > > - return panel->funcs->enable(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (panel->enabled) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->enable) > > + ret = panel->funcs->enable(panel); > > + > > + if (ret >= 0) > > + panel->enabled = true; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_enable); > > > > @@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); > > */ > > int drm_panel_disable(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->disable) > > - return panel->funcs->disable(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (!panel->enabled) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->disable) > > + ret = panel->funcs->disable(panel); > > + > > + panel->enabled = false; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_disable); > > > > @@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); > > */ > > int drm_panel_unprepare(struct drm_panel *panel) > > { > > - if (panel && panel->funcs && panel->funcs->unprepare) > > - return panel->funcs->unprepare(panel); > > + int ret = -ENOSYS; > > > > - return panel ? -ENOSYS : -EINVAL; > > + if (!panel) > > + return -EINVAL; > > + > > + if (!panel->prepared) > > + return 0; > > + > > + if (panel->funcs && panel->funcs->unprepare) > > + ret = panel->funcs->unprepare(panel); > > + > > + panel->prepared = false; > > + > > + return ret; > > } > > EXPORT_SYMBOL(drm_panel_unprepare); > > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > index 624bd15ecfab..7493500fc9bd 100644 > > --- a/include/drm/drm_panel.h > > +++ b/include/drm/drm_panel.h > > @@ -65,6 +65,9 @@ struct drm_panel_funcs { > > * @prepare: > > * > > * Turn on panel and perform set up. > > + * When the panel is successfully prepared the prepare() function > > + * will not be called again until the panel has been unprepared. > > + * > > */ > > int (*prepare)(struct drm_panel *panel); > > > > @@ -72,6 +75,8 @@ struct drm_panel_funcs { > > * @enable: > > * > > * Enable panel (turn on back light, etc.). > > + * When the panel is successfully enabled the enable() function > > + * will not be called again until the panel has been disabled. > > */ > > int (*enable)(struct drm_panel *panel); > > > > @@ -79,6 +84,7 @@ struct drm_panel_funcs { > > * @disable: > > * > > * Disable panel (turn off back light, etc.). > > + * If the panel is already disabled the disable() function is not called. > > */ > > int (*disable)(struct drm_panel *panel); > > > > @@ -86,6 +92,7 @@ struct drm_panel_funcs { > > * @unprepare: > > * > > * Turn off panel. > > + * If the panel is already unprepared the unprepare() function is not called. > > */ > > int (*unprepare)(struct drm_panel *panel); > > > > @@ -145,6 +152,20 @@ struct drm_panel { > > * Panel entry in registry. > > */ > > struct list_head list; > > + > > + /** > > + * @prepared: > > + * > > + * Set to true when the panel is successfully prepared. > > + */ > > + bool prepared; > > + > > + /** > > + * @enabled: > > + * > > + * Set to true when the panel is successfully enabled. > > + */ > > + bool enabled; > > }; > > > > void drm_panel_init(struct drm_panel *panel);
Hi Sam, On Mon, Aug 05, 2019 at 06:51:17PM +0200, Sam Ravnborg wrote: > > On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote: > > > Many panel drivers duplicate logic to prevent prepare to be called > > > for a panel that is already prepared. > > > Likewise for enable. > > > > > > Implement this logic in drm_panel so the individual drivers > > > no longer needs this. > > > A panel is considered prepared/enabled only if the prepare/enable call > > > succeeds. > > > For disable/unprepare it is unconditionally considered > > > disabled/unprepared. > > > > > > This allows calls to prepare/enable again, even if there were > > > some issue disabling a regulator or similar during disable/unprepare. > > > > Is this the right place to handle this ? Shouldn't the upper layers > > ensure than enable/disable and prepare/unprepare are correcty balanced, > > and not called multiple times ? Adding enabled and prepared state to > > drm_panel not only doesn't align well with atomic state handling, but > > also would hide issues in upper layers that should really be fixed > > there. > > The main rationale behind starting on this was that ~15 panel drivers > already implements logic to prevent the prepare/enable/disable/unprepare > functions to be called out of order. > $ cd drivers/gpu/drm/panel/; git grep enabled | grep bool | wc -l > > Several of the panel drivers also implements a > mipi_dsi_driver.shutdown() or platform_driver.shutdown(). > To the best of my knowledge we cannot guarantee that the upper layers > have done the proper disable()/unprepare() dance before a shutdown. If the display controller drivers all behaved correctly, their .shutdown() implementation would disable all the output, and thus disable the panels. I think that's the best way forward, and we should ideally remove .shutdown() from the panel drivers, as otherwise the panel may be disabled before the display driver .shutdown() operation is called, and weird things can then happen. This being said, guaranteeing proper operation of the display controller drivers isn't easy, so I'm not calling for removing .shutdown() from panel drivers right now, but I think we shouldn't accept that operation in new panel drivers going forward. > So the flags exists to allow the driver to unconditionally call > disable() / unprepare() in the shutdown methods. > Same goes for *_driver.remove() I'd rather get rid of the hacks instead of trying to refactor them in generic hack-support helpers ;-) But I get your point, as an interim measure this is probably our best option. > One improvement could be to detect if the panel is prepare() when > upper layers call enable() and warn/error in this situation. > With the current implementation this is not checked at all. > Likewise for unprepare() (require it was never enabled or disable() was > caled first) > > I claim the check exists for the benefit of .remove and .shutdown, > so we could also check if prepare() or enable() is called twice. > > Adding logic to call prepare() automagically would hide probems in upper > layers and this was only briefly considered - and discarded as hiding > bugs. I agree with you. > So to sum up: > - Moving the checks from drivers to the core is a good thing > - The core shall check that a panel is prepared when enable is called > and error out if not (or warn). > - The core shall check that a panel is disabled when unprepare is called > and error out if not (or warn). > The core shall check if prepare() and enable() is called out of order. > > The patch needs to be extended to cover the last three points. > > Laurent / Emil / Thierry - agree/comments? Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Maybe with a note in the Documentation/gpu/todo.rst to remove .shutdown() in panel drivers as a long term goal ? > Note: Did a quick round to see if could spot any wrong use of > drm_panel_* functions. > Most looked good, but then I did not do a throughly check. > > bridge/analogix/analogix_dp_core.c looks fishy. > Looks like analogix_dp_prepare_panel() is a nop the way it is called. > I did not look too much on this, maybe I am wrong. > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > > > Cc: Sean Paul <sean@poorly.run> > > > Cc: Thierry Reding <thierry.reding@gmail.com> > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > --- > > > drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- > > > include/drm/drm_panel.h | 21 ++++++++++++ > > > 2 files changed, 75 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > > index da19d5b4a2f4..0853764040de 100644 > > > --- a/drivers/gpu/drm/drm_panel.c > > > +++ b/drivers/gpu/drm/drm_panel.c > > > @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); > > > */ > > > int drm_panel_prepare(struct drm_panel *panel) > > > { > > > - if (panel && panel->funcs && panel->funcs->prepare) > > > - return panel->funcs->prepare(panel); > > > + int ret = -ENOSYS; > > > > > > - return panel ? -ENOSYS : -EINVAL; > > > + if (!panel) > > > + return -EINVAL; > > > + > > > + if (panel->prepared) > > > + return 0; > > > + > > > + if (panel->funcs && panel->funcs->prepare) > > > + ret = panel->funcs->prepare(panel); > > > + > > > + if (ret >= 0) > > > + panel->prepared = true; > > > + > > > + return ret; > > > } > > > EXPORT_SYMBOL(drm_panel_prepare); > > > > > > @@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); > > > */ > > > int drm_panel_enable(struct drm_panel *panel) > > > { > > > - if (panel && panel->funcs && panel->funcs->enable) > > > - return panel->funcs->enable(panel); > > > + int ret = -ENOSYS; > > > > > > - return panel ? -ENOSYS : -EINVAL; > > > + if (!panel) > > > + return -EINVAL; > > > + > > > + if (panel->enabled) > > > + return 0; > > > + > > > + if (panel->funcs && panel->funcs->enable) > > > + ret = panel->funcs->enable(panel); > > > + > > > + if (ret >= 0) > > > + panel->enabled = true; > > > + > > > + return ret; > > > } > > > EXPORT_SYMBOL(drm_panel_enable); > > > > > > @@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); > > > */ > > > int drm_panel_disable(struct drm_panel *panel) > > > { > > > - if (panel && panel->funcs && panel->funcs->disable) > > > - return panel->funcs->disable(panel); > > > + int ret = -ENOSYS; > > > > > > - return panel ? -ENOSYS : -EINVAL; > > > + if (!panel) > > > + return -EINVAL; > > > + > > > + if (!panel->enabled) > > > + return 0; > > > + > > > + if (panel->funcs && panel->funcs->disable) > > > + ret = panel->funcs->disable(panel); > > > + > > > + panel->enabled = false; > > > + > > > + return ret; > > > } > > > EXPORT_SYMBOL(drm_panel_disable); > > > > > > @@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); > > > */ > > > int drm_panel_unprepare(struct drm_panel *panel) > > > { > > > - if (panel && panel->funcs && panel->funcs->unprepare) > > > - return panel->funcs->unprepare(panel); > > > + int ret = -ENOSYS; > > > > > > - return panel ? -ENOSYS : -EINVAL; > > > + if (!panel) > > > + return -EINVAL; > > > + > > > + if (!panel->prepared) > > > + return 0; > > > + > > > + if (panel->funcs && panel->funcs->unprepare) > > > + ret = panel->funcs->unprepare(panel); > > > + > > > + panel->prepared = false; > > > + > > > + return ret; > > > } > > > EXPORT_SYMBOL(drm_panel_unprepare); > > > > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > > index 624bd15ecfab..7493500fc9bd 100644 > > > --- a/include/drm/drm_panel.h > > > +++ b/include/drm/drm_panel.h > > > @@ -65,6 +65,9 @@ struct drm_panel_funcs { > > > * @prepare: > > > * > > > * Turn on panel and perform set up. > > > + * When the panel is successfully prepared the prepare() function > > > + * will not be called again until the panel has been unprepared. > > > + * > > > */ > > > int (*prepare)(struct drm_panel *panel); > > > > > > @@ -72,6 +75,8 @@ struct drm_panel_funcs { > > > * @enable: > > > * > > > * Enable panel (turn on back light, etc.). > > > + * When the panel is successfully enabled the enable() function > > > + * will not be called again until the panel has been disabled. > > > */ > > > int (*enable)(struct drm_panel *panel); > > > > > > @@ -79,6 +84,7 @@ struct drm_panel_funcs { > > > * @disable: > > > * > > > * Disable panel (turn off back light, etc.). > > > + * If the panel is already disabled the disable() function is not called. > > > */ > > > int (*disable)(struct drm_panel *panel); > > > > > > @@ -86,6 +92,7 @@ struct drm_panel_funcs { > > > * @unprepare: > > > * > > > * Turn off panel. > > > + * If the panel is already unprepared the unprepare() function is not called. > > > */ > > > int (*unprepare)(struct drm_panel *panel); > > > > > > @@ -145,6 +152,20 @@ struct drm_panel { > > > * Panel entry in registry. > > > */ > > > struct list_head list; > > > + > > > + /** > > > + * @prepared: > > > + * > > > + * Set to true when the panel is successfully prepared. > > > + */ > > > + bool prepared; > > > + > > > + /** > > > + * @enabled: > > > + * > > > + * Set to true when the panel is successfully enabled. > > > + */ > > > + bool enabled; > > > }; > > > > > > void drm_panel_init(struct drm_panel *panel);
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index da19d5b4a2f4..0853764040de 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); */ int drm_panel_prepare(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->prepare) - return panel->funcs->prepare(panel); + int ret = -ENOSYS; - return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (panel->prepared) + return 0; + + if (panel->funcs && panel->funcs->prepare) + ret = panel->funcs->prepare(panel); + + if (ret >= 0) + panel->prepared = true; + + return ret; } EXPORT_SYMBOL(drm_panel_prepare); @@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_enable(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->enable) - return panel->funcs->enable(panel); + int ret = -ENOSYS; - return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (panel->enabled) + return 0; + + if (panel->funcs && panel->funcs->enable) + ret = panel->funcs->enable(panel); + + if (ret >= 0) + panel->enabled = true; + + return ret; } EXPORT_SYMBOL(drm_panel_enable); @@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); */ int drm_panel_disable(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->disable) - return panel->funcs->disable(panel); + int ret = -ENOSYS; - return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (!panel->enabled) + return 0; + + if (panel->funcs && panel->funcs->disable) + ret = panel->funcs->disable(panel); + + panel->enabled = false; + + return ret; } EXPORT_SYMBOL(drm_panel_disable); @@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); */ int drm_panel_unprepare(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->unprepare) - return panel->funcs->unprepare(panel); + int ret = -ENOSYS; - return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (!panel->prepared) + return 0; + + if (panel->funcs && panel->funcs->unprepare) + ret = panel->funcs->unprepare(panel); + + panel->prepared = false; + + return ret; } EXPORT_SYMBOL(drm_panel_unprepare); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 624bd15ecfab..7493500fc9bd 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -65,6 +65,9 @@ struct drm_panel_funcs { * @prepare: * * Turn on panel and perform set up. + * When the panel is successfully prepared the prepare() function + * will not be called again until the panel has been unprepared. + * */ int (*prepare)(struct drm_panel *panel); @@ -72,6 +75,8 @@ struct drm_panel_funcs { * @enable: * * Enable panel (turn on back light, etc.). + * When the panel is successfully enabled the enable() function + * will not be called again until the panel has been disabled. */ int (*enable)(struct drm_panel *panel); @@ -79,6 +84,7 @@ struct drm_panel_funcs { * @disable: * * Disable panel (turn off back light, etc.). + * If the panel is already disabled the disable() function is not called. */ int (*disable)(struct drm_panel *panel); @@ -86,6 +92,7 @@ struct drm_panel_funcs { * @unprepare: * * Turn off panel. + * If the panel is already unprepared the unprepare() function is not called. */ int (*unprepare)(struct drm_panel *panel); @@ -145,6 +152,20 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list; + + /** + * @prepared: + * + * Set to true when the panel is successfully prepared. + */ + bool prepared; + + /** + * @enabled: + * + * Set to true when the panel is successfully enabled. + */ + bool enabled; }; void drm_panel_init(struct drm_panel *panel);
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable. Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared. This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare. Signed-off-by: Sam Ravnborg <sam@ravnborg.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <maxime.ripard@bootlin.com> Cc: Sean Paul <sean@poorly.run> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- include/drm/drm_panel.h | 21 ++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-)