Re: [PATCH V8 06/43] drm/colorop: Add 1D Curve subtype

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

On 2025-04-08 12:40, Daniel Stone wrote:
> Hi there,
> 
> On Tue, 1 Apr 2025 at 20:53, Simon Ser <contact@emersion.fr> wrote:
>> On Tuesday, April 1st, 2025 at 17:14, Daniel Stone <daniel@fooishbar.org> wrote:
>>> 'plane' seems really incongruous here. The colorop can be created for
>>> any number of planes, but we're setting it to always be bound to a
>>> single plane at init, and that can only be changed later.
>>
>> I don't think the current design allows a single colorop to be re-used
>> between planes? I think as-is, drivers create one set of colorops per
>> plane and never share them between different planes?
> 
> OK, Harry's reply cleared that up perfectly - the flexibility that's
> there at the moment is about being able to reuse colorops for CRTCs in
> post-blend ops (great!), not shared between planes.
> 

Just to make sure we're talking about the same thing:

The design intent is to allow drm_colorops on a crtc (once we implement
that), not to be able to share the same drm_colorop between a plane and
crtc.

>>> 1. Is it guaranteed that, if any plane on a device supports the
>>> COLOR_PIPELINE property, all planes will support COLOR_PIPELINE?
>>> (Given the amdgpu cursor-plane discussion, it looks like no, which is
>>> unfortunate but oh well.)
>>
>> I don't think so. (They could all expose a COLOR_PIPELINE with the only
>> choice as the zero bypass pipeline, but that sounds silly.)
> 
> Works for me: so any planes could not have colour pipelines, and the
> result would be undefined (well, less defined) colour.

Yes, basically it would be what we have now (without color pipelines).

> 
>>> 2. Is it guaranteed that, if a COLOR_PIPELINE property exists on a
>>> plane, that BYPASS will be one of the supported values? (The current
>>> implementation does this, which seems sensible, but if the plan is to
>>> not make this a uAPI invariant, e.g. to support planes with mandatory
>>> CM steps, this should probably be explicitly documented.)
>>
>> Yes. This is a hard requirement, mentioned in the design doc IIRC.
> 
> Nice. I guess that's kind of implicit given pre-colorop behaviour
> expectations. We'd probably need a client cap analogous to universal
> planes to expose planes with mandatory colorop steps. This should
> probably be enforced with igt.
> 

Yes.

The API allows for the option of non-bypassable individual
colorops, but the pipeline as a whole must support bypass
currently.

>>> 3. Can a given color pipeline potentially be used on different planes,
>>> i.e. a colorop used to represent a separate hardware processing block
>>> which may be used on any plane but only one plane at a time? (This
>>> should be documented either way, and if they are unique per plane, igt
>>> should enforce this.)
>>
>> Right now, I don't think so. Could be a future extension I suppose, but
>> I think we need to properly sit down and think about all of the possible
>> consequences. Maybe using the same pipeline ID isn't the best uAPI here.
> 
> I'm with you on this. I think if we were trying to express a single
> color-transformation block which was shared between multiple planes
> (MTK is probably the closest to this conceptually from what I've
> seen), having an immutable COLOR_PIPELINE_SHARED = { ids... } property
> would be the best way to achieve this.
> 
>>> 3. Can a given color pipeline be active on multiple planes at a time?
>>> (If so, the implementation definitely needs rethinking: the colorop
>>> would need to have a list of planes.)
>>
>> I don't think so.
> 
> Great. But probably needs igt.
> 

Right, otherwise a driver dev might implement colorops in a way that
breaks this. Though, if DRM helpers are used it should be fairly safe,
I would think.

>>> 4. Can a given color pipeline be active on multiple planes on multiple
>>> CRTCs at a time?
>>
>> Ditto.
> 
> Ditto.
> 
>>> 5. For a given colorop property, is it an invariant that the colorop
>>> will only appear in one color pipeline at a time? (I believe so, but
>>> this probably needs documenting and/or igt.)
>>
>> I don't really understand why that would matter to user-space.
> 
> Plane A: COLOR_PIPELINE@123 = { 1D_CURVE@456 }
> Plane B: COLOR_PIPELINE@789 = { 1D_CURVE@456 }
> 

Yeah, a simple IGT test that parses all color pipelines and checks
that colorops don't occur in multiple pipelines is needed.

> If userspace wasn't defensive about this, it would program the curve
> for 456 twice, and unless they were the same you'd get undesirable
> results.
> 
> The existing implementation is fine here, I think it just needs better
> igt to codify the expectations we all have.
> 
>>> Either way, I suspect that clorop->plane is the wrong thing to do, and
>>> that it maybe wants to be a list of planes in the drm_colorop_state?
>>
>> I don't think so, for a given plane, there can only be a single pipeline
>> active at a time.
> 
> Yeah, again that was just not having grasped that the colorop not
> being derived from the plane was actually about allowing for it to be
> attached to a single CRTC instead, rather than potentially multiple
> planes. I have no concerns around this.
> 
> As it stands, I've gone through the implementation pretty thoroughly,
> as well as our use of it in Weston. I'm happy with how it looks for
> pre-blend, and I'm even happier that the implementation is written to
> apply easily to apply to post-blend CRTC pipelines.
> 
> With the suggested uAPI doc fixes and igt additions, this series is:
> Reviewed-by: Daniel Stone <daniels@collabora.com>
> 
> Thanks everyone for the immense amount of work that's gone into this. :)
> 

Thanks, Daniel. It wouldn't have been possible without the help
from many. And the hackfests really helped.

Harry

> Cheers,
> Daniel