Re: [PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/crtc-helper: fix locking for drm_helper_disable_unused_functions
Date: Thu, 20 Mar 2014 13:06:32 +0000	[thread overview]
Message-ID: <20140320130632.GE4463@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1395320482-3962-1-git-send-email-daniel.vetter@ffwll.ch>

On Thu, Mar 20, 2014 at 02:01:21PM +0100, Daniel Vetter wrote:
> We have two calling contexts for thise function:
> 
> - In the crtc helper code itself as part of the ->set_config
>   implementation. In this calling context all modeset locks are
>   already held, as they should.
> 
> - In drivers not implementing fastboot before the fbdev/fbcon setup
>   and initialization. This has been added for all drivers in
> 
>   commit 76a39dbfb2d1bc45219839e5a95d4ceaf6ca114f
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>   Date:   Sun Jan 20 23:12:54 2013 +0100
> 
>       drm/fb-helper: don't disable everything in initial_config
> 
>   In this calling context we do not hold any modeset locks since the
>   immediately following call to initialize the fbev emulation grabs
>   all these locks themselves.
> 
> - There are two exceptions to the above rule: shmob doesn't have fbdev
>   emulation support. I've manually checked the callchain up to the
>   driver load function and no kms locks are held.
> 
> The right fix therefore is to split this helper into an internal and
> external version and add the required locking to the function exported
> to drivers.
> 
> This remedies locking inconsistencies exposed by me adding locking
> WARNs as part of the recent kerneldoc abi polishing done in
> 
> commit 62ff94a5492175759546f8bc61383189d6b49122
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jan 23 22:18:47 2014 +0100
> 
>     drm/crtc-helper: remove LOCKING from kerneldoc
> 
> and
> 
> commit 63951385052f7974155fa38f962f0f4e9847f90a
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jan 23 15:14:15 2014 +0100
> 
>     drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index a85517854073..9d8fb32357c0 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -278,17 +278,7 @@ drm_encoder_disable(struct drm_encoder *encoder)
>  		encoder->bridge->funcs->post_disable(encoder->bridge);
>  }
>  
> -/**
> - * drm_helper_disable_unused_functions - disable unused objects
> - * @dev: DRM device
> - *
> - * This function walks through the entire mode setting configuration of @dev. It
> - * will remove any crtc links of unused encoders and encoder links of
> - * disconnected connectors. Then it will disable all unused encoders and crtcs
> - * either by calling their disable callback if available or by calling their
> - * dpms callback with DRM_MODE_DPMS_OFF.
> - */
> -void drm_helper_disable_unused_functions(struct drm_device *dev)
> +static void __drm_helper_disable_unused_functions(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
> @@ -323,6 +313,23 @@ void drm_helper_disable_unused_functions(struct drm_device *dev)
>  		}
>  	}
>  }
> +
> +/**
> + * drm_helper_disable_unused_functions - disable unused objects
> + * @dev: DRM device
> + *
> + * This function walks through the entire mode setting configuration of @dev. It
> + * will remove any crtc links of unused encoders and encoder links of
> + * disconnected connectors. Then it will disable all unused encoders and crtcs
> + * either by calling their disable callback if available or by calling their
> + * dpms callback with DRM_MODE_DPMS_OFF.
> + */
> +void drm_helper_disable_unused_functions(struct drm_device *dev)
> +{
> +	drm_modeset_lock_all(dev);
> +	__drm_helper_disable_unused_functions(dev);
> +	drm_modeset_unlock_all(dev);
> +}
>  EXPORT_SYMBOL(drm_helper_disable_unused_functions);

drm_helper_disable_unused_functions() is called by
drm_crtc_helper_disable() which is expected to be holding the lock.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre