| Message ID | 20230104211754.1967591-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: document better that drivers shouldn't use drm_minor directly | expand |
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
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; > > /**
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 >
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 >
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; > > /**
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
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; /**
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(-)