Re: [RFC PATCH 06/20] lib: Add video format information library

[Boris Brezillon]

On Thu, 21 Mar 2019 09:20:41 +0100
Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:

> Hi Boris,
> 
> On Wed, Mar 20, 2019 at 02:39:44PM +0100, Boris Brezillon wrote:
> > On Tue, 19 Mar 2019 22:57:11 +0100
> > Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> >  
> > > Move the DRM formats API to turn this into a more generic image formats API
> > > to be able to leverage it into some other places of the kernel, such as
> > > v4l2 drivers.
> > >
> > > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> > > ---
> > >  include/linux/image-formats.h | 240 +++++++++++-
> > >  lib/Kconfig                   |   7 +-
> > >  lib/Makefile                  |   3 +-
> > >  lib/image-formats-selftests.c | 326 +++++++++++++++-
> > >  lib/image-formats.c           | 760 +++++++++++++++++++++++++++++++++++-
> > >  5 files changed, 1336 insertions(+)
> > >  create mode 100644 include/linux/image-formats.h
> > >  create mode 100644 lib/image-formats-selftests.c
> > >  create mode 100644 lib/image-formats.c
> > >  
> >
> > [...]
> >  
> > > --- /dev/null
> > > +++ b/lib/image-formats.c
> > > @@ -0,0 +1,760 @@
> > > +#include <linux/bug.h>
> > > +#include <linux/image-formats.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/math64.h>
> > > +
> > > +#include <uapi/drm/drm_fourcc.h>
> > > +
> > > +static const struct image_format_info formats[] = {
> > > +	{  
> >
> > ...
> >  
> > > +	},
> > > +};
> > > +
> > > +#define __image_format_lookup(_field, _fmt)			\
> > > +	({							\
> > > +		const struct image_format_info *format = NULL;	\
> > > +		unsigned i;					\
> > > +								\
> > > +		for (i = 0; i < ARRAY_SIZE(formats); i++)	\
> > > +			if (formats[i]._field == _fmt)		\
> > > +				format = &formats[i];		\
> > > +								\
> > > +		format;						\
> > > +	})
> > > +
> > > +/**
> > > + * __image_format_drm_lookup - query information for a given format
> > > + * @drm: DRM fourcc pixel format (DRM_FORMAT_*)
> > > + *
> > > + * The caller should only pass a supported pixel format to this function.
> > > + *
> > > + * Returns:
> > > + * The instance of struct image_format_info that describes the pixel format, or
> > > + * NULL if the format is unsupported.
> > > + */
> > > +const struct image_format_info *__image_format_drm_lookup(u32 drm)
> > > +{
> > > +	return __image_format_lookup(drm_fmt, drm);
> > > +}
> > > +EXPORT_SYMBOL(__image_format_drm_lookup);
> > > +
> > > +/**
> > > + * image_format_drm_lookup - query information for a given format
> > > + * @drm: DRM fourcc pixel format (DRM_FORMAT_*)
> > > + *
> > > + * The caller should only pass a supported pixel format to this function.
> > > + * Unsupported pixel formats will generate a warning in the kernel log.
> > > + *
> > > + * Returns:
> > > + * The instance of struct image_format_info that describes the pixel format, or
> > > + * NULL if the format is unsupported.
> > > + */
> > > +const struct image_format_info *image_format_drm_lookup(u32 drm)
> > > +{
> > > +	const struct image_format_info *format;
> > > +
> > > +	format = __image_format_drm_lookup(drm);
> > > +
> > > +	WARN_ON(!format);
> > > +	return format;
> > > +}
> > > +EXPORT_SYMBOL(image_format_drm_lookup);  
> >
> > I think this function and the DRM formats table should be moved in
> > drivers/gpu/drm/drm_image_format.c since they are DRM specific. The
> > remaining functions can IMHO be placed in include/linux/image-formats.h
> > as static inline funcs. This way you can get rid of lib/image-formats.c
> > and the associated Kconfig entry.  
> 
> I'm not quite sure what you mean. The whole point of the series is to
> split out that table out of DRM so that we can use it in other places,
> so surely putting it back into DRM defeats the purpose?

Sorry, I hadn't read the patch series entirely when replying to this
email. I thought you were planning to create one table for DRM formats
and one for V4L ones and then just use the common infra to have a
generic image_format representation, hence my suggestion.