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@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > --- > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
