On Fri, May 05, 2017 at 08:58:17AM +0200, Daniel Vetter wrote:
> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote:
> > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
> > > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
> > > > Add standard optinal property blobs for YCbCr to RGB conversion CSC
> > > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined
> > > > to YCBCR_ENCODING property to activate the CSC and preoffset
> > > > properties. For simplicity the new properties are stored in the
> > > > drm_plane object, because the YCBCR_ENCODING is already there. The
> > > > blob contents are defined in the uapi/drm/drm_mode.h header.
> > > >
> > > > Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
> > >
> > > Not sure we want to make this yuv2rgb specific, plenty of planes have
> > > generic degamma/offset, csc, gamme/offset hw. I think what we want instead
> > > is the generic csc support, plus a ycbcr2rgb mode of "bypass".
> >
> > My idea is to not expose this. And instead just expose a normal
> > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
> > that can't be done by the hw.
>
> But that means we'd need to have a bit-perfect match with a canonical
> conversion matrix (even if maybe the hw has a rounding bug and implements
> something slightly different than the standard).
I don't see what this has to do with bit correctness. Different hw is
likely to use different precision for this stuff anyway, so I'm 100%
sure we'll not get the same results from all hardware. And really,
getting 100% same output doesn't seem all that important here
anyway.
Anyway, all I'm saying is that we should expose a standard pipeline:
ycbcr->rgb -> degamma -> ctm -> gamma
And each driver can then expose whatever parts of that they wish. If you
want to expoe a programmable matrix you expose it as the ctm. I don't
think having a programmable matrix as both ycbcr->rgb and ctm would
really benefit us.
> That seems a bit an
> awkward interface. Or is your idea that hw with a ctm would simply not
> expose the colorspace enum, if it doesn't also have fixed-function bits on
> top of the ctm?
> -Daniel
>
> >
> > >
> > > And as usual, this needs some userspace compositor which actually uses it
> > > (not just a demo, since there's a huge difference between a demo and
> > > something that has to Just Work like a real compositor).
> > > -Daniel
> > >
> > > > ---
> > > > drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++
> > > > drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++
> > > > drivers/gpu/drm/drm_color_mgmt.c | 55 +++++++++++++++++++++++++++++++++++--
> > > > drivers/gpu/drm/drm_plane.c | 6 ++++
> > > > include/drm/drm_color_mgmt.h | 3 ++
> > > > include/drm/drm_plane.h | 4 +++
> > > > include/uapi/drm/drm_mode.h | 12 ++++++++
> > > > 7 files changed, 106 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > index bcef93d..87a2d55 100644
> > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > @@ -733,6 +733,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > > struct drm_device *dev = plane->dev;
> > > > struct drm_mode_config *config = &dev->mode_config;
> > > > int ret;
> > > > + bool dummy;
> > > >
> > > > if (property == config->prop_fb_id) {
> > > > struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> > > > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > > state->zpos = val;
> > > > } else if (property == plane->ycbcr_encoding_property) {
> > > > state->ycbcr_encoding = val;
> > > > + } else if (property == plane->ycbcr_decode_csc_property) {
> > > > + ret = drm_atomic_replace_property_blob_from_id(dev,
> > > > + &state->ycbcr_decode_csc, val,
> > > > + sizeof(struct drm_ycbcr_decode_csc),
> > > > + &dummy);
> > > > + return ret;
> > > > + } else if (property == plane->ycbcr_csc_preoffset_property) {
> > > > + ret = drm_atomic_replace_property_blob_from_id(dev,
> > > > + &state->ycbcr_csc_preoffset, val,
> > > > + sizeof(struct drm_ycbcr_csc_preoffset),
> > > > + &dummy);
> > > > + return ret;
> > > > } else if (plane->funcs->atomic_set_property) {
> > > > return plane->funcs->atomic_set_property(plane, state,
> > > > property, val);
> > > > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> > > > *val = state->zpos;
> > > > } else if (property == plane->ycbcr_encoding_property) {
> > > > *val = state->ycbcr_encoding;
> > > > + } else if (property == plane->ycbcr_decode_csc_property) {
> > > > + *val = state->ycbcr_decode_csc ?
> > > > + state->ycbcr_decode_csc->base.id : 0;
> > > > + } else if (property == plane->ycbcr_csc_preoffset_property) {
> > > > + *val = state->ycbcr_csc_preoffset ?
> > > > + state->ycbcr_csc_preoffset->base.id : 0;
> > > > } else if (plane->funcs->atomic_get_property) {
> > > > return plane->funcs->atomic_get_property(plane, state, property, val);
> > > > } else {
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 8be9719..6ecc32f 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> > > > {
> > > > memcpy(state, plane->state, sizeof(*state));
> > > >
> > > > + if (state->ycbcr_decode_csc)
> > > > + drm_property_blob_get(state->ycbcr_decode_csc);
> > > > +
> > > > + if (state->ycbcr_csc_preoffset)
> > > > + drm_property_blob_get(state->ycbcr_csc_preoffset);
> > > > +
> > > > if (state->fb)
> > > > drm_framebuffer_get(state->fb);
> > > >
> > > > @@ -3278,6 +3284,9 @@ struct drm_plane_state *
> > > > */
> > > > void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> > > > {
> > > > + drm_property_blob_put(state->ycbcr_decode_csc);
> > > > + drm_property_blob_put(state->ycbcr_csc_preoffset);
> > > > +
> > > > if (state->fb)
> > > > drm_framebuffer_put(state->fb);
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > > index 245b14a..319ed46 100644
> > > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > > @@ -95,8 +95,23 @@
> > > > *
> > > > * "YCBCR_ENCODING"
> > > > * Optional plane enum property to control YCbCr to RGB
> > > > - * conversion. The driver provides a subset of standard
> > > > - * enum values supported by the DRM plane.
> > > > + * conversion. The driver provides a subset of standard enum
> > > > + * values supported by the DRM plane. The possible encodings
> > > > + * include the standard conversions and a possibility to select a
> > > > + * custom conversion matrix and preoffset vector.
> > > > + *
> > > > + * "YCBCR_DECODE_CSC"
> > > > + * Optional plane property blob to set YCbCr to RGB conversion
> > > > + * matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > > > + * defined in uapi/drm/drm_mode.h. Whether this property is
> > > > + * used depends on the value of YCBCR_ENCODING property.
> > > > + *
> > > > + * "YCBCR_CSC_PREOFFSET"
> > > > + * Optional plane property blob to configure YCbCr offset before
> > > > + * YCbCr to RGB CSC is applied. The blob contains struct
> > > > + * drm_ycbcr_csc_preoffset which is defined in
> > > > + * uapi/drm/drm_mode.h. Whether this property is used depends on
> > > > + * the value of YCBCR_ENCODING property.
> > > > */
> > > >
> > > > /**
> > > > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > > > [DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> > > > [DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> > > > [DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> > > > + [DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range",
> > > > + [DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range",
> > > > + [DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector",
> > > > };
> > > >
> > > > /**
> > > > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > > > * drm_plane object. The supported encodings should be provided in the
> > > > * enum_list parameter. The enum_list parameter should not contain the
> > > > * enum names, because the standard names are added by this function.
> > > > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created
> > > > + * based on the provided enum_list.
> > > > */
> > > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > > struct drm_prop_enum_list *enum_list,
> > > > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > > {
> > > > struct drm_device *dev = plane->dev;
> > > > struct drm_property *prop;
> > > > + bool ycbcr_decode_csc_create = false;
> > > > + bool ycbcr_csc_preoffset_create = false;
> > > > unsigned int i;
> > > >
> > > > if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> > > > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > > enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
> > > >
> > > > enum_list[i].name = ycbcr_encoding_name[encoding];
> > > > +
> > > > + if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE ||
> > > > + encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE)
> > > > + ycbcr_decode_csc_create = true;
> > > > +
> > > > + if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) {
> > > > + ycbcr_decode_csc_create = true;
> > > > + ycbcr_csc_preoffset_create = true;
> > > > + }
> > > > }
> > > >
> > > > prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> > > > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > > plane->ycbcr_encoding_property = prop;
> > > > drm_object_attach_property(&plane->base, prop, default_mode);
> > > >
> > > > + if (ycbcr_decode_csc_create) {
> > > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > > + DRM_MODE_PROP_BLOB,
> > > > + "YCBCR_DECODE_CSC", 0);
> > > > + if (!prop)
> > > > + return -ENOMEM;
> > > > + plane->ycbcr_decode_csc_property = prop;
> > > > + drm_object_attach_property(&plane->base, prop, 0);
> > > > + }
> > > > +
> > > > + if (ycbcr_csc_preoffset_create) {
> > > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > > + DRM_MODE_PROP_BLOB,
> > > > + "YCBCR_CSC_PREOFFSET", 0);
> > > > + if (!prop)
> > > > + return -ENOMEM;
> > > > + plane->ycbcr_csc_preoffset_property = prop;
> > > > + drm_object_attach_property(&plane->base, prop, 0);
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > index 007c4d7..d10c942 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
> > > > if (plane->ycbcr_encoding_property)
> > > > drm_property_destroy(dev, plane->ycbcr_encoding_property);
> > > >
> > > > + if (plane->ycbcr_decode_csc_property)
> > > > + drm_property_destroy(dev, plane->ycbcr_decode_csc_property);
> > > > +
> > > > + if (plane->ycbcr_csc_preoffset_property)
> > > > + drm_property_destroy(dev, plane->ycbcr_csc_preoffset_property);
> > > > +
> > > > memset(plane, 0, sizeof(*plane));
> > > > }
> > > > EXPORT_SYMBOL(drm_plane_cleanup);
> > > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > > index 1771394..bdfd37e 100644
> > > > --- a/include/drm/drm_color_mgmt.h
> > > > +++ b/include/drm/drm_color_mgmt.h
> > > > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding {
> > > > DRM_PLANE_YCBCR_BT601_LIMITED_RANGE,
> > > > DRM_PLANE_YCBCR_BT709_LIMITED_RANGE,
> > > > DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE,
> > > > + DRM_PLANE_YCBCR_CSC_FULL_RANGE,
> > > > + DRM_PLANE_YCBCR_CSC_LIMITED_RANGE,
> > > > + DRM_PLANE_YCBCR_CSC_PREOFFSET,
> > > > };
> > > >
> > > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > index 4d0510f..63b1e0c 100644
> > > > --- a/include/drm/drm_plane.h
> > > > +++ b/include/drm/drm_plane.h
> > > > @@ -115,6 +115,8 @@ struct drm_plane_state {
> > > >
> > > > /* YCbCr to RGB conversion */
> > > > enum drm_plane_ycbcr_encoding ycbcr_encoding;
> > > > + struct drm_property_blob *ycbcr_decode_csc;
> > > > + struct drm_property_blob *ycbcr_csc_preoffset;
> > > >
> > > > /* Clipped coordinates */
> > > > struct drm_rect src, dst;
> > > > @@ -529,6 +531,8 @@ struct drm_plane {
> > > > struct drm_property *rotation_property;
> > > >
> > > > struct drm_property *ycbcr_encoding_property;
> > > > + struct drm_property *ycbcr_decode_csc_property;
> > > > + struct drm_property *ycbcr_csc_preoffset_property;
> > > > };
> > > >
> > > > #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > > index 8c67fc0..8c9568d 100644
> > > > --- a/include/uapi/drm/drm_mode.h
> > > > +++ b/include/uapi/drm/drm_mode.h
> > > > @@ -543,6 +543,18 @@ struct drm_color_lut {
> > > > __u16 reserved;
> > > > };
> > > >
> > > > +struct drm_ycbcr_decode_csc {
> > > > + /* Conversion matrix in 2-complement s32.32 format. */
> > > > + __s64 ry, rcb, rcr;
> > > > + __s64 gy, gcb, gcr;
> > > > + __s64 by, bcb, bcr;
> > > > +};
> > > > +
> > > > +struct drm_ycbcr_csc_preoffset {
> > > > + /* Offset vector in 2-complement s.32 format. */
> > > > + __s32 y, cb, cr;
> > > > +};
> > > > +
> > > > #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > > > #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> > > > #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> > > > --
> > > > 1.9.1
> > > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
