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: 13233 bytes --]

On Mon, 4 Sep 2023 13:44:49 +0000
"Shankar, Uma" <uma.shankar@intel.com> wrote:

> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Pekka
> > Paalanen
> > Sent: Wednesday, August 30, 2023 5:59 PM
> > To: Shankar, Uma <uma.shankar@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah@intel.com>; dri-devel@lists.freedesktop.org; wayland-
> > devel@lists.freedesktop.org
> > Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane
> > Color Pipeline
> > 
> > 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!  
> 
> Thanks Pekka for the feedback and useful inputs.

Hi Uma,

sorry to say, but the overall feeling I get from this proposal is that
it is just the current color related KMS properties wrapped in a
pipeline blob. That is not the re-design I believe we are looking for,
and the feeling is based on several details that are just copied from
the current property design. Also the "private" stuff has to go.

All the varying LUT entries, varying LUT precision, 1D/3D LUTs, varying
LUT tap distribution, and parametrized curves are good development, but
right now we are looking at things one step higher level: the overall
color pipeline design and how to represent any operation. Most of this
series is considering details below the current attention level, hence
I'm paying attention only to the first few patches.

More below.

> > > > > +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.  
> 
> Structure to be used to interpret the blob should be defined as UAPI only and is not
> expected to change once agreed upon. It should be interpreted like a standard property.
> So structure to be used, say for 3dLut or 1dlut or CTM operations should be standardized
> and fixed. No versioning of structure should be done and same rules/restrictions as of UAPI
> property should be applied. 

That sounds like a UAPI that cannot be extended, either. So in a few
years we'd be looking at replacing it. Or maybe you are just
re-inventing the KMS object property system as blobs?

Replacing a single KMS property is much easier than replacing the
foundations of the whole color pipeline design.


> ...

> > 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.  
> 
> Name and type can be standardized in enum and well documented in the UAPI.
> For all the standard hardware blocks common for all vendors and serving most of the
> common usecases, we can define standard names in enum. These can be easily
> interpreted by a table as done in many cases already in driver and userspace.

Yeah, but it won't help with the type-specific parameter blobs that
are totally custom per each operation type. With the KMS property
system we could have more generic introspection into those as well, at
least naming all the parameters they have.

Plus, you don't have to patch drm_info every time something new gets
added in the kernel in order to see it named.

> ...

> > Therefore, I'm not yet convinced with the "all blobs" design.  
> 
> Looking forward to collaboratively solve the problem for the community.
> Will improve the design based on all the feedback.

I have the feeling that we should also see Harry's draft.

Intel's special LUT type is a whole another story, and we can always
have that fitted with any UAPI paradigm.

> ...

> > > > > +		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.  
> 
> By retaining state here, we mean the state set by the same client while using the same
> pipeline. If client wants to just alter one or subset of the hardware blocks in the pipeline,
> it can just send that to driver. Rest of the pipeline which was previously programmed by the
> same client will remain intact.
> 
> However once the client switches pipeline, driver will disable the pipeline and client will have
> to program all the blocks again with the new 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.  
> 
> Yes agree, we are thinking to reset and disable the pipeline once client switches.
> One of the ideas could be to use file_priv to achieve that.

I would assume that each color operation in each pipeline is already
independent. Userspace cannot know which color operation is mapped
to which hardware block, and should not need to care. That means the
kernel maintains software state with the UAPI objects, not with the
hardware blocks. Therefore, there is no inherent need to reset any
pipeline on switch. Hardware programming is a different matter.

In the hackfest or after it, people raised the possibility of having a
standard property in each KMS colorop object if possible: boolean
"pass-through", or any equivalent. If an object is set to pass-through,
it is as if it did not exist - it does not alter the values going
through it in any way. Not even clamping if that could make a
difference.

This allows adding new arbitrary blocks in old pipelines without
forcing userspace to abandon the pipeline, as long as userspace learns
to use the "pass-through" from the start. This reduces the number of
alternative pipelines that need to be advertised when adding new
features.

If an object is not pass-through, then userspace absolutely must
understand the values programmed into every property of that object, or
there is no knowing what the object actually does. If userspace does
not know what an object does, the object can be assumed to produce an
unexpected result, i.e. wrong result. No userspace is going to willingly
malfunction.

> ...

> > > > > +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?  
> 
> Partially explained above, but private_flags can help in differentiating this.
> A link for implementation shared above for reference.

But they are all CB_PRIVATE, so what positions do they take in the pipeline?

The names CB_PRE_CSC and CB_POST_CSC imply that the name defines the
position in a pipeline. That is wrong with the names since the order is
defined by the record order in a pipeline blob, right?

> > > > > +		.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?  
> 
> Statement is true for all other generic blocks, only for private blocks this is a
> bit different. Here interpretation depends on the private_flags which can be
> documented by the respective vendor for the custom HAL implementation.

Exceptions to a rule are bad API design. In this case, it can easily be
avoided.

Now I'm actually confused about how 'name' and 'type' depend on
'private_flags'. You have to decipher 'private_flags' to understand
what 'name' means too?

Hmm, but isn't 'name' used for identifying the block/operation in the
blob that sets up the parameters for a whole pipeline? But then 'name'
does not uniquely identify a block?


Thanks,
pq

> ...

> > > > > +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 --]