On 2/3/23 13:56, 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. > It has implementations in other drivers but I have yet to see anyone using it. Without that it does nothing, unless there are proprietary userspace pieces that make use of this. >> 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. > The compositor could deal with it but this feels like the compositor taking over things that should really be in the hands of a display driver. >> >>> 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. > Maybe that's the right approach. My initial idea was to tag along an existing property but that turns out to be challenging when that existing property doesn't even have a userspace implementation. IMO the existing colorspace property shouldn't be a user-space controllable property. I'll have to take a closer look at the hdr_static_metadata to understand whether we might run into similar issues with it. The alternative is adding a new property to let userspace pick the encoding and the min bpc value but that conflicts with the expectation for a driver to always pick an encoding to satisfy the bandwidth requirements for the mode on the wire [1]. In this scenario userspace would need to take full ownership of the wire encoding and live with the consequences. If the bandwidth is not sufficient a driver would then need to reject the commit without having a mechanism to tell userspace the reason why. The driver understands the bandwidth requirements but there is currently no way for userspace to do so, in particular for MST/USB-C/USB4/Thunderbold scenarios. [1] https://lists.freedesktop.org/archives/amd-gfx/2022-December/087423.html I don't think giving userspace full control of the encoding buys us anything other than headaches. Harry
