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 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(-) > >>
Attachment:
pgpFSTrtP3aYT.pgp
Description: OpenPGP digital signature
