[PATCH] drm/amd/display: Allow faking displays as VRR capable.

[Harry Wentland]

On 2019-04-30 3:56 p.m., Mario Kleiner wrote:
> [CAUTION: External Email]
> 
> On Tue, Apr 30, 2019 at 2:22 PM Kazlauskas, Nicholas
> <Nicholas.Kazlauskas at amd.com> wrote:
>>
>> On 4/30/19 3:44 AM, Michel Dänzer wrote:
>>> [CAUTION: External Email]
>>>
>>> On 2019-04-30 9:37 a.m., Mario Kleiner wrote:
>>>> Allow to detect any connected display to be marked as
>>>> VRR capable. This is useful for testing the basics of
>>>> VRR mode, e.g., scheduling and timestamping, BTR, and
>>>> transition logic, on non-VRR capable displays, e.g.,
>>>> to perform IGT test-suit kms_vrr test runs.
>>>>
>>>> This fake VRR display mode is enabled by setting the
>>>> optional module parameter amdgpu.fakevrrdisplay=1.
>>>>
>>>> It will try to use VRR range info parsed from EDID on
>>>> DisplayPort displays which have a compatible EDID,
>>>> but not compatible DPCD caps for Adaptive Sync. E.g.,
>>>> NVidia G-Sync compatible displays expose a proper EDID,
>>>> but not proper DPCD caps.
>>>>
>>>> It will use a hard-coded VRR range of 30 Hz - 144 Hz on
>>>> other displays without suitable EDID, e.g., standard
>>>> DisplayPort, HDMI, DVI monitors.
>>>>
>>>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
>>>>
>>>> [...]
>>>>
>>>>   struct amdgpu_mgpu_info mgpu_info = {
>>>>        .mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
>>>> @@ -665,6 +666,16 @@ MODULE_PARM_DESC(halt_if_hws_hang, "Halt if HWS hang is detected (0 = off (defau
>>>>   MODULE_PARM_DESC(dcfeaturemask, "all stable DC features enabled (default))");
>>>>   module_param_named(dcfeaturemask, amdgpu_dc_feature_mask, uint, 0444);
>>>>
>>>> +/**
>>>> + * DOC: fakevrrdisplay (int)
>>>> + * Override detection of VRR displays to mark any display as VRR capable, even
>>>> + * if it is not. Useful for basic testing of VRR without need to attach such a
>>>> + * display, e.g., for igt tests.
>>>> + * Setting 1 enables faking VRR. Default value, 0, does normal detection.
>>>> + */
>>>> +module_param_named(fakevrrdisplay, amdgpu_fake_vrr_display, int, 0644);
>>>> +MODULE_PARM_DESC(fakevrrdisplay, "Detect any display as VRR capable (0 = off (default), 1 = on)");
>>>
>>> amdgpu has too many module parameters already; IMHO this kind of niche
>>> use-case doesn't justify adding yet another one. For the vast majority
>>> of users, this would just be another knob to break things, resulting in
>>> support burden for us.
>>>
>>> How about e.g. making the vrr_capable property mutable, or adding
>>> another property for this?
>>>
>>>
>>> --
>>> Earthling Michel Dänzer               |              https://www.amd.com
>>> Libre software enthusiast             |             Mesa and X developer
>>>
>>
>> Since vrr_capable is already an optional property I think making it
>> mutable could potentially be an option. It would allow for userspace to
>> be able to disable capability as well that way.
> 
> Yes, that would have been useful for at least my case. In my own
> toolkit i will need to control vrr on/off on a run-by-run basis,
> depending on what the users experiment scripts want. So i'll add code
> to manipulate my fullscreen windows attached XAtom directly and
> override Mesa's choices. A bit of a hack, but should hopefully work.
> 
> At least for my special niche, more easily accessible (== RandR output
> props) info is always helpful. Other things that would probably make
> my case easier would be optional properties to report the "vrr_active"
> state back, so that the toolkit can know cheaply via a simple query
> without doubt at any point in time if vrr is active or not, because i
> need to use very different ways of scheduling swapbuffers and
> correctness checking the results for vrr vs. non-vrr.
> 
> Or some vrr_range property, so the toolkit can know if it is operating
> the hw in normal vrr range, or in BTR mode, and adapt its swapbuffers
> scheduling, e.g., to try to help the current vrr/btr code a bit to
> make the "right" decisions for stable timing.
> 
> Of course that makes userspace clients more dependent on current hw
> implementation details, so i can see why it's probably not a popular
> choice for a generic api. The best long-term solution is to have
> proper api for the client to just provide a target presentation
> timestamp and leave the rest to the magic little elves inside the
> machine.
> 
>>
>> It's a pretty niche usecase though. However, as Michel said, it would
>> probably just end up being another setting that allows users to break
>> their own setup.
>>
>> Nicholas Kazlauskas
> 
> Ok, fair enough, thank you two for the feedback. I assumed users
> wouldn't mess with this module parameter, given that it has zero
> practical end-user value and can essentially only be used for cheap
> IGT regression testing, but then one never knows if users think it is
> some magic "get Freesync for free" switch and poke the button anyway.
> I'll keep the patch locally for my own testing then.
> 

Users will absolutely set this, report varying levels of success and
then report bugs when they see unexpected behavior.

If it wasn't for this I'd be all for making things more configurable and
allowing users to force things.

> I assume your team at AMD probably has enough FreeSync displays around
> for some IGT test setup to do automated/regular regression testing?
> There was quite a bit of talk about the regression testing topic on
> last XDC, but i haven't kept up to date on how this is progressing
> outside of Intel?
> 

We have plenty of FreeSync displays in our team and have one hooked up
for testing with IGT.

Harry

> I'll try to contribute some more tests to the igt/kms_vrr test, maybe
> porting some of the tests from my own toolkit, as far as they can be
> sensibly ported without producing too much false positives.
> 
> -mario
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>