| Message ID | 20190430073724.505-1-mario.kleiner.de@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: Allow faking displays as VRR capable. | expand |
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@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?
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@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. 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
On Tue, Apr 30, 2019 at 2:22 PM Kazlauskas, Nicholas <Nicholas.Kazlauskas@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@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. 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? 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
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@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@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 23c3375623d7..351af38e7fd3 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -159,6 +159,7 @@ extern uint amdgpu_dc_feature_mask; extern struct amdgpu_mgpu_info mgpu_info; extern int amdgpu_ras_enable; extern uint amdgpu_ras_mask; +extern int amdgpu_fake_vrr_display; #ifdef CONFIG_DRM_AMDGPU_SI extern int amdgpu_si_support; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 1e2cc9d68a05..3a9fc0fbc76e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -134,6 +134,7 @@ int amdgpu_emu_mode = 0; uint amdgpu_smu_memory_pool_size = 0; /* FBC (bit 0) disabled by default*/ uint amdgpu_dc_feature_mask = 0; +int amdgpu_fake_vrr_display = 0; 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)"); + static const struct pci_device_id pciidlist[] = { #ifdef CONFIG_DRM_AMDGPU_SI {0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI}, diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 1854506e3e8f..148ec5bb9fa8 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6921,6 +6921,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, edid_check_required = is_dp_capable_without_timing_msa( adev->dm.dc, amdgpu_dm_connector); + + /* Force detection of not-quite adaptive sync capable + * displays as vrr capable if requested by moduleparam. + * Works, e.g., with G-Sync displays. + */ + if (!edid_check_required && amdgpu_fake_vrr_display) { + edid_check_required = true; + DRM_INFO("amdgpu.fakevrrdisplay=1 -> Force vrr.\n"); + } } } if (edid_check_required == true && (edid->version > 1 || @@ -6948,6 +6957,12 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, amdgpu_dm_connector->max_vfreq = range->max_vfreq; amdgpu_dm_connector->pixel_clock_mhz = range->pixel_clock_mhz * 10; + + DRM_DEBUG("edid vrr %d: %d - %d Hz, clock %d Mhz\n", + i, amdgpu_dm_connector->min_vfreq, + amdgpu_dm_connector->max_vfreq, + amdgpu_dm_connector->pixel_clock_mhz); + break; } @@ -6958,6 +6973,21 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector, } } + /* Fake a vrr display with hard-coded properties if none was + * detected and faking one is requested by module parameter. + */ + if (!freesync_capable && amdgpu_fake_vrr_display) { + freesync_capable = true; + amdgpu_dm_connector->min_vfreq = 30; + amdgpu_dm_connector->max_vfreq = 144; + amdgpu_dm_connector->pixel_clock_mhz = 590; + + DRM_INFO("fake vrr: %d - %d Hz, clock %d Mhz\n", + amdgpu_dm_connector->min_vfreq, + amdgpu_dm_connector->max_vfreq, + amdgpu_dm_connector->pixel_clock_mhz); + } + update: if (dm_con_state) dm_con_state->freesync_capable = freesync_capable;
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@gmail.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 11 +++++++ .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 30 +++++++++++++++++++ 3 files changed, 42 insertions(+)