Re: [RFC PATCH v3 01/23] drm: Don't treat 0 as -1 in drm_fixp2int_ceil
On 12/06, Melissa Wen wrote:
> On 11/08, Harry Wentland wrote:
> > Unit testing this in VKMS shows that passing 0 into
> > this function returns -1, which is highly counter-
> > intuitive. Fix it by checking whether the input is
> > >= 0 instead of > 0.
> >
> Nice finding. Thanks!
>
> Could you add the fixes tag? AFAIU, this one:
>
> Fixes: 64566b5e767f9 ("drm: Add drm_fixp_from_fraction and drm_fixp2int_ceil")
> Reviewed-by: Melissa Wen
cc'ing msm people:
Hi,
Only msm and vkms currently use this function.
I see many points where msm resorts to a conditional that avoid passing
0 into this function. For example:
if (temp2_fp)
temp = drm_fixp2int_ceil(temp2_fp);
else
temp = 0;
Can someone from msm ack this patch too?
Thanks,
Melissa
>
> > Signed-off-by: Harry Wentland
> > Reviewed-by: Simon Ser
> > ---
> > include/drm/drm_fixed.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> > index 6ea339d5de08..0c9f917a4d4b 100644
> > --- a/include/drm/drm_fixed.h
> > +++ b/include/drm/drm_fixed.h
> > @@ -95,7 +95,7 @@ static inline int drm_fixp2int_round(s64 a)
> >
> > static inline int drm_fixp2int_ceil(s64 a)
> > {
> > - if (a > 0)
> > + if (a >= 0)
> > return drm_fixp2int(a + DRM_FIXED_ALMOST_ONE);
> > else
> > return drm_fixp2int(a - DRM_FIXED_ALMOST_ONE);
> > --
> > 2.42.1
> >
Re: [RFC PATCH v3 06/23] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Wed, 8 Nov 2023 11:36:25 -0500 Harry Wentland wrote: > v3: > - Describe DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE (Sebastian) > - Ask for clear documentation of colorop behavior (Sebastian) > > v2: > - Update colorop visualizations to match reality (Sebastian, Alex Hung) > - Updated wording (Pekka) > - Change BYPASS wording to make it non-mandatory (Sebastian) > - Drop cover-letter-like paragraph from COLOR_PIPELINE Plane Property >section (Pekka) > - Use PQ EOTF instead of its inverse in Pipeline Programming example > (Melissa) > - Add "Driver Implementer's Guide" section (Pekka) > - Add "Driver Forward/Backward Compatibility" section (Sebastian, Pekka) > > Signed-off-by: Harry Wentland > --- > Documentation/gpu/rfc/color_pipeline.rst | 352 +++ > 1 file changed, 352 insertions(+) > create mode 100644 Documentation/gpu/rfc/color_pipeline.rst > > diff --git a/Documentation/gpu/rfc/color_pipeline.rst > b/Documentation/gpu/rfc/color_pipeline.rst > new file mode 100644 > index ..efc70570a592 > --- /dev/null > +++ b/Documentation/gpu/rfc/color_pipeline.rst > @@ -0,0 +1,352 @@ > + > +Linux Color Pipeline API > + > + > +What problem are we solving? > + > + > +We would like to support pre-, and post-blending complex color > +transformations in display controller hardware in order to allow for > +HW-supported HDR use-cases, as well as to provide support to > +color-managed applications, such as video or image editors. > + > +It is possible to support an HDR output on HW supporting the Colorspace > +and HDR Metadata drm_connector properties, but that requires the > +compositor or application to render and compose the content into one > +final buffer intended for display. Doing so is costly. > + > +Most modern display HW offers various 1D LUTs, 3D LUTs, matrices, and other > +operations to support color transformations. These operations are often > +implemented in fixed-function HW and therefore much more power efficient than > +performing similar operations via shaders or CPU. > + > +We would like to make use of this HW functionality to support complex color > +transformations with no, or minimal CPU or shader load. > + > + > +How are other OSes solving this problem? > + > + > +The most widely supported use-cases regard HDR content, whether video or > +gaming. > + > +Most OSes will specify the source content format (color gamut, encoding > transfer > +function, and other metadata, such as max and average light levels) to a > driver. > +Drivers will then program their fixed-function HW accordingly to map from a > +source content buffer's space to a display's space. > + > +When fixed-function HW is not available the compositor will assemble a > shader to > +ask the GPU to perform the transformation from the source content format to > the > +display's format. > + > +A compositor's mapping function and a driver's mapping function are usually > +entirely separate concepts. On OSes where a HW vendor has no insight into > +closed-source compositor code such a vendor will tune their color management > +code to visually match the compositor's. On other OSes, where both mapping > +functions are open to an implementer they will ensure both mappings match. > + > +This results in mapping algorithm lock-in, meaning that no-one alone can > +experiment with or introduce new mapping algorithms and achieve > +consistent results regardless of which implementation path is taken. > + > +Why is Linux different? > +=== > + > +Unlike other OSes, where there is one compositor for one or more drivers, on > +Linux we have a many-to-many relationship. Many compositors; many drivers. > +In addition each compositor vendor or community has their own view of how > +color management should be done. This is what makes Linux so beautiful. > + > +This means that a HW vendor can now no longer tune their driver to one > +compositor, as tuning it to one could make it look fairly different from > +another compositor's color mapping. > + > +We need a better solution. > + > + > +Descriptive API > +=== > + > +An API that describes the source and destination colorspaces is a descriptive > +API. It describes the input and output color spaces but does not describe > +how precisely they should be mapped. Such a mapping includes many minute > +design decision that can greatly affect the look of the final result. > + > +It is not feasible to describe such mapping with enough detail to ensure the > +same result from each implementation. In fact, these mappings are a very > active > +research area. > + > + > +Prescriptive API > + > + > +A prescriptive API describes not the source and destination colorspaces. It > +instead prescribes a recipe for how to manipulate pixel values to arrive at > the > +desired outcome. > + > +This recipe is generally a
Re: [RFC PATCH v3 07/23] drm/colorop: Introduce new drm_colorop mode object
On Wed, 8 Nov 2023 11:36:26 -0500
Harry Wentland wrote:
> This patches introduces a new drm_colorop mode object. This
> object represents color transformations and can be used to
> define color pipelines.
>
> We also introduce the drm_colorop_state here, as well as
> various helpers and state tracking bits.
>
> v3:
> - Drop TODO for lock (it's handled in drm_modeset_drop_locks)
>(Melissa)
> - Don't get plane state when getting colorop state
> - Make some functions static (kernel test robot)
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/Makefile| 1 +
> drivers/gpu/drm/drm_atomic.c| 70 +
> drivers/gpu/drm/drm_atomic_helper.c | 12 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 48 +
> drivers/gpu/drm/drm_colorop.c | 152 +++
> drivers/gpu/drm/drm_mode_config.c | 7 ++
> drivers/gpu/drm/drm_plane_helper.c | 2 +-
> include/drm/drm_atomic.h| 82 +++
> include/drm/drm_atomic_uapi.h | 1 +
> include/drm/drm_colorop.h | 157
> include/drm/drm_mode_config.h | 18
> include/drm/drm_plane.h | 2 +
> include/uapi/drm/drm.h | 3 +
> include/uapi/drm/drm_mode.h | 1 +
> 14 files changed, 555 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/drm_colorop.c
> create mode 100644 include/drm/drm_colorop.h
...
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 8662b5aeea0c..841d393fb84e 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1238,6 +1238,9 @@ extern "C" {
> */
> #define DRM_IOCTL_MODE_CLOSEFB DRM_IOWR(0xD0, struct
> drm_mode_closefb)
>
> +#define DRM_IOCTL_MODE_GETCOLOROPRESOURCES DRM_IOWR(0xD0, struct
> drm_mode_get_colorop_res)
> +#define DRM_IOCTL_MODE_GETCOLOROP DRM_IOWR(0xD1, struct
> drm_mode_get_colorop)
Aren't these dead code now?
Thanks,
pq
pgpBi_7UGuORQ.pgp
Description: OpenPGP digital signature
Re: [RFC PATCH v3 08/23] drm/colorop: Add TYPE property
On Wed, 8 Nov 2023 11:36:27 -0500
Harry Wentland wrote:
> Add a read-only TYPE property. The TYPE specifies the colorop
> type, such as enumerated curve, 1D LUT, CTM, 3D LUT, PWL LUT,
> etc.
>
> v3:
> - Make TYPE a range property
> - Move enum drm_colorop_type to uapi header
> - Fix drm_get_colorop_type_name description
>
> For now we're only introducing an enumerated 1D LUT type to
> illustrate the concept.
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/drm_atomic.c | 4 +--
> drivers/gpu/drm/drm_atomic_uapi.c | 8 +-
> drivers/gpu/drm/drm_colorop.c | 43 ++-
> include/drm/drm_colorop.h | 17 +++-
> include/uapi/drm/drm_mode.h | 4 +++
> 5 files changed, 71 insertions(+), 5 deletions(-)
>
...
> static int drm_atomic_set_writeback_fb_for_connector(
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index a295ab96aee1..902b13e300fd 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -32,12 +32,17 @@
>
> /* TODO big colorop doc, including properties, etc. */
>
> +static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
> + { DRM_COLOROP_1D_CURVE, "1D Curve" },
> +};
> +
> /* Init Helpers */
>
> int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
> - struct drm_plane *plane)
> + struct drm_plane *plane, enum drm_colorop_type type)
> {
> struct drm_mode_config *config = &dev->mode_config;
> + struct drm_property *prop;
> int ret = 0;
>
> ret = drm_mode_object_add(dev, &colorop->base, DRM_MODE_OBJECT_COLOROP);
> @@ -46,12 +51,28 @@ int drm_colorop_init(struct drm_device *dev, struct
> drm_colorop *colorop,
>
> colorop->base.properties = &colorop->properties;
> colorop->dev = dev;
> + colorop->type = type;
> colorop->plane = plane;
>
> list_add_tail(&colorop->head, &config->colorop_list);
> colorop->index = config->num_colorop++;
>
> /* add properties */
> +
> + /* type */
> + prop = drm_property_create_range(dev,
> + DRM_MODE_PROP_IMMUTABLE,
> + "TYPE", type, type);
Why was this made a range property?
My review comment for the doc patch is wrong, the doc should mention
enum drm_colorop_type.
> +
> + if (!prop)
> + return -ENOMEM;
> +
> + colorop->type_property = prop;
> +
> + drm_object_attach_property(&colorop->base,
> +colorop->type_property,
> +colorop->type);
> +
> return ret;
> }
...
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 4e3251ff894a..0ebf0f480dc8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -858,6 +858,10 @@ struct drm_color_lut {
> __u16 reserved;
> };
>
> +enum drm_colorop_type {
> + DRM_COLOROP_1D_CURVE
> +};
Thanks,
pq
pgpCxmwQGJzYF.pgp
Description: OpenPGP digital signature
Re: [RFC PATCH v3 06/23] drm/doc/rfc: Describe why prescriptive color pipeline is needed
On Wed, 8 Nov 2023 11:36:25 -0500
Harry Wentland wrote:
> v3:
> - Describe DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE (Sebastian)
> - Ask for clear documentation of colorop behavior (Sebastian)
>
> v2:
> - Update colorop visualizations to match reality (Sebastian, Alex Hung)
> - Updated wording (Pekka)
> - Change BYPASS wording to make it non-mandatory (Sebastian)
> - Drop cover-letter-like paragraph from COLOR_PIPELINE Plane Property
>section (Pekka)
> - Use PQ EOTF instead of its inverse in Pipeline Programming example
> (Melissa)
> - Add "Driver Implementer's Guide" section (Pekka)
> - Add "Driver Forward/Backward Compatibility" section (Sebastian, Pekka)
>
> Signed-off-by: Harry Wentland
> ---
> Documentation/gpu/rfc/color_pipeline.rst | 352 +++
> 1 file changed, 352 insertions(+)
> create mode 100644 Documentation/gpu/rfc/color_pipeline.rst
>
> diff --git a/Documentation/gpu/rfc/color_pipeline.rst
> b/Documentation/gpu/rfc/color_pipeline.rst
> new file mode 100644
> index ..efc70570a592
> --- /dev/null
> +++ b/Documentation/gpu/rfc/color_pipeline.rst
...
> +An example of a drm_colorop object might look like one of these::
> +
> +/* 1D enumerated curve */
> +Color operation 42
> +├─ "TYPE": immutable enum {1D enumerated curve, 1D LUT, 3x3 matrix, 3x4
> matrix, 3D LUT, etc.} = 1D enumerated curve
> +├─ "BYPASS": bool {true, false}
> +├─ "CURVE_1D_TYPE": enum {sRGB EOTF, sRGB inverse EOTF, PQ EOTF, PQ
> inverse EOTF, …}
> +└─ "NEXT": immutable color operation ID = 43
If TYPE is a range, then all examples like this need fixing.
Thanks,
pq
pgpMN4v9OsBLG.pgp
Description: OpenPGP digital signature
Re: [RFC PATCH v3 09/23] drm/color: Add 1D Curve subtype
On Wed, 8 Nov 2023 11:36:28 -0500
Harry Wentland wrote:
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 18 ++
> drivers/gpu/drm/drm_colorop.c | 39 +++
> include/drm/drm_colorop.h | 20
> 3 files changed, 72 insertions(+), 5 deletions(-)
>
...
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 902b13e300fd..b1c271f90a16 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -36,6 +36,11 @@ static const struct drm_prop_enum_list
> drm_colorop_type_enum_list[] = {
> { DRM_COLOROP_1D_CURVE, "1D Curve" },
> };
>
> +static const struct drm_prop_enum_list drm_colorop_curve_1d_type_enum_list[]
> = {
> + { DRM_COLOROP_1D_CURVE_SRGB_EOTF, "sRGB EOTF" },
> + { DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF, "sRGB Inverse EOTF" },
> +};
> +
> /* Init Helpers */
>
> int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
> @@ -73,6 +78,20 @@ int drm_colorop_init(struct drm_device *dev, struct
> drm_colorop *colorop,
> colorop->type_property,
> colorop->type);
>
> + /* curve_1d_type */
> + /* TODO move to mode_config? */
> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> + "CURVE_1D_TYPE",
> + drm_colorop_curve_1d_type_enum_list,
> +
> ARRAY_SIZE(drm_colorop_curve_1d_type_enum_list));
> + if (!prop)
> + return -ENOMEM;
> +
> + colorop->curve_1d_type_property = prop;
> + drm_object_attach_property(&colorop->base,
> +colorop->curve_1d_type_property,
> +0);
> +
> return ret;
> }
> EXPORT_SYMBOL(drm_colorop_init);
> @@ -177,6 +196,11 @@ static const char * const colorop_type_name[] = {
> [DRM_COLOROP_1D_CURVE] = "1D Curve",
> };
>
> +static const char * const colorop_curve_1d_type_name[] = {
> + [DRM_COLOROP_1D_CURVE_SRGB_EOTF] = "sRGB EOTF",
> + [DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF] = "sRGB Inverse EOTF",
> +};
Can't you use drm_colorop_curve_1d_type_enum_list to avoid duplicating the same?
> +
> /**
> * drm_get_colorop_type_name - return a string for colorop type
> * @type: colorop type to compute name of
> @@ -191,3 +215,18 @@ const char *drm_get_colorop_type_name(enum
> drm_colorop_type type)
>
> return colorop_type_name[type];
> }
> +
> +/**
> + * drm_get_colorop_curve_1d_type_name - return a string for 1D curve type
> + * @range: 1d curve type to compute name of
> + *
> + * In contrast to the other drm_get_*_name functions this one here returns a
> + * const pointer and hence is threadsafe.
> + */
> +const char *drm_get_colorop_curve_1d_type_name(enum
> drm_colorop_curve_1d_type type)
> +{
> + if (WARN_ON(type >= ARRAY_SIZE(colorop_curve_1d_type_name)))
> + return "unknown";
> +
> + return colorop_curve_1d_type_name[type];
> +}
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
> index 0386440afe40..965db7ec488f 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -30,6 +30,11 @@
> #include
> #include
>
> +enum drm_colorop_curve_1d_type {
> + DRM_COLOROP_1D_CURVE_SRGB_EOTF,
> + DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF
> +};
...
Thanks,
pq
pgpW_FZUxS97S.pgp
Description: OpenPGP digital signature
Re: [RFC PATCH v3 13/23] drm/plane: Add COLOR PIPELINE property
On Wed, 8 Nov 2023 11:36:32 -0500
Harry Wentland wrote:
> We're adding a new enum COLOR PIPELINE property. This
> property will have entries for each COLOR PIPELINE by
> referencing the DRM object ID of the first drm_colorop
> of the pipeline. 0 disables the entire COLOR PIPELINE.
I didn't find the call that actually creates that property, where is it?
> Userspace can use this to discover the available color
> pipelines, as well as set the desired one. The color
> pipelines are programmed via properties on the actual
> drm_colorop objects.
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/drm_atomic.c | 46 +++
> drivers/gpu/drm/drm_atomic_state_helper.c | 5 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 44 ++
> include/drm/drm_atomic_uapi.h | 2 +
> include/drm/drm_plane.h | 8
> 5 files changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ccf26b034433..cf3cb6d1239f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1463,6 +1463,52 @@ drm_atomic_add_affected_planes(struct drm_atomic_state
> *state,
> }
> EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>
> +/**
> + * drm_atomic_add_affected_colorops - add colorops for plane
> + * @state: atomic state
> + * @plane: DRM plane
> + *
> + * This function walks the current configuration and adds all colorops
> + * currently used by @plane to the atomic configuration @state. This is
> useful
> + * when an atomic commit also needs to check all currently enabled colorop on
> + * @plane, e.g. when changing the mode. It's also useful when re-enabling a
> plane
> + * to avoid special code to force-enable all colorops.
> + *
> + * Since acquiring a colorop state will always also acquire the w/w mutex of
> the
> + * current plane for that colorop (if there is any) adding all the colorop
> states for
> + * a plane will not reduce parallelism of atomic updates.
> + *
> + * Returns:
> + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is
> EDEADLK
> + * then the w/w mutex code has detected a deadlock and the entire atomic
> + * sequence must be restarted. All other errors are fatal.
> + */
> +int
> +drm_atomic_add_affected_colorops(struct drm_atomic_state *state,
> + struct drm_plane *plane)
> +{
> + struct drm_colorop *colorop;
> + struct drm_colorop_state *colorop_state;
> +
> + WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
> +
> + drm_dbg_atomic(plane->dev,
> +"Adding all current colorops for [plane:%d:%s] to %p\n",
> +plane->base.id, plane->name, state);
> +
> + drm_for_each_colorop(colorop, plane->dev) {
> + if (colorop->plane != plane)
> + continue;
> +
> + colorop_state = drm_atomic_get_colorop_state(state, colorop);
> + if (IS_ERR(colorop_state))
> + return PTR_ERR(colorop_state);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_add_affected_colorops);
> +
> /**
> * drm_atomic_check_only - check whether a given config would work
> * @state: atomic configuration to check
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..3c5f2c8e33d0 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -267,6 +267,11 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
> plane_state->color_range = val;
> }
>
> + if (plane->color_pipeline_property) {
> + /* default is always NULL, i.e., bypass */
> + plane_state->color_pipeline = NULL;
> + }
> +
> if (plane->zpos_property) {
> if (!drm_object_property_get_default_value(&plane->base,
> plane->zpos_property,
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a8f7a8a6639a..c6629fdaa114 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -256,6 +256,38 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state
> *plane_state,
> }
> EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>
> +
> +/**
> + * drm_atomic_set_colorop_for_plane - set colorop for plane
> + * @plane_state: atomic state object for the plane
> + * @colorop: colorop to use for the plane
> + *
> + * Changing the assigned framebuffer for a plane requires us to grab a
> reference
> + * to the new fb and drop the reference to the old fb, if there is one. This
> + * function takes care of all these details besides updating the pointer in
> the
> + * state object itself.
This paragraph does not seem to talk about this function.
> + */
> +void
> +drm_atomic_set_colorop_
Re: what are the protocol rules about uniqueness of event and request names?
On Fri, Dec 8, 2023 at 12:44 AM jleivent wrote: > > On Thu, 7 Dec 2023 22:06:07 + > David Edmundson wrote: > > > The generated C code be full of conflicts. The > > MY_PROTOCOL_REQUESTEVENT_SINCE_VERSION define for a start. > > > > I think it might compile in C, but other generators exist that might > > not and it's making life much more confusing than it needs to be. I > > would strongly avoid it. > > > > David > > To be clear, I wasn't intending it to sound like I wanted to add an > event and a request with the same name myself. I'm writing some > middleware that sits between a Wayland compositor and some of its > clients, and I would like to know if it might encounter an interface > that has an event and a request with the same name. > > I think you've answered that it's not a good idea for a protocol author > to do that, but it also sounds like it's a possibility that someone > could do it anyway because there's no direct rule against it. So > maybe I should take the necessary precautions. I think a more useful thing to do would be to add this restriction (an interface cannot have an event and a request with the same name) to the documentation and to wayland-scanner. > > Thanks, > Jonathan >
Re: [RFC PATCH v3 15/23] drm/vkms: Add enumerated 1D curve colorop
On Wed, 8 Nov 2023 11:36:34 -0500
Harry Wentland wrote:
> This patch introduces a VKMS color pipeline that includes two
> drm_colorops for named transfer functions. For now the only ones
> supported are sRGB EOTF, sRGB Inverse EOTF, and a Linear TF.
> We will expand this in the future but I don't want to do so
> without accompanying IGT tests.
>
> We introduce a new vkms_luts.c file that hard-codes sRGB EOTF,
> sRGB Inverse EOTF, and a linear EOTF LUT. These have been
> generated with 256 entries each as IGT is currently testing
> only 8 bpc surfaces. We will likely need higher precision
> but I'm reluctant to make that change without clear indication
> that we need it. We'll revisit and, if necessary, regenerate
> the LUTs when we have IGT tests for higher precision buffers.
VKMS could just implement the sRGB TF formula rather than an opaque
array of numbers that supposedly computes the same'ish.
Why is even the identity curve (which you call linear) encoded as a LUT?
> v2:
> - Add commit description
> - Fix sRGB EOTF LUT definition
> - Add linear and sRGB inverse EOTF LUTs
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/vkms/Makefile| 4 +-
> drivers/gpu/drm/vkms/vkms_colorop.c | 85 +++
> drivers/gpu/drm/vkms/vkms_composer.c | 45 ++
> drivers/gpu/drm/vkms/vkms_drv.h | 4 +
> drivers/gpu/drm/vkms/vkms_luts.c | 802 +++
> drivers/gpu/drm/vkms/vkms_luts.h | 12 +
> drivers/gpu/drm/vkms/vkms_plane.c| 2 +
> 7 files changed, 953 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/vkms/vkms_colorop.c
> create mode 100644 drivers/gpu/drm/vkms/vkms_luts.c
> create mode 100644 drivers/gpu/drm/vkms/vkms_luts.h
>
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 1b28a6a32948..c38455c46be4 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -6,6 +6,8 @@ vkms-y := \
> vkms_formats.o \
> vkms_crtc.o \
> vkms_composer.o \
> - vkms_writeback.o
> + vkms_writeback.o \
> + vkms_colorop.o \
> + vkms_luts.o
>
> obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
> b/drivers/gpu/drm/vkms/vkms_colorop.c
> new file mode 100644
> index ..9a26b9fdc4a2
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MAX_COLOR_PIPELINES 5
> +
> +const int vkms_initialize_tf_pipeline(struct drm_plane *plane, struct
> drm_prop_enum_list *list)
> +{
> +
> + struct drm_colorop *op, *prev_op;
> + struct drm_device *dev = plane->dev;
> + int ret;
> +
> + /* 1st op: 1d curve */
> + op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> + if (!op) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + return -ENOMEM;
> + }
> +
> + ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
> + if (ret)
> + return ret;
> +
> + list->type = op->base.id;
> + list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", op->base.id);
This is the string name for the pipeline (enum value).
I don't think including the ID in the name is good, because the ID
might be different on every load of the driver. IIRC you said userspace
is ok to hardcode the pipeline name and look for it, instead of doing
the discovery.
> +
> + prev_op = op;
> +
> + /* 2nd op: 1d curve */
> + op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> + if (!op) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + return -ENOMEM;
> + }
> +
> + ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
> + if (ret)
> + return ret;
> +
> + drm_colorop_set_next_property(prev_op, op);
> +
> + return 0;
> +}
> +
> +int vkms_initialize_colorops(struct drm_plane *plane)
> +{
> + struct drm_device *dev = plane->dev;
> + struct drm_property *prop;
> + struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES];
> + int len = 0;
> + int ret;
> +
> + /* Add "Bypass" (i.e. NULL) pipeline */
> + pipelines[len].type = 0;
> + pipelines[len].name = "Bypass";
> + len++;
Should there be a helper to replace the drm_property_create_enum() call
below that would always add "Bypass"?
Maybe that would push driver authors towards always supporting bypass
if at all possible.
It could also be called "Identity" pipeline, or is that too strict? I
wonder what happens with an YUV FB.
> +
> + /* Add pipeline consisting of transfer functions */
> + ret = vkms_initialize_tf_pipeline(plane, &(pipelines[len]));
> + if (ret)
> + return ret;
> + len++;
> +
> + /* Create COLOR_PIPELINE property and attach */
> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +
Re: [RFC PATCH v3 17/23] drm/colorop: Introduce DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE
On Wed, 8 Nov 2023 11:36:36 -0500
Harry Wentland wrote:
> With the introduction of the pre-blending color pipeline we
> can no longer have color operations that don't have a clear
> position in the color pipeline. We deprecate all existing
> plane properties. For upstream drivers those are:
> - COLOR_ENCODING
> - COLOR_RANGE
>
> Userspace that registers with the
> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE will be permitted to set
> the COLOR_PIPELINE plane property and drm_colorop properties.
> But it won't be allowed to set COLOR_ENCODING and
> COLOR_RANGE. Userspace that does not set this client cap
> will not be permitted to touch the color pipeline.
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 23 ++-
> drivers/gpu/drm/drm_ioctl.c | 7 +++
> include/drm/drm_file.h| 7 +++
> include/uapi/drm/drm.h| 15 +++
> 4 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index c6629fdaa114..69c56982e2d0 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -610,10 +610,26 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> } else if (property == plane->zpos_property) {
> state->zpos = val;
> } else if (property == plane->color_encoding_property) {
> + if (file_priv->plane_color_pipeline) {
> + drm_dbg_atomic(dev,
> +"Setting COLOR_PIPELINE plane property
> not permitted when DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set\n");
> + return -EINVAL;
> + }
> state->color_encoding = val;
> } else if (property == plane->color_range_property) {
> + if (file_priv->plane_color_pipeline) {
> + drm_dbg_atomic(dev,
> +"Setting COLOR_PIPELINE plane property
> not permitted when DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set\n");
> + return -EINVAL;
> + }
> state->color_range = val;
The above two cases mention COLOR_PIPELINE when they should probably
refer to something else.
Thanks,
pq
> } else if (property == plane->color_pipeline_property) {
> + if (!file_priv->plane_color_pipeline) {
> + drm_dbg_atomic(dev,
> +"Setting COLOR_PIPELINE plane property
> not permitted unless DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set\n");
> + return -EINVAL;
> + }
> +
> /* find DRM colorop object */
> struct drm_colorop *colorop = NULL;
> colorop = drm_colorop_find(dev, file_priv, val);
> @@ -1158,6 +1174,12 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
> case DRM_MODE_OBJECT_COLOROP: {
> + if (!file_priv->plane_color_pipeline) {
> + drm_dbg_atomic(prop->dev,
> +"[OBJECT:%d] is a colorop but
> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE not set\n",
> +obj->id);
> + ret = -EINVAL;
> + }
> struct drm_colorop *colorop = obj_to_colorop(obj);
> struct drm_colorop_state *colorop_state;
>
> @@ -1170,7 +1192,6 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> ret = drm_atomic_colorop_set_property(colorop,
> colorop_state, file_priv,
> prop, prop_value);
> -
> break;
> }
> default:
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 44fda68c28ae..0d869658e13e 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -361,6 +361,13 @@ drm_setclientcap(struct drm_device *dev, void *data,
> struct drm_file *file_priv)
> return -EINVAL;
> file_priv->writeback_connectors = req->value;
> break;
> + case DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE:
> + if (!file_priv->atomic)
> + return -EINVAL;
> + if (req->value > 1)
> + return -EINVAL;
> + file_priv->plane_color_pipeline = req->value;
> + break;
> default:
> return -EINVAL;
> }
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index e1b5b4282f75..bf11b646c898 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -204,6 +204,13 @@ struct drm_file {
>*/
> bool writeback_connectors;
>
> + /**
> + * @plane_color_pipeline:
> + *
> + * True if client understands plane color pipelines
> + */
> + bool plane_color_pipeline;
> +
> /**
>
Re: [RFC PATCH v3 18/23] drm/colorop: Add 3x4 CTM type
On Wed, 8 Nov 2023 11:36:37 -0500
Harry Wentland wrote:
> This type is used to support a 3x4 matrix in colorops. A 3x4
> matrix uses the last column as a "bias" column. Some HW exposes
> support for 3x4. The calculation looks like:
>
> out matrixin
> |R| |0 1 2 3 | | R |
> |G| = |4 5 6 7 | x | G |
> |B| |8 9 10 12| | B |
>|1.0|
>
The bottom-right element should be 11, not 12.
> This is also the first colorop where we need a blob property to
> program the property. For that we'll introduce a new DATA
> property that can be used by all colorop TYPEs requiring a
> blob. The way a DATA blob is read depends on the TYPE of
> the colorop.
>
> We only create the DATA property for property types that
> need it.
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 30 ++
> drivers/gpu/drm/drm_colorop.c | 16
> include/drm/drm_colorop.h | 19 +++
> include/uapi/drm/drm_mode.h | 19 ++-
> 4 files changed, 83 insertions(+), 1 deletion(-)
...
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 0ebf0f480dc8..b5d4dd5660d9 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -847,6 +847,22 @@ struct drm_color_ctm {
> __u64 matrix[9];
> };
>
> +struct drm_color_ctm_3x4 {
> + /*
> + * Conversion matrix with 3x4 dimensions in S31.32 sign-magnitude
> + * (not two's complement!) format.
> + *
> + * TODO what's the format?
> + *
> + * out matrix in
> + * |R| |0 1 2 3 | | R |
> + * |G| = |4 5 6 7 | x | G |
> + * |B| |8 9 10 12| | B |
> + * |1.0|
> + */
Again 11, not 12.
Thanks,
pq
> + __u64 matrix[12];
> +};
> +
> struct drm_color_lut {
> /*
>* Values are mapped linearly to 0.0 - 1.0 range, with 0x0 == 0.0 and
> @@ -859,7 +875,8 @@ struct drm_color_lut {
> };
>
> enum drm_colorop_type {
> - DRM_COLOROP_1D_CURVE
> + DRM_COLOROP_1D_CURVE,
> + DRM_COLOROP_CTM_3X4,
> };
>
> /**
pgpgqfoauglFr.pgp
Description: OpenPGP digital signature
Re: [RFC PATCH v3 20/23] drm/vkms: Use s32 for internal color pipeline precision
On Wed, 8 Nov 2023 11:36:39 -0500
Harry Wentland wrote:
> Certain operations require us to preserve values below 0.0 and
> above 1.0 (0x0 and 0x respectively in 16 bpc unorm). One
> such operation is a BT709 encoding operation followed by its
> decoding operation, or the reverse.
>
> We'll use s32 values as intermediate in and outputs of our
> color operations, for the operations where it matters.
>
> For now this won't apply to LUT operations. We might want to
> update those to work on s32 as well, but it's unclear how
> that should work for unorm LUT definitions. We'll revisit
> that once we add LUT + CTM tests.
I think it's quite clear how those LUTs work: the assumed domain is
always [0.0, 1.0] unless otherwise stated. Anything outside of the
domain is theoretically undefined, but since raising an error is out of
the question, a LUT can clamp its input to its defined domain.
Btw. this reflects all the way to the colorop UAPI.
> In order to allow for this we'll also invert the nesting of our
> colorop processing loops. We now use the pixel iteration loop
> on the outside and the colorop iteration on the inside.
Yes, that's how it is probably the best, which kind of breaks my
previous comment about moving conditionals outside of the innermost
loop. You could pre-construct an array of function pointers and
arguments to run through in the innermost loop, but it's not clear to
me if that has a significant performance improvement.
> v3:
> - Use new colorop->next pointer
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 55 +---
> drivers/gpu/drm/vkms/vkms_drv.h | 4 ++
> 2 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 9010415e4bd6..d04a235b9fcd 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -164,7 +164,7 @@ static void apply_lut(const struct vkms_crtc_state
> *crtc_state, struct line_buff
> }
> }
>
> -static void apply_colorop(struct pixel_argb_u16 *pixel, struct drm_colorop
> *colorop)
> +static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop
> *colorop)
IIRC, this will cause an implicit cast from s32 to u16, which is very
different from a clamp. I wouldn't call this "won't apply".
> {
> /* TODO is this right? */
> struct drm_colorop_state *colorop_state = colorop->state;
> @@ -191,25 +191,54 @@ static void apply_colorop(struct pixel_argb_u16 *pixel,
> struct drm_colorop *colo
>
> static void pre_blend_color_transform(const struct vkms_plane_state
> *plane_state, struct line_buffer *output_buffer)
> {
> - struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
> + struct drm_colorop *colorop;
> + struct pixel_argb_s32 pixel;
>
> - while (colorop) {
> - struct drm_colorop_state *colorop_state;
> + for (size_t x = 0; x < output_buffer->n_pixels; x++) {
> +
> + /*
> + * Some operations, such as applying a BT709 encoding matrix,
> + * followed by a decoding matrix, require that we preserve
> + * values above 1.0 and below 0.0 until the end of the pipeline.
> + *
> + * Convert values to s32 for our internal pipeline and go back
> + * to u16 values at the end.
> + */
> + pixel.a = output_buffer->pixels[x].a;
> + pixel.r = output_buffer->pixels[x].r;
> + pixel.g = output_buffer->pixels[x].g;
> + pixel.b = output_buffer->pixels[x].b;
> +
> + colorop = plane_state->base.base.color_pipeline;
> + while (colorop) {
> + struct drm_colorop_state *colorop_state;
>
> - if (!colorop)
> - return;
> + if (!colorop)
> + return;
>
> - /* TODO this is probably wrong */
> - colorop_state = colorop->state;
> + /* TODO this is probably wrong */
> + colorop_state = colorop->state;
>
> - if (!colorop_state)
> - return;
> + if (!colorop_state)
> + return;
>
> - for (size_t x = 0; x < output_buffer->n_pixels; x++)
> if (!colorop_state->bypass)
> - apply_colorop(&output_buffer->pixels[x],
> colorop);
> + apply_colorop(&pixel, colorop);
> +
> + colorop = colorop->next;
> + }
>
> - colorop = colorop->next;
> + /* clamp pixel */
> + pixel.a = max(min(pixel.a, 0x), 0x0);
> + pixel.r = max(min(pixel.r, 0x), 0x0);
> + pixel.g = max(min(pixel.g, 0x), 0x0);
> + pixel.b = max(min(pixel.b
Re: [RFC PATCH v3 21/23] drm/vkms: add 3x4 matrix in color pipeline
On Wed, 8 Nov 2023 11:36:40 -0500
Harry Wentland wrote:
> We add two 3x4 matrices into the VKMS color pipeline. The reason
> we're adding matrices is so that we can test that application
> of a matrix and its inverse yields an output equal to the input
> image.
Would it not be better to mimic what a hardware driver might likely
have? Or maybe that will be just a few more pipelines?
People testing their compositors would likely expect a more usual
pipeline arrangement.
> One complication with the matrix implementation has to do with
> the fact that the matrix entries are in signed-magnitude fixed
> point, whereas the drm_fixed.h implementation uses 2s-complement.
> The latter one is the one that we want for easy addition and
> subtraction, so we convert all entries to 2s-complement.
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/vkms/vkms_colorop.c | 32 +++-
> drivers/gpu/drm/vkms/vkms_composer.c | 27 +++
> 2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
> b/drivers/gpu/drm/vkms/vkms_colorop.c
> index 9a26b9fdc4a2..4e37e805c443 100644
> --- a/drivers/gpu/drm/vkms/vkms_colorop.c
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -31,7 +31,37 @@ const int vkms_initialize_tf_pipeline(struct drm_plane
> *plane, struct drm_prop_e
>
> prev_op = op;
>
> - /* 2nd op: 1d curve */
> + /* 2nd op: 3x4 matrix */
> + op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> + if (!op) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + return -ENOMEM;
> + }
> +
> + ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_CTM_3X4);
> + if (ret)
> + return ret;
> +
> + drm_colorop_set_next_property(prev_op, op);
> +
> + prev_op = op;
> +
> + /* 3rd op: 3x4 matrix */
> + op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> + if (!op) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + return -ENOMEM;
> + }
> +
> + ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_CTM_3X4);
> + if (ret)
> + return ret;
> +
> + drm_colorop_set_next_property(prev_op, op);
> +
> + prev_op = op;
> +
> + /* 4th op: 1d curve */
> op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> if (!op) {
> DRM_ERROR("KMS: Failed to allocate colorop\n");
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index d04a235b9fcd..c278fb223188 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -164,6 +164,30 @@ static void apply_lut(const struct vkms_crtc_state
> *crtc_state, struct line_buff
> }
> }
>
> +static void apply_3x4_matrix(struct pixel_argb_s32 *pixel, const struct
> drm_color_ctm_3x4 *matrix)
> +{
> + s64 rf, gf, bf;
> +
> + rf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[0]),
> drm_int2fixp(pixel->r)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[1]),
> drm_int2fixp(pixel->g)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[2]),
> drm_int2fixp(pixel->b)) +
> + drm_sm2fixp(matrix->matrix[3]);
Again, if you went for performance, you'd make a copy of the matrix in
fixp format in advance, to avoid having to convert the same thing for
every processed pixel.
> +
> + gf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[4]),
> drm_int2fixp(pixel->r)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[5]),
> drm_int2fixp(pixel->g)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[6]),
> drm_int2fixp(pixel->b)) +
> + drm_sm2fixp(matrix->matrix[7]);
> +
> + bf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[8]),
> drm_int2fixp(pixel->r)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[9]),
> drm_int2fixp(pixel->g)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[10]),
> drm_int2fixp(pixel->b)) +
> + drm_sm2fixp(matrix->matrix[11]);
Likewise the repetition of int2fixp three times for the same value is
probably hurting unless the compiler knows to eliminate the redundant
calls.
> +
> + pixel->r = drm_fixp2int(rf);
> + pixel->g = drm_fixp2int(gf);
> + pixel->b = drm_fixp2int(bf);
Btw. why pick s32 and not fixp for your intermediate type?
Using both you get the limitations of both in range and precision.
Thanks,
pq
> +}
> +
> static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop
> *colorop)
> {
> /* TODO is this right? */
> @@ -185,6 +209,9 @@ static void apply_colorop(struct pixel_argb_s32 *pixel,
> struct drm_colorop *colo
> DRM_DEBUG_DRIVER("unkown colorop 1D curve type
> %d\n", colorop_state->curve_1d_type);
> break;
> }
> + } else if (colorop->type == DRM_COLOROP_CTM_3X4) {
> + if (colorop_state->data)
> + apply_3x4_mat
Re: [RFC PATCH v3 23/23] drm/vkms: Add tests for CTM handling
On Wed, 8 Nov 2023 11:36:42 -0500
Harry Wentland wrote:
> A whole slew of tests for CTM handling that greatly helped in
> debugging the CTM code. The extent of tests might seem a bit
> silly but they're fast and might someday help save someone
> else's day when debugging this.
To be honest, this looks pretty limited testing. I guess the more
exhaustive tests are in IGT though, would be nice to mention that in
the commit message.
There is not a single case for out of [0.0, 1.0] input nor output.
The offset part is always zero.
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 258 ++
> drivers/gpu/drm/vkms/vkms_composer.c | 2 +-
> 2 files changed, 259 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> index ad4c2f72fd1e..3eaa2233afbb 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> @@ -3,6 +3,7 @@
> #include
>
> #include
> +#include
>
> #define TEST_LUT_SIZE 16
>
> @@ -80,11 +81,268 @@ static void vkms_color_srgb_inv_srgb(struct kunit *test)
> }
> }
>
> +#define FIXPT_HALF(DRM_FIXED_ONE >> 1)
> +#define FIXPT_QUARTER (DRM_FIXED_ONE >> 2)
> +
> +const struct drm_color_ctm_3x4 test_matrix_3x4_50_desat = { {
> + FIXPT_HALF, FIXPT_QUARTER, FIXPT_QUARTER, 0,
> + FIXPT_QUARTER, FIXPT_HALF, FIXPT_QUARTER, 0,
> + FIXPT_QUARTER, FIXPT_QUARTER, FIXPT_HALF, 0
> +} };
> +
> +static void vkms_color_ctm_3x4_50_desat(struct kunit *test)
> +{
> + struct pixel_argb_s32 ref, out;
> +
> + /* full white */
> + ref.a = 0x0;
> + ref.r = 0x;
> + ref.g = 0x;
> + ref.b = 0x;
> +
> + memcpy(&out, &ref, sizeof(out));
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +
> + /* full black */
> + ref.a = 0x0;
This is transparent, btw.
> + ref.r = 0x0;
> + ref.g = 0x0;
> + ref.b = 0x0;
> +
> + memcpy(&out, &ref, sizeof(out));
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +
> + /* 50% grey */
> + ref.a = 0x0;
> + ref.r = 0x8000;
> + ref.g = 0x8000;
> + ref.b = 0x8000;
> +
> + memcpy(&out, &ref, sizeof(out));
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +
> + /* full red to 50% desat */
> + ref.a = 0x0;
> + ref.r = 0x7fff;
> + ref.g = 0x3fff;
> + ref.b = 0x3fff;
> +
> + out.a = 0x0;
> + out.r = 0x;
> + out.g = 0x0;
> + out.b = 0x0;
> +
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +}
> +
> +const struct drm_color_ctm_3x4 test_matrix_3x4_bt709_enc = { {
> + 0x366cf400ull, 0xb7175900ull, 0x000127bb300ull, 0,
> + 0x80001993b3a0ull, 0x80005609fe80ull, 0x6f9db200ull, 0,
> + 0x9d70a400ull, 0x80008f011100ull, 0x8e6f9330ull, 0
> +} };
Would be nice to have a comment explaining where these values came
from, like a snippet of Python printing these. The conversion from real
numbers to this representation is the interesting part.
> +
> +static void vkms_color_ctm_3x4_bt709(struct kunit *test)
> +{
> + struct pixel_argb_s32 ref, out;
> +
> + /* full white to bt709 */
> + ref.a = 0x0;
> + ref.r = 0xfffe; /* off by one in 16bpc not a big deal */
> + ref.g = 0x0;
> + ref.b = 0x0;
> +
> + out.a = 0x0;
> + out.r = 0x;
> + out.g = 0x;
> + out.b = 0x;
> +
> + apply_3x4_matrix(&out, &test_matrix_3x4_bt709_enc);
> +
> + /* red 255 */
> + KUNIT_EXPECT_GT(test, out.r, 0xfe00);
This allows result grossly greater than 1.0.
> +
> + /* green 0 */
> + KUNIT_EXPECT_LT(test, out.g, 0x0100);
This allows grossly negative results.
> +
> + /* blue 0 */
> + KUNIT_EXPECT_LT(test, out.b, 0x0100);
> +
> + /* full black to bt709 */
> + ref.a = 0x0;
> + ref.r = 0x0; /* off by one in 16bpc not a big deal */
> + ref.g = 0x0;
> + ref.b = 0x0;
> +
> + out.a = 0x0;
> + out.r = 0x0;
> + out.g = 0x0;
> + out.b = 0x0;
> +
> + apply_3x4_matrix(&out, &test_matrix_3x4_bt709_enc);
> +
> + /* red 0 */
> + KUNIT_EXPECT_LT(test, out.r, 0x100);
> +
> + /* green 0 */
> + KUNIT_EXPECT_LT(test, out.g, 0x0100);
> +
> + /* blue 0 */
> + KUNIT_EXPECT_LT(test, out.b, 0x0100);
> +
> + /* gray to bt709 */
> + ref.a = 0x0;
> + ref.r = 0x7fff; /* off by one in 16bpc not a big deal */
> + ref.g = 0x0;
> + ref.b = 0x0;
This ref does not seem to be used?
> +
> + out.a = 0x0;
> + out.r = 0x7fff;
> + out.g = 0x7fff;
> + out.b = 0x7fff;
> +
> + apply_
Re: what are the protocol rules about uniqueness of event and request names?
On Fri, 8 Dec 2023 12:54:35 +0100 Sebastian Wick wrote: > ... > I think a more useful thing to do would be to add this restriction (an > interface cannot have an event and a request with the same name) to > the documentation and to wayland-scanner. > Also: an event and request with the same name would probably confuse anyone using WAYLAND_DEBUG. But: Would changing wayland-scanner to prevent this be backward compatible? Can't someone somewhere already have an event/request pair with the same name in their own private protocol extension?
