Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[Sam Ravnborg <sam@ravnborg.org>]

On Tue, Feb 08, 2022 at 02:44:25AM +0200, Laurent Pinchart wrote:
> Hello,
> 
> On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote:
> > On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote:
> > >
> > > There is a number of bridge drivers that supports a single media bus
> > > format for DSI. Add a helper to avoid duplicating the code.
> > >
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++
> > >  include/drm/drm_atomic_helper.h     |  7 +++++
> > >  2 files changed, 48 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index a7a05e1e26bb..94f313dc196f 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
> > >         return input_fmts;
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
> > > +
> > > +/**
> > > + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
> > 
> > Is the description right? It's called "input" format but it defines an
> > output format?
> > 
> > 
> > > + * @bridge: bridge control structure
> > > + * @bridge_state: new bridge state
> > > + * @crtc_state: new CRTC state
> > > + * @conn_state: new connector state
> > > + * @output_fmt: tested output bus format
> > > + * @num_input_fmts: will contain the size of the returned array
> > 
> > Maybe indicate that it's always 1 in the comments?
> > 
> > > + *
> > > + * This helper is an implementation of the
> > > + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
> > > + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
> > > + *
> > > + * RETURNS
> > 
> > kernel-doc can't parse this return syntax and warns:
> > 
> > warning: No description found for return value of
> > 'drm_atomic_helper_bridge_dsi_input_bus_fmt'
> > 
> > 
> > > + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
> > > + * or NULL if the allocation failed
> > > + */
> > > +u32 *
> > > +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
> > > +                                          struct drm_bridge_state *bridge_state,
> > > +                                          struct drm_crtc_state *crtc_state,
> > > +                                          struct drm_connector_state *conn_state,
> > > +                                          u32 output_fmt,
> > > +                                          unsigned int *num_input_fmts)
> > > +{
> > > +       u32 *input_fmts;
> > > +
> > > +       *num_input_fmts = 0;
> > > +
> > > +       input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);
> > 
> > I probably wouldn't have bothered with `kcalloc` for something that's
> > always just one value and you're setting it. Why not just
> > 
> > input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> > 
> > ...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you
> > said it didn't compile?
> > 
> > Also: if it was common for others to want to provide fixed formats, I
> > wonder about adding a helper function that did most of the work here?
> > Dunno what it would be named since it's already a bit a of handful,
> > but I'd expect to call it like:
> > 
> > static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 };
> > return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)
> > 
> > Then my_neat_helper() could do kmemdup() on the array passed and fill
> > in "num_output_formats" to be either the array size of 0 (if the
> > kmemdup failed).
> 
> I quite like that approach. We could even have a wrapper macro that adds
> the ARRAY_SIZE() argument automatically.

Agree, will try to give that a spin.
And will then also process all the nice feedback from Douglas.

	Sam