Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed

From: Harry Wentland <harry.wentland@amd.com>
To: Sebastian Wick <sebastian.wick@redhat.com>,
	Pekka Paalanen <ppaalanen@gmail.com>
Cc: "Sasha McIntosh" <sashamcintosh@google.com>,
	"Abhinav Kumar" <quic_abhinavk@quicinc.com>,
	"Shashank Sharma" <shashank.sharma@amd.com>,
	"Xaver Hugl" <xaver.hugl@gmail.com>,
	"Hector Martin" <marcan@marcan.st>,
	"Liviu Dudau" <Liviu.Dudau@arm.com>,
	"Victoria Brekenfeld" <victoria@system76.com>,
	dri-devel@lists.freedesktop.org,
	wayland-devel@lists.freedesktop.org,
	"Melissa Wen" <mwen@igalia.com>,
	"Michel Dänzer" <mdaenzer@redhat.com>,
	"Jonas Ådahl" <jadahl@redhat.com>,
	"Joshua Ashton" <joshua@froggi.es>,
	"Aleix Pol" <aleixpol@kde.org>,
	"Naseer Ahmed" <quic_naseer@quicinc.com>,
	"Uma Shankar" <uma.shankar@intel.com>,
	"Christopher Braga" <quic_cbraga@quicinc.com>,
	"Arthur Grillo" <arthurgrillo@riseup.net>
Subject: Re: [RFC PATCH v2 06/17] drm/doc/rfc: Describe why prescriptive color pipeline is needed
Date: Tue, 7 Nov 2023 11:52:11 -0500	[thread overview]
Message-ID: <947016a2-8df2-4f76-86dc-a375c26d5f22@amd.com> (raw)
In-Reply-To: <20231026173003.GA319477@toolbox>

On 2023-10-26 13:30, Sebastian Wick wrote:
> On Thu, Oct 26, 2023 at 11:57:47AM +0300, Pekka Paalanen wrote:
>> On Wed, 25 Oct 2023 15:16:08 -0500 (CDT)
>> Alex Goins <agoins@nvidia.com> wrote:
>>
>>> Thank you Harry and all other contributors for your work on this. Responses
>>> inline -
>>>
>>> On Mon, 23 Oct 2023, Pekka Paalanen wrote:
>>>
>>>> On Fri, 20 Oct 2023 11:23:28 -0400
>>>> Harry Wentland <harry.wentland@amd.com> wrote:
>>>>   
>>>>> On 2023-10-20 10:57, Pekka Paalanen wrote:  
>>>>>> On Fri, 20 Oct 2023 16:22:56 +0200
>>>>>> Sebastian Wick <sebastian.wick@redhat.com> wrote:
>>>>>>     
>>>>>>> Thanks for continuing to work on this!
>>>>>>>
>>>>>>> On Thu, Oct 19, 2023 at 05:21:22PM -0400, Harry Wentland wrote:    

snip

>>
>>>>>> I think we also need a definition of "informational".
>>>>>>
>>>>>> Counter-example 1: a colorop that represents a non-configurable    
>>>>>
>>>>> Not sure what's "counter" for these examples?
>>>>>   
>>>>>> YUV<->RGB conversion. Maybe it determines its operation from FB pixel
>>>>>> format. It cannot be set to bypass, it cannot be configured, and it
>>>>>> will alter color values.  
>>>
>>> Would it be reasonable to expose this is a 3x4 matrix with a read-only blob and
>>> no BYPASS property? I already brought up a similar idea at the XDC HDR Workshop
>>> based on the principle that read-only blobs could be used to express some static
>>> pipeline elements without the need to define a new type, but got mixed opinions.
>>> I think this demonstrates the principle further, as clients could detect this
>>> programmatically instead of having to special-case the informational element.
>>
> 
> I'm all for exposing fixed color ops but I suspect that most of those
> follow some standard and in those cases instead of exposing the matrix
> values one should prefer to expose a named matrix (e.g. BT.601, BT.709,
> BT.2020).
> 

Agreed.

> As a general rule: always expose the highest level description. Going
> from a name to exact values is trivial, going from values to a name is
> much harder.
> 
>> If the blob depends on the pixel format (i.e. the driver automatically
>> chooses a different blob per pixel format), then I think we would need
>> to expose all the blobs and how they correspond to pixel formats.
>> Otherwise ok, I guess.
>>
>> However, do we want or need to make a color pipeline or colorop
>> conditional on pixel formats? For example, if you use a YUV 4:2:0 type
>> of pixel format, then you must use this pipeline and not any other. Or
>> floating-point type of pixel format. I did not anticipate this before,
>> I assumed that all color pipelines and colorops are independent of the
>> framebuffer pixel format. A specific colorop might have a property that
>> needs to agree with the framebuffer pixel format, but I didn't expect
>> further limitations.
> 
> We could simply fail commits when the pipeline and pixel format don't
> work together. We'll probably need some kind of ingress no-op node
> anyway and maybe could list pixel formats there if required to make it
> easier to find a working configuration.
> 

The problem with failing commits is that user-space has no idea why it
failed. If this means that userspace falls back to SW composition for
NV12 and P010 it would avoid HW offloading in one of the most important
use-cases on AMD HW for power-saving purposes.

snip

>>> Despite being programmable, the LUTs are updated in a manner that is less
>>> efficient as compared to e.g. the non-static "degamma" LUT. Would it be helpful
>>> if there was some way to tag operations according to their performance,
>>> for example so that clients can prefer a high performance one when they
>>> intend to do an animated transition? I recall from the XDC HDR workshop
>>> that this is also an issue with AMD's 3DLUT, where updates can be too
>>> slow to animate.
>>
>> I can certainly see such information being useful, but then we need to
>> somehow quantize the performance.
>>
>> What I was left puzzled about after the XDC workshop is that is it
>> possible to pre-load configurations in the background (slow), and then
>> quickly switch between them? Hardware-wise I mean.
> 
> We could define that pipelines with a lower ID are to be preferred over
> higher IDs.
> 
> The issue is that if programming a pipeline becomes too slow to be
> useful it probably should just not be made available to user space.
> 
> The prepare-commit idea for blob properties would help to make the
> pipelines usable again, but until then it's probably a good idea to just
> not expose those pipelines.
> 

It's a bit of a judgment call what's too slow, though. The value of having
a HW colorop might outweigh the cost of the programming time for some
compositors but not for others.

Harry

>>
>>
>> Thanks,
>> pq
> 
> 