diff mbox series

[v12,01/38] drm/doc: document recommended component helper usage

Message ID 1549696387-28268-2-git-send-email-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Implement HDCP2.2 | expand

Commit Message

Ramalingam C Feb. 9, 2019, 7:12 a.m. UTC 
From: Daniel Vetter <daniel.vetter@intel.com>

Now that component has docs it's worth spending a few words and
hyperlinks on recommended best practices in drm.

Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/driver-api/component.rst |  2 ++
 Documentation/gpu/drm-internals.rst    |  5 +++++
 drivers/gpu/drm/drm_drv.c              | 14 ++++++++++++++
 3 files changed, 21 insertions(+)

Comments

Daniel Vetter Feb. 11, 2019, 8:31 a.m. UTC | #1 
On Sat, Feb 09, 2019 at 12:42:30PM +0530, Ramalingam C wrote:
> From: Daniel Vetter <daniel.vetter@intel.com>
> 
> Now that component has docs it's worth spending a few words and
> hyperlinks on recommended best practices in drm.
> 
> Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Just a quick reminder: When sending out other people's patches, you need
to add your own s-o-b line per https://developercertificate.org/

Cheers, Daniel

> ---
>  Documentation/driver-api/component.rst |  2 ++
>  Documentation/gpu/drm-internals.rst    |  5 +++++
>  drivers/gpu/drm/drm_drv.c              | 14 ++++++++++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst
> index 2da4a8f20607..57e37590733f 100644
> --- a/Documentation/driver-api/component.rst
> +++ b/Documentation/driver-api/component.rst
> @@ -1,3 +1,5 @@
> +.. _component:
> +
>  ======================================
>  Component Helper for Aggregate Drivers
>  ======================================
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index 3ae23a5454ac..966bd2d9f0cc 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -93,6 +93,11 @@ Device Instance and Driver Handling
>  Driver Load
>  -----------
>  
> +Component Helper Usage
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_drv.c
> +   :doc: component helper usage recommendations
>  
>  IRQ Helper Library
>  ~~~~~~~~~~~~~~~~~~
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..aae413003705 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -457,6 +457,20 @@ static void drm_fs_inode_free(struct inode *inode)
>  }
>  
>  /**
> + * DOC: component helper usage recommendations
> + *
> + * DRM drivers that drive hardware where a logical device consists of a pile of
> + * independent hardware blocks are recommended to use the :ref:`component helper
> + * library<component>`. The entire device initialization procedure should be run
> + * from the &component_master_ops.master_bind callback, starting with
> + * drm_dev_init(), then binding all components with component_bind_all() and
> + * finishing with drm_dev_register(). For consistency and easier sharing of
> + * components across drivers the opaque pointer passed to all components through
> + * component_bind_all() should point at &struct drm_device of the device
> + * instance, not some driver specific private structure.
> + */
> +
> +/**
>   * drm_dev_init - Initialise new DRM device
>   * @dev: DRM device
>   * @driver: DRM driver
> -- 
> 2.7.4
>
Laurent Pinchart Feb. 12, 2019, 12:44 p.m. UTC | #2 
Hi Daniel,

Thank you for the patch.

On Sat, Feb 09, 2019 at 12:42:30PM +0530, Ramalingam C wrote:
> From: Daniel Vetter <daniel.vetter@intel.com>
> 
> Now that component has docs it's worth spending a few words and
> hyperlinks on recommended best practices in drm.
> 
> Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/driver-api/component.rst |  2 ++
>  Documentation/gpu/drm-internals.rst    |  5 +++++
>  drivers/gpu/drm/drm_drv.c              | 14 ++++++++++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst
> index 2da4a8f20607..57e37590733f 100644
> --- a/Documentation/driver-api/component.rst
> +++ b/Documentation/driver-api/component.rst
> @@ -1,3 +1,5 @@
> +.. _component:
> +
>  ======================================
>  Component Helper for Aggregate Drivers
>  ======================================
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index 3ae23a5454ac..966bd2d9f0cc 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -93,6 +93,11 @@ Device Instance and Driver Handling
>  Driver Load
>  -----------
>  
> +Component Helper Usage
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_drv.c
> +   :doc: component helper usage recommendations
>  
>  IRQ Helper Library
>  ~~~~~~~~~~~~~~~~~~
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..aae413003705 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -457,6 +457,20 @@ static void drm_fs_inode_free(struct inode *inode)
>  }
>  
>  /**
> + * DOC: component helper usage recommendations
> + *
> + * DRM drivers that drive hardware where a logical device consists of a pile of
> + * independent hardware blocks are recommended to use the :ref:`component helper
> + * library<component>`.

That's the first recommendation I would challenge :-) When using
drm_bridge and drm_panel, the component framework is not strictly
needed. The DRM/KMS probe function can defer probe when a bridge or
panel is missing. Only when two-way dependencies exist between the
display controller and the external components is the component
framework required.

> The entire device initialization procedure should be run
> + * from the &component_master_ops.master_bind callback, starting with
> + * drm_dev_init(), then binding all components with component_bind_all() and
> + * finishing with drm_dev_register().

The omapdss driver, for instance, performs hardware-specific
initialization of the DSS in the probe() handler itself (DT-related
parsing, memory mapping, IRQ registration, ...). I don't see why this
would need to move to the .master_bind() callback.

> For consistency and easier sharing of
> + * components across drivers the opaque pointer passed to all components through
> + * component_bind_all() should point at &struct drm_device of the device
> + * instance, not some driver specific private structure.

I'd say "shall" instead of "should" to make this a hard requirement. The
opaque pointer is something that I have never really liked in the
component framework as it hinders reusability. I have a few ideas on how
to fix that but haven't had time to try them out yet as the changes
would be quite intrusive. Long term major changes will be needed there
anyway as it makes no sense to have to frameworks with similar purposes
in DRM/KMS and V4L2.

> + */
> +
> +/**
>   * drm_dev_init - Initialise new DRM device
>   * @dev: DRM device
>   * @driver: DRM driver
Daniel Vetter Feb. 12, 2019, 12:52 p.m. UTC | #3 
On Tue, Feb 12, 2019 at 02:44:03PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Sat, Feb 09, 2019 at 12:42:30PM +0530, Ramalingam C wrote:
> > From: Daniel Vetter <daniel.vetter@intel.com>
> > 
> > Now that component has docs it's worth spending a few words and
> > hyperlinks on recommended best practices in drm.
> > 
> > Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  Documentation/driver-api/component.rst |  2 ++
> >  Documentation/gpu/drm-internals.rst    |  5 +++++
> >  drivers/gpu/drm/drm_drv.c              | 14 ++++++++++++++
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst
> > index 2da4a8f20607..57e37590733f 100644
> > --- a/Documentation/driver-api/component.rst
> > +++ b/Documentation/driver-api/component.rst
> > @@ -1,3 +1,5 @@
> > +.. _component:
> > +
> >  ======================================
> >  Component Helper for Aggregate Drivers
> >  ======================================
> > diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> > index 3ae23a5454ac..966bd2d9f0cc 100644
> > --- a/Documentation/gpu/drm-internals.rst
> > +++ b/Documentation/gpu/drm-internals.rst
> > @@ -93,6 +93,11 @@ Device Instance and Driver Handling
> >  Driver Load
> >  -----------
> >  
> > +Component Helper Usage
> > +~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_drv.c
> > +   :doc: component helper usage recommendations
> >  
> >  IRQ Helper Library
> >  ~~~~~~~~~~~~~~~~~~
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 381581b01d48..aae413003705 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -457,6 +457,20 @@ static void drm_fs_inode_free(struct inode *inode)
> >  }
> >  
> >  /**
> > + * DOC: component helper usage recommendations
> > + *
> > + * DRM drivers that drive hardware where a logical device consists of a pile of
> > + * independent hardware blocks are recommended to use the :ref:`component helper
> > + * library<component>`.
> 
> That's the first recommendation I would challenge :-) When using
> drm_bridge and drm_panel, the component framework is not strictly
> needed. The DRM/KMS probe function can defer probe when a bridge or
> panel is missing. Only when two-way dependencies exist between the
> display controller and the external components is the component
> framework required.

In the component docs themselves (just landed) we explicitly restrict
component to only the cases where there's no other subsytem specific way
to assemble the pile. I'll add that here, explicitly pointing out
panels/bridges.

> > The entire device initialization procedure should be run
> > + * from the &component_master_ops.master_bind callback, starting with
> > + * drm_dev_init(), then binding all components with component_bind_all() and
> > + * finishing with drm_dev_register().
> 
> The omapdss driver, for instance, performs hardware-specific
> initialization of the DSS in the probe() handler itself (DT-related
> parsing, memory mapping, IRQ registration, ...). I don't see why this
> would need to move to the .master_bind() callback.

It's a bit the odd one out then. Not sure whether there's a hard reason
for that or not.

> > For consistency and easier sharing of
> > + * components across drivers the opaque pointer passed to all components through
> > + * component_bind_all() should point at &struct drm_device of the device
> > + * instance, not some driver specific private structure.
> 
> I'd say "shall" instead of "should" to make this a hard requirement. The
> opaque pointer is something that I have never really liked in the
> component framework as it hinders reusability. I have a few ideas on how
> to fix that but haven't had time to try them out yet as the changes
> would be quite intrusive. Long term major changes will be needed there
> anyway as it makes no sense to have to frameworks with similar purposes
> in DRM/KMS and V4L2.

We have a pile of components around i915 (but not for the core drm_device)
which point at anything but drm_device. Otoh you'll never going to reuse
those pieces. I think anytime you want to reuse component drivers across
drivers you actually want a new subsystem (like drm_bridge/panel) with
dedicated lookup functions, and no void * pointers anywhere. I'd go as far
and say code reuse across drivers is mostly out of scope for component.
-Daniel

> 
> > + */
> > +
> > +/**
> >   * drm_dev_init - Initialise new DRM device
> >   * @dev: DRM device
> >   * @driver: DRM driver
> 
> -- 
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst
index 2da4a8f20607..57e37590733f 100644
--- a/Documentation/driver-api/component.rst
+++ b/Documentation/driver-api/component.rst
@@ -1,3 +1,5 @@ 
+.. _component:
+
 ======================================
 Component Helper for Aggregate Drivers
 ======================================
diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index 3ae23a5454ac..966bd2d9f0cc 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -93,6 +93,11 @@  Device Instance and Driver Handling
 Driver Load
 -----------
 
+Component Helper Usage
+~~~~~~~~~~~~~~~~~~~~~~
+
+.. kernel-doc:: drivers/gpu/drm/drm_drv.c
+   :doc: component helper usage recommendations
 
 IRQ Helper Library
 ~~~~~~~~~~~~~~~~~~
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 381581b01d48..aae413003705 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -457,6 +457,20 @@  static void drm_fs_inode_free(struct inode *inode)
 }
 
 /**
+ * DOC: component helper usage recommendations
+ *
+ * DRM drivers that drive hardware where a logical device consists of a pile of
+ * independent hardware blocks are recommended to use the :ref:`component helper
+ * library<component>`. The entire device initialization procedure should be run
+ * from the &component_master_ops.master_bind callback, starting with
+ * drm_dev_init(), then binding all components with component_bind_all() and
+ * finishing with drm_dev_register(). For consistency and easier sharing of
+ * components across drivers the opaque pointer passed to all components through
+ * component_bind_all() should point at &struct drm_device of the device
+ * instance, not some driver specific private structure.
+ */
+
+/**
  * drm_dev_init - Initialise new DRM device
  * @dev: DRM device
  * @driver: DRM driver