[PATCH] drm: document better that drivers shouldn't use drm_minor directly

[PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Daniel Vetter]

The documentation for struct drm_minor already states this, but that's
not always that easy to find.

Also due to historical reasons we still have the minor-centric (like
drm_debugfs_create_files), but since this is now getting fixed we can
put a few more pointers in place as to how this should be done
ideally.

Motvated by some discussion with Rodrigo on irc about how drm/xe
should lay out its sysfs interfaces.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Wambui Karuga <wambui.karugax@gmail.com>
Cc: Maíra Canal <mcanal@igalia.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Melissa Wen <mwen@igalia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/drm/drm_device.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 9923c7a6885e..b40e07e004ee 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -87,10 +87,23 @@ struct drm_device {
 	 */
 	void *dev_private;
 
-	/** @primary: Primary node */
+	/**
+	 * @primary:
+	 *
+	 * Primary node. Drivers should not interact with this
+	 * directly. debugfs interface can be registered with
+	 * drm_debugfs_add_file(), and sysfs should be directly added on the
+	 * hardwire struct device @dev.
+	 */
 	struct drm_minor *primary;
 
-	/** @render: Render node */
+	/**
+	 * @render:
+	 *
+	 * Render node. Drivers should not interact with this directly ever.
+	 * Drivers should not expose any additional interfaces in debugfs or
+	 * sysfs on thise node.
+	 */
 	struct drm_minor *render;
 
 	/**
-- 
2.37.2

Re: [PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On Wed, Jan 04, 2023 at 10:17:54PM +0100, Daniel Vetter wrote:
> The documentation for struct drm_minor already states this, but that's
> not always that easy to find.
> 
> Also due to historical reasons we still have the minor-centric (like
> drm_debugfs_create_files), but since this is now getting fixed we can
> put a few more pointers in place as to how this should be done
> ideally.
> 
> Motvated by some discussion with Rodrigo on irc about how drm/xe

  ^ Motivated

> should lay out its sysfs interfaces.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Wambui Karuga <wambui.karugax@gmail.com>
> Cc: Maíra Canal <mcanal@igalia.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Melissa Wen <mwen@igalia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Maíra Canal]

On 1/4/23 18:17, Daniel Vetter wrote:
> The documentation for struct drm_minor already states this, but that's
> not always that easy to find.
> 
> Also due to historical reasons we still have the minor-centric (like
> drm_debugfs_create_files), but since this is now getting fixed we can
> put a few more pointers in place as to how this should be done
> ideally.
> 
> Motvated by some discussion with Rodrigo on irc about how drm/xe
> should lay out its sysfs interfaces.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Wambui Karuga <wambui.karugax@gmail.com>
> Cc: Maíra Canal <mcanal@igalia.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Melissa Wen <mwen@igalia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   include/drm/drm_device.h | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..b40e07e004ee 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -87,10 +87,23 @@ struct drm_device {
>   	 */
>   	void *dev_private;
>   
> -	/** @primary: Primary node */
> +	/**
> +	 * @primary:
> +	 *
> +	 * Primary node. Drivers should not interact with this
> +	 * directly. debugfs interface can be registered with
> +	 * drm_debugfs_add_file(), and sysfs should be directly added on the
> +	 * hardwire struct device @dev.
> +	 */
>   	struct drm_minor *primary;
>   
> -	/** @render: Render node */
> +	/**
> +	 * @render:
> +	 *
> +	 * Render node. Drivers should not interact with this directly ever.
> +	 * Drivers should not expose any additional interfaces in debugfs or
> +	 * sysfs on thise node.

I believe you meant s/thise/this.

Apart from this small nit,

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra Canal

> +	 */
>   	struct drm_minor *render;
>   
>   	/**

Re: [PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

On 01/04, Daniel Vetter wrote:
> The documentation for struct drm_minor already states this, but that's
> not always that easy to find.
> 
> Also due to historical reasons we still have the minor-centric (like
> drm_debugfs_create_files), but since this is now getting fixed we can
> put a few more pointers in place as to how this should be done
> ideally.
> 
> Motvated by some discussion with Rodrigo on irc about how drm/xe
> should lay out its sysfs interfaces.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Wambui Karuga <wambui.karugax@gmail.com>
> Cc: Maíra Canal <mcanal@igalia.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Melissa Wen <mwen@igalia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  include/drm/drm_device.h | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..b40e07e004ee 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -87,10 +87,23 @@ struct drm_device {
>  	 */
>  	void *dev_private;
>  
> -	/** @primary: Primary node */
> +	/**
> +	 * @primary:
> +	 *
> +	 * Primary node. Drivers should not interact with this
> +	 * directly. debugfs interface can be registered with
> +	 * drm_debugfs_add_file(), and sysfs should be directly added on the
> +	 * hardwire struct device @dev.
> +	 */
>  	struct drm_minor *primary;
>  
> -	/** @render: Render node */
> +	/**
> +	 * @render:
> +	 *
> +	 * Render node. Drivers should not interact with this directly ever.
> +	 * Drivers should not expose any additional interfaces in debugfs or
> +	 * sysfs on thise node.
> +	 */
>  	struct drm_minor *render;
>  
>  	/**

yeah, with those typos fixed, LGTM.

Reviewed-by: Melissa Wen <mwen@igalia.com>

Thanks!

> -- 
> 2.37.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Rodrigo Vivi]

On Wed, Jan 04, 2023 at 10:17:54PM +0100, Daniel Vetter wrote:
> The documentation for struct drm_minor already states this, but that's
> not always that easy to find.
> 
> Also due to historical reasons we still have the minor-centric (like
> drm_debugfs_create_files), but since this is now getting fixed we can
> put a few more pointers in place as to how this should be done
> ideally.
> 
> Motvated by some discussion with Rodrigo on irc about how drm/xe
> should lay out its sysfs interfaces.

Thank you!

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Wambui Karuga <wambui.karugax@gmail.com>
> Cc: Maíra Canal <mcanal@igalia.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Melissa Wen <mwen@igalia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  include/drm/drm_device.h | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..b40e07e004ee 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -87,10 +87,23 @@ struct drm_device {
>  	 */
>  	void *dev_private;
>  
> -	/** @primary: Primary node */
> +	/**
> +	 * @primary:
> +	 *
> +	 * Primary node. Drivers should not interact with this
> +	 * directly. debugfs interface can be registered with
> +	 * drm_debugfs_add_file(), and sysfs should be directly added on the
> +	 * hardwire struct device @dev.
> +	 */
>  	struct drm_minor *primary;
>  
> -	/** @render: Render node */
> +	/**
> +	 * @render:
> +	 *
> +	 * Render node. Drivers should not interact with this directly ever.
> +	 * Drivers should not expose any additional interfaces in debugfs or
> +	 * sysfs on thise node.
> +	 */
>  	struct drm_minor *render;
>  
>  	/**
> -- 
> 2.37.2
> 

Re: [PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Jani Nikula]

On Wed, 04 Jan 2023, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The documentation for struct drm_minor already states this, but that's
> not always that easy to find.
>
> Also due to historical reasons we still have the minor-centric (like
> drm_debugfs_create_files), but since this is now getting fixed we can
> put a few more pointers in place as to how this should be done
> ideally.

Care to expand on the vague "this is now getting fixed" part a bit?

I've seen the "Introduce debugfs device-centered functions" series from
Maíra, but that doesn't solve everything. Not everything can use
drm_debugfs_add_file().

BR,
Jani.

>
> Motvated by some discussion with Rodrigo on irc about how drm/xe
> should lay out its sysfs interfaces.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Wambui Karuga <wambui.karugax@gmail.com>
> Cc: Maíra Canal <mcanal@igalia.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Melissa Wen <mwen@igalia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  include/drm/drm_device.h | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..b40e07e004ee 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -87,10 +87,23 @@ struct drm_device {
>  	 */
>  	void *dev_private;
>  
> -	/** @primary: Primary node */
> +	/**
> +	 * @primary:
> +	 *
> +	 * Primary node. Drivers should not interact with this
> +	 * directly. debugfs interface can be registered with
> +	 * drm_debugfs_add_file(), and sysfs should be directly added on the
> +	 * hardwire struct device @dev.
> +	 */
>  	struct drm_minor *primary;
>  
> -	/** @render: Render node */
> +	/**
> +	 * @render:
> +	 *
> +	 * Render node. Drivers should not interact with this directly ever.
> +	 * Drivers should not expose any additional interfaces in debugfs or
> +	 * sysfs on thise node.
> +	 */
>  	struct drm_minor *render;
>  
>  	/**

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Daniel Vetter]

On Thu, Jan 05, 2023 at 02:39:21PM +0200, Jani Nikula wrote:
> On Wed, 04 Jan 2023, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > The documentation for struct drm_minor already states this, but that's
> > not always that easy to find.
> >
> > Also due to historical reasons we still have the minor-centric (like
> > drm_debugfs_create_files), but since this is now getting fixed we can
> > put a few more pointers in place as to how this should be done
> > ideally.
> 
> Care to expand on the vague "this is now getting fixed" part a bit?
> 
> I've seen the "Introduce debugfs device-centered functions" series from
> Maíra, but that doesn't solve everything. Not everything can use
> drm_debugfs_add_file().

Yeah this is only step 1 of the entire project, there's still more in the
todo entry:

https://dri.freedesktop.org/docs/drm/gpu/todo.html#clean-up-the-debugfs-support

We need the same trick on the connector and crtc too. I think with those
we should have most things covered?
-Daniel

> 
> BR,
> Jani.
> 
> >
> > Motvated by some discussion with Rodrigo on irc about how drm/xe
> > should lay out its sysfs interfaces.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Wambui Karuga <wambui.karugax@gmail.com>
> > Cc: Maíra Canal <mcanal@igalia.com>
> > Cc: Maxime Ripard <maxime@cerno.tech>
> > Cc: Melissa Wen <mwen@igalia.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  include/drm/drm_device.h | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> > index 9923c7a6885e..b40e07e004ee 100644
> > --- a/include/drm/drm_device.h
> > +++ b/include/drm/drm_device.h
> > @@ -87,10 +87,23 @@ struct drm_device {
> >  	 */
> >  	void *dev_private;
> >  
> > -	/** @primary: Primary node */
> > +	/**
> > +	 * @primary:
> > +	 *
> > +	 * Primary node. Drivers should not interact with this
> > +	 * directly. debugfs interface can be registered with
> > +	 * drm_debugfs_add_file(), and sysfs should be directly added on the
> > +	 * hardwire struct device @dev.
> > +	 */
> >  	struct drm_minor *primary;
> >  
> > -	/** @render: Render node */
> > +	/**
> > +	 * @render:
> > +	 *
> > +	 * Render node. Drivers should not interact with this directly ever.
> > +	 * Drivers should not expose any additional interfaces in debugfs or
> > +	 * sysfs on thise node.
> > +	 */
> >  	struct drm_minor *render;
> >  
> >  	/**
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] drm: document better that drivers shouldn't use drm_minor directly

[Daniel Vetter]

The documentation for struct drm_minor already states this, but that's
not always that easy to find.

Also due to historical reasons we still have the minor-centric (like
drm_debugfs_create_files), but since this is now getting fixed we can
put a few more pointers in place as to how this should be done
ideally. Note that debugfs isn't there yet for all cases (debugfs
files on kms objects like crtc/connector aren't supported, neither
debugfs files with full fops), so the debugfs side of this is still
rather aspirational and more for new users than converting everything
existing. todo.rst covers the additional work needed already.

Motivated by some discussion with Rodrigo on irc about how drm/xe
should lay out its sysfs interfaces.

v2: Make the debugfs situation clearer in the commit message, but
don't elaborate more in the actual kerneldoc to avoid distracting from
the main message around sysfs (Jani)

Also fix some typos.

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Acked-by: Maxime Ripard <maxime@cerno.tech>
Acked-by: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Wambui Karuga <wambui.karugax@gmail.com>
Cc: Maíra Canal <mcanal@igalia.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Melissa Wen <mwen@igalia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/drm/drm_device.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index a68c6a312b46..7cf4afae2e79 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -87,10 +87,23 @@ struct drm_device {
 	 */
 	void *dev_private;
 
-	/** @primary: Primary node */
+	/**
+	 * @primary:
+	 *
+	 * Primary node. Drivers should not interact with this
+	 * directly. debugfs interfaces can be registered with
+	 * drm_debugfs_add_file(), and sysfs should be directly added on the
+	 * hardware (and not character device node) struct device @dev.
+	 */
 	struct drm_minor *primary;
 
-	/** @render: Render node */
+	/**
+	 * @render:
+	 *
+	 * Render node. Drivers should not interact with this directly ever.
+	 * Drivers should not expose any additional interfaces in debugfs or
+	 * sysfs on this node.
+	 */
 	struct drm_minor *render;
 
 	/** @accel: Compute Acceleration node */
-- 
2.39.0