Re: [Intel-gfx] [RFC v2 01/22] drm: RFC for Plane Color Hardware Pipeline

Harry Wentland
2021-10-26T15:12:01Z

Thanks, Uma, for the updated patches. I'm finally finding
time to go through them.

On 2021-10-15 03:42, Pekka Paalanen wrote:
> On Thu, 14 Oct 2021 19:44:25 +0000
> "Shankar, Uma" <uma.shankar@intel.com> wrote:
>
>>> -----Original Message-----
>>> From: Pekka Paalanen <ppaalanen@gmail.com>
>>> Sent: Wednesday, October 13, 2021 2:01 PM
>>> To: Shankar, Uma <uma.shankar@intel.com>
>>> Cc: harry.wentland@amd.com; ville.syrjala@linux.intel.com; intel-
>>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>>> brian.starkey@arm.com; sebastian@sebastianwick.net;
>>> Shashank.Sharma@amd.com
>>> Subject: Re: [RFC v2 01/22] drm: RFC for Plane Color Hardware Pipeline
>>>
>>> On Tue, 12 Oct 2021 20:58:27 +0000
>>> "Shankar, Uma" <uma.shankar@intel.com> wrote:
>>>
>>>>> -----Original Message-----
>>>>> From: Pekka Paalanen <ppaalanen@gmail.com>
>>>>> Sent: Tuesday, October 12, 2021 4:01 PM
>>>>> To: Shankar, Uma <uma.shankar@intel.com>
>>>>> Cc: intel-gfx@lists.freedesktop.org;
>>>>> dri-devel@lists.freedesktop.org; harry.wentland@amd.com;
>>>>> ville.syrjala@linux.intel.com; brian.starkey@arm.com;
>>>>> sebastian@sebastianwick.net; Shashank.Sharma@amd.com
>>>>> Subject: Re: [RFC v2 01/22] drm: RFC for Plane Color Hardware
>>>>> Pipeline
>>>>>
>>>>> On Tue, 7 Sep 2021 03:08:43 +0530 Uma Shankar
>>>>> <uma.shankar@intel.com> wrote:
>>>>>
>>>>>> This is a RFC proposal for plane color hardware blocks.
>>>>>> It exposes the property interface to userspace and calls out the
>>>>>> details or interfaces created and the intended purpose.
>>>>>>
>>>>>> Credits: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>>>> ---
>>>>>> Documentation/gpu/rfc/drm_color_pipeline.rst | 167
>>>>>> +++++++++++++++++++
>>>>>> 1 file changed, 167 insertions(+) create mode 100644
>>>>>> Documentation/gpu/rfc/drm_color_pipeline.rst
>>>>>>
>>>>>> diff --git a/Documentation/gpu/rfc/drm_color_pipeline.rst
>>>>>> b/Documentation/gpu/rfc/drm_color_pipeline.rst
>>>>>> new file mode 100644
>>>>>> index 000000000000..0d1ca858783b
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/gpu/rfc/drm_color_pipeline.rst
>>>>>> @@ -0,0 +1,167 @@
>>>>>> +==================================================
>>>>>> +Display Color Pipeline: Proposed DRM Properties
>
> ...
>
>>> cf. BT.2100 Annex 1, "The relationship between the OETF, the EOTF and
>>> the OOTF", although I find those diagrams somewhat confusing still. It
>>> does not seem to clearly account for transmission non-linear encoding being different from the display EOTF.
>>>
>>> Different documents use OOTF to refer to different things. Then there
>>> is also the fundamental difference between PQ and HLG systems, where
>>> OOTF is by definition in different places of the camera-transmission-display pipeline.
>>
>> Agree this is a bit confusing, I admit I was much more confused than what you were for sure.
>> Will do some research to get my understanding in place. Thanks for all the inputs.
>
> I'm sure I was at least equally confused or even more at the start. I
> have just had a year of casual reading, discussions, and a W3C workshop
> attendance to improve my understanding. :-)
>
> Now I know enough to be dangerous.
>
> ...
>
>>>>>> +
>>>>>> +UAPI Name: PLANE_DEGAMMA_MODE
>>>>>> +Description: Enum property with values as blob_id's which
>>>>>> +advertizes
>>>>>> the
>>>>>
>>>>> Is enum with blob id values even a thing?
>>>>
>>>> Yeah we could have. This is a dynamic enum created with blobs. Each
>>>> entry contains the data structure to extract the full color
>>>> capabilities of the hardware. It’s a very interesting play with
>>>> blobs (@ville.syrjala@linux.intel.com brainchild)
>>>
>>> Yes, I think I can imagine how that works. The blobs are created
>>> immutable, unable to change once the plane has been advertised to
>>> userspace, because their IDs are used as enum values. But that is ok,
>>> because the blob describes capabilities that cannot change during the
>>> device's lifetime... or can they? What if you would want to extend the
>>> blob format, do you need a KMS client cap to change the format which
>>> would be impossible because you can't change an enum definition after it has been installed so you cannot swap the blob to a new one?
>>>
>>> This also relies on enums being identified by their string names,
>>> because the numerical value is not a constant but a blob ID and gets
>>> determined when the immutable blob is created.
>>>
>>> Did I understand correctly?
>>
>> Yes that’s right. We are not expecting these structures to change as
>> they represent the platforms capabilities.
>
> Until there comes a new platform whose capabilities you cannot quite
> describe with the existing structure. What's the plan to deal with that?
> A new enum value, like LUT2 instead of LUT? I suppose that works.
>

See my comment on the coverletter. It would be great if we could come
up with a PWL definition that's generic enough to work on all HW
that uses PWL and not require compositors to learn a new LUT2
type in the future.

>>
>>> As a userspace developer, I can totally work with that.
>>>
>>>>>> + possible degamma modes and lut ranges supported by the platform.
>>>>>> + This allows userspace to query and get the plane degamma color
>>>>>> + caps and choose the appropriate degamma mode and create lut values
>>>>>> + accordingly.
>>>>>
>>>>> I agree that some sort of "mode" switch is necessary, and
>>>>> advertisement of capabilities as well.
>>>>>
>>>>
>>>> This enum with blob id's is an interesting way to advertise segmented lut tables.
>>>>
>>>>>> +
>>>>>> +UAPI Name: PLANE_DEGAMMA_LUT
>>>>>> +Description: Blob property which allows a userspace to provide LUT values
>>>>>> + to apply degamma curve using the h/w plane degamma processing
>>>>>> + engine, thereby making the content as linear for further color
>>>>>> + processing. Userspace gets the size of LUT and precision etc
>>>>>> + from PLANE_DEGAMA_MODE_PROPERTY
>>>>>
>>>>> So all degamma modes will always be some kind of LUT? That may be
>>>>> a bit restrictive, as I understand AMD may have predefined or
>>>>> parameterised curves that are not LUTs. So there should be room
>>>>> for an arbitrary structure of parameters, which can be passed in
>>>>> as a blob id, and the contents defined by the degamma mode.
>>>>
>>>> For Intel's hardware these are luts but yeah AMD hardware seems to
>>>> have these as fixed function units. We should think of a way to have
>>>> this option as well in the UAPI. We could extend the DEGAMMA_MODE
>>>> property to have all the info, and DEGAMMA_LUT_PROPERTY may not be
>>>> needed based on some of the attributes passed via DEGAMMA_MODE. This
>>>> can be
>>> one way to have room for both.
>>>> @harry.wentland@amd.com thoughts ?
>>>
>>> Yeah, though I don't think you can use DEGAMMA_MODE property to pass
>>> parameters to fixed-function curves. The parameters need another
>>> property. Would be natural to use the other property for LUT data when mode it set to LUT.
>>>
>>> A trivial and made-up example of a parameterised fixed-function curve
>>> is pow(x, gamma), where gamma is the parameter.
>>

It's a bit HW dependent. Some of AMD HW has some pre-defined EOTF
ROMs who allowing us to program a RAM LUT in the same block. Other
HW split those into two independent, consecutive blocks, a pre-defined
EOTF ROM first, followed by a Gamma Correction RAM LUT.

These can probably both be supported the the proposed PLANE_DEGAMMA_LUT
with enum values for the predefined (sRGB, BT2020, etc.) EOTFs, as
well as an option for a programmable LUT.

Harry

>> We can maybe have a parallel property as well with proper
>> documentation if this helps with flexibility for varying hardware
>> vendors. UAPI document will list what to use and when. May be
>> platform will not even list the other one which may make things less
>> complicated to userspace.
>
> I'm not sure I got what you're thinking. My idea is that the
> interpretation of the blob contents depends on the value of the mode
> enum. Obviously the two will always need to be set together by
> userspace to ensure they match, otherwise DRM needs to reject the
> commit.
>
>
> The rest of your comments I agree with.
>
>
> Thanks,
> pq
>