Re: [PATCH 1/2] drm: Make control nodes master-less

[Thomas Hellstrom <thellstrom@vmware.com>]

On 03/05/2014 03:32 PM, David Herrmann wrote:
> Hi
>
> On Wed, Feb 19, 2014 at 2:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> Like for render-nodes, there is no point in maintaining the master concept
>> for control nodes, so set the struct drm_file::master pointer to NULL.
>>
>> At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always
>> allowed when called through the control node. Previously the caller also
>> needed to be master.
>>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> ---
>>  drivers/gpu/drm/drm_drv.c  |    5 +++--
>>  drivers/gpu/drm/drm_fops.c |    5 +++--
>>  include/drm/drmP.h         |    5 +++++
>>  3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 345be03..42af8bd 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp,
>>                 retcode = -EINVAL;
>>         } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
>>                    ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !file_priv->authenticated) ||
>> -                  ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
>> -                  (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) ||
>> +                  (((ioctl->flags & DRM_MASTER) && !file_priv->is_master) &&
>> +                   !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) ||
>> +                  (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) ||
>>                    (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
>>                 retcode = -EACCES;
>>         } else {
>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> index 7f2af9a..08a3196 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>         /* if there is no current master make this fd it, but do not create
>>          * any master object for render clients */
>>         mutex_lock(&dev->struct_mutex);
>> -       if (!priv->minor->master && !drm_is_render_client(priv)) {
>> +       if (!priv->minor->master && !drm_is_render_client(priv) &&
>> +           !drm_is_control(priv)) {
>>                 /* create a new master */
>>                 priv->minor->master = drm_master_create(priv->minor);
>>                 if (!priv->minor->master) {
>> @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>                                 goto out_close;
>>                         }
>>                 }
>> -       } else if (!drm_is_render_client(priv)) {
>> +       } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
>>                 /* get a reference to the master */
>>                 priv->master = drm_master_get(priv->minor->master);
> Maybe I missed some other patches, but how is this going to work?
> drm_crtc.c expects priv->master to be non-NULL (eg.,
> drm_mode_getresources()). This is fine for render-nodes as they don't
> allow any of those ioctls, but the control-node does allow them.
>
> Thanks
> David
>
>

Hi!

You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
I guess this, and Daniel's reply prompts a discussion about control
nodes and the master concept.

First I'd like to give some background about the use-case: I'd like to
use the control node for a daemon that tells the vmwgfx driver about the
host GUI layout of the screen: What connectors are enabled, the
preferred mode of each output and some driver private information. The
daemon will run as root and only a single instance per device. Trying to
do this as-is will cause the vmwgfx driver to BUG() because it currently
assumes one active master per device. Not one active master per minor
(although that could be changed if needed).

Looking at

http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/

it doesn't really make sense to keep the master concept on the control
node, but it perhaps makes sense to keep it on future modesetting nodes
to allow multiple modesetters to open the same device node?

In that case all master access in drm_crtc.c would be changed for now to
be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY

/Thomas