On Tuesday, April 1st, 2025 at 17:14, Daniel Stone <dan...@fooishbar.org> wrote:
>
>
> Hi Alex,
>
> On Wed, 26 Mar 2025 at 23:50, Alex Hung alex.h...@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?
> 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.)
> 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.
> 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.
> 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.
> 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.
