Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 17, 2023 at 05:37:51PM +0200, Pekka Paalanen wrote:
> On Fri, 17 Mar 2023 16:14:38 +0200
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, Mar 17, 2023 at 03:35:53PM +0200, Pekka Paalanen wrote:
> > > On Fri, 17 Mar 2023 14:50:40 +0200
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >   
> > > > On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote:  
> > > > > On Fri, 17 Mar 2023 01:01:38 +0200
> > > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > >     
> > > > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick wrote:    
> > > > > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä
> > > > > > > <ville.syrjala@linux.intel.com> wrote:      
> > > > > > > >
> > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen wrote:      
> > > > > > > > > On Thu, 16 Mar 2023 12:47:51 +0200
> > > > > > > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > > > > > >      
> > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen wrote:      
> > > > > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200
> > > > > > > > > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > > > > > > > >      
> > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick wrote:      
> > > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland <harry.wentland@amd.com> wrote:      
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > We want compositors to be able to set the output
> > > > > > > > > > > > > > colorspace on DP and HDMI outputs, based on the
> > > > > > > > > > > > > > caps reported from the receiver via EDID.      
> > > > > > > > > > > > >
> > > > > > > > > > > > > About that... The documentation says that user space has to check the
> > > > > > > > > > > > > EDID for what the sink actually supports. So whatever is in
> > > > > > > > > > > > > supported_colorspaces is just what the driver/hardware is able to set
> > > > > > > > > > > > > but doesn't actually indicate that the sink supports it.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So the only way to enable bt2020 is by checking if the sink supports
> > > > > > > > > > > > > both RGB and YUV variants because both could be used by the driver.
> > > > > > > > > > > > > Not great at all. Something to remember for the new property.      
> > > > > > > > > > > >
> > > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like maybe it
> > > > > > > > > > > > is since I can't immediately spot anything in CTA-861 to
> > > > > > > > > > > > forbid it :/      
> > > > > > > > > > >
> > > > > > > > > > > Wouldn't the driver do the same EDID check before choosing whether it
> > > > > > > > > > > uses RGB or YCbCr signalling?      
> > > > > > > > > >
> > > > > > > > > > I suppose it could. The modeset would then fail, which is perhaps      
> > > > > > > > >
> > > > > > > > > Could? What are they missing?      
> > > > > > > >
> > > > > > > > The fact that the new property that also affects the rgb->ycbcr matrix
> > > > > > > > doesn't even exist?      
> > > > > > > 
> > > > > > > I think the question was about the current Colorspace property.    
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > We need to be able to set ColourPrimaries infoframe field for the sink.
> > > > > Only userspace knows what ColourPrimaries it uses, and the driver has
> > > > > no need to care at all, other than tell the sink what we have.
> > > > > 
> > > > > When a driver chooses to use YCbCr, it needs to use the
> > > > > MatrixCoefficients the sink expects.
> > > > > 
> > > > > If we send the infoframe to the sink telling the signal uses BT.2020
> > > > > ColourPrimaries, does that same bit pattern also tell the sink we are
> > > > > using the BT.2020 NCL MatrixCoefficients if the driver chooses YCbCr?
> > > > > 
> > > > > Do drivers actually use BT.2020 NCL MatrixCoefficients in that case?    
> > > > 
> > > > No. I think I've repeated this same line a thousand times already:
> > > > The current colorspace property *only* affects the infoframe/msa/sdp,
> > > > nothing else.  
> > > 
> > > That's the problem. I don't know what that means.
> > > 
> > > Does it mean that the sink expects BT.2020 NCL MatrixCoefficients to
> > > have been used?  
> > 
> > Yes, assuming that is the colorspace property value you picked.
> > 
> > > 
> > > And the driver will never use BT.2020 NCL MatrixCoefficients in any
> > > circumstances?  
> > 
> > Correct.
> > 
> > > 
> > > See the conflict? The sink will be decoding the signal incorrectly,
> > > because we are encoding it with the wrong MatrixCoefficients if the
> > > driver happens to silently choose YCbCr and userspace wants to send
> > > BT2020 ColourPrimaries indicated in the infoframe.  
> > 
> > Yes. And hence I thought pretty much everyone already
> > agreed that a new property is needed.
> 
> I think I was confused as well with the re-posting of this series,
> thinking it could be salvageable somehow and tried to understand how.
> Up to Harry, I think I've left enough annoying questions by now. :-)
> 
> > To make sure we actually understand what we're implementing
> > I think it should start out very minimal. Eg just three values:
> > - unspecified RGB + BT.601 YCbCr
> > - unspecified RGB + BT.709 YCbCr
> > - BT.2020 RGB + BT.2020 YCbCr NCL
> 
> ColourPrimaries + MatrixCoefficients, respectively. Sounds fine.
> 
> I recall hearing that DP spec actually has something like "unspecified"
> while HDMI has only "default colorimetry" which is specified, but I'm
> guessing that many monitors and TVs just don't implement it like they
> should, so it's effectively unspecified.

DP in theory might have default RGB (whatever that might mean) vs.
sRGB, although at some point I think it was just vague RGB vs. CEA RGB,
which I think in i915 we might be using to indicate limited vs. full
quantization range instead. I think that somehow fixed some monitors
(while many others still get the quantization range horrible wrong of
course).

HDMI/CTA-861-? IIRC didn't have anything but just "RGB", and in some
footnote CTA-861-? then goes on to talk about the sRGB bit in the EDID.
In the end it didn't seem to say anything definitive what the RGB
colorimetry really means.

> 
> "unspecified" in UAPI is ok as long as there will be another distinct
> value for "HDMI default colorimetry" or such.
> 
> I'm not sure why anyone would want to use "unspecified" but I guess
> it's necessary for UAPI backward compatibility.

Just because the specs don't really seem to specify anything
sensible. We could just call it "RGB" and leave it at that of
course.

> 
> > 
> > And that would control:
> > - basic colorimetry metadata transmitted to the sink
> > - MatrixCoefficients used for the potential RGB->YCbCr conversion
> > 
> > Transfer funcs, primaries, etc. would be left out (apart from
> > the potential metadata aspect).
> 
> Primaries left out? What are your "unspecified RGB" and "BT.2020 RGB"
> above then?

It all seems too open to interpretation to make it anything
but "undefined".

> 
> Asking from another angle, using infoframes, is it possible to tell the
> sink to use BT.2020 YCbCr NCL without *also* implying BT.2020
> ColourPrimaries? Joshua seemed to be saying "no".

I don't think so. The BT.2020 cases seems to be more strictrly
defined.

> 
> 
> > > > > 
> > > > > If they don't, then YCbCr BT.2020 has never worked, which is another
> > > > > nail in the coffin for "Colorspace" property.    
> > > > 
> > > > That is the same nail we've been talking about all along I thought.
> > > >   
> > > > > But it still means that
> > > > > RGB BT.2020 may have worked correctly, and then drivers would regress
> > > > > if they started picking YCbCr for any reason where they previously used
> > > > > RGB.    
> > > > 
> > > > The policy has been to use RGB if at all possible. Only falling back
> > > > to YCbCr 4:2:0 if absolutely necessary (eg. EDID says 4:2:0 must
> > > > be used, or there's not enough bandwidth for 4:4:4, etc.). If the
> > > > behaviour suddenly changes then it probably means the driver was
> > > > doing something illegal before by using RGB 4:4:4.  
> > > 
> > > Ok.
> > >   
> > > > > > > > >
> > > > > > > > > I mean, drivers are already automatically choosing between RGB and YCbCr
> > > > > > > > > signalling based on e.g. available bandwidth. Surely they already will
> > > > > > > > > not attempt to send a signal format to a monitor that does not say it
> > > > > > > > > supports that?      
> > > > > > > 
> > > > > > > That's exactly what they do. The drivers don't check the EDID for the
> > > > > > > colorimetry the sink supports and the responsibility is punted off to
> > > > > > > user space.    
> > > > > 
> > > > > I suspect there are two different things:
> > > > > 
> > > > > - which of RGB, YCbCr 4:4:4, YCbCr 4:2:0 can the sink take
> > > > > - the supported MatrixCoefficients for each of the YCbCr
> > > > > 
> > > > > Surely drivers are already checking the former point?    
> > > > 
> > > > Yes.
> > > >   
> > > > > 
> > > > > I'm not surprised if they are not checking the latter point, but they
> > > > > do need to, because it is the driver making the choice between RGB and
> > > > > some YCbCr.    
> > > > 
> > > > This point has been irrelevant since we always select BT.709
> > > > and there is no optional feature bit in EDID to check for that.
> > > > Presumaly it is mandatory for sinks to support both BT.601 and
> > > > BT.709 whenever they support YCbCr in general.  
> > > 
> > > Ok, so BT.601 and BT.709 MatrixCoefficients are cool. How do you tell
> > > the sink which one you used, btw?  
> > 
> > Through the same infoframe/msa/sdp stuff. But that only works
> > correctly if the colorspace property is left at the default value.
> > 
> > > 
> > > What about BT.2020 MatrixCoefficients?  
> > 
> > It would have to work the same way, if we actually ever used
> > this.
> 
> Good.
> 
> 
> Thanks,
> pq



-- 
Ville Syrjälä
Intel