[PATCH v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Tomi Valkeinen]

On 13/12/16 01:07, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tuesday 20 Sep 2016 16:57:59 Tomi Valkeinen wrote:
>> On 19/09/16 15:27, Laurent Pinchart wrote:
>>> The omapdrm DSS manager enable/disable operations check the DSS manager
>>> state to avoid double enabling/disabling. Move that code to the DSS
>>> manager to decrease the dependency of the DRM layer to the DSS layer.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>
>>>  drivers/gpu/drm/omapdrm/dss/dispc.c  | 1 -
>>>  drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++
>>>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 3 ---
>>>  3 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
>>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 535240fba671..ab150bf21dd8
>>> 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>>> @@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel channel)
>>>  {
>>>  	return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
>>>  }
>>> -EXPORT_SYMBOL(dispc_mgr_is_enabled);
>>>
>>>  void dispc_wb_enable(bool enable)
>>>  {
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c
>>> b/drivers/gpu/drm/omapdrm/dss/output.c index 24f859488201..f0be621895fa
>>> 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/output.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
>>> @@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config);
>>>
>>>  int dss_mgr_enable(enum omap_channel channel)
>>>  {
>>> +	if (dispc_mgr_is_enabled(channel))
>>> +		return 0;
>>> +
>>>  	return dss_mgr_ops->enable(channel);
>>>  }

Looking at this again, I think this is not correct. The functions in
output.c are just trampolines basically. They just pass the calls to
omapdrm. Let's not add any logic there.

Also, the above could even cause a crash, as the HW is not necessarily
enabled before the ->enable() is called.

>>>  EXPORT_SYMBOL(dss_mgr_enable);
>>>  
>>>  void dss_mgr_disable(enum omap_channel channel)
>>>  {
>>> +	if (!dispc_mgr_is_enabled(channel))
>>> +		return;
>>> +
>>>  	dss_mgr_ops->disable(channel);
>>>  }
>>>  EXPORT_SYMBOL(dss_mgr_disable);
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 4b7e16786e1e..a0c26592fc69
>>> 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> @@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc
>>> *crtc, bool enable)
>>>  		return;
>>>  	}
>>>
>>> -	if (dispc_mgr_is_enabled(channel) == enable)
>>> -		return;
>>> -
>>>  	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
>>>  		/*
>>>  		 * Digit output produces some sync lost interrupts during the
>>
>> With this change omap_crtc_set_enabled() will do extra work if the
>> output is already enabled/disabled, and, if I'm not mistaken, will do
>> omap_irq_wait() there which might lead to issues.
> 
> That's correct, but the DRM core nowadays should really guarantee that CRTCs 
> won't be enabled or disabled multiple times. 

Ok. So we could probably add a dev_warn() there, as getting that state
wrong might cause issues that are difficult to debug. It would be best
to check the HW state, but if you want to remove the call to dispc, I
guess just checking omap_crtc->enabled is good enough.

The comment for omap_crtc_set_enabled talks about suspend/resume.
Perhaps at some point it was called from suspend/resume, even for crtcs
that were already disabled, which would explain the need for the check.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161213/4c8f8b43/attachment.sig>