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@xxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
