[Intel-gfx] [PATCH 05/14] drm: Extract drm_master_open

[Chris Wilson]

On Tue, Jun 14, 2016 at 08:51:00PM +0200, Daniel Vetter wrote:
> And pull out the primary_client check to make it really obvious that
> this can't happen on control/render nodes. Bonus that we can avoid the
> master lock in this case.

...as the minor->type is statically assigned on creation and so
inspection doesn't require the lock.

> v2: Don't leak locks on error path (and simplify control flow while
> at it), reported by Julia.
> 
> Cc: Julia Lawall <julia.lawall at lip6.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_auth.c     | 19 ++++++++++++++++++-
>  drivers/gpu/drm/drm_fops.c     | 13 ++-----------
>  drivers/gpu/drm/drm_internal.h |  2 +-
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 1d314ae13560..9c1e2081cd58 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -120,7 +120,7 @@ static struct drm_master *drm_master_create(struct drm_device *dev)
>   * This function must be called with dev::struct_mutex held.
>   * Returns negative error code on failure. Zero on success.
>   */
> -int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> +static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>  {
>  	struct drm_master *old_master;
>  	int ret;
> @@ -226,6 +226,23 @@ out_unlock:
>  	return ret;
>  }
>  
> +int drm_master_open(struct drm_file *file_priv)
> +{
> +	struct drm_device *dev = file_priv->minor->dev;
> +	int ret = 0;
> +
> +	/* if there is no current master make this fd it, but do not create
> +	 * any master object for render clients */

The second half of this comment is out of place, it is referring to
drm_is_primary_client().

> +	mutex_lock(&dev->master_mutex);
> +	if (!file_priv->minor->master)
> +		ret = drm_new_set_master(dev, file_priv);
> +	else
> +		file_priv->master = drm_master_get(file_priv->minor->master);
> +	mutex_unlock(&dev->master_mutex);
> +
> +	return ret;
> +}
> +
>  struct drm_master *drm_master_get(struct drm_master *master)
>  {
>  	kref_get(&master->refcount);
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 50b3de990aee..e2522672719a 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -229,19 +229,11 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
>  			goto out_prime_destroy;
>  	}
>  
> -	/* if there is no current master make this fd it, but do not create
> -	 * any master object for render clients */
> -	mutex_lock(&dev->master_mutex);
> -	if (drm_is_primary_client(priv) && !priv->minor->master) {
> -		/* create a new master */
> -		ret = drm_new_set_master(dev, priv);
> +	if (drm_is_primary_client(priv)) {
> +		ret = drm_master_open(priv);
>  		if (ret)

Both drm_is_primary_client() and drm_master_open() are acting on
drm_minor not drm_file. Idle speculation if that makes the code any
clearer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre