> -----Original Message-----
> From: Pekka Paalanen <ppaalanen@xxxxxxxxx>
> Sent: Tuesday, September 5, 2023 5:03 PM
> To: Shankar, Uma <uma.shankar@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; wayland-
> devel@xxxxxxxxxxxxxxxxxxxxx; Harry Wentland <harry.wentland@xxxxxxx>;
> Sebastian Wick <sebastian.wick@xxxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx;
> Jonas Adahl <jadahl@xxxxxxxxxx>
> Subject: Re: [RFC 01/33] drm/doc/rfc: Add RFC document for proposed Plane
> Color Pipeline
>
> On Mon, 4 Sep 2023 13:44:49 +0000
> "Shankar, Uma" <uma.shankar@xxxxxxxxx> wrote:
>
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> > > Of Pekka Paalanen
> > > Sent: Wednesday, August 30, 2023 5:59 PM
> > > To: Shankar, Uma <uma.shankar@xxxxxxxxx>
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Borah, Chaitanya Kumar
> > > <chaitanya.kumar.borah@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> > > wayland- devel@xxxxxxxxxxxxxxxxxxxxx
> > > 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@xxxxxxxxx> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Harry Wentland <harry.wentland@xxxxxxx>
> > > > > Sent: Wednesday, August 30, 2023 1:10 AM
> > > > > To: Shankar, Uma <uma.shankar@xxxxxxxxx>;
> > > > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
> > > > > devel@xxxxxxxxxxxxxxxxxxxxx
> > > > > Cc: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>;
> > > > > wayland- devel@xxxxxxxxxxxxxxxxxxxxx
> > > > > 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@xxxxxxxxx>
> > > > > > Signed-off-by: Chaitanya Kumar Borah
> > > > > > <chaitanya.kumar.borah@xxxxxxxxx>
> > > > > > Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> > > > > > ---
> > > > > > .../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.
Hi Pekka,
Ok, got the concerns in general. We will try to evaluate in deeper detail the
property based design and come back if there are some issues or inputs.
At Intel we don't need private as of now, but we thought of having an option to
enable any custom hardware or vendor. But we can drop the same for now if
community doesn't feel the need for it.
> 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.
We will need to precisely describe the hardware in userspace. Number of luts, precision,
segments etc.., we can't just pass EOTF's as enum from userspace and let driver put
hardcoded values to LUT. This will be nothing but an extension of descriptive behaviour.
This will be needed as there are multiple colorspaces possible and even LUTS can be
used to perform tone mapping. So, we need userspace to be able to program luts directly.
This is something we must expose to userspace. We will check if this can be fitted in
property based approach.
Thanks for all the feedback and comments on the design. We will work on it and get back.
Regards,
Uma Shankar
> 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/
> >
