| Message ID | b58c65b26e59dff7917d90846cb797a11ca09efa.1515494838.git-series.maxime.ripard@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/sun4i: Support more planes, zpos and plane-wide alpha | expand |
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__ */
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.
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
On Tue, Jan 16, 2018 at 09:17:24PM +0100, Maxime Ripard wrote: > 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. > Thanks for your response. I will hold on my changes (https://patchwork.kernel.org/patch/10161339/) as it depends on your patch. > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
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__ */
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(+)