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

[Harry Wentland <harry.wentland@amd.com>]

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
>