Re: [PATCH RFC 1/5] drm: Support post-blend color pipeline API

["Nícolas F. R. A. Prado" <nfraprado@collabora.com>]

On Tue, 2025-08-26 at 13:25 +0100, Daniel Stone wrote:
> Hi,
> 
> On Mon, 25 Aug 2025 at 19:45, Xaver Hugl <xaver.hugl@gmail.com>
> wrote:
> > > > @@ -416,6 +417,24 @@ int drm_mode_object_get_properties(struct
> > > > drm_mode_object *obj, bool atomic,
> > > > +               if (post_blend_color_pipeline && obj->type ==
> > > > DRM_MODE_OBJECT_CRTC) {
> > > > +                       struct drm_crtc *crtc =
> > > > obj_to_crtc(obj);
> > > > +                       struct drm_mode_config mode_config =
> > > > crtc->dev->mode_config;
> > > > +
> > > > +                       if (prop ==
> > > > mode_config.gamma_lut_property ||
> > > > +                           prop ==
> > > > mode_config.degamma_lut_property ||
> > > > +                           prop ==
> > > > mode_config.gamma_lut_size_property ||
> > > > +                           prop == mode_config.ctm_property)
> > > > +                               continue;
> > > > +               }
> > > > +
> > > > +               if (!post_blend_color_pipeline && obj->type ==
> > > > DRM_MODE_OBJECT_CRTC) {
> > > > +                       struct drm_crtc *crtc =
> > > > obj_to_crtc(obj);
> > > > +
> > > > +                       if (prop == crtc-
> > > > >color_pipeline_property)
> > > > +                               continue;
> > > > +               }
> > > 
> > > Hmmm. One issue with this is that it makes things like drm_info
> > > harder: if drm_info opted into the client cap, it would no longer
> > > be
> > > able to see any GAMMA_LUT/etc programmed by the prior userspace.
> > > So I
> > > think allowing at least read-only access would be reasonable
> > > here.
> > 
> > FWIW the cap for per-plane pipelines also hides COLOR_RANGE and
> > COLOR_ENCODING properties from the client.
> > 
> > From a compositor POV, I slightly prefer hiding the properties
> > entirely, but ignoring them on the compositor side when a color
> > pipeline is available would also be trivial.
> 
> It makes it impossible to do smooth transitions from legacy clients,
> as the old (current) properties can't be read back.
> 
> I assume the atomic state would also carry the old values (even if
> the
> drivers are specified to have to ignore them), so there would be an
> odd transition:
> * pre-colorop userspace sets GAMMA_LUT to invert brightness
> * colorop userspace takes over, does not set any colorops on the
> CRTC,
> brightness is no longer inverted (presumably? depends on what the
> default set of colorops is? and what the drivers do?), but the atomic
> state still carries the old gamma_lut blob
> * pre-colorop userspace takes over, does not touch GAMMA_LUT,
> brightness is inverted as the colorop from the previous atomic state
> is ignored and the pre-atomic one now takes precedence
> 
> This isn't necessarily wrong per se, but does seem kind of janky and
> error-prone: like should the old state be reset to zero/bypass for
> commits from colorop-aware clients? Or should we explicitly allow 0
> but no other value?

Hi, thanks for the feedback!

Based on this discussion, this is my understanding for the changes
desired on the series and their reasonings:

1. Add a driver cap, DRM_CAP_POST_BLEND_COLOR_PIPELINE, which drivers
will use to signal they support post-blend color pipelines.
  - Reason: Allow userspace to figure out that the driver doesn't
support post-blend color pipelines and choose to not set the client
cap, DRM_CLIENT_CAP_POST_BLEND_COLOR_PIPELINE, so it can use legacy
color management instead.
2. Make it so setting the client cap,
DRM_CLIENT_CAP_POST_BLEND_COLOR_PIPELINE, fails if the driver cap,
DRM_CAP_POST_BLEND_COLOR_PIPELINE, isn't set
  - Reason: Prevent userspace from making color management unusable if
the driver doesn't support post-blend color pipelines, as the legacy
color-management properties (GAMMA_LUT, DEGAMMA_LUT, CTM) would be
unwriteable with the client cap set.
3. Make legacy color-management properties (GAMMA_LUT, DEGAMMA_LUT,
CTM) read-only if the client cap,
DRM_CLIENT_CAP_POST_BLEND_COLOR_PIPELINE, is set
  - Reason: Allow drm_info to print legacy color management
configuration while still enabling post-blend color pipelines through
the client cap. Also to allow smooth handover from pre-colorop
userspace client to colorop-ready userspace client, as the latter can
now replicate the legacy color configuration through the colorops.

Side note: Smooth handover back to pre-colorop userspace after tweaking
the colorops to something else would not be possible without making the
legacy properties writable too, so that the client could update them to
match the colorops setting before switching back. I don't imagine this
would be a common use case, and colorops are a superset of the legacy
properties so there are cases where it wouldn't even be possible to
replicate the colorop setting on the legacy properties, but thought I'd
mention this limitation for completeness' sake.

Also, as Xaver noted, this feedback also applies to pre-blend pipelines
and its legacy color-management properties (COLOR_ENCODING,
COLOR_RANGE), so the same changes would be desirable there for the same
reasons. So we should share this feedback on that series as well.


Does this sound right?

-- 
Thanks,

Nícolas