Re: [PATCH v4 12/17] drm/uAPI: Add "preferred color format" drm property as setting for userspace

[Werner Sembach]

Am 01.07.21 um 15:24 schrieb Pekka Paalanen:
> On Thu, 1 Jul 2021 14:50:13 +0200
> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote:
>
>> Am 01.07.21 um 10:07 schrieb Pekka Paalanen:
>>
>>> On Wed, 30 Jun 2021 11:20:18 +0200
>>> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote:
>>>  
>>>> Am 30.06.21 um 10:41 schrieb Pekka Paalanen:
>>>>  
>>>>> On Tue, 29 Jun 2021 13:39:18 +0200
>>>>> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote:
>>>>>    
>>>>>> Am 29.06.21 um 13:17 schrieb Pekka Paalanen:    
>>>>>>> On Tue, 29 Jun 2021 08:12:54 +0000
>>>>>>> Simon Ser <contact@xxxxxxxxxxx> wrote:
>>>>>>>       
>>>>>>>> On Tuesday, June 22nd, 2021 at 09:15, Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote:
>>>>>>>>       
>>>>>>>>> yes, I think this makes sense, even if it is a property that one can't
>>>>>>>>> tell for sure what it does before hand.
>>>>>>>>>
>>>>>>>>> Using a pair of properties, preference and active, to ask for something
>>>>>>>>> and then check what actually worked is good for reducing the
>>>>>>>>> combinatorial explosion caused by needing to "atomic TEST_ONLY commit"
>>>>>>>>> test different KMS configurations. Userspace has a better chance of
>>>>>>>>> finding a configuration that is possible.
>>>>>>>>>
>>>>>>>>> OTOH, this has the problem than in UI one cannot tell the user in
>>>>>>>>> advance which options are truly possible. Given that KMS properties are
>>>>>>>>> rarely completely independent, and in this case known to depend on
>>>>>>>>> several other KMS properties, I think it is good enough to know after
>>>>>>>>> the fact.
>>>>>>>>>
>>>>>>>>> If a driver does not use what userspace prefers, there is no way to
>>>>>>>>> understand why, or what else to change to make it happen. That problem
>>>>>>>>> exists anyway, because TEST_ONLY commits do not give useful feedback
>>>>>>>>> but only a yes/no.    
>>>>>>>> By submitting incremental atomic reqs with TEST_ONLY (i.e. only changing one
>>>>>>>> property at a time), user-space can discover which property makes the atomic
>>>>>>>> commit fail.    
>>>>>>> That works if the properties are independent of each other. Color
>>>>>>> range, color format, bpc and more may all be interconnected,
>>>>>>> allowing only certain combinations to work.
>>>>>>>
>>>>>>> If all these properties have "auto" setting too, then it would be
>>>>>>> possible to probe each property individually, but that still does not
>>>>>>> tell which combinations are valid.
>>>>>>>
>>>>>>> If you probe towards a certain configuration by setting the properties
>>>>>>> one by one, then depending on the order you pick the properties, you
>>>>>>> may come to a different conclusion on which property breaks the
>>>>>>> configuration.    
>>>>>> My mind crossed another point that must be considered: When plugin in
>>>>>> a Monitor a list of possible Resolutions+Framerate combinations is
>>>>>> created for xrandr and other userspace (I guess by atomic checks? but
>>>>>> I don't know).    
>>>>> Hi,
>>>>>
>>>>> I would not think so, but I hope to be corrected if I'm wrong.
>>>>>
>>>>> My belief is that the driver collects a list of modes from EDID, some
>>>>> standard modes, and maybe some other hardcoded modes, and then
>>>>> validates each entry against all the known limitations like vertical
>>>>> and horizontal frequency limits, discarding modes that do not fit.
>>>>>
>>>>> Not all limitations are known during that phase, which is why KMS
>>>>> property "link-status" exists. When userspace actually programs a mode
>>>>> (not a TEST_ONLY commit), the link training may fail. The kernel prunes
>>>>> the mode from the list and sets the link status property to signal
>>>>> failure, and sends a hotplug uevent. Userspace needs to re-check the
>>>>> mode list and try again.
>>>>>
>>>>> That is a generic escape hatch for when TEST_ONLY commit succeeds, but
>>>>> in reality the hardware cannot do it, you just cannot know until you
>>>>> actually try for real. It causes end user visible flicker if it happens
>>>>> on an already running connector, but since it usually happens when
>>>>> turning a connector on to begin with, there is no flicker to be seen,
>>>>> just a small delay in finding a mode that works.
>>>>>    
>>>>>> During this drm
>>>>>> properties are already considered, which is no problem atm because as
>>>>>> far as i can tell there is currently no drm property that would make
>>>>>> a certain Resolutions+Framerate combination unreachable that would be
>>>>>> possible with everything on default.    
>>>>> I would not expect KMS properties to be considered at all. It would
>>>>> reject modes that are actually possible if the some KMS properties were
>>>>> changed. So at least going forward, current KMS property values cannot
>>>>> factor in.    
>>>> At least the debugfs variable "force_yuv420_output" did change the 
>>>> available modes here: 
>>>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5165 
>>>> before my patch 
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68eb3ae3c63708f823aeeb63bb15197c727bd9bf  
>>> Hi,
>>>
>>> debugfs is not proper UAPI, so we can just ignore it. Display servers
>>> cannot be expected to poke in debugfs. Debugfs is not even supposed to
>>> exist in production systems, but I'm sure people use it for hacking
>>> stuff. But that's all it is for: developer testing and hacking.  
>> e.g. Ubuntu has it active by default, but only read (and writable) by root.
> Hi,
>
> that's normal, yes. Root can do damage anyway, and it's useful for
> debugging. KMS clients OTOH often do not run as root.
>
>>>  
>>>> Forcing a color format via a DRM property in this function would 
>>>> reintroduce the problem.  
>>> The property would need to be defined differently because its presence
>>> could otherwise break existing userspace. Well, I suppose it could
>>> break existing userspace no matter what, so we would need the generic
>>> "reset to sane defaults" mechanism first IMO.
>>>
>>> DRM has client caps for exposing video modes that old userspace might
>>> not expect, to avoid breaking old userspace. Care needs to be taken
>>> with all new UAPI, because if a kernel upgrade makes something wrong,
>>> it's the kernel's fault no matter what userspace is doing, in principle.  
>> Can you give me a link describing how I define this caps?
> I don't have any, but you can find all the existing ones by grepping
> for DRM_CLIENT_CAP_.
>
> I'm not saying that we need it, but mentioning them as a possible
> workaround if userspace breakage seems imminent or is proven.
>
>>> Debugfs has no problem breaking userspace AFAIU, since it's not proper
>>> UAPI.
>>>  
>>>> And I think i915 driver works similar in this regard.
>>>>  
>>>>>    
>>>>>> However for example forcing YCbCr420 encoding would limit the
>>>>>> available resolutions (my screen for example only supports YCbCr420
>>>>>> on 4k@60 and @50Hz and on no other resolution or frequency (native is
>>>>>> 2560x1440@144Hz).
>>>>>>
>>>>>> So would a "force color format" that does not get resetted on
>>>>>> repluging/reenabling a monitor break the output, for example, of an
>>>>>> not updated xrandr, unaware of this new property?    
>>>>> Yes, not because the mode list would be missing the mode, but because
>>>>> actually setting the mode would fail.    
>>>> Well, like described above, I think the mode would actually be missing, 
>>>> which is also an unexpected behavior from a user perspective.  
>>> I think that is not how the property should work.
>>>
>>> If KMS properties would affect the list of modes, then userspace would
>>> need to set the properties for real (TEST_ONLY cannot change anything)
>>> and re-fetch the mode lists (maybe there is a hotplug event, maybe
>>> not). That workflow just doesn't work.  
>> The properties are set before the list is created in the first place.
>> Because, in my example, the properties get set before the monitor is
>> plugged in and the list can only be created as soon as the monitor is
>> plugged in.
> That's just an accident, it's not what I mean.
>
> What I mean is, we cannot have the KMS properties affect the list of
> modes, because then userspace that want to use specific values on those
> properties would have to program those properties first, and then get
> the list of modes. KMS UAPI does not work that way AFAIK.
>
> If the initial mode list is created on hotplug like you say, then the
> initial list could already be missing some modes that would be valid if
> some KMS properties had different values.

Depends if the mode list is created by TEST_ONLY:

- The force properties should return false on TEST_ONLY

- The force properties should not prevent the mode from showing up in the list

If the list is created by TEST_ONLY both things can't be fulfilled at the same time obviously.

I hope some can give more insights or has an idea how the properties could work best.

>
>>> A very interesting question is when should link-status failure not drop
>>> the current mode from the mode list, if other KMS properties affect the
>>> bandwidth etc. requirements of the mode. That mechanism is engineered
>>> for old userspace that doesn't actually handle link-status but does
>>> handle hotplug, so the hotplug triggers re-fetching the mode list and
>>> userspace maybe trying again with a better luck since the offending
>>> mode is gone. How to keep that working when introducing KMS properties
>>> forcing the cable format, I don't know.
>>>
>>> As long as the other affecting KMS properties are all "auto", the
>>> driver will probably exhaust all possibilities to make the mode work
>>> before signalling link-status failure and pruning the mode.
>>> Theoretically, as I have no idea what drivers actually do.  
>> Isn't that exactly how the "preferred color format" property works in
>> my patchset now?
> There was an argument that "preferred" with no guarantees is not
> useful enough. So I'm considering the force property instead.
> The problem is, "auto" is not the only possible value.
>
> When the value is not "auto", should link failure drop the mode or not?
> Userspace might change the value back to "auto" next time. If you
> dropped the mode, it would be gone. If you didn't drop the mode,
> userspace might be stuck picking the same non-working mode again and
> again if it doesn't know about the force mode property.
>
> You could argue that changing the value back to "auto" needs to reset
> the mode list, but that only gets us back to the "need to set
> properties before getting mode list".
>
> Maybe there needs to be an assumption that if "force color format" is
> not "auto", then link failure does not drop modes and userspace knows
> to handle this. Messy.
>
> I'm afraid I just don't know to give any clear answer. It's also
> possible that, as I'm not a kernel dev, I have some false assumptions
> here.
>
>
> Thanks,
> pq
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx