Re: [Intel-gfx] [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane Color Pipeline

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 13770 bytes --]

On Wed, 30 Aug 2023 08:59:36 +0000
"Shankar, Uma" <uma.shankar@intel.com> wrote:

> > -----Original Message-----
> > From: Harry Wentland <harry.wentland@amd.com>
> > Sent: Wednesday, August 30, 2023 1:10 AM
> > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org
> > Cc: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; wayland-
> > devel@lists.freedesktop.org
> > Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane Color
> > Pipeline
> > 
> > 
> > 
> > On 2023-08-29 12:03, Uma Shankar wrote:  
> > > Add the documentation for the new proposed Plane Color Pipeline.
> > >
> > > Co-developed-by: Chaitanya Kumar Borah
> > > <chaitanya.kumar.borah@intel.com>
> > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > ---
> > >   .../gpu/rfc/plane_color_pipeline.rst          | 394 ++++++++++++++++++
> > >   1 file changed, 394 insertions(+)
> > >   create mode 100644 Documentation/gpu/rfc/plane_color_pipeline.rst
> > >
> > > diff --git a/Documentation/gpu/rfc/plane_color_pipeline.rst
> > > b/Documentation/gpu/rfc/plane_color_pipeline.rst
> > > new file mode 100644
> > > index 000000000000..60ce515b6ea7
> > > --- /dev/null
> > > +++ b/Documentation/gpu/rfc/plane_color_pipeline.rst

...

Hi Uma!

> > > +This color pipeline is then packaged within a blob for the user space
> > > +to retrieve it. Details can be found in the next section
> > > +  
> > 
> > Not sure I like blobs that contain other blob ids.  
> 
> It provides flexibility and helps with just one interface to userspace. Its easy to handle and
> manage once we get the hang of it 😊.
> 
> We can clearly define the steps of parsing and data structures to be used while interpreting
> and parsing the blobs.

Don't forget extendability. Possibly every single struct will need some
kind of versioning, and then it's not simple to parse anymore. Add to
that new/old kernel vs. old/new userspace, and it seems a bit
nightmarish to design.

Also since it's records inside a single blob, it's like a new file
format: every record needs a standard header that allows skipping it
appropriately if userspace does not understand it, or you need a
standard index telling where everything is. Making all records the same
size would waste space, and extendability requires variable size.

I also would not assume that we can declare a standard set of blocks
and that nothing else will be needed. The existing hardware is too
diverse for that from what I have understood. I assume that some
hardware have blocks unique to them, and they want to at least expose
that functionality through a UAPI that allows at least generic
enumeration of functionality, even if it needs specialized userspace
code to actually make use of.

If we go with

+struct drm_color_op {
+	enum color_op_block name;
+	enum color_op_type type;
+	u32 blob_id;
+	u32 private_flags;
+};

as in your proposal, I believe it can work (sorry, looking further
down, I have assumed too much of 'type'), but the enumerations will
become long, and the details blob_id is still specific to 'type'. This
is unavoidable, but we can still choose the form between blobs and
properties, integers and strings.

I have a feeling that introspection will be valuable here, to help
people understand what their hardware could do if they had the code to
use it. 'name' and 'type' being integers require a translation table to
strings before they are readable, and it would be best if the kernel
itself provided that translation.

I don't understand how 'private_flags' could be useful. There must not
be any "hidden" features. Everything a block can be programmed to do
via this UAPI must be clearly documented, there cannot be anything
private. If two hardware versions of a block differ in a meaningful or
significant way, they need to be exposed as different types of blocks.

OTOH, if one goes with a (new) DRM object with string named properties
model, all that struct versioning and file format hassle has mostly a
clear and well-understood solution. We only need to define the rules of
how userspace needs to deal with properties or values it does not
understand, so that the kernel can keep adding more.

Therefore, I'm not yet convinced with the "all blobs" design.

> > > +Exposing a color pipeline to user space
> > > +=======================================
> > > +
> > > +To advertise the available color pipelines, an immutable ENUM
> > > +property "GET_COLOR_PIPELINE" is introduced.
> > > +This enum property has blob id's as values. With each blob id
> > > +representing a distinct color pipeline based on underlying HW
> > > +capabilities and their respective combinations.
> > > +
> > > +The following output of drm_info [1], shows how color pipelines are
> > > +visible to userspace.
> > > +
> > > +├───Plane 0
> > > +    │   ├───Object ID: 31
> > > +    │   ├───CRTCs: {0}
> > > +    │   ├───Legacy info
> > > +        ...
> > > +    │       ├───"GET_COLOR_PIPELINE" (immutable): enum {no color pipeline,
> > > +						color pipeline 1, color pipeline 2}=  
> > no color pipeline  
> > > +
> > > +To understand the capabilities of individual pipelines, first the
> > > +userspace need to retrieve the pipeline blob with the blob ids
> > > +retrieved by reading the enum property.
> > > +
> > > +Once the color pipeline is retrieved, user can then parse through
> > > +individual drm_color_op blocks to understand the capabilities of each
> > > +hardware block.
> > > +
> > > +Check IGT series to see how user space can parse through color pipelines.
> > > +Refer the IGT series here:
> > > +https://patchwork.freedesktop.org/series/123018/
> > > +
> > > +Setting up a color pipeline
> > > +===========================
> > > +
> > > +Once the user space decides on a color pipeline, it can set the
> > > +pipeline and the corresponding data for the hardware blocks within
> > > +the pipeline with the BLOB property "SET_COLOR_PIPELINE".
> > > +
> > > +To achieve this two structures are introduced
> > > +
> > > +1.	struct drm_color_op_data: It represents data to be passed onto individual
> > > +							  color hardware blocks. It  
> > contains three members  
> > > +                      a) name: to identify the color operation block
> > > +                      b) blob_id: pointing to the blob with data for the
> > > +                                  corresponding hardware block
> > > +
> > > +	struct drm_color_op_data {
> > > +		enum color_op_block name;

Why is this a global fixed enum rather than a pipeline specific ordinal
or a unique-per-device ID?

There is no reason to believe that a 'name' always matches a hardware
block 1:1. When drivers accumulate multiple different alternative
pipelines due to backwards-compatibility reasons, the same 'name' could
be implemented by different hardware blocks, or the same hardware block
could implement different 'name's from different pipelines.

The names have also a problem. If you name something "pre-csc", then
how do you name the thing that the next hardware version adds between
"pre-csc" and "csc"?

> > > +		u32 blob_id;
> > > +	};
> > > +
> > > +2.	struct drm_color_pipeline: This structure represents the aggregate
> > > +                                   pipeline to be set. it contains the following  members
> > > +					  a) num: pipeline number to be selected
> > > +					  b) size: size of the data to be passed onto  
> > the driver  
> > > +					  c) data: array of struct  
> > drm_color_op_data with data for  
> > > +                                                   the hardware block/s that userspace wants to
> > > +                                                   set values for.
> > > +
> > > +	struct drm_color_pipeline {
> > > +		int num;
> > > +		int size;
> > > +		struct drm_color_op_data *data;
> > > +	};
> > > +
> > > +	User can either:
> > > +	1. send data for all the color operations in a pipeline as shown in [2].
> > > +	   The color operation data need not be in order that the pipeline advertises
> > > +	   however, it should not contain data for any
> > > +	   color operation that is not present in the pipeline.
> > > +
> > > +	   Note: This check for pipeline is not yet implemented but if the
> > > +	   wider proposal is accepted we have few solutions in mind.
> > > +
> > > +	2. send data for a subset of color operations as shown in [3].For the
> > > +	   color operation that userspace does not send data will retain their
> > > +	   older state.

Retaining existing state, especially for operations that userspace does
not understand, can lead to incorrect and unexpected results. That's
why I say that userspace must understand all operations in a pipeline,
and all parameters of all used operations before it can actually use
that pipeline.

Otherwise we have the same problem as KMS properties have in general
today: when new things are added that userspace does not understand,
how will the userspace be able to maintain its old behaviour?

That question has two answers today:
- userspace learns to program everything, and accidentally
  (mal)functions until then
- you do not switch between KMS clients that might leave incorrect
  state in not-understood properties

Neither is a good answer, and the problem does not seem to have any
practical traction either.

For color pipelines I hope we can, and believe that we must, do better
to maintain correct behaviour while extending functionality.

> > > +
> > > +	3. reset/disable the pipeline by setting the "SET_COLOR_PIPELINE" blob
> > > +	   property to NULL as shown in both [2] and [3]
> > > +

Is it a reset and disable, or only disable?

How is the reset state defined, if that state becomes active when the
pipeline is next enabled and data not set for the operations?

> > > +	4. change the color pipeline as demonstrated in [3].
> > > +	   On the new pipeline, the user is expected to setup all the color hardware  
> > block  
> > > +	   Once the user requests a pipeline change, the driver will provide it a clean  
> > slate  
> > > +           which means that all the data previously set by the user will be discarded  
> > even if  
> > > +           there are common hardware blocks between the two(previous and current)  
> > pipelines.  
> > > +

Yes, alternative pipelines need to be completely independent.

> > > +IGT implementation can be found here [4]
> > > +
> > > +Representing Fixed function hardware
> > > +====================================
> > > +
> > > +To provide support for fixed function hardware, the driver could
> > > +expose vendor specific struct drm_color_op with parameters that both
> > > +the userspace and driver agrees on. To demonstrate, let's consider a
> > > +hyphothetical fixed function hardware block that converts BT601 to BT2020.
> > > +The driver can choose to advertise the block as such.
> > > +
> > > +struct drm_color_op color_pipeline_X[] = {
> > > +	{
> > > +		.name = DRM_CB_PRIVATE,

What if the hardware has 5 different custom blocks like this, multiple
in the same pipeline. How do you 'name' them?

> > > +		.type = FIXED_FUNCTION,

I have been assuming that 'type' uniquely defines both the operation and
the contents of the parameter blob, but this does not look like it.
What defines the operation and the parameters?

> > > +		.blob_id = 45;
> > > +	},
> > > +}
> > > +
> > > +Where the blob represents some vendor specific enums, strings or any
> > > +other appropriate data types which both the user-space and drivers are aligned  
> > on.  

We have a word for that "data ... aligned on": UAPI.

> > > +
> > > +blob:45 {
> > > +	VENDORXXX_BT602_TO_BT2020,

Repeating the question from above, how will userspace know that the
contents of this blob need to be VENDORXXX_BT602_TO_BT2020 and not
something else?

> > > +};
> > > +
> > > +For enabling or passing parameters to such blocks, the user can send
> > > +data to the driver wrapped within a blob like any other color operation block.
> > > +
> > > +	struct drm_color_op_data {
> > > +		.name = DRM_CB_PRIVATE;
> > > +		.blob_id = 46;
> > > +	} ;
> > > +
> > > +where blob with id 46 contains data to enable the fixed function hardware(s).
> > > +  
> > 
> > One major thing missing from the RFC is an explanation on why we want to go with a
> > prescriptive model, rather than a descriptive model. This question came up in Dave's
> > response to Simon's RFC:
> > https://lore.kernel.org/dri-
> > devel/CAPM=9tz54Jc1HSjdh5A7iG4X8Gvgg46qu7Ezvgnmj4N6gbY+Kw@mail.gmail.co
> > m/
> > 
> > This is a rough attempt at such an explanation:
> > https://gitlab.freedesktop.org/hwentland/linux/-
> > /merge_requests/5/diffs?commit_id=bc99737623796b39925767d6f8cbc097ad0b4d
> > 8d  

Hey Harry, that's a good piece!

> 
> Sure Harry, totally agree to this and will include in documentation to highlight the rationale
> of going with prescriptive model.

Uma, the cover letter had descriptive and prescriptive mixed up.


Thanks,
pq

> > Harry
> >   
> > > +References
> > > +==========
> > > +
> > > +[1] https://gitlab.freedesktop.org/emersion/drm_info
> > > +[2]
> > > +https://patchwork.freedesktop.org/patch/554827/?series=123018&rev=1
> > > +[3]
> > > +https://patchwork.freedesktop.org/patch/554826/?series=123018&rev=1
> > > +[4] https://patchwork.freedesktop.org/series/123018/  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]