linux-kernel - Re: [PATCH 06/19] drm/blend: Add a generic alpha property

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180109135913.wgyfvdfdffigxcll@flea>
Date:   Tue, 9 Jan 2018 14:59:13 +0100
From:   Maxime Ripard <maxime.ripard@...e-electrons.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     dri-devel@...ts.freedesktop.org, Chen-Yu Tsai <wens@...e.org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        thomas@...sch.nl
Subject: Re: [PATCH 06/19] drm/blend: Add a generic alpha property

Hi Laurent,

On Tue, Jan 09, 2018 at 02:34:47PM +0200, Laurent Pinchart 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 <boris.brezillon@...e-electrons.com>
> > Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> > ---
> >  Documentation/gpu/kms-properties.csv |  2 +-
> >  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> >  drivers/gpu/drm/drm_atomic_helper.c  |  1 +-
> >  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
> >  include/drm/drm_blend.h              |  1 +-
> >  include/drm/drm_plane.h              |  6 +++++-
> >  6 files changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/kms-properties.csv
> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..a3c3969c1992
> > 100644
> > --- a/Documentation/gpu/kms-properties.csv
> > +++ b/Documentation/gpu/kms-properties.csv
> > @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD
> > ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> >  ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> >  ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> > -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> > +,,"""alpha""",RANGE,"Min=0, Max=255",Plane,Opacity of the plane from
> > transparent (0) to opaque (255) ,,"""colorkey""",RANGE,"Min=0,
> > Max=0x01ffffff",Plane,TBD
> 
> I think more documentation is needed. You should explain how the property 
> operates and which formats it is applicable to. For instance you need to 
> clarify what happens for format that contain an alpha component.

That's a very good point, and I'm not quite sure what should happen in
the first place :)

Should we forbid that configuration?

> >  /**
> > + * drm_plane_create_alpha_property - create a new alpha property
> > + * @plane: drm plane
> > + * @alpha: initial value of alpha, from 0 (transparent) to 255 (opaque)
> 
> Do you have a use case for initializing the alpha value to something else than 
> fully opaque ?

I don't, but thought it might be useful. Maybe it isn't, in which case
I'm totally fine with it getting away.

> > + * This function initializes a generic, mutable, alpha property and
> > + * enables support for it in the DRM core.
> > + *
> > + * Drivers can then attach this property to their plane to enable
> > + * support for configurable plane alpha.
> 
> The function attaches the property to the plane, is the
> documentation outdated ?

Yes, I'll fix it.

> > + * Returns:
> > + * 0 on success, negative error code on failure.
> > + */
> > +int drm_plane_create_alpha_property(struct drm_plane *plane, u8 alpha)
> > +{
> > +	struct drm_property *prop;
> > +
> > +	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, 255);
> 
> Do you think the 0-255 range will fit all use cases ?

This is kind of the same discussion we're having with Daniel, but are
there hardware or formats with an alpha component wider than 255?

Thanks!
Maxime

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)