On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote: > On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote: > > > > > > On 2/3/23 11:00, Ville Syrjälä wrote: > > > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > >> > > >> > > >> On 2/3/23 10:19, Ville Syrjälä wrote: > > >>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > >>>> > > >>>> > > >>>> On 2/3/23 07:59, Sebastian Wick wrote: > > >>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > >>>>> <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > >>>>>> > > >>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +0000, Joshua Ashton wrote: > > >>>>>>> Userspace has no way of controlling or knowing the pixel encoding > > >>>>>>> currently, so there is no way for it to ever get the right values here. > > >>>>>> > > >>>>>> That applies to a lot of the other values as well (they are > > >>>>>> explicitly RGB or YCC). The idea was that this property sets the > > >>>>>> infoframe/MSA/SDP value exactly, and other properties should be > > >>>>>> added to for use userspace to control the pixel encoding/colorspace > > >>>>>> conversion(if desired, or userspace just makes sure to > > >>>>>> directly feed in correct kind of data). > > >>>>> > > >>>>> I'm all for getting userspace control over pixel encoding but even > > >>>>> then the kernel always knows which pixel encoding is selected and > > >>>>> which InfoFrame has to be sent. Is there a reason why userspace would > > >>>>> want to control the variant explicitly to the wrong value? > > >>>>> > > >>>> > > >>>> I've asked this before but haven't seen an answer: Is there an existing > > >>>> upstream userspace project that makes use of this property (other than > > >>>> what Joshua is working on in gamescope right now)? That would help us > > >>>> understand the intent better. > > >>> > > >>> The intent was to control the infoframe colorimetry bits, > > >>> nothing more. No idea what real userspace there was, if any. > > >>> > > >>>> > > >>>> I don't think giving userspace explicit control over the exact infoframe > > >>>> values is the right thing to do. > > >>> > > >>> Only userspace knows what kind of data it's stuffing into > > >>> the pixels (and/or how it configures the csc units/etc.) to > > >>> generate them. > > >>> > > >> > > >> Yes, but userspace doesn't control or know whether we drive > > >> RGB or YCbCr on the wire. In fact, in some cases our driver > > >> needs to fallback to YCbCr420 for bandwidth reasons. There > > >> is currently no way for userspace to know that and I don't > > >> think it makes sense. > > > > > > People want that control as well for whatever reason. We've > > > been asked to allow YCbCr 4:4:4 output many times. > > > > > > The automagic 4:2:0 fallback I think is rather fundementally > > > incompatible with fancy color management. How would we even > > > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915 > > > that stuff is just always BT.709 limited range, no questions > > > asked. > > > > > > > We use what we're telling the display, i.e., the value in the > > colorspace property. That way we know whether to use a BT.2020 > > or BT.709 matrix. > > And given how these things have gone in the past I think > that is likey to bite someone at in the future. Also not > what this property was meant to do nor does on any other > driver AFAIK. > > > I don't see how it's fundamentally incompatible with fancy > > color management stuff. > > > > If we start forbidding drivers from falling back to YCbCr > > (whether 4:4:4 or 4:2:0) we will break existing behavior on > > amdgpu and will see bug reports. > > The compositors could deal with that if/when they start doing > the full color management stuff. The current stuff only really > works when the kernel is allowed to do whatever it wants. > > > > > > So I think if userspace wants real color management it's > > > going to have to set up the whole pipeline. And for that > > > we need at least one new property to control the RGB->YCbCr > > > conversion (or to explicitly avoid it). > > > > > > And given that the proposed patch just swept all the > > > non-BT.2020 issues under the rug makes me think no > > > one has actually come up with any kind of consistent > > > plan for anything else really. > > > > > > > Does anyone actually use the non-BT.2020 colorspace stuff? > > No idea if anyone is using any of it. It's a bit hard to do > right now outside the full passthrough case since we have no > properties to control how the hardware will convert stuff. > > Anyways, sounds like what you're basically proposing is > getting rid of this property and starting from scratch. Hmm. I guess one option would be to add that property to control the output encoding, but include a few extra "automagic" values to it which would retain the kernel's freedom to select whether to do the RGB->YCbCr conversion or not. enum output_encoding { auto rgb=default/nodata,ycbcr=bt601 auto rgb=default/nodata,ycbcr=bt709 auto rgb=bt2020,ycbcr=bt2020 passthrough rgb->ycbcr bt601, rgb->ycbcr bt709, rgb->ycbcr bt2020, } and then if you leave the colorspae property to "default" the kernel will pick the "right" value based on the output_encoding prop. That would leave all the weird stuff in the colorspace property alone and thus would still allow someone to do more than just the basic stuff explicitly. -- Ville Syrjälä Intel