On 12/14/22 03:55, Pekka Paalanen 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? This is from the DP spec. It stands for Main Stream Attribute. It's a packet similar to an SDP that is encoded in the DP main signal stream (as opposed to an AUX message). > > 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? > True, but I would imagine that can be confusing for developers of DRM clients when enabling support for a new feature. I would guess it's much less confusing if drivers only exposed functionality that are expected to work (and are hopefully tested regularly). Harry > > 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(-) >>>>
