Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-03-06 Thread Stefan Schake 
Hey,

On Wed, Feb 21, 2018 at 2:04 PM, Maxime Ripard
 wrote:
>
> What is the behaviour of the tegra engine when it has both a
> pixel-alpha and a plane-alpha?
>
> Atmel at least will drop one (but I'm not sure which one anymore).

Sorry, I have no more on the Tegra beyond that commit. But I did have some
more play time with drm_hwcomposer and from the rendering, it certainly
expects both to apply at the same time. To further complicate it, from
what I can tell with the VC4 HVS, you can actually configure it to
1) use only pixel alpha 2) use only plane alpha or 3) mix both pixel
and plane alpha, with 3 being what drm_hwc seems to want.

I think once writeback lands it would be a good idea to have some tests
that establish DRM plane blending standards beyond text in docs :)

Regards,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-03-05 Thread Daniel Vetter 
On Mon, Mar 05, 2018 at 12:08:12PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday, 5 March 2018 10:58:41 EET Daniel Vetter wrote:
> > On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
> > > On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> > >> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> > >>> On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
> >  On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> > > Some drivers duplicate the logic to create a property to store a
> > > per-plane alpha.
> > > 
> > > This is especially useful if we ever want to support extra
> > > protocols for Wayland like:
> > > https://lists.freedesktop.org/archives/wayland-devel/2017-August/03
> > > 4741.html
> > > 
> > > Let's create a helper in order to move that to the core.
> > > 
> > > Cc: Laurent Pinchart 
> > > Reviewed-by: Boris Brezillon 
> > > Signed-off-by: Maxime Ripard 
> > > ---
> > > 
> > >  Documentation/gpu/kms-properties.csv |  2 +-
> > >  drivers/gpu/drm/drm_atomic.c |  4 -
> > >  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> > >  drivers/gpu/drm/drm_blend.c  | 32 +++-
> > >  include/drm/drm_blend.h  |  1 +-
> > >  include/drm/drm_plane.h  |  6 +-
> > >  6 files changed, 48 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/gpu/kms-properties.csv
> > > b/Documentation/gpu/kms-properties.csv index
> > > 927b65e14219..25ad3503d663
> > > 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=Driver dependant",Plane,Opacity of
> > > the plane from transparent (0) to fully opaque (MAX). If this
> > > property is set to a value different than max, and that the pixel
> > > will define an alpha component, the property will have precendance
> > > and the pixel value will be ignored.
> > >> 
> > >> Please don't document new properties in that csv file, it's an
> > >> unreadable mess. Instead follow how we document standardized
> > >> properties nowadays in full-blown sections. For plane blending we
> > >> have:
> > >> 
> > >> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition->
> > >>  >> properties
> > > 
> > > Ack
> > > 
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > index 8185e3468a23..5a6f29524f12 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> > >   * plane (in 16.16)
> > >   * @src_w: width of visible portion of plane (in 16.16)
> > >   * @src_h: height of visible portion of plane (in 16.16)
> > > + * @alpha: opacity of the plane
> > >   * @rotation: rotation of the plane
> > >   * @zpos: priority of the given plane on crtc (optional)
> > >   * Note that multiple active planes on the same crtc can have an
> > >   identical
> > > @@ -105,6 +106,9 @@ struct drm_plane_state {
> > > uint32_t src_x, src_y;
> > > uint32_t src_h, src_w;
> > > 
> > > +   /* Plane opacity */
> > > +   u8 alpha;
> >  
> >  We may want to make that u16. The general we expect 16bpc for most
> >  color related things, but since this is a range prop I suppose we
> >  should just expose the actual hardware range. But making it u16 might
> >  avoid some head scratching for the first person to have hardware with
> >  higher precision. Either that or we should make the prop creation
> >  fail if the driver asks for more bits than we have in the state.
> > >>> 
> > >>> I'm tempted to go one step further and always make the alpha property
> > >>> 16-bits wide for new users (we can't do so for existing users as it
> > >>> could break userspace), and let drivers convert that internally to
> > >>> the range they need. There could however be drawbacks I don't
> > >>> foresee.
> > >> 
> > >> I think scaling the range to match the hw is the most sensible (yes
> > >> I'm flip-flopping around here). And once someone needs more than u8,
> > >> we can extend the internal representation easily. The external
> > >> representation in the property is an u64, that /should/ be enough for
> > >> the next few years :-)
> > > 
> > > Just

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-03-05 Thread Laurent Pinchart 
Hi Daniel,

On Monday, 5 March 2018 10:58:41 EET Daniel Vetter wrote:
> On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
> > On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> >> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> >>> On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
>  On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> > Some drivers duplicate the logic to create a property to store a
> > per-plane alpha.
> > 
> > This is especially useful if we ever want to support extra
> > protocols for Wayland like:
> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/03
> > 4741.html
> > 
> > Let's create a helper in order to move that to the core.
> > 
> > Cc: Laurent Pinchart 
> > Reviewed-by: Boris Brezillon 
> > Signed-off-by: Maxime Ripard 
> > ---
> > 
> >  Documentation/gpu/kms-properties.csv |  2 +-
> >  drivers/gpu/drm/drm_atomic.c |  4 -
> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> >  drivers/gpu/drm/drm_blend.c  | 32 +++-
> >  include/drm/drm_blend.h  |  1 +-
> >  include/drm/drm_plane.h  |  6 +-
> >  6 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/kms-properties.csv
> > b/Documentation/gpu/kms-properties.csv index
> > 927b65e14219..25ad3503d663
> > 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=Driver dependant",Plane,Opacity of
> > the plane from transparent (0) to fully opaque (MAX). If this
> > property is set to a value different than max, and that the pixel
> > will define an alpha component, the property will have precendance
> > and the pixel value will be ignored.
> >> 
> >> Please don't document new properties in that csv file, it's an
> >> unreadable mess. Instead follow how we document standardized
> >> properties nowadays in full-blown sections. For plane blending we
> >> have:
> >> 
> >> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-> 
> >> >> properties
> > 
> > Ack
> > 
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 8185e3468a23..5a6f29524f12 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >   * plane (in 16.16)
> >   * @src_w: width of visible portion of plane (in 16.16)
> >   * @src_h: height of visible portion of plane (in 16.16)
> > + * @alpha: opacity of the plane
> >   * @rotation: rotation of the plane
> >   * @zpos: priority of the given plane on crtc (optional)
> >   * Note that multiple active planes on the same crtc can have an
> >   identical
> > @@ -105,6 +106,9 @@ struct drm_plane_state {
> > uint32_t src_x, src_y;
> > uint32_t src_h, src_w;
> > 
> > +   /* Plane opacity */
> > +   u8 alpha;
>  
>  We may want to make that u16. The general we expect 16bpc for most
>  color related things, but since this is a range prop I suppose we
>  should just expose the actual hardware range. But making it u16 might
>  avoid some head scratching for the first person to have hardware with
>  higher precision. Either that or we should make the prop creation
>  fail if the driver asks for more bits than we have in the state.
> >>> 
> >>> I'm tempted to go one step further and always make the alpha property
> >>> 16-bits wide for new users (we can't do so for existing users as it
> >>> could break userspace), and let drivers convert that internally to
> >>> the range they need. There could however be drawbacks I don't
> >>> foresee.
> >> 
> >> I think scaling the range to match the hw is the most sensible (yes
> >> I'm flip-flopping around here). And once someone needs more than u8,
> >> we can extend the internal representation easily. The external
> >> representation in the property is an u64, that /should/ be enough for
> >> the next few years :-)
> > 
> > Just to make sure we're on the same page, you want to keep the u8, and
> > if the hardware uses say an u16, the driver for that hardware will do
> > the upscaling?
> 
> The idea is that we'd set the u16 limit in the property and so inform
> userspace that a different

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-03-05 Thread Daniel Vetter 
On Wed, Feb 21, 2018 at 10:39:02PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday, 19 February 2018 23:58:40 EET Daniel Vetter wrote:
> > On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> > > On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
> > >> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> > >> Some drivers duplicate the logic to create a property to store a
> > >>> per-plane alpha.
> > >>> 
> > >>> This is especially useful if we ever want to support extra protocols
> > >>> for Wayland like:
> > >>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741
> > >>> .html
> > >>> 
> > >>> Let's create a helper in order to move that to the core.
> > >>> 
> > >>> Cc: Laurent Pinchart 
> > >>> Reviewed-by: Boris Brezillon 
> > >>> Signed-off-by: Maxime Ripard 
> > >>> ---
> > >>> 
> > >>>  Documentation/gpu/kms-properties.csv |  2 +-
> > >>>  drivers/gpu/drm/drm_atomic.c |  4 -
> > >>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> > >>>  drivers/gpu/drm/drm_blend.c  | 32 -
> > >>>  include/drm/drm_blend.h  |  1 +-
> > >>>  include/drm/drm_plane.h  |  6 +-
> > >>>  6 files changed, 48 insertions(+), 1 deletion(-)
> > >>> 
> > >>> diff --git a/Documentation/gpu/kms-properties.csv
> > >>> b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> > >>> 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=Driver dependant",Plane,Opacity of the
> > >>> plane from transparent (0) to fully opaque (MAX). If this property is
> > >>> set to a value different than max, and that the pixel will define an
> > >>> alpha component, the property will have precendance and the pixel value
> > >>> will be ignored.
> > 
> > Please don't document new properties in that csv file, it's an
> > unreadable mess. Instead follow how we document standardized
> > properties nowadays in full-blown sections. For plane blending we
> > have:
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop
> > erties
> > 
> > >> Those semantics don't seem particularly good to me. I think we would want
> > >> the per-pixel alpha and global alpha both to be effecive at the same
> > >> time. You can always decide to ignore the per-pixel alpha by using a
> > >> pixel format without alpha.
> > > 
> > > That makes sense to me. However, it also brings a new question: how should
> > > a driver that supports either global alpha or pixel alpha but not both
> > > signal that to userspace, and how should it reacts when userspace selects
> > > a format with an alpha channel and set a global alpha value other than
> > > fully opaque ? To make things more complex, note that some drivers
> > > support combining global alpha and pixel alpha only for a subset of the
> > > formats with an alpha channel (for instance for ARGB 1555 formats, but
> > > not for ARGB  formats).
> > 
> > atomic_check can reject unsupported configs. Userspace needs to fall
> > back somehow (either switch to xrgb or make alpha fully opaque or just
> > give up on that plane). We have a lot of such corner-cases we don't
> > tell userspace about explicitly at all.
> 
> I'm OK with failing the commit in case in invalid configuration is requested. 
> However, using a check-only commit to find out whether combining global alpha 
> and pixel alpha is supported doesn't seem a good idea to me. First of all 
> userspace would need to try that for all formats, making it cumbersome. Then, 
> an atomic commit is a black box, we don't report the failure cause to 
> userspace. It makes it hard to use it to test support for a feature as it 
> could fail for an unrelated reason. Finally, we'd open the door to various 
> kind of heuristics implemented differently in different userspace stacks, and 
> that would increase the risk of breaking userspace. I'd rather have an 
> explicitly documented way to perform such checks.

I am _not_ against more explicit checks. I only object against them in the
name of "future proofing" without a solid pile of different driver and
userspace implementations as demonstration vehicles that the hints are
actually useful and needed by userspace.

DRM history is full of fake-generic uapi that turned out to be useful for
exactly the 1 driver/userspace combo it was originally implemented for,
and frankly for

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-03-05 Thread Daniel Vetter 
On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
> On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> > On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart
> >  wrote:
> > > Hi Ville,
> > >
> > > On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
> > >> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> > >> > Some drivers duplicate the logic to create a property to store a 
> > >> > per-plane
> > >> > alpha.
> > >> >
> > >> > This is especially useful if we ever want to support extra protocols 
> > >> > for
> > >> > Wayland like:
> > >> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht
> > >> > ml
> > >> >
> > >> > Let's create a helper in order to move that to the core.
> > >> >
> > >> > Cc: Laurent Pinchart 
> > >> > Reviewed-by: Boris Brezillon 
> > >> > Signed-off-by: Maxime Ripard 
> > >> > ---
> > >> >
> > >> >  Documentation/gpu/kms-properties.csv |  2 +-
> > >> >  drivers/gpu/drm/drm_atomic.c |  4 -
> > >> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> > >> >  drivers/gpu/drm/drm_blend.c  | 32 
> > >> > +-
> > >> >  include/drm/drm_blend.h  |  1 +-
> > >> >  include/drm/drm_plane.h  |  6 +-
> > >> >  6 files changed, 48 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/Documentation/gpu/kms-properties.csv
> > >> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> > >> > 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=Driver dependant",Plane,Opacity of the
> > >> > plane from transparent (0) to fully opaque (MAX). If this property is 
> > >> > set
> > >> > to a value different than max, and that the pixel will define an alpha
> > >> > component, the property will have precendance and the pixel value will 
> > >> > be
> > >> > ignored.
> > 
> > Please don't document new properties in that csv file, it's an
> > unreadable mess. Instead follow how we document standardized
> > properties nowadays in full-blown sections. For plane blending we
> > have:
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties
> 
> Ack
> 
> > >> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > >> > index 8185e3468a23..5a6f29524f12 100644
> > >> > --- a/include/drm/drm_plane.h
> > >> > +++ b/include/drm/drm_plane.h
> > >> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> > >> >   * plane (in 16.16)
> > >> >   * @src_w: width of visible portion of plane (in 16.16)
> > >> >   * @src_h: height of visible portion of plane (in 16.16)
> > >> > + * @alpha: opacity of the plane
> > >> >   * @rotation: rotation of the plane
> > >> >   * @zpos: priority of the given plane on crtc (optional)
> > >> >   * Note that multiple active planes on the same crtc can have an
> > >> >   identical
> > >> > @@ -105,6 +106,9 @@ struct drm_plane_state {
> > >> > uint32_t src_x, src_y;
> > >> > uint32_t src_h, src_w;
> > >> >
> > >> > +   /* Plane opacity */
> > >> > +   u8 alpha;
> > >>
> > >> We may want to make that u16. The general we expect 16bpc for most color
> > >> related things, but since this is a range prop I suppose we should just
> > >> expose the actual hardware range. But making it u16 might avoid some head
> > >> scratching for the first person to have hardware with higher precision.
> > >> Either that or we should make the prop creation fail if the driver asks
> > >> for more bits than we have in the state.
> > >
> > > I'm tempted to go one step further and always make the alpha property 
> > > 16-bits
> > > wide for new users (we can't do so for existing users as it could break
> > > userspace), and let drivers convert that internally to the range they 
> > > need.
> > > There could however be drawbacks I don't foresee.
> > 
> > I think scaling the range to match the hw is the most sensible (yes
> > I'm flip-flopping around here). And once someone needs more than u8,
> > we can extend the internal representation easily. The external
> > representation in the property is an u64, that /should/ be enough for
> > the next few years :-)
> 
> Just to make sure we're on the same page, you want to keep the u8, and
> if the hardware uses say an u16, the driver for that hardware will do
> the upscaling?

The idea is that we'd set the u16 limit in the property and

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-21 Thread Laurent Pinchart 
Hi Daniel,

On Monday, 19 February 2018 23:58:40 EET Daniel Vetter wrote:
> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> > On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
> >> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> >> Some drivers duplicate the logic to create a property to store a
> >>> per-plane alpha.
> >>> 
> >>> This is especially useful if we ever want to support extra protocols
> >>> for Wayland like:
> >>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741
> >>> .html
> >>> 
> >>> Let's create a helper in order to move that to the core.
> >>> 
> >>> Cc: Laurent Pinchart 
> >>> Reviewed-by: Boris Brezillon 
> >>> Signed-off-by: Maxime Ripard 
> >>> ---
> >>> 
> >>>  Documentation/gpu/kms-properties.csv |  2 +-
> >>>  drivers/gpu/drm/drm_atomic.c |  4 -
> >>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> >>>  drivers/gpu/drm/drm_blend.c  | 32 -
> >>>  include/drm/drm_blend.h  |  1 +-
> >>>  include/drm/drm_plane.h  |  6 +-
> >>>  6 files changed, 48 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/Documentation/gpu/kms-properties.csv
> >>> b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> >>> 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=Driver dependant",Plane,Opacity of the
> >>> plane from transparent (0) to fully opaque (MAX). If this property is
> >>> set to a value different than max, and that the pixel will define an
> >>> alpha component, the property will have precendance and the pixel value
> >>> will be ignored.
> 
> Please don't document new properties in that csv file, it's an
> unreadable mess. Instead follow how we document standardized
> properties nowadays in full-blown sections. For plane blending we
> have:
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop
> erties
> 
> >> Those semantics don't seem particularly good to me. I think we would want
> >> the per-pixel alpha and global alpha both to be effecive at the same
> >> time. You can always decide to ignore the per-pixel alpha by using a
> >> pixel format without alpha.
> > 
> > That makes sense to me. However, it also brings a new question: how should
> > a driver that supports either global alpha or pixel alpha but not both
> > signal that to userspace, and how should it reacts when userspace selects
> > a format with an alpha channel and set a global alpha value other than
> > fully opaque ? To make things more complex, note that some drivers
> > support combining global alpha and pixel alpha only for a subset of the
> > formats with an alpha channel (for instance for ARGB 1555 formats, but
> > not for ARGB  formats).
> 
> atomic_check can reject unsupported configs. Userspace needs to fall
> back somehow (either switch to xrgb or make alpha fully opaque or just
> give up on that plane). We have a lot of such corner-cases we don't
> tell userspace about explicitly at all.

I'm OK with failing the commit in case in invalid configuration is requested. 
However, using a check-only commit to find out whether combining global alpha 
and pixel alpha is supported doesn't seem a good idea to me. First of all 
userspace would need to try that for all formats, making it cumbersome. Then, 
an atomic commit is a black box, we don't report the failure cause to 
userspace. It makes it hard to use it to test support for a feature as it 
could fail for an unrelated reason. Finally, we'd open the door to various 
kind of heuristics implemented differently in different userspace stacks, and 
that would increase the risk of breaking userspace. I'd rather have an 
explicitly documented way to perform such checks.

> >> Also, where's the userspace that wants this feature?
> >> 
> >> 
> >> 
> >>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >>> index 8185e3468a23..5a6f29524f12 100644
> >>> --- a/include/drm/drm_plane.h
> >>> +++ b/include/drm/drm_plane.h
> >>> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >>>   * plane (in 16.16)
> >>>   * @src_w: width of visible portion of plane (in 16.16)
> >>>   * @src_h: height of visible portion of plane (in 16.16)
> >>> + * @alpha: opacity of the plane
> >>>   * @rotation: rotation of the plane
> >>>   * @zpos: priority of the given plane on crtc (optional)
> >>>   * Note that multiple active

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-21 Thread Maxime Ripard 
On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart
>  wrote:
> > Hi Ville,
> >
> > On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
> >> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> >> > Some drivers duplicate the logic to create a property to store a 
> >> > per-plane
> >> > alpha.
> >> >
> >> > This is especially useful if we ever want to support extra protocols for
> >> > Wayland like:
> >> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht
> >> > ml
> >> >
> >> > Let's create a helper in order to move that to the core.
> >> >
> >> > Cc: Laurent Pinchart 
> >> > Reviewed-by: Boris Brezillon 
> >> > Signed-off-by: Maxime Ripard 
> >> > ---
> >> >
> >> >  Documentation/gpu/kms-properties.csv |  2 +-
> >> >  drivers/gpu/drm/drm_atomic.c |  4 -
> >> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> >> >  drivers/gpu/drm/drm_blend.c  | 32 +-
> >> >  include/drm/drm_blend.h  |  1 +-
> >> >  include/drm/drm_plane.h  |  6 +-
> >> >  6 files changed, 48 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/Documentation/gpu/kms-properties.csv
> >> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> >> > 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=Driver dependant",Plane,Opacity of the
> >> > plane from transparent (0) to fully opaque (MAX). If this property is set
> >> > to a value different than max, and that the pixel will define an alpha
> >> > component, the property will have precendance and the pixel value will be
> >> > ignored.
> 
> Please don't document new properties in that csv file, it's an
> unreadable mess. Instead follow how we document standardized
> properties nowadays in full-blown sections. For plane blending we
> have:
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

Ack

> >> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> > index 8185e3468a23..5a6f29524f12 100644
> >> > --- a/include/drm/drm_plane.h
> >> > +++ b/include/drm/drm_plane.h
> >> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >> >   * plane (in 16.16)
> >> >   * @src_w: width of visible portion of plane (in 16.16)
> >> >   * @src_h: height of visible portion of plane (in 16.16)
> >> > + * @alpha: opacity of the plane
> >> >   * @rotation: rotation of the plane
> >> >   * @zpos: priority of the given plane on crtc (optional)
> >> >   * Note that multiple active planes on the same crtc can have an
> >> >   identical
> >> > @@ -105,6 +106,9 @@ struct drm_plane_state {
> >> > uint32_t src_x, src_y;
> >> > uint32_t src_h, src_w;
> >> >
> >> > +   /* Plane opacity */
> >> > +   u8 alpha;
> >>
> >> We may want to make that u16. The general we expect 16bpc for most color
> >> related things, but since this is a range prop I suppose we should just
> >> expose the actual hardware range. But making it u16 might avoid some head
> >> scratching for the first person to have hardware with higher precision.
> >> Either that or we should make the prop creation fail if the driver asks
> >> for more bits than we have in the state.
> >
> > I'm tempted to go one step further and always make the alpha property 
> > 16-bits
> > wide for new users (we can't do so for existing users as it could break
> > userspace), and let drivers convert that internally to the range they need.
> > There could however be drawbacks I don't foresee.
> 
> I think scaling the range to match the hw is the most sensible (yes
> I'm flip-flopping around here). And once someone needs more than u8,
> we can extend the internal representation easily. The external
> representation in the property is an u64, that /should/ be enough for
> the next few years :-)

Just to make sure we're on the same page, you want to keep the u8, and
if the hardware uses say an u16, the driver for that hardware will do
the upscaling?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-21 Thread Maxime Ripard 
Hi,

On Tue, Feb 20, 2018 at 04:10:28PM +0100, Stefan Schake wrote:
> On Fri, Feb 16, 2018 at 7:20 PM, Ville Syrjälä
>  wrote:
> > On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> >> Some drivers duplicate the logic to create a property to store a per-plane
> >> alpha.
> >>
> >> This is especially useful if we ever want to support extra protocols for
> >> Wayland like:
> >> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
> >>
> >> Let's create a helper in order to move that to the core.
> >>
> >> Cc: Laurent Pinchart 
> >> Reviewed-by: Boris Brezillon 
> >> Signed-off-by: Maxime Ripard 
> >> ---
> >>  Documentation/gpu/kms-properties.csv |  2 +-
> >>  drivers/gpu/drm/drm_atomic.c |  4 -
> >>  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> >>  drivers/gpu/drm/drm_blend.c  | 32 +-
> >>  include/drm/drm_blend.h  |  1 +-
> >>  include/drm/drm_plane.h  |  6 +-
> >>  6 files changed, 48 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/gpu/kms-properties.csv 
> >> b/Documentation/gpu/kms-properties.csv
> >> index 927b65e14219..25ad3503d663 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=Driver dependant",Plane,Opacity of the 
> >> plane from transparent (0) to fully opaque (MAX). If this property is set 
> >> to a value different than max, and that the pixel will define an alpha 
> >> component, the property will have precendance and the pixel value will be 
> >> ignored.
> >
> > Those semantics don't seem particularly good to me. I think we would want 
> > the
> > per-pixel alpha and global alpha both to be effecive at the same time. You 
> > can
> > always decide to ignore the per-pixel alpha by using a pixel format without
> > alpha.
> >
> > Also, where's the userspace that wants this feature?
> 
> drm_hwcomposer uses an 8-bit per-plane alpha property, and from what I
> can tell the semantics are that both pixel and plane alpha apply
> simultaneously if present.
> Here is what I think was the kernel-side for this:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306122
> 
> I've added Sean Paul, he might be able to give a more definitive answer.

What is the behaviour of the tegra engine when it has both a
pixel-alpha and a plane-alpha?

Atmel at least will drop one (but I'm not sure which one anymore).

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-20 Thread Stefan Schake 
On Fri, Feb 16, 2018 at 7:20 PM, Ville Syrjälä
 wrote:
> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
>> Some drivers duplicate the logic to create a property to store a per-plane
>> alpha.
>>
>> This is especially useful if we ever want to support extra protocols for
>> Wayland like:
>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
>>
>> Let's create a helper in order to move that to the core.
>>
>> Cc: Laurent Pinchart 
>> Reviewed-by: Boris Brezillon 
>> Signed-off-by: Maxime Ripard 
>> ---
>>  Documentation/gpu/kms-properties.csv |  2 +-
>>  drivers/gpu/drm/drm_atomic.c |  4 -
>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
>>  drivers/gpu/drm/drm_blend.c  | 32 +-
>>  include/drm/drm_blend.h  |  1 +-
>>  include/drm/drm_plane.h  |  6 +-
>>  6 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/gpu/kms-properties.csv 
>> b/Documentation/gpu/kms-properties.csv
>> index 927b65e14219..25ad3503d663 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=Driver dependant",Plane,Opacity of the 
>> plane from transparent (0) to fully opaque (MAX). If this property is set to 
>> a value different than max, and that the pixel will define an alpha 
>> component, the property will have precendance and the pixel value will be 
>> ignored.
>
> Those semantics don't seem particularly good to me. I think we would want the
> per-pixel alpha and global alpha both to be effecive at the same time. You can
> always decide to ignore the per-pixel alpha by using a pixel format without
> alpha.
>
> Also, where's the userspace that wants this feature?

drm_hwcomposer uses an 8-bit per-plane alpha property, and from what I
can tell the semantics are that both pixel and plane alpha apply
simultaneously if present.
Here is what I think was the kernel-side for this:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306122

I've added Sean Paul, he might be able to give a more definitive answer.

Regards,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-19 Thread Daniel Vetter 
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart
 wrote:
> Hi Ville,
>
> On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
>> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
>> > Some drivers duplicate the logic to create a property to store a per-plane
>> > alpha.
>> >
>> > This is especially useful if we ever want to support extra protocols for
>> > Wayland like:
>> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht
>> > ml
>> >
>> > Let's create a helper in order to move that to the core.
>> >
>> > Cc: Laurent Pinchart 
>> > Reviewed-by: Boris Brezillon 
>> > Signed-off-by: Maxime Ripard 
>> > ---
>> >
>> >  Documentation/gpu/kms-properties.csv |  2 +-
>> >  drivers/gpu/drm/drm_atomic.c |  4 -
>> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
>> >  drivers/gpu/drm/drm_blend.c  | 32 +-
>> >  include/drm/drm_blend.h  |  1 +-
>> >  include/drm/drm_plane.h  |  6 +-
>> >  6 files changed, 48 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/gpu/kms-properties.csv
>> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
>> > 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=Driver dependant",Plane,Opacity of the
>> > plane from transparent (0) to fully opaque (MAX). If this property is set
>> > to a value different than max, and that the pixel will define an alpha
>> > component, the property will have precendance and the pixel value will be
>> > ignored.

Please don't document new properties in that csv file, it's an
unreadable mess. Instead follow how we document standardized
properties nowadays in full-blown sections. For plane blending we
have:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

>> Those semantics don't seem particularly good to me. I think we would want
>> the per-pixel alpha and global alpha both to be effecive at the same time.
>> You can always decide to ignore the per-pixel alpha by using a pixel format
>> without alpha.
>
> That makes sense to me. However, it also brings a new question: how should a
> driver that supports either global alpha or pixel alpha but not both signal
> that to userspace, and how should it reacts when userspace selects a format
> with an alpha channel and set a global alpha value other than fully opaque ?
> To make things more complex, note that some drivers support combining global
> alpha and pixel alpha only for a subset of the formats with an alpha channel
> (for instance for ARGB 1555 formats, but not for ARGB  formats).

atomic_check can reject unsupported configs. Userspace needs to fall
back somehow (either switch to xrgb or make alpha fully opaque or just
give up on that plane). We have a lot of such corner-cases we don't
tell userspace about explicitly at all.

>> Also, where's the userspace that wants this feature?
>>
>> 
>>
>> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> > index 8185e3468a23..5a6f29524f12 100644
>> > --- a/include/drm/drm_plane.h
>> > +++ b/include/drm/drm_plane.h
>> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
>> >   * plane (in 16.16)
>> >   * @src_w: width of visible portion of plane (in 16.16)
>> >   * @src_h: height of visible portion of plane (in 16.16)
>> > + * @alpha: opacity of the plane
>> >   * @rotation: rotation of the plane
>> >   * @zpos: priority of the given plane on crtc (optional)
>> >   * Note that multiple active planes on the same crtc can have an
>> >   identical
>> > @@ -105,6 +106,9 @@ struct drm_plane_state {
>> > uint32_t src_x, src_y;
>> > uint32_t src_h, src_w;
>> >
>> > +   /* Plane opacity */
>> > +   u8 alpha;
>>
>> We may want to make that u16. The general we expect 16bpc for most color
>> related things, but since this is a range prop I suppose we should just
>> expose the actual hardware range. But making it u16 might avoid some head
>> scratching for the first person to have hardware with higher precision.
>> Either that or we should make the prop creation fail if the driver asks
>> for more bits than we have in the state.
>
> I'm tempted to go one step further and always make the alpha property 16-bits
> wide for new users (we can't do so for existing users as it could break
> userspace), and let drivers convert that internally to the range they need.
> There could however

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-19 Thread Laurent Pinchart 
Hi Ville,

On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> > Some drivers duplicate the logic to create a property to store a per-plane
> > alpha.
> > 
> > This is especially useful if we ever want to support extra protocols for
> > Wayland like:
> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht
> > ml
> > 
> > Let's create a helper in order to move that to the core.
> > 
> > Cc: Laurent Pinchart 
> > Reviewed-by: Boris Brezillon 
> > Signed-off-by: Maxime Ripard 
> > ---
> > 
> >  Documentation/gpu/kms-properties.csv |  2 +-
> >  drivers/gpu/drm/drm_atomic.c |  4 -
> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
> >  drivers/gpu/drm/drm_blend.c  | 32 +-
> >  include/drm/drm_blend.h  |  1 +-
> >  include/drm/drm_plane.h  |  6 +-
> >  6 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/kms-properties.csv
> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> > 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=Driver dependant",Plane,Opacity of the
> > plane from transparent (0) to fully opaque (MAX). If this property is set
> > to a value different than max, and that the pixel will define an alpha
> > component, the property will have precendance and the pixel value will be
> > ignored.
> 
> Those semantics don't seem particularly good to me. I think we would want
> the per-pixel alpha and global alpha both to be effecive at the same time.
> You can always decide to ignore the per-pixel alpha by using a pixel format
> without alpha.

That makes sense to me. However, it also brings a new question: how should a 
driver that supports either global alpha or pixel alpha but not both signal 
that to userspace, and how should it reacts when userspace selects a format 
with an alpha channel and set a global alpha value other than fully opaque ? 
To make things more complex, note that some drivers support combining global 
alpha and pixel alpha only for a subset of the formats with an alpha channel 
(for instance for ARGB 1555 formats, but not for ARGB  formats).

> Also, where's the userspace that wants this feature?
> 
> 
> 
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 8185e3468a23..5a6f29524f12 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >   * plane (in 16.16)
> >   * @src_w: width of visible portion of plane (in 16.16)
> >   * @src_h: height of visible portion of plane (in 16.16)
> > + * @alpha: opacity of the plane
> >   * @rotation: rotation of the plane
> >   * @zpos: priority of the given plane on crtc (optional)
> >   * Note that multiple active planes on the same crtc can have an
> >   identical
> > @@ -105,6 +106,9 @@ struct drm_plane_state {
> > uint32_t src_x, src_y;
> > uint32_t src_h, src_w;
> > 
> > +   /* Plane opacity */
> > +   u8 alpha;
> 
> We may want to make that u16. The general we expect 16bpc for most color
> related things, but since this is a range prop I suppose we should just
> expose the actual hardware range. But making it u16 might avoid some head
> scratching for the first person to have hardware with higher precision.
> Either that or we should make the prop creation fail if the driver asks
> for more bits than we have in the state.

I'm tempted to go one step further and always make the alpha property 16-bits 
wide for new users (we can't do so for existing users as it could break 
userspace), and let drivers convert that internally to the range they need. 
There could however be drawbacks I don't foresee.

> Oh, and you should plug this into the state dumper as well.
> 
> > +
> > 
> > /* Plane rotation */
> > unsigned int rotation;
> > @@ -481,6 +485,7 @@ enum drm_plane_type {
> >   * @funcs: helper functions
> >   * @properties: property tracking for this plane
> >   * @type: type of plane (overlay, primary, cursor)
> > + * @alpha_property: alpha property for this plane
> >   * @zpos_property: zpos property for this plane
> >   * @rotation_property: rotation property for this plane
> >   * @helper_private: mid-layer private data
> > @@ -556,6 +561,7 @@ struct drm_plane {
> >  */
> > 
> > struct drm_plane_state *state;
> > +   struct drm_property

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-19 Thread Laurent Pinchart 
Hi Maxime,

Thank you for the patch.

On Friday, 16 February 2018 19:39:29 EET Maxime Ripard wrote:
> Some drivers duplicate the logic to create a property to store a per-plane
> alpha.
> 
> This is especially useful if we ever want to support extra protocols for
> Wayland like:
> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
> 
> Let's create a helper in order to move that to the core.
> 
> Cc: Laurent Pinchart 
> Reviewed-by: Boris Brezillon 
> Signed-off-by: Maxime Ripard 
> ---
>  Documentation/gpu/kms-properties.csv |  2 +-
>  drivers/gpu/drm/drm_atomic.c |  4 -
>  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
>  drivers/gpu/drm/drm_blend.c  | 32 +-
>  include/drm/drm_blend.h  |  1 +-
>  include/drm/drm_plane.h  |  6 +-
>  6 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/kms-properties.csv
> b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> 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=Driver dependant",Plane,Opacity of the
> plane from transparent (0) to fully opaque (MAX). If this property is set
> to a value different than max, and that the pixel will define an alpha
> component, the property will have precendance and the pixel value will be
> ignored. The alpha value is represented as straight alpha, ie the colors
> haven't been pre-adjusted for their opacity by multiplication. Therefore,
> the equation to get a color value for one pixel, assuming two planes A and
> B, will be (color_a * alpha_a + color_b * alpha_b * (MAX - alpha_a) / MAX)
> / (alpha_a + alpha_b * (MAX - alpha_a) / MAX)
> ,,"""colorkey""",RANGE,"Min=0, Max=0x01ff",Plane,TBD
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d9ad20040a1..3defc56a1ef2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -753,6 +753,8 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane, state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> + } else if (property == plane->alpha_property) {
> + state->alpha = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK))
>   return -EINVAL;
> @@ -814,6 +816,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->alpha_property) {
> + *val = state->alpha;
>   } else if (property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index ae3cbfe9e01c..2b88f593aab4
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3482,6 +3482,10 @@ void drm_atomic_helper_plane_reset(struct drm_plane
> *plane) if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_MODE_ROTATE_0;
> +
> + /* Reset the alpha value to fully opaque if it matters */
> + if (plane->alpha_property)
> + plane->state->alpha = plane->alpha_property->values[1];
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 4c62dff14893..a5dea7cbed2c 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -104,6 +104,38 @@
>   */
> 
>  /**
> + * drm_plane_create_alpha_property - create a new alpha property
> + * @plane: drm plane
> + * @max_alpha: maximum value of alpha
> + *
> + * This function initializes a generic, mutable, alpha property and
> + * enables support for it in the DRM core.

s/initializes/creates/

I would also mention that it attaches the property to the plane.

> + * The alpha property will be allowed to be within the bounds of 0
> + * (transparent) to @max_alpha (opaque)

s/$/./

> + *
> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_plane_create_alpha_property(struct drm_plane *plane, u16 max_alpha)
> +{
> + struct drm_property *prop;
> +
> + prop =

Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-16 Thread Ville Syrjälä 
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
> Some drivers duplicate the logic to create a property to store a per-plane
> alpha.
> 
> This is especially useful if we ever want to support extra protocols for
> Wayland like:
> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
> 
> Let's create a helper in order to move that to the core.
> 
> Cc: Laurent Pinchart 
> Reviewed-by: Boris Brezillon 
> Signed-off-by: Maxime Ripard 
> ---
>  Documentation/gpu/kms-properties.csv |  2 +-
>  drivers/gpu/drm/drm_atomic.c |  4 -
>  drivers/gpu/drm/drm_atomic_helper.c  |  4 -
>  drivers/gpu/drm/drm_blend.c  | 32 +-
>  include/drm/drm_blend.h  |  1 +-
>  include/drm/drm_plane.h  |  6 +-
>  6 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/kms-properties.csv 
> b/Documentation/gpu/kms-properties.csv
> index 927b65e14219..25ad3503d663 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=Driver dependant",Plane,Opacity of the plane 
> from transparent (0) to fully opaque (MAX). If this property is set to a 
> value different than max, and that the pixel will define an alpha component, 
> the property will have precendance and the pixel value will be ignored.

Those semantics don't seem particularly good to me. I think we would want the
per-pixel alpha and global alpha both to be effecive at the same time. You can
always decide to ignore the per-pixel alpha by using a pixel format without
alpha.

Also, where's the userspace that wants this feature?


> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8185e3468a23..5a6f29524f12 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
>   *   plane (in 16.16)
>   * @src_w: width of visible portion of plane (in 16.16)
>   * @src_h: height of visible portion of plane (in 16.16)
> + * @alpha: opacity of the plane
>   * @rotation: rotation of the plane
>   * @zpos: priority of the given plane on crtc (optional)
>   *   Note that multiple active planes on the same crtc can have an identical
> @@ -105,6 +106,9 @@ struct drm_plane_state {
>   uint32_t src_x, src_y;
>   uint32_t src_h, src_w;
>  
> + /* Plane opacity */
> + u8 alpha;

We may want to make that u16. The general we expect 16bpc for most color
related things, but since this is a range prop I suppose we should just
expose the actual hardware range. But making it u16 might avoid some head
scratching for the first person to have hardware with higher precision.
Either that or we should make the prop creation fail if the driver asks
for more bits than we have in the state.

Oh, and you should plug this into the state dumper as well.

> +
>   /* Plane rotation */
>   unsigned int rotation;
>  
> @@ -481,6 +485,7 @@ enum drm_plane_type {
>   * @funcs: helper functions
>   * @properties: property tracking for this plane
>   * @type: type of plane (overlay, primary, cursor)
> + * @alpha_property: alpha property for this plane
>   * @zpos_property: zpos property for this plane
>   * @rotation_property: rotation property for this plane
>   * @helper_private: mid-layer private data
> @@ -556,6 +561,7 @@ struct drm_plane {
>*/
>   struct drm_plane_state *state;
>  
> + struct drm_property *alpha_property;
>   struct drm_property *zpos_property;
>   struct drm_property *rotation_property;
>  };
> -- 
> git-series 0.9.1
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/8] drm/blend: Add a generic alpha property

2018-02-16 Thread Maxime Ripard 
Some drivers duplicate the logic to create a property to store a per-plane
alpha.

This is especially useful if we ever want to support extra protocols for
Wayland like:
https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html

Let's create a helper in order to move that to the core.

Cc: Laurent Pinchart 
Reviewed-by: Boris Brezillon 
Signed-off-by: Maxime Ripard 
---
 Documentation/gpu/kms-properties.csv |  2 +-
 drivers/gpu/drm/drm_atomic.c |  4 -
 drivers/gpu/drm/drm_atomic_helper.c  |  4 -
 drivers/gpu/drm/drm_blend.c  | 32 +-
 include/drm/drm_blend.h  |  1 +-
 include/drm/drm_plane.h  |  6 +-
 6 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/kms-properties.csv 
b/Documentation/gpu/kms-properties.csv
index 927b65e14219..25ad3503d663 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=Driver dependant",Plane,Opacity of the plane 
from transparent (0) to fully opaque (MAX). If this property is set to a value 
different than max, and that the pixel will define an alpha component, the 
property will have precendance and the pixel value will be ignored. The alpha 
value is represented as straight alpha, ie the colors haven't been pre-adjusted 
for their opacity by multiplication. Therefore, the equation to get a color 
value for one pixel, assuming two planes A and B, will be (color_a * alpha_a + 
color_b * alpha_b * (MAX - alpha_a) / MAX) / (alpha_a + alpha_b * (MAX - 
alpha_a) / MAX)
 ,,"""colorkey""",RANGE,"Min=0, Max=0x01ff",Plane,TBD
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d9ad20040a1..3defc56a1ef2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -753,6 +753,8 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->src_w = val;
} else if (property == config->prop_src_h) {
state->src_h = val;
+   } else if (property == plane->alpha_property) {
+   state->alpha = val;
} else if (property == plane->rotation_property) {
if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK))
return -EINVAL;
@@ -814,6 +816,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_w;
} else if (property == config->prop_src_h) {
*val = state->src_h;
+   } else if (property == plane->alpha_property) {
+   *val = state->alpha;
} else if (property == plane->rotation_property) {
*val = state->rotation;
} else if (property == plane->zpos_property) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index ae3cbfe9e01c..2b88f593aab4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3482,6 +3482,10 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
*plane)
if (plane->state) {
plane->state->plane = plane;
plane->state->rotation = DRM_MODE_ROTATE_0;
+
+   /* Reset the alpha value to fully opaque if it matters */
+   if (plane->alpha_property)
+   plane->state->alpha = plane->alpha_property->values[1];
}
 }
 EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 4c62dff14893..a5dea7cbed2c 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -104,6 +104,38 @@
  */
 
 /**
+ * drm_plane_create_alpha_property - create a new alpha property
+ * @plane: drm plane
+ * @max_alpha: maximum value of alpha
+ *
+ * This function initializes a generic, mutable, alpha property and
+ * enables support for it in the DRM core.
+ *
+ * The alpha property will be allowed to be within the bounds of 0
+ * (transparent) to @max_alpha (opaque)
+ *
+ * Returns:
+ * 0 on success, negative error code on failure.
+ */
+int drm_plane_create_alpha_property(struct drm_plane *plane, u16 max_alpha)
+{
+   struct drm_property *prop;
+
+   prop = drm_property_create_range(plane->dev, 0, "alpha", 0, max_alpha);
+   if (!prop)
+   return -ENOMEM;
+
+   drm_object_attach_property(>base, prop, max_alpha);
+   plane->alpha_property = prop;
+
+   if (plane->state)
+   plane->state->alpha = max_alpha;
+
+   return 0;
+}