linux-kernel - Re: [PATCH 01/19] drm/fourcc: Add a function to tell if the format embeds alpha

Open Source and information security mailing list archives

Message-ID: <20180115154744.GA28269@arm.com>
Date:   Mon, 15 Jan 2018 15:47:44 +0000
From:   Ayan Halder <ayan.halder@....com>
To:     Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Seung-Woo Kim <sw0312.kim@...sung.com>,
        linux-kernel@...r.kernel.org,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Chen-Yu Tsai <wens@...e.org>, dri-devel@...ts.freedesktop.org,
        Daniel Vetter <daniel.vetter@...el.com>, thomas@...sch.nl,
        linux-arm-kernel@...ts.infradead.org,
        Mark Yao <mark.yao@...k-chips.com>
Subject: Re: [PATCH 01/19] drm/fourcc: Add a function to tell if the format
 embeds alpha

On Tue, Jan 09, 2018 at 02:28:33PM +0100, Maxime Ripard wrote:
> Hi Laurent,
>
> On Tue, Jan 09, 2018 at 02:29:58PM +0200, Laurent Pinchart wrote:
> > On Tuesday, 9 January 2018 12:56:20 EET Maxime Ripard wrote:
> > > There's a bunch of drivers that duplicate the same function to know if a
> > > particular format embeds an alpha component or not.
> > >
> > > Let's create a helper to avoid duplicating that logic.
> > >
> > > Cc: Boris Brezillon <boris.brezillon@...e-electrons.com>
> > > Cc: Eric Anholt <eric@...olt.net>
> > > Cc: Inki Dae <inki.dae@...sung.com>
> > > Cc: Joonyoung Shim <jy0922.shim@...sung.com>
> > > Cc: Kyungmin Park <kyungmin.park@...sung.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > Cc: Mark Yao <mark.yao@...k-chips.com>
> > > Cc: Seung-Woo Kim <sw0312.kim@...sung.com>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> > > ---
> > >  drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++-
> > >  include/drm/drm_fourcc.h     |  1 +-
> > >  2 files changed, 44 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > index 9c0152df45ad..6e6227d6a46b 100644
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -348,3 +348,46 @@ int drm_format_plane_height(int height, uint32_t
> > > format, int plane) return height / info->vsub;
> > >  }
> > >  EXPORT_SYMBOL(drm_format_plane_height);
> > > +
> > > +/**
> > > + * drm_format_has_alpha - get whether the format embeds an alpha component
> > > + * @format: pixel format (DRM_FORMAT_*)
> > > + *
> > > + * Returns:
> > > + * true if the format embeds an alpha component, false otherwise.
> > > + */
> > > +bool drm_format_has_alpha(uint32_t format)
> > > +{
> > > + switch (format) {
> > > + case DRM_FORMAT_ARGB4444:
> > > + case DRM_FORMAT_ABGR4444:
> > > + case DRM_FORMAT_RGBA4444:
> > > + case DRM_FORMAT_BGRA4444:
> > > + case DRM_FORMAT_ARGB1555:
> > > + case DRM_FORMAT_ABGR1555:
> > > + case DRM_FORMAT_RGBA5551:
> > > + case DRM_FORMAT_BGRA5551:
> > > + case DRM_FORMAT_ARGB8888:
> > > + case DRM_FORMAT_ABGR8888:
> > > + case DRM_FORMAT_RGBA8888:
> > > + case DRM_FORMAT_BGRA8888:
> > > + case DRM_FORMAT_ARGB2101010:
> > > + case DRM_FORMAT_ABGR2101010:
> > > + case DRM_FORMAT_RGBA1010102:
> > > + case DRM_FORMAT_BGRA1010102:
> > > + case DRM_FORMAT_AYUV:
> > > + case DRM_FORMAT_XRGB8888_A8:
> > > + case DRM_FORMAT_XBGR8888_A8:
> > > + case DRM_FORMAT_RGBX8888_A8:
> > > + case DRM_FORMAT_BGRX8888_A8:
> > > + case DRM_FORMAT_RGB888_A8:
> > > + case DRM_FORMAT_BGR888_A8:
> > > + case DRM_FORMAT_RGB565_A8:
> > > + case DRM_FORMAT_BGR565_A8:
> > > +         return true;
> > > +
> > > + default:
> > > +         return false;
> > > + }
> > > +}
> > > +EXPORT_SYMBOL(drm_format_has_alpha);
> >
> > How about adding the information to struct drm_format_info instead ?
> > drm_format_has_alpha() could then be implemented as
> >
> > bool drm_format_has_alpha(uint32_t format)
> > {
> >     const struct drm_format_info *info;
> >
> >     info = drm_format_info(format);
> >     return info ? info->has_alpha : false;
> > }
>
> I considered it, and wasn't too sure about if adding more fields to
> drm_format_info was ok. I can definitely do it that way.
Are you going to send an updated patch with the change mentioned here.
Or should I update my patch (https://patchwork.kernel.org/patch/10161023/)
and change the type of '.alpha' to boolean to denote if the color
format has an alpha channel or not.

> > although drivers should really use the drm_framebuffer::format field directly
> > in most cases, so the helper might not be needed at all.
>
> The drivers converted in my serie shouldn't be too hard to convert to
> use drm_format_info directly, so that can be removed as well.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives