All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Julia Lawall <julia.lawall@lip6.fr>
Subject: Re: [Intel-gfx] [PATCH 05/14] drm: Extract drm_master_open
Date: Wed, 15 Jun 2016 12:41:21 +0100	[thread overview]
Message-ID: <20160615114121.GI4184@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1465930269-7883-6-git-send-email-daniel.vetter@ffwll.ch>

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@lip6.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Chris Wilson <chris@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2016-06-15 11:41 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
2016-06-15 11:23   ` Chris Wilson
2016-06-15 12:00   ` Emil Velikov
2016-06-14 18:50 ` [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better Daniel Vetter
2016-06-15 11:26   ` [Intel-gfx] " Chris Wilson
2016-06-15 12:10   ` Emil Velikov
2016-06-14 18:50 ` [PATCH 03/14] drm: Link directly from drm_master to drm_device Daniel Vetter
2016-06-15 11:29   ` Chris Wilson
2016-06-15 12:50   ` [Intel-gfx] " Emil Velikov
2016-06-15 15:45     ` Daniel Vetter
2016-06-16 20:04       ` Emil Velikov
2016-06-14 18:50 ` [PATCH 04/14] drm: Move master functions into drm_auth.c Daniel Vetter
2016-06-15 11:31   ` Chris Wilson
2016-06-15 15:47     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 05/14] drm: Extract drm_master_open Daniel Vetter
2016-06-15 11:41   ` Chris Wilson [this message]
2016-06-14 18:51 ` [PATCH 06/14] drm: Extract drm_master_relase Daniel Vetter
2016-06-15 11:43   ` [Intel-gfx] " Chris Wilson
2016-06-16  8:18     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET Daniel Vetter
2016-06-15 11:48   ` Chris Wilson
2016-06-15 15:49     ` [Intel-gfx] " Daniel Vetter
2016-06-14 18:51 ` [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release Daniel Vetter
2016-06-15 11:51   ` Chris Wilson
2016-06-14 18:51 ` [PATCH 09/14] drm: Protect authmagic with master_mutex Daniel Vetter
2016-06-15 11:54   ` [Intel-gfx] " Chris Wilson
2016-06-15 15:52     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 10/14] drm: Mark authmagic ioctls as unlocked Daniel Vetter
2016-06-15 11:55   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 11/14] drm: Mark set/drop master ioctl " Daniel Vetter
2016-06-15 11:56   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device Daniel Vetter
2016-06-15 12:10   ` [Intel-gfx] " Chris Wilson
2016-06-15 16:01     ` Daniel Vetter
2016-06-15 16:33       ` Chris Wilson
2016-06-15 19:54         ` [Intel-gfx] " Daniel Vetter
2016-06-14 18:51 ` [PATCH 13/14] drm: Clean up drm_crtc.h Daniel Vetter
2016-06-14 18:51 ` [PATCH 14/14] drm: Use dev->name as fallback for dev->unique Daniel Vetter
2016-06-15  5:42 ` ✓ Ro.CI.BAT: success for Cruft removal around drm_master Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160615114121.GI4184@nuc-i3427.alporthouse.com \
    --to=chris@chris-wilson.co.uk \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=julia.lawall@lip6.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.