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 > > <laurent.pinchart@xxxxxxxxxxxxxxxx> 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 <laurent.pinchart@xxxxxxxxxxxxxxxx> > > >> > Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx> > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> > > >> > --- > > >> > > > >> > 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 range applies. But that's probably going to be ignored. Could we do the property itself as u16 range, and (for now, only internally in drm in drm_plane_state) throw the lower u8 bits away? Or just let drivers do this. Sorry that I'm flip-flopping around on this, but we just have an ongoing discussion about a range/size mixup in the CTM uapi, I think assuming that all userspace will correctly scale is not realistic. So larger scale in the uapi (but maybe not internally) from the start seems like a good idea. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
