Re: [RFC 02/33] drm: Add color operation structure
On Mon, 4 Sep 2023 14:10:05 +
"Shankar, Uma" wrote:
> > -Original Message-
> > From: dri-devel On Behalf Of Pekka
> > Paalanen
> > Sent: Wednesday, August 30, 2023 6:30 PM
> > To: Shankar, Uma
> > Cc: intel-...@lists.freedesktop.org; Borah, Chaitanya Kumar
> > ; dri-devel@lists.freedesktop.org; wayland-
> > de...@lists.freedesktop.org
> > Subject: Re: [RFC 02/33] drm: Add color operation structure
> >
> > On Tue, 29 Aug 2023 21:33:51 +0530
> > Uma Shankar wrote:
> >
> > > From: Chaitanya Kumar Borah
> > >
> > > Each Color Hardware block will be represented uniquely in the color
> > > pipeline. Define the structure to represent the same.
> > >
> > > These color operations will form the building blocks of a color
> > > pipeline which best represents the underlying Hardware. Color
> > > operations can be re-arranged, substracted or added to create distinct
> > > color pipelines to accurately describe the Hardware blocks present in
> > > the display engine.
> > >
> > > Co-developed-by: Uma Shankar
> > > Signed-off-by: Uma Shankar
> > > Signed-off-by: Chaitanya Kumar Borah
> > > ---
> > > include/uapi/drm/drm_mode.h | 72
> > > +
> > > 1 file changed, 72 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index ea1b639bcb28..882479f41745 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -943,6 +943,78 @@ struct hdr_output_metadata {
> > > };
> > > };
> > >
> > > +/**
> > > + * enum color_op_block
> > > + *
> > > + * Enums to identify hardware color blocks.
> > > + *
> > > + * @DRM_CB_PRE_CSC: LUT before the CTM unit
> > > + * @DRM_CB_CSC: CTM hardware supporting 3x3 matrix
> > > + * @DRM_CB_POST_CSC: LUT after the CTM unit
> > > + * @DRM_CB_3D_LUT: LUT hardware with coefficients for all
> > > + * color components
> > > + * @DRM_CB_PRIVATE: Vendor specific hardware unit. Vendor
> > > + * can expose a custom hardware by defining a
> > > + * color operation block with this name as
> > > + * identifier
> >
> > This naming scheme does not seem to work. It assumes a far too rigid
> > pipeline,
> > just like the old KMS property design. What if you have two other operations
> > between PRE_CSC and CSC?
> >
> > What sense do PRE_CSC and POST_CSC make if you don't happen to have a CSC
> > operation?
>
> Sure, we can re-look at the naming. However, it will be good to define some
> standard
> operations common to all vendors and keep the rest as vendor private.
My opinion is that these "names" should not even be an enum at all.
You're talking about operations, but operation is 'type', not 'name'.
There needs to be no pre-defined, hardcoded pipeline structure in the
UAPI design. Your naming scheme, and the old KMS color property design,
assume that there is such a rigid pre-defined structure that all
hardware and drivers must adhere to. That does not work.
Drivers need to be able to expose any arbitrary operations in any
arbitrary order in each pipeline, with no pre-determined or hinted
"intended use". We want to define the operations, and not say what they
should be used for. Usage examples are for accompanying documentation,
not API tokens.
This idea is at the core of the new color pipeline design we have been
discussing.
> > What if a driver put POST_CSC before PRE_CSC in its pipeline?
> >
> > What if your CSC is actually a series of three independent operations, and
> > in
> > addition you have PRE_CSC and POST_CSC?
>
> We should try to standardized the operations as much as possible and leave
> rest as
> vendor private. Current proposal allows us to do that.
>
> > 3D_LUT is an operation category, not a name. The same could be said about
> > private.
>
> Sure, will fix this.
>
> > Given that all these are also UAPI, do we also need protect old userspace
> > from
> > seeing values it does not understand?
>
> For the values userspace doesn't understand, it can ignore the blocks. We
> should ensure
> that userspace always gets a clean state wrt color hardware state and no
> baggage from
> another client should be there. With that there is no burden of disabling
> that particular
> block will be there on an older userspace.
>
RE: [RFC 02/33] drm: Add color operation structure
> -Original Message-
> From: dri-devel On Behalf Of Pekka
> Paalanen
> Sent: Wednesday, August 30, 2023 6:30 PM
> To: Shankar, Uma
> Cc: intel-...@lists.freedesktop.org; Borah, Chaitanya Kumar
> ; dri-devel@lists.freedesktop.org; wayland-
> de...@lists.freedesktop.org
> Subject: Re: [RFC 02/33] drm: Add color operation structure
>
> On Tue, 29 Aug 2023 21:33:51 +0530
> Uma Shankar wrote:
>
> > From: Chaitanya Kumar Borah
> >
> > Each Color Hardware block will be represented uniquely in the color
> > pipeline. Define the structure to represent the same.
> >
> > These color operations will form the building blocks of a color
> > pipeline which best represents the underlying Hardware. Color
> > operations can be re-arranged, substracted or added to create distinct
> > color pipelines to accurately describe the Hardware blocks present in
> > the display engine.
> >
> > Co-developed-by: Uma Shankar
> > Signed-off-by: Uma Shankar
> > Signed-off-by: Chaitanya Kumar Borah
> > ---
> > include/uapi/drm/drm_mode.h | 72
> > +
> > 1 file changed, 72 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index ea1b639bcb28..882479f41745 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -943,6 +943,78 @@ struct hdr_output_metadata {
> > };
> > };
> >
> > +/**
> > + * enum color_op_block
> > + *
> > + * Enums to identify hardware color blocks.
> > + *
> > + * @DRM_CB_PRE_CSC: LUT before the CTM unit
> > + * @DRM_CB_CSC: CTM hardware supporting 3x3 matrix
> > + * @DRM_CB_POST_CSC: LUT after the CTM unit
> > + * @DRM_CB_3D_LUT: LUT hardware with coefficients for all
> > + * color components
> > + * @DRM_CB_PRIVATE: Vendor specific hardware unit. Vendor
> > + * can expose a custom hardware by defining a
> > + * color operation block with this name as
> > + * identifier
>
> This naming scheme does not seem to work. It assumes a far too rigid pipeline,
> just like the old KMS property design. What if you have two other operations
> between PRE_CSC and CSC?
>
> What sense do PRE_CSC and POST_CSC make if you don't happen to have a CSC
> operation?
Sure, we can re-look at the naming. However, it will be good to define some
standard
operations common to all vendors and keep the rest as vendor private.
> What if a driver put POST_CSC before PRE_CSC in its pipeline?
>
> What if your CSC is actually a series of three independent operations, and in
> addition you have PRE_CSC and POST_CSC?
We should try to standardized the operations as much as possible and leave rest
as
vendor private. Current proposal allows us to do that.
> 3D_LUT is an operation category, not a name. The same could be said about
> private.
Sure, will fix this.
> Given that all these are also UAPI, do we also need protect old userspace from
> seeing values it does not understand?
For the values userspace doesn't understand, it can ignore the blocks. We
should ensure
that userspace always gets a clean state wrt color hardware state and no
baggage from
another client should be there. With that there is no burden of disabling that
particular
block will be there on an older userspace.
> > + */
> > +enum color_op_block {
> > + DRM_CB_INVAL = -1,
> > +
> > + DRM_CB_PRE_CSC = 0,
> > + DRM_CB_CSC,
> > + DRM_CB_POST_CSC,
> > + DRM_CB_3D_LUT,
> > +
> > + /* Any new generic hardware block can be updated here */
> > +
> > + /*
> > +* PRIVATE is kept at 255 to make it future proof and leave
> > +* scope for any new addition
> > +*/
> > + DRM_CB_PRIVATE = 255,
> > + DRM_CB_MAX = DRM_CB_PRIVATE,
> > +};
> > +
> > +/**
> > + * enum color_op_type
> > + *
> > + * These enums are to identify the mathematical operation that
> > + * a hardware block is capable of.
> > + * @CURVE_1D: It represents a one dimensional lookup table
> > + * @CURVE_3D: Represents lut value for each color component for 3d
> > +lut capable hardware
> > + * @MATRIX: It represents co-efficients for a CSC/CTM matrix hardware
> > + * @FIXED_FUNCTION: To enable and program any custom fixed function
> > +hardware unit */ enum color_op_type {
> > + CURVE_1D,
> > + CURVE_3D,
> > + MATRIX,
> > + FIXED_FUNCTION,
>
> My assumption was that a color_op_type would clearly and uniquely define the
> mathematical model of t
Re: [RFC 02/33] drm: Add color operation structure
On Tue, 29 Aug 2023 21:33:51 +0530
Uma Shankar wrote:
> From: Chaitanya Kumar Borah
>
> Each Color Hardware block will be represented uniquely
> in the color pipeline. Define the structure to represent
> the same.
>
> These color operations will form the building blocks of
> a color pipeline which best represents the underlying
> Hardware. Color operations can be re-arranged, substracted
> or added to create distinct color pipelines to accurately
> describe the Hardware blocks present in the display engine.
>
> Co-developed-by: Uma Shankar
> Signed-off-by: Uma Shankar
> Signed-off-by: Chaitanya Kumar Borah
> ---
> include/uapi/drm/drm_mode.h | 72 +
> 1 file changed, 72 insertions(+)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ea1b639bcb28..882479f41745 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -943,6 +943,78 @@ struct hdr_output_metadata {
> };
> };
>
> +/**
> + * enum color_op_block
> + *
> + * Enums to identify hardware color blocks.
> + *
> + * @DRM_CB_PRE_CSC: LUT before the CTM unit
> + * @DRM_CB_CSC: CTM hardware supporting 3x3 matrix
> + * @DRM_CB_POST_CSC: LUT after the CTM unit
> + * @DRM_CB_3D_LUT: LUT hardware with coefficients for all
> + * color components
> + * @DRM_CB_PRIVATE: Vendor specific hardware unit. Vendor
> + * can expose a custom hardware by defining a
> + * color operation block with this name as
> + * identifier
This naming scheme does not seem to work. It assumes a far too rigid
pipeline, just like the old KMS property design. What if you have two
other operations between PRE_CSC and CSC?
What sense do PRE_CSC and POST_CSC make if you don't happen to have a
CSC operation?
What if a driver put POST_CSC before PRE_CSC in its pipeline?
What if your CSC is actually a series of three independent operations,
and in addition you have PRE_CSC and POST_CSC?
3D_LUT is an operation category, not a name. The same could be said
about private.
Given that all these are also UAPI, do we also need protect old
userspace from seeing values it does not understand?
> + */
> +enum color_op_block {
> + DRM_CB_INVAL = -1,
> +
> + DRM_CB_PRE_CSC = 0,
> + DRM_CB_CSC,
> + DRM_CB_POST_CSC,
> + DRM_CB_3D_LUT,
> +
> + /* Any new generic hardware block can be updated here */
> +
> + /*
> + * PRIVATE is kept at 255 to make it future proof and leave
> + * scope for any new addition
> + */
> + DRM_CB_PRIVATE = 255,
> + DRM_CB_MAX = DRM_CB_PRIVATE,
> +};
> +
> +/**
> + * enum color_op_type
> + *
> + * These enums are to identify the mathematical operation that
> + * a hardware block is capable of.
> + * @CURVE_1D: It represents a one dimensional lookup table
> + * @CURVE_3D: Represents lut value for each color component for 3d lut
> capable hardware
> + * @MATRIX: It represents co-efficients for a CSC/CTM matrix hardware
> + * @FIXED_FUNCTION: To enable and program any custom fixed function hardware
> unit
> + */
> +enum color_op_type {
> + CURVE_1D,
> + CURVE_3D,
> + MATRIX,
> + FIXED_FUNCTION,
My assumption was that a color_op_type would clearly and uniquely
define the mathematical model of the operation and the UABI structure
of the parameter blob. That means we need different values for uniform
vs. exponentially vs. programmable distributed 1D LUT, etc.
If there is a 1D curve with pre-programmed (fixed and named) curves, we
need to enumerate all the curve types somehow. Probably each fixed
curve type should not be a different operation type, because that would
explode the number of alternative pipelines.
A 3D curve in my mind is a function {x,y,z} = f(t), while I suspect you
meant a 3D LUT which is a {x,y,z} = f(t,u,v) - a 3-vector field in
three dimensional space.
A matrix element could be with or without an offset vector I guess.
FIXED_FUNCTION would need to be replaced with e.g. your example
VENDORXXX_BT602_TO_BT2020 to work.
Have I missed something, how did you intend this to work?
Thanks,
pq
> +};
> +
> +/**
> + * @struct drm_color_op
> + *
> + * This structure is used to represent the capability of
> + * individual color hardware blocks.
> + *
> + * @name: a standardized enum to identify the color hardware block
> + * @type: The type of mathematical operation it can perform
> + * @blob_id: Id pointing to a blob containing information about
> + * the hardware block which advertizes its capabilities
> + * to the userspace. It can be an optional field depending
> + * on the members "name" and "type".
> + * @private_flags: This can be used to provide vendor specific hints
> + * to user space
> + */
> +struct drm_color_op {
> + enum color_op_block name;
> + enum color_op_type type;
> + __u32 blob_id;
> + __u32 private_flags;
> +};
> +
> /**
> *
