by Laurent Pinchart

Maxime Ripard <[email protected]> writes:

> Now that the core has a drm format helper to tell if a format embeds an
> alpha component in it, let's use it.
>
> Cc: Eric Anholt <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>

Reviewed-by: Eric Anholt <[email protected]>

Attachments:

signature.asc (832.00 B)

2018-01-11 15:59:05

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 <[email protected]>
> > > > Cc: Eric Anholt <[email protected]>
> > > > Cc: Inki Dae <[email protected]>
> > > > Cc: Joonyoung Shim <[email protected]>
> > > > Cc: Kyungmin Park <[email protected]>
> > > > Cc: Laurent Pinchart <[email protected]>
> > > > Cc: Mark Yao <[email protected]>
> > > > Cc: Seung-Woo Kim <[email protected]>
> > > > Signed-off-by: Maxime Ripard <[email protected]>
> > > > ---
> > > > 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

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (3.93 kB)
signature.asc (833.00 B)
Download all attachments

2018-01-17 09:20:33

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH 06/19] drm/blend: Add a generic alpha property

On Thu, Jan 11, 2018 at 08:34:58PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Thursday, 11 January 2018 18:36:10 EET Daniel Vetter wrote:
> > On Thu, Jan 11, 2018 at 4:58 PM, Maxime Ripard wrote:
> > > On Tue, Jan 09, 2018 at 03:28:34PM +0100, Daniel Vetter wrote:
> > >> On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
> > >>> On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> > >>>> On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> > >>>>> Some drivers duplicate the logic to create a property to store a
> > >>>>> per-plane alpha.
> > >>>>>
> > >>>>> Let's create a helper in order to move that to the core.
> > >>>>>
> > >>>>> Cc: Boris Brezillon <[email protected]>
> > >>>>> Cc: Laurent Pinchart <[email protected]>
> > >>>>> Signed-off-by: Maxime Ripard <[email protected]>
> > >>>>
> > >>>> Do we have userspace for this?
> > >>>
> > >>> Wayland seems to be on its way to implement this, with ChromeOS using
> > >>> it:
> > >>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741
> > >>> .html
> > >>>
> > >>> and more specifically:
> > >>> https://chromium.googlesource.com/chromium/src/+/master/third_party/way
> > >>> land-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1
> > >>> .xml#118
> > >>
> > >> Yay, would be good to include these links in the patch description.
> > >> Really happy we're having a real standard now used by multiple people.
> > >
> > > I will.
> > >
> > >> > > Is encoding a fixed 0-255 range really the best idea?
> > >> >
> > >> > I don't really know, is there hardware or formats where there is more
> > >> > than 255? Or did you mean less than that?
> > >>
> > >> 30bit I'd assume wants more alpha. In the past we've done some
> > >> fixed-point stuff (e.g. for LUT), using the 0.0-1.0 float range. Using
> > >> that for the blend equation docs is also what I recommend (and that we
> > >> map from 0-255 to 0.0-1.0 logically). Ofc the hw might not do any of that
> > >> ... I think 0.16 fixed point, stored in a u16 is probably best. That's
> > >> what we're doing for gamma tables already, and that way drivers can
> > >> simply throw away the lower bits.
> > >
> > > But that would also break the two users of that property that won't be
> > > able to move to the generic property (with the same name) without
> > > breaking userspace. The point of that patch was to allow some code
> > > consolidation, and that would mean failing to do so here :/
> >
> > Let me try to clarify:
> > - We'll keep the exact existing property semantics with the 0-255
> > range for the userspace visible property.
> >
> > - But internally, in the decode value that we store into
> > drm_plane_state, we'll do the slightly more future proof thing with a
> > few more bits.
> >
> > That gives us the option of exposing those bits in the future without
> > having to change all the drivers again (which we have to do for this
> > series here already anyway, since the decoded value moves into
> > drm_plane_state from driver subclasses).
> >
> > Definitely not asking to break userspace here :-)
>
> As the property range is available to userspace, we could allow drivers to
> expose a driver-dependent number of bits for the alpha value without breaking
> anything. We can even require new drivers to expose 16 bits regardless of the
> number of bits that the hardware can handle, or we could keep that driver-
> specific.
>
> Please note, however, that U0.16 can only represent [0.0, 0.999984741...]
> while we need [0.0, 1.0]. As all the hardware I know map the full range of
> values ([0, 255] for 8-bit for instance) to [0.0, 1.0] I wouldn't pretend that
> the 16-bit value exposed to userspace is U0.16.

So this would involve not changing too much the current patch, but
instead extending the type from an u8 to an u16. Would that work for
you Daniel?

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (3.96 kB)
signature.asc (833.00 B)
Download all attachments

2018-01-17 09:30:09

by Daniel Vetter

[permalink] [raw]

Subject: Re: [PATCH 06/19] drm/blend: Add a generic alpha property

On Wed, Jan 17, 2018 at 10:20:24AM +0100, Maxime Ripard wrote:
> On Thu, Jan 11, 2018 at 08:34:58PM +0200, Laurent Pinchart wrote:
> > Hi Daniel,
> >
> > On Thursday, 11 January 2018 18:36:10 EET Daniel Vetter wrote:
> > > On Thu, Jan 11, 2018 at 4:58 PM, Maxime Ripard wrote:
> > > > On Tue, Jan 09, 2018 at 03:28:34PM +0100, Daniel Vetter wrote:
> > > >> On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
> > > >>> On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> > > >>>> On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> > > >>>>> Some drivers duplicate the logic to create a property to store a
> > > >>>>> per-plane alpha.
> > > >>>>>
> > > >>>>> Let's create a helper in order to move that to the core.
> > > >>>>>
> > > >>>>> Cc: Boris Brezillon <[email protected]>
> > > >>>>> Cc: Laurent Pinchart <[email protected]>
> > > >>>>> Signed-off-by: Maxime Ripard <[email protected]>
> > > >>>>
> > > >>>> Do we have userspace for this?
> > > >>>
> > > >>> Wayland seems to be on its way to implement this, with ChromeOS using
> > > >>> it:
> > > >>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741
> > > >>> .html
> > > >>>
> > > >>> and more specifically:
> > > >>> https://chromium.googlesource.com/chromium/src/+/master/third_party/way
> > > >>> land-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1
> > > >>> .xml#118
> > > >>
> > > >> Yay, would be good to include these links in the patch description.
> > > >> Really happy we're having a real standard now used by multiple people.
> > > >
> > > > I will.
> > > >
> > > >> > > Is encoding a fixed 0-255 range really the best idea?
> > > >> >
> > > >> > I don't really know, is there hardware or formats where there is more
> > > >> > than 255? Or did you mean less than that?
> > > >>
> > > >> 30bit I'd assume wants more alpha. In the past we've done some
> > > >> fixed-point stuff (e.g. for LUT), using the 0.0-1.0 float range. Using
> > > >> that for the blend equation docs is also what I recommend (and that we
> > > >> map from 0-255 to 0.0-1.0 logically). Ofc the hw might not do any of that
> > > >> ... I think 0.16 fixed point, stored in a u16 is probably best. That's
> > > >> what we're doing for gamma tables already, and that way drivers can
> > > >> simply throw away the lower bits.
> > > >
> > > > But that would also break the two users of that property that won't be
> > > > able to move to the generic property (with the same name) without
> > > > breaking userspace. The point of that patch was to allow some code
> > > > consolidation, and that would mean failing to do so here :/
> > >
> > > Let me try to clarify:
> > > - We'll keep the exact existing property semantics with the 0-255
> > > range for the userspace visible property.
> > >
> > > - But internally, in the decode value that we store into
> > > drm_plane_state, we'll do the slightly more future proof thing with a
> > > few more bits.
> > >
> > > That gives us the option of exposing those bits in the future without
> > > having to change all the drivers again (which we have to do for this
> > > series here already anyway, since the decoded value moves into
> > > drm_plane_state from driver subclasses).
> > >
> > > Definitely not asking to break userspace here :-)
> >
> > As the property range is available to userspace, we could allow drivers to
> > expose a driver-dependent number of bits for the alpha value without breaking
> > anything. We can even require new drivers to expose 16 bits regardless of the
> > number of bits that the hardware can handle, or we could keep that driver-
> > specific.
> >
> > Please note, however, that U0.16 can only represent [0.0, 0.999984741...]
> > while we need [0.0, 1.0]. As all the hardware I know map the full range of
> > values ([0, 255] for 8-bit for instance) to [0.0, 1.0] I wouldn't pretend that
> > the 16-bit value exposed to userspace is U0.16.
>
> So this would involve not changing too much the current patch, but
> instead extending the type from an u8 to an u16. Would that work for
> you Daniel?

Yeah, Laurent's clarification is what I've meant ... And hopefully it's
enough future-proofing that we don't need to redo this all again.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch