Re: [PATCH v3 00/17] Enable Colorspace connector property in amdgpu

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

On Tue, 7 Mar 2023 10:10:50 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> This patchset is based on Joshua's previous patchset [1], as well
> as my previous patchset [2].
> 
> It is
> - enabling support for the colorspace property in amdgpu, as well as
> - allowing drivers to specify the supported set of colorspaces, and
> - deprecating the BT2020_YCC and BT2020_RGB properties in favor of
>   a common BT2020 property. We leave the BT2020_CYCC property untouched
>   for now, same as the other _YVV properties. If they'll see use later
>   we might need to do something similar there, or allow userspace to
>   decide on the output encoding (RGB vs YUV).
> 
> Colorspace, Infoframes, and YCbCr matrix
> ---------------------------------------
> 
> Even though the initial intent of the colorspace property was to set the
> colorspace field in the respective HDMI AVI and DP SDP infoframes that
> is not sufficient in all scenarios. For DP the colorspace information
> also affects the MSA (main stream attribute) packet. For YUV output the
> colorspace affects the RGB-to-YCbCr conversion matrix. The colorspace
> field of the infopackets also depends on the encoding used, which is
> something that is decided by the driver and not known to userspace.
> 
> For these reasons a driver will need to be able to select the supported
> colorspaces at property creation.
> 
> Note: There seems to be an understanding that the colorspace property
> should ONLY modify the infoframe. While this is current behavior and
> sufficient in some cases it is nowhere specified that this should be the
> only use of this property. As outlined above this limitation is not
> going to work in all cases.
> 
> This patchset does not affect current behavior for the drivers that
> implement this property: i915 and vc4.
> 
> In the future we might want to give userspace control over the encoding
> format on the wire, in particular to avoid use of YUV420 when image
> fidelity is important. This work would likely go hand in hand with a
> min_bpc property and wouldn't conflict with the work done in this
> patchset.
> 
> Colorspace on crtc or connector?
> --------------------------------
> 
> There have been suggestions of programming 'colorspace' on the drm_crtc
> but I don't think the crtc is the right place for this property. The
> drm_plane and drm_crtc will be used to offload color processing that
> would normally be done via the GFX or other pipelines. The drm_connector
> controls the signalling with the display and ensures the wire format is
> appropriate for the encoding by programming the RGB-to-YCbCr matrix.
> 
> [1] https://patchwork.freedesktop.org/series/113632/
> [2] https://patchwork.freedesktop.org/series/111865/

Hi Harry,

this is a really good cover letter.

I've given all the comments I have on this iteration.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]