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@xxxxxxxxx> wrote: > >>> -----Original Message----- >>> From: Pekka Paalanen <ppaalanen@xxxxxxxxx> >>> Sent: Wednesday, October 13, 2021 2:01 PM >>> To: Shankar, Uma <uma.shankar@xxxxxxxxx> >>> Cc: harry.wentland@xxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx; intel- >>> gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; >>> brian.starkey@xxxxxxx; sebastian@xxxxxxxxxxxxxxxxx; >>> Shashank.Sharma@xxxxxxx >>> 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@xxxxxxxxx> wrote: >>> >>>>> -----Original Message----- >>>>> From: Pekka Paalanen <ppaalanen@xxxxxxxxx> >>>>> Sent: Tuesday, October 12, 2021 4:01 PM >>>>> To: Shankar, Uma <uma.shankar@xxxxxxxxx> >>>>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx; harry.wentland@xxxxxxx; >>>>> ville.syrjala@xxxxxxxxxxxxxxx; brian.starkey@xxxxxxx; >>>>> sebastian@xxxxxxxxxxxxxxxxx; Shashank.Sharma@xxxxxxx >>>>> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> >>>>>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >>>>>> --- >>>>>> 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@xxxxxxxxxxxxxxx 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@xxxxxxx 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 >
