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.
