> -----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.
> > > > +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.
New vs old userspace problem exists even today as you rightly highlighted in mail below,
however we are planning to propose that we clean the hardware state once the userspace
client switches or same client switches the pipeline.
> 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.
The design currently implements 1 hardware block by a struct drm_color_op data structure.
Multiple such blocks make the pipeline. So userspace just needs to get the pipeline and then
parse blocks 1 by 1. For blocks which it doesn't understand it can just skip and move to the
next one. Each block is differentiated by a unique "name" standardized by an enum which will be
part of the UAPI. Thus we will have scope for variable size blob to represent the particular hardware
pipeline, userspace can parse and implement whichever blocks it understands. Only rule defined
by UAPI is the way the respective block is to be parsed and programmed.
> 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.
Yeah, this is right and for that reason we came up with an idea of DRM_CB_PRIVATE
name for the hardware block. This will tell userspace that this is private hardware block
for a particular hardware vendor. Generic userspace will ignore this block. Vendor specific
HAL or compositor implementation can parse and use this block. To interpret the blob_id
and assign to a respective data structure, private_flags will be used. These private_flags will
be agreed upon by HAL and vendor implementation, generic userspace will ignore them.
For all other cases, private_flags is a don't care field.
> 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.
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.
> 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.
The idea of private_flags partially explained above is to have an option for vendors to
expose hardware blocks unique or custom to their hardware. private_flags can help
differentiate incase multiple such hardware blocks exists on the platform.
For how we intend to use it, please refer below patch in the series:
https://patchwork.freedesktop.org/patch/554928/?series=123024&rev=1
Also, IGT showing the usage:
https://patchwork.freedesktop.org/patch/554831/?series=123018&rev=1
> 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.
With the current blob approach with fixed data structure for parsing, we can generalize
all the hardware blocks and custom implementations. It is future proof with scope to
add any new hardware block in future as well. Rules to interpret, parse, program the
hardware can be defined in the blob approach as well and ideally should be defined and
fixed in the UAPI documentation.
> 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.
> > > > +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.
>
We have some fixed hardware operations in the color pipeline, its good to define and
identify them with name as identifier. For any other custom block we can use the PRIVATE type.
Driver should expose the name of the hardware block for a particular pipeline based on the
actual usage, and as long as that is standardized it should be ok if same hardware block is
named differently across pipeline and vice-versa.
> 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"?
Most of the common hardware blocks can be named based on the operation they are intended to
be used for. For some custom blocks it can be defined as PRIVATE. But this area can surely be improved
based on the suggestion to make it more generic.
> > > > + 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.
> > > > +
> > > > + 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?
It is reset and 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?
Idea is to set NULL to all blobs in the pipeline and disable the hardware blocks.
New pipeline should allocate new blobs and enable them back.
> > > > + 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.
Agree
> > > > +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?
Partially explained above, but private_flags can help in differentiating this.
A link for implementation shared above for reference.
> > > > + .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.
> > > > + .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.
Oh ok.
> > > > +
> > > > +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?
Explained above.
> > > > +};
> > > > +
> > > > +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@xxxxxxxxx
> l
> > > .co
> > > m/
> > >
> > > This is a rough attempt at such an explanation:
> > > https://gitlab.freedesktop.org/hwentland/linux/-
> > >
> /merge_requests/5/diffs?commit_id=bc99737623796b39925767d6f8cbc097ad
> > > 0b4d
> > > 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.
Agree, thanks for pointing out. Will fix the documentation.
Thanks Pekka for all the pointers.
Regards,
Uma Shankar
>
> 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/
