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

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

On 2025-04-01 15:53, Simon Ser wrote:
> 
> 
> 
> 
> 
> On Tuesday, April 1st, 2025 at 17:14, Daniel Stone <daniel@fooishbar.org> wrote:
> 
>>
>>
>> Hi Alex,
>>
>> On Wed, 26 Mar 2025 at 23:50, Alex Hung alex.hung@amd.com wrote:
>>
>>> +static int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
>>> + struct drm_plane *plane, enum drm_colorop_type type)
>>> +{
>>> + struct drm_mode_config *config = &dev->mode_config;
>>> + struct drm_property *prop;
>>> + int ret = 0;
>>> +
>>> + ret = drm_mode_object_add(dev, &colorop->base, DRM_MODE_OBJECT_COLOROP);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + colorop->base.properties = &colorop->properties;
>>> + colorop->dev = dev;
>>> + colorop->type = type;
>>> + colorop->plane = plane;
>>
>> '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?
> 

Yeah, with the current design a colorop always belongs to a plane.

In the future when we introduce crtc colorops they could be
associated with a single crtc instead.

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

Correct.

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

If this wasn't the case then those pipes would be doing undefined
things with current implementations.

>> 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 think it'd be easier to tie a colorop to a single pipeline, which is
tied to a single plane. I'd imagine HW is rarely designed to allow
arbitrary routing of individual HW blocks. Muxes are costly.

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

It's tied specifically to a single plane.

Harry

>> 4. Can a given color pipeline be active on multiple planes on multiple
>> CRTCs at a time?
> 
> 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.
> 
>> 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.