diff mbox

[01/19] drm/fourcc: Add a function to tell if the format embeds alpha

Message ID b58c65b26e59dff7917d90846cb797a11ca09efa.1515494838.git-series.maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard Jan. 9, 2018, 10:56 a.m. UTC
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@free-electrons.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mark Yao <mark.yao@rock-chips.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/drm_fourcc.c | 43 +++++++++++++++++++++++++++++++++++++-
 include/drm/drm_fourcc.h     |  1 +-
 2 files changed, 44 insertions(+)

Comments

Boris BREZILLON Jan. 9, 2018, 12:26 p.m. UTC | #1
On Tue,  9 Jan 2018 11:56:20 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> 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@free-electrons.com>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> Cc: Eric Anholt <eric@anholt.net>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mark Yao <mark.yao@rock-chips.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-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);
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 6942e84b6edd..e08fc22c5f78 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -69,5 +69,6 @@ int drm_format_vert_chroma_subsampling(uint32_t format);
>  int drm_format_plane_width(int width, uint32_t format, int plane);
>  int drm_format_plane_height(int height, uint32_t format, int plane);
>  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> +bool drm_format_has_alpha(uint32_t format);
>  
>  #endif /* __DRM_FOURCC_H__ */
Laurent Pinchart Jan. 9, 2018, 12:29 p.m. UTC | #2
Hi Maxime,

Thank you for the patch.

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@free-electrons.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mark Yao <mark.yao@rock-chips.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-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;
}

although drivers should really use the drm_framebuffer::format field directly 
in most cases, so the helper might not be needed at all.

> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 6942e84b6edd..e08fc22c5f78 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -69,5 +69,6 @@ int drm_format_vert_chroma_subsampling(uint32_t format);
>  int drm_format_plane_width(int width, uint32_t format, int plane);
>  int drm_format_plane_height(int height, uint32_t format, int plane);
>  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
> *buf);
> +bool drm_format_has_alpha(uint32_t format);
> 
>  #endif /* __DRM_FOURCC_H__ */
Maxime Ripard Jan. 9, 2018, 1:28 p.m. UTC | #3
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@free-electrons.com>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mark Yao <mark.yao@rock-chips.com>
> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-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.

> 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
Ayan Halder Jan. 15, 2018, 3:47 p.m. UTC | #4
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@free-electrons.com>
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Cc: Inki Dae <inki.dae@samsung.com>
> > > Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> > > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Mark Yao <mark.yao@rock-chips.com>
> > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-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@lists.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.
Ayan Halder Jan. 16, 2018, 10:37 a.m. UTC | #5
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@free-electrons.com>
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Cc: Inki Dae <inki.dae@samsung.com>
> > > Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> > > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Mark Yao <mark.yao@rock-chips.com>
> > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Maxime Ripard Jan. 16, 2018, 8:17 p.m. UTC | #6
Hi Ayan,

On Mon, Jan 15, 2018 at 03:47:44PM +0000, Ayan Halder wrote:
> On Tue, Jan 09, 2018 at 02:28:33PM +0100, Maxime Ripard wrote:
> > 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@free-electrons.com>
> > > > Cc: Eric Anholt <eric@anholt.net>
> > > > Cc: Inki Dae <inki.dae@samsung.com>
> > > > Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> > > > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Mark Yao <mark.yao@rock-chips.com>
> > > > Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-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.

Yes, I already had those patches ready. I'm just waiting for the
discussion on the alpha property to settle before sending a v2.

Maxime
diff mbox

Patch

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);
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 6942e84b6edd..e08fc22c5f78 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -69,5 +69,6 @@  int drm_format_vert_chroma_subsampling(uint32_t format);
 int drm_format_plane_width(int width, uint32_t format, int plane);
 int drm_format_plane_height(int height, uint32_t format, int plane);
 const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
+bool drm_format_has_alpha(uint32_t format);
 
 #endif /* __DRM_FOURCC_H__ */