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 can see the sense in allowing different pipelines to move between
different planes, but then it shouldn't be set at init time. I think
you want to just drop the 'plane' argument here, and only set the
plane during an atomic commit when actually switching the state.
It would also be helpful for userspace to know how color pipelines
could be used, and to back that up with igt. Questions I have whilst
reading this:
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.)
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.)
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.)
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.)
4. Can a given color pipeline be active on multiple planes on multiple
CRTCs at a time?
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.)
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?
Cheers,
Daniel
