Re: [V7 08/45] Documentation/gpu: document drm_colorop

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

On 2025-01-13 03:18, Simon Ser wrote:
> This patch should probably come after all patches introducing the
> properties referenced in the docs, e.g. NEXT and COLOR_PIPELINE.
> Probably after "[13/45] drm/colorop: Introduce
> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE"?
> 
>> +/**
>> + * DOC: overview
>> + *
>> + * A colorop represents a single color operation. Colorops are chained
>> + * via the NEXT property and make up color pipelines. Color pipelines
>> + * are advertised and selected via the COLOR_PIPELINE &drm_plane
>> + * property.
>> + *
>> + * A colorop will be of a certain type, advertised by the read-only TYPE
>> + * property. Each type of colorop will advertise a different set of
>> + * properties and is programmed in a different manner. Types can be
>> + * enumerated 1D curves, 1D LUTs, 3D LUTs, matrices, etc. See the
>> + * &drm_colorop_type documentation for information on each type.
> 
> It's not super nice to refer to internal kernel docs here, because AFAIU
> this section is mostly written towards user-space developers. User-space
> developers have no idea how internal kernel structs work.
> 
> It would be nicer to have a list of colorop types here, without referring
> to kernel internals. For instance, we have a list of 
> 

I'm not sure I follow. This is linking to the drm_colorop_type
(from drm_mode.h) enum documentation in drm-uapi.html.

Duplicating it here would mean that sooner or later the two
docs will get out of sync.

I agree with the rest and we'll reflect that in v8.

Harry

>> + * If a colorop advertises the BYPASS property it can be bypassed.
>> + *
>> + * Since colorops cannot stand-alone and are used to describe colorop
>> + * operations on a plane they don't have their own locking mechanism but
>> + * are locked and programmed along with their associated &drm_plane.
> 
> This sounds a bit too internal for overview docs - maybe should be in
> the struct drm_colorop docs instead?
> 
>> + * Colorops are only advertised and valid for atomic drivers and atomic
>> + * userspace that signals the DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE client
> 
> Nit: this cap can be linkified with a "&".
> 
>> + * cap. When a driver advertises the COLOR_PIPELINE property on a
>> + * &drm_plane and userspace signals the
>> + * DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE the driver shall ignore all other
>> + * plane color properties, such as COLOR_ENCODING and COLOR_RANGE.
> 
> Perhaps this should appear first in the doc? Start with instructions to
> enable, then describe how it works?
> 
>> + * More information about colorops and color pipelines can be found at
>> + * rfc/color_pipeline.rst.
> 
> Perhaps we should note that this document contains information about
> design/architectural decisions? The "reference" should be this section, the
> RFC doc is just about the motivation I believe. That is, if the API evolves,
> the RFC is unliekly to get updated, but this document will.