[PATCH 07/12] drm: drop redundant drm_file->is_master

[David Herrmann]

Hi

On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote:
>> The drm_file->is_master field is redundant as it's equivalent to:
>>     drm_file->master && drm_file->master == drm_file->minor->master
>>
>> 1) "=>"
>>   Whenever we set drm_file->is_master, we also set:
>>       drm_file->minor->master = drm_file->master;
>>
>>   Whenever we clear drm_file->is_master, we also call:
>>       drm_master_put(&drm_file->minor->master);
>>   which implicitly clears it to NULL.
>>
>> 2) "<="
>>   minor->master cannot be set if it is non-NULL. Therefore, it stays as
>>   is unless a file drops it.
>>
>>   If minor->master is NULL, it is only set by places that also adjust
>>   drm_file->is_master.
>>
>> Therefore, we can safely drop is_master and replace it by an inline helper
>> that matches:
>>     drm_file->master && drm_file->master == drm_file->minor->master
>>
>> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
>
> Docbook for drm_is_master is missing, otherwise Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> And one question below which doesn't really matter for this patch here.
> See below

Fixed, thanks!

> [snip]
>
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index d91e09f..e1bb585 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -387,8 +387,6 @@ struct drm_prime_file_private {
>>  struct drm_file {
>>       unsigned always_authenticated :1;
>>       unsigned authenticated :1;
>> -     /* Whether we're master for a minor. Protected by master_mutex */
>> -     unsigned is_master :1;
>>       /* true when the client has asked us to expose stereo 3D mode flags */
>>       unsigned stereo_allowed :1;
>>       /*
>> @@ -1034,7 +1032,7 @@ struct drm_device {
>>       /** \name Locks */
>>       /*@{ */
>>       struct mutex struct_mutex;      /**< For others */
>> -     struct mutex master_mutex;      /**< For drm_minor::master and drm_file::is_master */
>> +     struct mutex master_mutex;      /**< For drm_minor::master */
>>       /*@} */
>>
>>       /** \name Usage Counters */
>> @@ -1172,6 +1170,11 @@ static inline bool drm_is_primary_client(const struct drm_file *file_priv)
>>       return file_priv->minor->type == DRM_MINOR_LEGACY;
>>  }
>>
>
> Docbook here please ...
>> +static inline bool drm_is_master(const struct drm_file *file)
>> +{
>
> Hm, we don't have any means to synchronize is_master checks with
> concurrent ioctls and stuff. Do we care? Orthogonal issue really.

We don't.. My drm-master reworks contains a comment about that. It's
not really problematic as all ioctls run for a determinate time in
kernel-code except for drm_read(), but drm_read() is per-file, not
per-device, so we're fine. However, with unfortunate timings, we might
really end up with problems.

vmwgfx solves this by using separate locks and verifying them against
the current master. it's not perfect and I'm not sure I like it better
than no locks, but at least they were aware of the problem.

Btw., the only thing I know how to solve it properly is to make
"master_mutex" an rwlock and all f_op entries take the read-lock,
while master modifications take the write-lock. Not sure it's worth
it, though.

Thanks
David

>> +     return file->master && file->master == file->minor->master;
>> +}
>> +
>>  /******************************************************************/
>>  /** \name Internal function definitions */
>>  /*@{*/
>> --
>> 2.0.2
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch