diff mbox

drm: Allow also control clients to check the drm version

Message ID 1442304702-51301-2-git-send-email-thellstrom@vmware.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Hellstrom Sept. 15, 2015, 8:11 a.m. UTC 
This should be harmless.
Vmware will, due to old infrastructure reasons, be using a privileged
control client to supply GUI layout information rather than obtaining
it from the device. That control client will be needing access to DRM
version information.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
---
 drivers/gpu/drm/drm_ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

David Herrmann Sept. 16, 2015, 2:35 p.m. UTC | #1 
Hi

On Tue, Sep 15, 2015 at 10:11 AM, Thomas Hellstrom
<thellstrom@vmware.com> wrote:
> This should be harmless.
> Vmware will, due to old infrastructure reasons, be using a privileged
> control client to supply GUI layout information rather than obtaining
> it from the device. That control client will be needing access to DRM
> version information.

I didn't add it to render-nodes as I wasn't aware of any driver still
using the version-information. I'm not a big fan on relying on magic
numbers, as it doesn't work well with (stable) backports, but if you
need it for backwards-compat on vmwgfx, I'm fine with it. But I
certainly don't want to encourage new driver authors to use it.

> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Reviewed-by: Brian Paul <brianp@vmware.com>
> Reviewed-by: Sinclair Yeh <syeh@vmware.com>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 9a860ca..d93e737 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -520,7 +520,8 @@ EXPORT_SYMBOL(drm_ioctl_permit);
>
>  /** Ioctl table */
>  static const struct drm_ioctl_desc drm_ioctls[] = {
> -       DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version,
> +                     DRM_UNLOCKED|DRM_RENDER_ALLOW|DRM_CONTROL_ALLOW),

Why the line-break? None of the other ioctl definitions cares for the
80ch limit. I'd prefer keeping this uniform.

Acked-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

>         DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0),
>         DRM_IOCTL_DEF(DRM_IOCTL_GET_MAGIC, drm_getmagic, 0),
>         DRM_IOCTL_DEF(DRM_IOCTL_IRQ_BUSID, drm_irq_by_busid, DRM_MASTER|DRM_ROOT_ONLY),
> --
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Thomas Hellstrom Sept. 16, 2015, 3:03 p.m. UTC | #2 
Hi, David,

On 09/16/2015 04:35 PM, David Herrmann wrote:
> Hi
>
> On Tue, Sep 15, 2015 at 10:11 AM, Thomas Hellstrom
> <thellstrom@vmware.com> wrote:
>> This should be harmless.
>> Vmware will, due to old infrastructure reasons, be using a privileged
>> control client to supply GUI layout information rather than obtaining
>> it from the device. That control client will be needing access to DRM
>> version information.
> I didn't add it to render-nodes as I wasn't aware of any driver still
> using the version-information.

In fact you did add it to render nodes (commit 3d3b78c), but this is for
control nodes.

>  I'm not a big fan on relying on magic
> numbers, as it doesn't work well with (stable) backports, but if you
> need it for backwards-compat on vmwgfx, I'm fine with it. But I
> certainly don't want to encourage new driver authors to use it.

We view the driver version information as user-space api version
information, which I can't see changing
with stable backports (except perhaps in very unlikely situations).
Of course, there are other ways to signal feature availability, but
we've traditionally been checking version minor.

>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> Reviewed-by: Brian Paul <brianp@vmware.com>
>> Reviewed-by: Sinclair Yeh <syeh@vmware.com>
>> ---
>>  drivers/gpu/drm/drm_ioctl.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 9a860ca..d93e737 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -520,7 +520,8 @@ EXPORT_SYMBOL(drm_ioctl_permit);
>>
>>  /** Ioctl table */
>>  static const struct drm_ioctl_desc drm_ioctls[] = {
>> -       DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>> +       DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version,
>> +                     DRM_UNLOCKED|DRM_RENDER_ALLOW|DRM_CONTROL_ALLOW),
> Why the line-break? None of the other ioctl definitions cares for the
> 80ch limit. I'd prefer keeping this uniform.

OK, I'll fix that up.

>
> Acked-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David

Thanks,
Thomas

>
>>         DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0),
>>         DRM_IOCTL_DEF(DRM_IOCTL_GET_MAGIC, drm_getmagic, 0),
>>         DRM_IOCTL_DEF(DRM_IOCTL_IRQ_BUSID, drm_irq_by_busid, DRM_MASTER|DRM_ROOT_ONLY),
>> --
>> 2.1.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=-dqJN9pZD2sav6M76SFMIM359ZWmPz3fLT0oU30MVRw&s=ICMYbx449lJwRcYDljH6Z8Zp9SQO6mX71E9Y9OgL_SA&e=
David Herrmann Sept. 16, 2015, 6:08 p.m. UTC | #3 
Hi

On Wed, Sep 16, 2015 at 5:03 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> Hi, David,
>
> On 09/16/2015 04:35 PM, David Herrmann wrote:
>> Hi
>>
>> On Tue, Sep 15, 2015 at 10:11 AM, Thomas Hellstrom
>> <thellstrom@vmware.com> wrote:
>>> This should be harmless.
>>> Vmware will, due to old infrastructure reasons, be using a privileged
>>> control client to supply GUI layout information rather than obtaining
>>> it from the device. That control client will be needing access to DRM
>>> version information.
>> I didn't add it to render-nodes as I wasn't aware of any driver still
>> using the version-information.
>
> In fact you did add it to render nodes (commit 3d3b78c), but this is for
> control nodes.

Sorry, my bad. For control nodes is also fine.

Thanks
David
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 9a860ca..d93e737 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -520,7 +520,8 @@  EXPORT_SYMBOL(drm_ioctl_permit);
 
 /** Ioctl table */
 static const struct drm_ioctl_desc drm_ioctls[] = {
-	DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW|DRM_CONTROL_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_MAGIC, drm_getmagic, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_IRQ_BUSID, drm_irq_by_busid, DRM_MASTER|DRM_ROOT_ONLY),