On Tue, Feb 14, 2023 at 10:18:49PM +0100, Sebastian Wick wrote: > On Tue, Feb 14, 2023 at 9:10 PM Ville Syrjälä > <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > > > On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote: > > > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland <harry.wentland@xxxxxxx> wrote: > > > > > > > > > > > > > > > > On 2/14/23 10:49, Sebastian Wick wrote: > > > > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä > > > > > <ville.syrjala@xxxxxxxxxxxxxxx> 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. > > > > > > > > > > I don't really think it's a question of if we want it but rather how > > > > > we get there. Harry is completely right that if we would make the > > > > > subsampling controllable by user space instead of the kernel handling > > > > > it magically, user space which does not adapt to the new control won't > > > > > be able to light up some modes which worked before. > > > > > > > > > > > > > Thanks for continuing this discussion and touching on the model of how > > > > we get to where we want to go. > > > > > > > > > This is obviously a problem and not one we can easily fix. We would > > > > > need a new cap for user space to signal "I know that I can control > > > > > bpc, subsampling and compression to lower the bandwidth and light up > > > > > modes which otherwise fail". That cap would also remove all the > > > > > properties which require kernel magic to work (that's also what I > > > > > proposed for my KMS color pipeline API). > > > > > > > > > > We all want to expose more of the scanout capability and give user > > > > > space more control but I don't think an incremental approach works > > > > > here and we would all do better if we accept that the current API > > > > > requires kernel magic to work and has a few implicit assumptions baked > > > > > in. > > > > > > > > > > With all that being said, I think the right decision here is to > > > > > > > > > > 1. Ignore subsampling for now > > > > > 2. Let the kernel select YCC or RGB on the cable > > > > > 3. Let the kernel figure out the conversion between RGB and YCC based > > > > > on the color space selected > > > > > 4. Let the kernel send the correct infoframe based on the selected > > > > > color space and cable encoding > > > > > 5. Only expose color spaces for which the kernel can do the conversion > > > > > and send the infoframe > > > > > > > > I agree. We don't want to break or change existing behavior (that is > > > > used by userspace) and this will get us far without breaking things. > > > > > > > > > 6. Work on the new API which is hidden behind a cap > > > > > > > > > > > > > I assume you mean something like > > > > https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11 > > > > > > Something like that, yes. The main point being a cap which removes a > > > lot of properties and sets new expectations between user space and > > > kernel. The actual API is not that important. > > > > > > > Above you say that you don't think an incremental approach works > > > > here. Can you elaborate? > > > > > > Backwards compatibility is really hard. If we add another property to > > > control e.g. the color range infoframe which doesn't magically convert > > > colors, we now have to define how it interacts with the existing > > > property. > > > > I think that's easy. The old prop simply override the > > infoframe/etc. stuff. Pretty sure that's how we have it > > implemented in i915 since the start. > > > > And if you set it wrong it's you own fault. > > We need the "old" property to control both the conversion and the > infoframe and we need the "new" property to control only the > infoframe. Wait what? The old property only controls the infoframe. The new property (last I looked) was supposed to do more. Or did that plan change already when I wasn't paying attention. > So if we don't want to old property to to any conversion we > now also need a new value for the old property which disables the > property. So why not just go for a cap and remove the property > altogether? > > And again, as long as user space doesn't have complete control over > the entire pipeline we can't use the property which only sets the > infoframe anyway so you can't even have an incremental approach here. > > Let's turn the question around: what do we gain by keeping all the > properties and implicit assumptions in the current API around? They > make everything more complex so what's the benefit? > > I also want to be able to clearly document all the requirements of > user space and the guarantees from the kernel in the new API which we > can't do when we just incrementally change some properties here and > there. The documentation right now is basically a joke and almost > nothing has been defined properly and when things work, they work by > accident. Purely from a documentation and usability POV the > incremental approach is already going to be horrible IMO. > > > > > > We also have to figure out how a user space which doesn't > > > know about the new property behaves when another client has set that > > > property. If any property which currently might change the pixel > > > values is used, we can't expose the entire color pipeline because the > > > kernel might have to use some element in it to achieve its magic > > > conversion. So essentially you already have this hard device between > > > "old" and "new" and you can't use the new stuff incrementally. > > > > That problem exists with any new property. Old userspace and new > > userspace may interact badly enought that nothing works right. > > In that sense I think these props might even be pretty mundane > > as the worst you might get from setting the infoframe wrong is > > perhaps wrong colors on your display. > > > > To solve that particular problem there has been talk (for years) > > about some kind of "reset all" knob to make sure everything is > > at a safe default value. I have a feeling there was even some > > kind of semi-real proposal in recent times, but maybe I imgained > > it? > > I'm one of the people who argued for this but met only resistance. It > sure would be nice to have but also doesn't help with a lot of the > issues. > > > Right now I guess what you do is either just reboot, or muck > > around with modetest to manually reset properties. > > > > I have occasionally abused this btw, to set some prop with > > modetest and then run some real userspace that doesn't even > > know about said property. Easy way to test stuff :P > > > > -- > > Ville Syrjälä > > Intel > > -- Ville Syrjälä Intel
