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

[Daniel Stone <daniel@fooishbar.org>]

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.

> > 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.

> > 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.

> > 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.

> > 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 }

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. :)

Cheers,
Daniel