[PATCH] drm: Add DRM_MODE_TYPE_USERDEF flag to probed modes matching a video= argument
Hans de Goede
hdegoede at redhat.com
Mon Nov 11 17:20:25 UTC 2019
Hi,
On 11-11-2019 16:40, Daniel Vetter wrote:
> On Mon, Nov 11, 2019 at 12:06 PM Hans de Goede <hdegoede at redhat.com> wrote:
>>
>> Hi Daniel,
>>
>> On 11-11-2019 11:26, Daniel Vetter wrote:
>>> On Mon, Nov 11, 2019 at 10:50 AM Hans de Goede <hdegoede at redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 11-11-2019 10:25, Daniel Vetter wrote:
>>>>> On Sun, Nov 10, 2019 at 7:41 PM Hans de Goede <hdegoede at redhat.com> wrote:
>>>>>>
>>>>>> drm_helper_probe_add_cmdline_mode() prefers using a probed mode matching
>>>>>> a video= argument over calculating our own timings for the user specified
>>>>>> mode using CVT or GTF.
>>>>>>
>>>>>> But userspace code which is auto-configuring the mode may want to know that
>>>>>> the user has specified that mode on the kernel commandline so that it can
>>>>>> pick that mode over the mode which is marked as DRM_MODE_TYPE_PREFERRED.
>>>>>>
>>>>>> This commit sets the DRM_MODE_TYPE_USERDEF flag on the matching mode, just
>>>>>> as we would do on the user-specified mode when no matching probed mode is
>>>>>> found.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>>>
>>>>> Will existing userspace dtrt here with this? Some links to popular
>>>>> ones would be good (since essentially this is uapi fine tuning we need
>>>>> that anyway). With that will get my ack.
>>>>
>>>> A valid question, I've gone over what I consider the major userspace kms users:
>>>> -Xorg xserver modesetting driver does not check for this:
>>>> [hans at shalem xserver]$ ack DRM_MODE_TYPE_ hw/xfree86/drivers/modesetting/
>>>> hw/xfree86/drivers/modesetting/drmmode_display.c
>>>> 1321: if (kmode->type & DRM_MODE_TYPE_DRIVER)
>>>> 1323: if (kmode->type & DRM_MODE_TYPE_PREFERRED)
>>>> -Other Xorg drivers:
>>>> [hans at shalem driver]$ ls -d xf86-video-*
>>>> xf86-video-amdgpu xf86-video-intel xf86-video-qxl
>>>> xf86-video-armsoc xf86-video-modesetting xf86-video-sisusb
>>>> xf86-video-ati xf86-video-nouveau xf86-video-vmware
>>>> xf86-video-dummy xf86-video-omap xf86-video-voodoo
>>>> xf86-video-geode xf86-video-opentegra
>>>> These all only do the same checks as the Xorg modesetting driver
>>>> -mutter:
>>>> [hans at shalem mutter]$ ack DRM_MODE_TYPE_
>>>> src/backends/native/meta-output-kms.c
>>>> 261: if (drm_mode->type & DRM_MODE_TYPE_PREFERRED)
>>>>
>>>> So it seems nothing (that I can quickly find) in userspace is using this atm.
>>>>
>>>> The reason I wrote this patch is because about a year ago plymouth used to
>>>> fully rely on the kernel to setup the modes on monitors and would simply
>>>> inherit the modes setup by the kernel. Basically plymouth was relying on
>>>> fbcon to load first and setup modes.
>>>>
>>>> Deferred fbcon takeover (for flickerfree) means that this is no longer
>>>> happening. So now plymouth picks a mode itself. When I submitted the
>>>> plymouth change for plymouth to pick a mode itself the plymouth maintainer
>>>> (Ray Strode) was afraid that would break plymouth honoring video= arguments.
>>>> So currently plymouth still relies on the kernel to do the mode setup if
>>>> a video= argument is present on the kernel commandline.
>>>>
>>>> My other recent series, which adds support for e.g.
>>>> video=HDMI:panel_orientation=right_side_up, made me realize that relying
>>>> on the kernel for this is no good since the fbcon code has various
>>>> limitations wrt e.g. hotplug and this of course will not work when
>>>> fbcon deferred takeover is used and fbcon never loads before plymouth.
>>>>
>>>> So I wrote a patch for plymouth to check the DRM_MODE_TYPE_USERDEF
>>>> flag and prefer a mode with that flag over the PREFERRED mode:
>>>> https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/84
>>>>
>>>> And then I found out that for that code to work with modes which
>>>> are already in the list of probed modes, I need something like
>>>> the kernel patch we are discussing now.
>>>>
>>>> So if you want a link to an userspace consumer of this, I guess you
>>>> want a v2 with a link to the plymouth MR added. + maybe a blurb in
>>>> the commit message that to the best of my knowledge no userspace
>>>> kms consumers are checking for the DRM_MODE_TYPE_USERDEF flag?
>>>
>>> And how does just relying on plymouth fix this?
>>
>> The idea is to not automatically fix this, all I want to do is
>> provide enough info to userspace to make an informed decision,
>> plymouth honored the video= resolution, we want to
>> preserve that behavior.
>>
>> Likewise historically and currently compositors / Xorg pretty
>> much ignore video=, I do not want to introduce changes on the
>> kernel side which unilateral force a change here. If compositors
>> want to start honoring a video= provided/selected mode then that
>> is up to them.
>>
>>> Given what you figured out in your quick search I think we should set
>>> both USERDEF and PREFERRED, otherwise not much will happen here. Plus
>>> I guess we need some fixup code to make sure that only the cmdline
>>> mode is preferred, and no other mode? Except if all compositors pick
>>> the first preferred mode consistently, then I guess we just need to
>>> make sure it's first. Changing the meaning of USERDEF to also mean
>>> "preferred mode, pls use that" could be fraught with peril, e.g. you
>>> run a game on an old compositor (or with direct display) at lower
>>> refresh, maybe with a custom mode, then it crashes. And from then on
>>> all compositors insist on using your "preferred" mode.
>>
>> Right, I considered using PREFERRED instead (and clearing it on other
>> modes) and I deliberately did not do this. You say:
>>
>> "Changing the meaning of USERDEF to also mean
>> "preferred mode, pls use that" could be fraught with peril"
>>
>> But since no userspace currently checks for USERDEF (*) it means that
>> adding the flag will not cause anything to change. Where as adding
>> PREFERRED will all of a sudden cause compositors/Xorg to pick a
>> different default resolution. Arguably that is desirable, but it
>> is an ABI breaks of sort and I'm sure some users will be using
>> video=XXXxYYY to set a lower-resolution for fbcon (e.g. for readability
>> on HiDPI) and will not appreciate it if their compositor now also
>> picks this resolution.
>
> But we already have other modes that get the USERDEF flag. That's my
> worry, your new userspace code wont just fire on this, but on any user
> created mode? Or am I missing something here?
>
> Ok I rechecked, I did miss something, this stuff is unused. So yeah I
> think this is ok, but we really also need good documentation for this
> somewhere ... at least kerneldoc for the relevant ioctl structures
> needs to be fully covering all these flags and their meanings.
The flags are already documented here:
https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#c.drm_display_mode
And before this commit the documentation for the USERDEF flag reads:
"DRM_MODE_TYPE_USERDEF: Mode defined via kernel command line"
So it not being used for anything else matches the docs, note that this
patch updates the docs to match the new use:
"DRM_MODE_TYPE_USERDEF: Mode defined or selected via the kernel command line."
So I'm not entirely sure what more documentation you want for this ?
Regards,
Hans
>>>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>>>>>> index ef2c468205a2..4fed64be11f9 100644
>>>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>>>> @@ -157,6 +157,8 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>>>>> continue;
>>>>>> }
>>>>>>
>>>>>> + /* Mark the matching mode as being preferred by the user */
>>>>>> + mode->type |= DRM_MODE_TYPE_USERDEF;
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>>>>>> index e946e20c61d8..c7efb7487e9b 100644
>>>>>> --- a/include/drm/drm_modes.h
>>>>>> +++ b/include/drm/drm_modes.h
>>>>>> @@ -256,7 +256,8 @@ struct drm_display_mode {
>>>>>> * - DRM_MODE_TYPE_DRIVER: Mode created by the driver, which is all of
>>>>>> * them really. Drivers must set this bit for all modes they create
>>>>>> * and expose to userspace.
>>>>>> - * - DRM_MODE_TYPE_USERDEF: Mode defined via kernel command line
>>>>>> + * - DRM_MODE_TYPE_USERDEF: Mode defined or selected via the kernel
>>>>>> + * command line.
>>>>>> *
>>>>>> * Plus a big list of flags which shouldn't be used at all, but are
>>>>>> * still around since these flags are also used in the userspace ABI.
>>>>>> --
>>>>>> 2.23.0
>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
More information about the dri-devel
mailing list