On Wed, Dec 14, 2022 at 3:56 AM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > > On Tue, 13 Dec 2022 11:32:01 -0500 > Harry Wentland <harry.wentland@xxxxxxx> wrote: > > > On 12/13/22 05:23, Pekka Paalanen wrote: > > > On Mon, 12 Dec 2022 13:21:27 -0500 > > > Harry Wentland <harry.wentland@xxxxxxx> wrote: > > > > > >> Drivers might not support all colorspaces defined in > > >> dp_colorspaces and hdmi_colorspaces. This results in > > >> undefined behavior when userspace is setting an > > >> unsupported colorspace. > > >> > > >> Allow drivers to pass the list of supported colorspaces > > >> when creating the colorspace property. > > > > > > Hi Harry, > > > > > > what is there for drivers to support? Isn't this just infoframe data > > > that shall be sent down to the sink as-is with no other effect? > > > > > > > You have a good point. > > > > Right now the supported colorspaces de-facto depend on driver implementations > > as you can see in [1] for i915 and [2] for amdgpu. The amdgpu driver will > > also program the MSA [3] for DP connections, and a bunch of other things which > > are deeper in the driver. > > > > [1] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/i915/display/intel_dp.c#L1741 > > [2] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5155 > > [3] https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_stream_encoder.c#L368 > > > > I don't know why the DP VSC stuff needs to be in drivers. It should be > > common. The MSA packing would likely have to be driver specific since the > > mechanism of packing it is HW specific. > > What's MSA? I think it's Main Stream Attribute data. Part of DP. See slide 31 of this document: https://www.vesa.org/wp-content/uploads/2011/01/ICCE-Presentation-on-VESA-DisplayPort.pdf Alex > > I don't see it in > https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html > or anywhere under Documentation/gpu or in CTA-861-H. > > > I'll have a closer look and see if we can eliminate the "driver supported" > > bit. If we can't we'll probably need to describe the reasoning better. > > That would be nice, thanks! > > > Will it be a problem if the list of supported colorspaces differs between > > drivers? > > I do not think so. It's just normal KMS UAPI that one must always > inspect an enumeration to see what values are possible. Userspace > cannot use a header with pre-defined numerical values, they always need > to be introspected first like everything else about KMS properties. > > I know there were some opinions about hard-coding enum numerical values > in headers, but I think in the end everyone agreed to the introspection > even if it didn't seem useful at the time. > > Besides, if a driver never supported a given value but misbehaved or > refused, I don't think that counts as a kernel regression? > > > Thanks, > pq > > > > > Harry > > > > > Is the driver confusing colorimetry with color-representation (the > > > RGB-YCbCr conversion)? Or is this property defining both? > > > > > > I feel that the documentation of "Colorspace" KMS connector property > > > needs clarification, and a list of potentially available values with > > > explanations, more than just a reference to CTA-816-H which it does not > > > even do yet. > > > > > > Perhaps a table, where for each enum drm_colorspace entry has a row explaining: > > > > > > > > > Thanks, > > > pq > > > > > > > > >> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx> > > >> Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx> > > >> Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx> > > >> Cc: Vitaly.Prosyak@xxxxxxx > > >> Cc: Uma Shankar <uma.shankar@xxxxxxxxx> > > >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > >> Cc: Joshua Ashton <joshua@xxxxxxxxx> > > >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > >> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > >> --- > > >> drivers/gpu/drm/drm_connector.c | 140 +++++++++--------- > > >> .../gpu/drm/i915/display/intel_connector.c | 4 +- > > >> drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > > >> include/drm/drm_connector.h | 8 +- > > >> 4 files changed, 83 insertions(+), 71 deletions(-) > > >>
