All of lore.kernel.org
 help / color / mirror / Atom feed
* Render only DRM devices
@ 2022-10-11 11:04 Christian König
  2022-10-11 11:04 ` [PATCH 1/2] drm: remove DRM_MINOR_CONTROL Christian König
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Christian König @ 2022-10-11 11:04 UTC (permalink / raw)
  To: dri-devel, daniel, airlied

Hi guys,

we already have quite a bunch of devices which are essentially render only and don't expose any connectors or more general display functionality.

Just recently I ran into a case where an older X/DDX combination caused problems for such a device so I looked a bit into the possibility to allow drivers to disable the primary node and only expose the render node.

It turned out that this effectively hides the device from X, but OpenGL and Vulkan can still use it perfectly fine.

The only crux is that this is checked so early in the initialization that drivers don't have an opportunity to update their dev->driver_features. So we will always need a separate drm_driver structure for render only devices.

Please review and comment,
Christian.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] drm: remove DRM_MINOR_CONTROL
  2022-10-11 11:04 Render only DRM devices Christian König
@ 2022-10-11 11:04 ` Christian König
  2022-10-11 11:39   ` Simon Ser
  2022-10-11 11:04 ` [PATCH 2/2] drm: add DRIVER_RENDER_ONLY Christian König
  2022-10-11 11:37 ` Render only DRM devices Simon Ser
  2 siblings, 1 reply; 14+ messages in thread
From: Christian König @ 2022-10-11 11:04 UTC (permalink / raw)
  To: dri-devel, daniel, airlied; +Cc: Christian König

Not used any more. This makes room for up to 128 DRM devices.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_drv.c | 4 ++--
 include/drm/drm_file.h    | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..d81783f43452 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -126,8 +126,8 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	spin_lock_irqsave(&drm_minor_lock, flags);
 	r = idr_alloc(&drm_minors_idr,
 		      NULL,
-		      64 * type,
-		      64 * (type + 1),
+		      128 * type,
+		      128 * (type + 1),
 		      GFP_NOWAIT);
 	spin_unlock_irqrestore(&drm_minor_lock, flags);
 	idr_preload_end();
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index d780fd151789..a3be533e99e0 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -54,7 +54,6 @@ struct file;
  */
 enum drm_minor_type {
 	DRM_MINOR_PRIMARY,
-	DRM_MINOR_CONTROL,
 	DRM_MINOR_RENDER,
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] drm: add DRIVER_RENDER_ONLY
  2022-10-11 11:04 Render only DRM devices Christian König
  2022-10-11 11:04 ` [PATCH 1/2] drm: remove DRM_MINOR_CONTROL Christian König
@ 2022-10-11 11:04 ` Christian König
  2022-10-11 11:27   ` Thomas Zimmermann
  2022-10-11 11:37 ` Render only DRM devices Simon Ser
  2 siblings, 1 reply; 14+ messages in thread
From: Christian König @ 2022-10-11 11:04 UTC (permalink / raw)
  To: dri-devel, daniel, airlied; +Cc: Christian König

This allows to suppress creating the primary node. Useful for drivers
which don't expose any display functionality, but are render only by
design.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_drv.c   | 10 ++++++----
 drivers/gpu/drm/drm_prime.c |  2 +-
 include/drm/drm_drv.h       |  7 +++++++
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index d81783f43452..4a525f78a932 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -634,9 +634,11 @@ static int drm_dev_init(struct drm_device *dev,
 			goto err;
 	}
 
-	ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
-	if (ret)
-		goto err;
+	if (!drm_core_check_feature(dev, DRIVER_RENDER_ONLY)) {
+		ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
+		if (ret)
+			goto err;
+	}
 
 	ret = drm_legacy_create_map_hash(dev);
 	if (ret)
@@ -902,7 +904,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 		 driver->name, driver->major, driver->minor,
 		 driver->patchlevel, driver->date,
 		 dev->dev ? dev_name(dev->dev) : "virtual device",
-		 dev->primary->index);
+		 (dev->primary ?: dev->render)->index);
 
 	goto out_unlock;
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index a3f180653b8b..4afd3f15b135 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -740,7 +740,7 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 	}
 
 	/* Used by drm_gem_mmap() to lookup the GEM object */
-	priv->minor = obj->dev->primary;
+	priv->minor = obj->dev->primary ?: obj->dev->render;
 	fil->private_data = priv;
 
 	ret = drm_vma_node_allow(&obj->vma_node, priv);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index f6159acb8856..0a1450d47ca2 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -94,6 +94,13 @@ enum drm_driver_feature {
 	 * synchronization of command submission.
 	 */
 	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
+	/**
+	 * @DRIVER_RENDER_ONLY
+	 *
+	 * Driver is a render node only driver and don't want to support the
+	 * primary interface.
+	 */
+	DRIVER_RENDER_ONLY		= BIT(7),
 
 	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] drm: add DRIVER_RENDER_ONLY
  2022-10-11 11:04 ` [PATCH 2/2] drm: add DRIVER_RENDER_ONLY Christian König
@ 2022-10-11 11:27   ` Thomas Zimmermann
  2022-10-11 11:28     ` Christian König
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Zimmermann @ 2022-10-11 11:27 UTC (permalink / raw)
  To: Christian König, dri-devel, daniel, airlied; +Cc: Christian König


[-- Attachment #1.1: Type: text/plain, Size: 2923 bytes --]

Hi

Am 11.10.22 um 13:04 schrieb Christian König:
> This allows to suppress creating the primary node. Useful for drivers
> which don't expose any display functionality, but are render only by
> design.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/drm_drv.c   | 10 ++++++----
>   drivers/gpu/drm/drm_prime.c |  2 +-
>   include/drm/drm_drv.h       |  7 +++++++
>   3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index d81783f43452..4a525f78a932 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -634,9 +634,11 @@ static int drm_dev_init(struct drm_device *dev,
>   			goto err;
>   	}
>   
> -	ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
> -	if (ret)
> -		goto err;
> +	if (!drm_core_check_feature(dev, DRIVER_RENDER_ONLY)) {
> +		ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
> +		if (ret)
> +			goto err;
> +	}
>   
>   	ret = drm_legacy_create_map_hash(dev);
>   	if (ret)
> @@ -902,7 +904,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>   		 driver->name, driver->major, driver->minor,
>   		 driver->patchlevel, driver->date,
>   		 dev->dev ? dev_name(dev->dev) : "virtual device",
> -		 dev->primary->index);
> +		 (dev->primary ?: dev->render)->index);
>   
>   	goto out_unlock;
>   
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index a3f180653b8b..4afd3f15b135 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -740,7 +740,7 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>   	}
>   
>   	/* Used by drm_gem_mmap() to lookup the GEM object */
> -	priv->minor = obj->dev->primary;
> +	priv->minor = obj->dev->primary ?: obj->dev->render;
>   	fil->private_data = priv;
>   
>   	ret = drm_vma_node_allow(&obj->vma_node, priv);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index f6159acb8856..0a1450d47ca2 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -94,6 +94,13 @@ enum drm_driver_feature {
>   	 * synchronization of command submission.
>   	 */
>   	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> +	/**
> +	 * @DRIVER_RENDER_ONLY
> +	 *
> +	 * Driver is a render node only driver and don't want to support the
> +	 * primary interface.
> +	 */
> +	DRIVER_RENDER_ONLY		= BIT(7),

Is it really necessary to add a new flag?  If a driver sets DRIVER_GEM 
and omits DRIVER_MODESET wouldn't this have the same meaning?

Best regards
Thomas

>   
>   	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] drm: add DRIVER_RENDER_ONLY
  2022-10-11 11:27   ` Thomas Zimmermann
@ 2022-10-11 11:28     ` Christian König
  0 siblings, 0 replies; 14+ messages in thread
From: Christian König @ 2022-10-11 11:28 UTC (permalink / raw)
  To: Thomas Zimmermann, dri-devel, daniel, airlied; +Cc: Christian König

Am 11.10.22 um 13:27 schrieb Thomas Zimmermann:
> Hi
>
> Am 11.10.22 um 13:04 schrieb Christian König:
>> This allows to suppress creating the primary node. Useful for drivers
>> which don't expose any display functionality, but are render only by
>> design.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/drm_drv.c   | 10 ++++++----
>>   drivers/gpu/drm/drm_prime.c |  2 +-
>>   include/drm/drm_drv.h       |  7 +++++++
>>   3 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index d81783f43452..4a525f78a932 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -634,9 +634,11 @@ static int drm_dev_init(struct drm_device *dev,
>>               goto err;
>>       }
>>   -    ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
>> -    if (ret)
>> -        goto err;
>> +    if (!drm_core_check_feature(dev, DRIVER_RENDER_ONLY)) {
>> +        ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
>> +        if (ret)
>> +            goto err;
>> +    }
>>         ret = drm_legacy_create_map_hash(dev);
>>       if (ret)
>> @@ -902,7 +904,7 @@ int drm_dev_register(struct drm_device *dev, 
>> unsigned long flags)
>>            driver->name, driver->major, driver->minor,
>>            driver->patchlevel, driver->date,
>>            dev->dev ? dev_name(dev->dev) : "virtual device",
>> -         dev->primary->index);
>> +         (dev->primary ?: dev->render)->index);
>>         goto out_unlock;
>>   diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index a3f180653b8b..4afd3f15b135 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -740,7 +740,7 @@ int drm_gem_prime_mmap(struct drm_gem_object 
>> *obj, struct vm_area_struct *vma)
>>       }
>>         /* Used by drm_gem_mmap() to lookup the GEM object */
>> -    priv->minor = obj->dev->primary;
>> +    priv->minor = obj->dev->primary ?: obj->dev->render;
>>       fil->private_data = priv;
>>         ret = drm_vma_node_allow(&obj->vma_node, priv);
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index f6159acb8856..0a1450d47ca2 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -94,6 +94,13 @@ enum drm_driver_feature {
>>        * synchronization of command submission.
>>        */
>>       DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
>> +    /**
>> +     * @DRIVER_RENDER_ONLY
>> +     *
>> +     * Driver is a render node only driver and don't want to support 
>> the
>> +     * primary interface.
>> +     */
>> +    DRIVER_RENDER_ONLY        = BIT(7),
>
> Is it really necessary to add a new flag?  If a driver sets DRIVER_GEM 
> and omits DRIVER_MODESET wouldn't this have the same meaning?

Mhm, good point. I haven't thought about that.

Let me double check if anybody is actually using this.

Thanks,
Christian.

>
> Best regards
> Thomas
>
>>         /* IMPORTANT: Below are all the legacy flags, add new ones 
>> above. */
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Render only DRM devices
  2022-10-11 11:04 Render only DRM devices Christian König
  2022-10-11 11:04 ` [PATCH 1/2] drm: remove DRM_MINOR_CONTROL Christian König
  2022-10-11 11:04 ` [PATCH 2/2] drm: add DRIVER_RENDER_ONLY Christian König
@ 2022-10-11 11:37 ` Simon Ser
  2022-10-11 11:56   ` Christian König
  2 siblings, 1 reply; 14+ messages in thread
From: Simon Ser @ 2022-10-11 11:37 UTC (permalink / raw)
  To: Christian König; +Cc: airlied, dri-devel

On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:

> we already have quite a bunch of devices which are essentially render
> only and don't expose any connectors or more general display functionality.
> 
> Just recently I ran into a case where an older X/DDX combination
> caused problems for such a device so I looked a bit into the
> possibility to allow drivers to disable the primary node and only
> expose the render node.
> 
> It turned out that this effectively hides the device from X, but
> OpenGL and Vulkan can still use it perfectly fine.
> 
> The only crux is that this is checked so early in the initialization
> that drivers don't have an opportunity to update their
> dev->driver_features. So we will always need a separate drm_driver
> structure for render only devices.

Typically render-only devices still expose a primary node, but don't
expose any KMS resources on it. See drmIsKMS() in libdrm.

Primary nodes could still be used by older user-space for rendering with
legacy DRM authentication.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] drm: remove DRM_MINOR_CONTROL
  2022-10-11 11:04 ` [PATCH 1/2] drm: remove DRM_MINOR_CONTROL Christian König
@ 2022-10-11 11:39   ` Simon Ser
  2022-10-11 11:55     ` Christian König
  0 siblings, 1 reply; 14+ messages in thread
From: Simon Ser @ 2022-10-11 11:39 UTC (permalink / raw)
  To: Christian König; +Cc: airlied, dri-devel, Christian König

On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:

> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -54,7 +54,6 @@ struct file;
>   */
>  enum drm_minor_type {
>  	DRM_MINOR_PRIMARY,
> -	DRM_MINOR_CONTROL,
>  	DRM_MINOR_RENDER,
>  };

This makes me uncomfortable: this enum no longer matches DRM_NODE_* in
libdrm.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] drm: remove DRM_MINOR_CONTROL
  2022-10-11 11:39   ` Simon Ser
@ 2022-10-11 11:55     ` Christian König
  2022-10-25 13:59       ` Michał Winiarski
  0 siblings, 1 reply; 14+ messages in thread
From: Christian König @ 2022-10-11 11:55 UTC (permalink / raw)
  To: Simon Ser; +Cc: airlied, dri-devel, Christian König

Am 11.10.22 um 13:39 schrieb Simon Ser:
> On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -54,7 +54,6 @@ struct file;
>>    */
>>   enum drm_minor_type {
>>   	DRM_MINOR_PRIMARY,
>> -	DRM_MINOR_CONTROL,
>>   	DRM_MINOR_RENDER,
>>   };
> This makes me uncomfortable: this enum no longer matches DRM_NODE_* in
> libdrm.

Ah! There it was! I was remembering in the back of my head that we had 
somehow used this in libdrm as well, but couldn't really get where exactly.

But I don't really see a problem here. The control nodes are identified 
by name and we don't expose them for quite some time now without any 
negative impact.

Even the minor number distribution stays the same. So what bad can come 
from this?

Thanks,
Christian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Render only DRM devices
  2022-10-11 11:37 ` Render only DRM devices Simon Ser
@ 2022-10-11 11:56   ` Christian König
  2022-10-11 15:20     ` Simon Ser
  0 siblings, 1 reply; 14+ messages in thread
From: Christian König @ 2022-10-11 11:56 UTC (permalink / raw)
  To: Simon Ser; +Cc: airlied, dri-devel

Am 11.10.22 um 13:37 schrieb Simon Ser:
> On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>
>> we already have quite a bunch of devices which are essentially render
>> only and don't expose any connectors or more general display functionality.
>>
>> Just recently I ran into a case where an older X/DDX combination
>> caused problems for such a device so I looked a bit into the
>> possibility to allow drivers to disable the primary node and only
>> expose the render node.
>>
>> It turned out that this effectively hides the device from X, but
>> OpenGL and Vulkan can still use it perfectly fine.
>>
>> The only crux is that this is checked so early in the initialization
>> that drivers don't have an opportunity to update their
>> dev->driver_features. So we will always need a separate drm_driver
>> structure for render only devices.
> Typically render-only devices still expose a primary node, but don't
> expose any KMS resources on it. See drmIsKMS() in libdrm.
>
> Primary nodes could still be used by older user-space for rendering with
> legacy DRM authentication.

Yeah, and that's exactly what we try to avoid :)

Cheers,
Christian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Render only DRM devices
  2022-10-11 11:56   ` Christian König
@ 2022-10-11 15:20     ` Simon Ser
  2022-10-12  6:18       ` Christian König
  0 siblings, 1 reply; 14+ messages in thread
From: Simon Ser @ 2022-10-11 15:20 UTC (permalink / raw)
  To: Christian König; +Cc: airlied, dri-devel

On Tuesday, October 11th, 2022 at 13:56, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:

> Am 11.10.22 um 13:37 schrieb Simon Ser:
> 
> > On Tuesday, October 11th, 2022 at 13:04, Christian König ckoenig.leichtzumerken@gmail.com wrote:
> > 
> > > we already have quite a bunch of devices which are essentially render
> > > only and don't expose any connectors or more general display functionality.
> > > 
> > > Just recently I ran into a case where an older X/DDX combination
> > > caused problems for such a device so I looked a bit into the
> > > possibility to allow drivers to disable the primary node and only
> > > expose the render node.
> > > 
> > > It turned out that this effectively hides the device from X, but
> > > OpenGL and Vulkan can still use it perfectly fine.
> > > 
> > > The only crux is that this is checked so early in the initialization
> > > that drivers don't have an opportunity to update their
> > > dev->driver_features. So we will always need a separate drm_driver
> > > structure for render only devices.
> > > Typically render-only devices still expose a primary node, but don't
> > > expose any KMS resources on it. See drmIsKMS() in libdrm.
> > 
> > Primary nodes could still be used by older user-space for rendering with
> > legacy DRM authentication.
> 
> Yeah, and that's exactly what we try to avoid :)

But, wouldn't that regress user-space which renders using primary nodes +
DRM auth?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Render only DRM devices
  2022-10-11 15:20     ` Simon Ser
@ 2022-10-12  6:18       ` Christian König
  2022-10-12  7:25         ` Simon Ser
  0 siblings, 1 reply; 14+ messages in thread
From: Christian König @ 2022-10-12  6:18 UTC (permalink / raw)
  To: Simon Ser; +Cc: Dave Airlie, dri-devel

Am 11.10.22 um 17:20 schrieb Simon Ser:
> On Tuesday, October 11th, 2022 at 13:56, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>
>> Am 11.10.22 um 13:37 schrieb Simon Ser:
>>
>>> On Tuesday, October 11th, 2022 at 13:04, Christian König ckoenig.leichtzumerken@gmail.com wrote:
>>>
>>>> we already have quite a bunch of devices which are essentially render
>>>> only and don't expose any connectors or more general display functionality.
>>>>
>>>> Just recently I ran into a case where an older X/DDX combination
>>>> caused problems for such a device so I looked a bit into the
>>>> possibility to allow drivers to disable the primary node and only
>>>> expose the render node.
>>>>
>>>> It turned out that this effectively hides the device from X, but
>>>> OpenGL and Vulkan can still use it perfectly fine.
>>>>
>>>> The only crux is that this is checked so early in the initialization
>>>> that drivers don't have an opportunity to update their
>>>> dev->driver_features. So we will always need a separate drm_driver
>>>> structure for render only devices.
>>>> Typically render-only devices still expose a primary node, but don't
>>>> expose any KMS resources on it. See drmIsKMS() in libdrm.
>>> Primary nodes could still be used by older user-space for rendering with
>>> legacy DRM authentication.
>> Yeah, and that's exactly what we try to avoid :)
> But, wouldn't that regress user-space which renders using primary nodes +
> DRM auth?

Yes and that is completely intentional as well. At least for new 
hardware generations we shouldn't have any userspace using this any more.

DRM auth based authentication is seen as a security risk and we want to 
get rid of that for render only devices (at least for the new ones).

I should probably add a wider explanation to the commit message.

Thanks,
Christian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Render only DRM devices
  2022-10-12  6:18       ` Christian König
@ 2022-10-12  7:25         ` Simon Ser
  0 siblings, 0 replies; 14+ messages in thread
From: Simon Ser @ 2022-10-12  7:25 UTC (permalink / raw)
  To: Christian König; +Cc: Dave Airlie, dri-devel

On Wednesday, October 12th, 2022 at 08:18, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:

> DRM auth based authentication is seen as a security risk and we want to
> get rid of that for render only devices (at least for the new ones).

Right, that makes sense for new hw/drivers. So we really need this to
be opt-in, we can't just do it for all existing render-only drivers
without breaking the kernel no-regression rule.

> I should probably add a wider explanation to the commit message.

Good idea!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] drm: remove DRM_MINOR_CONTROL
  2022-10-11 11:55     ` Christian König
@ 2022-10-25 13:59       ` Michał Winiarski
  2022-10-25 15:52         ` Christian König
  0 siblings, 1 reply; 14+ messages in thread
From: Michał Winiarski @ 2022-10-25 13:59 UTC (permalink / raw)
  To: Christian König; +Cc: airlied, Christian König, dri-devel

On Tue, Oct 11, 2022 at 01:55:01PM +0200, Christian König wrote:
> Am 11.10.22 um 13:39 schrieb Simon Ser:
> > On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> > 
> > > --- a/include/drm/drm_file.h
> > > +++ b/include/drm/drm_file.h
> > > @@ -54,7 +54,6 @@ struct file;
> > >    */
> > >   enum drm_minor_type {
> > >   	DRM_MINOR_PRIMARY,
> > > -	DRM_MINOR_CONTROL,
> > >   	DRM_MINOR_RENDER,
> > >   };
> > This makes me uncomfortable: this enum no longer matches DRM_NODE_* in
> > libdrm.
> 
> Ah! There it was! I was remembering in the back of my head that we had
> somehow used this in libdrm as well, but couldn't really get where exactly.
> 
> But I don't really see a problem here. The control nodes are identified by
> name and we don't expose them for quite some time now without any negative
> impact.
> 
> Even the minor number distribution stays the same. So what bad can come from
> this?
> 
> Thanks,
> Christian.
> 

I proposed something similar in:
https://lore.kernel.org/dri-devel/20220817230600.272790-1-michal.winiarski@intel.com/
except acompanied by expanding the minor pool to accomodate more than
128 devices:

And after receiving similar feedback, that eventually evolved into
leaving the "legacy minors" alone, and changing the rules only for cases
where we have more than 64 devices  (when we run out of the "legacy
minors").

Perhaps something like this:
https://lore.kernel.org/dri-devel/20220911211443.581481-1-michal.winiarski@intel.com/
Would work for your usecase as well?

-Michał

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] drm: remove DRM_MINOR_CONTROL
  2022-10-25 13:59       ` Michał Winiarski
@ 2022-10-25 15:52         ` Christian König
  0 siblings, 0 replies; 14+ messages in thread
From: Christian König @ 2022-10-25 15:52 UTC (permalink / raw)
  To: Michał Winiarski, Christian König; +Cc: airlied, dri-devel

Am 25.10.22 um 15:59 schrieb Michał Winiarski:
> On Tue, Oct 11, 2022 at 01:55:01PM +0200, Christian König wrote:
>> Am 11.10.22 um 13:39 schrieb Simon Ser:
>>> On Tuesday, October 11th, 2022 at 13:04, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>>>
>>>> --- a/include/drm/drm_file.h
>>>> +++ b/include/drm/drm_file.h
>>>> @@ -54,7 +54,6 @@ struct file;
>>>>     */
>>>>    enum drm_minor_type {
>>>>    	DRM_MINOR_PRIMARY,
>>>> -	DRM_MINOR_CONTROL,
>>>>    	DRM_MINOR_RENDER,
>>>>    };
>>> This makes me uncomfortable: this enum no longer matches DRM_NODE_* in
>>> libdrm.
>> Ah! There it was! I was remembering in the back of my head that we had
>> somehow used this in libdrm as well, but couldn't really get where exactly.
>>
>> But I don't really see a problem here. The control nodes are identified by
>> name and we don't expose them for quite some time now without any negative
>> impact.
>>
>> Even the minor number distribution stays the same. So what bad can come from
>> this?
>>
>> Thanks,
>> Christian.
>>
> I proposed something similar in:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20220817230600.272790-1-michal.winiarski%40intel.com%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C085831b6e1b647ca1dbd08dab6911b4b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638023031607291573%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=56FxZ4UThGlbJ0ut8biicsYtIvtTZ8RGHISJqe%2BXixY%3D&amp;reserved=0
> except acompanied by expanding the minor pool to accomodate more than
> 128 devices:
>
> And after receiving similar feedback, that eventually evolved into
> leaving the "legacy minors" alone, and changing the rules only for cases
> where we have more than 64 devices  (when we run out of the "legacy
> minors").
>
> Perhaps something like this:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20220911211443.581481-1-michal.winiarski%40intel.com%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C085831b6e1b647ca1dbd08dab6911b4b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638023031607291573%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=dFRTx0%2Fi7a4aps57JWGtEJ6GoW5IfI5CQjFkig4KFnA%3D&amp;reserved=0
> Would work for your usecase as well?

We don't desperately need the additional minor numbers, this was merely 
just a cleanup to remove an unused define.

Christian.

>
> -Michał


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-10-25 15:52 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-11 11:04 Render only DRM devices Christian König
2022-10-11 11:04 ` [PATCH 1/2] drm: remove DRM_MINOR_CONTROL Christian König
2022-10-11 11:39   ` Simon Ser
2022-10-11 11:55     ` Christian König
2022-10-25 13:59       ` Michał Winiarski
2022-10-25 15:52         ` Christian König
2022-10-11 11:04 ` [PATCH 2/2] drm: add DRIVER_RENDER_ONLY Christian König
2022-10-11 11:27   ` Thomas Zimmermann
2022-10-11 11:28     ` Christian König
2022-10-11 11:37 ` Render only DRM devices Simon Ser
2022-10-11 11:56   ` Christian König
2022-10-11 15:20     ` Simon Ser
2022-10-12  6:18       ` Christian König
2022-10-12  7:25         ` Simon Ser

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.