dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v4 07/27] drm/amd/display: Use bigger VRR range if found in AMD vsdb
       [not found]   ` <7da80c4be3e20c126017646f783b80136bb0700e@intel.com>
@ 2026-03-17 10:03     ` Tomasz Pakuła
  2026-03-20 14:48       ` Leo Li
  0 siblings, 1 reply; 2+ messages in thread
From: Tomasz Pakuła @ 2026-03-17 10:03 UTC (permalink / raw)
  To: Jani Nikula, alexander.deucher, harry.wentland, sunpeng.li
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, simona,
	siqueira, dri-devel, amd-gfx, linux-kernel, bernhard.berger,
	michel.daenzer, daniel

On Thu, 2026-02-26 at 14:36 +0200, Jani Nikula wrote:
> On Mon, 16 Feb 2026, Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com> wrote:
> > [Why]
> > Some monitors only expose their full VRR range in AMD vsdb for some
> > reason.
> > 
> > [How]
> > Compare exposed ranges and use the bigger one.
> > Only adjust lower limit if it doesn't support LFC
> > 
> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4177
> > Signed-off-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 +++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > 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 b3bf5e0c19a5..f36059bb0324 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -13269,6 +13269,34 @@ static bool copy_range_to_amdgpu_connector(struct drm_connector *conn)
> >  	return is_freesync_capable(range);
> >  }
> >  
> > +static void extend_range_from_vsdb(struct drm_display_info *display,
> > +				   const struct amdgpu_hdmi_vsdb_info *vsdb)
> > +{
> > +	u16 vrr_min = display->monitor_range.min_vfreq;
> > +	u16 vrr_max = display->monitor_range.max_vfreq;
> > +
> > +	/* Always extend upper limit */
> > +	if (vsdb->max_refresh_rate_hz > vrr_max)
> > +		vrr_max = vsdb->max_refresh_rate_hz;
> > +
> > +	/*
> > +	 * Only extend lower limit if current one disables LFC.
> > +
> > +	 * During widespread testing, we found that some manufacturers probably
> > +	 * had issues with their monitors' lower VRR boundaries and adjusted
> > +	 * them up (Gigabyte X34GS with official range 48 - 180, AMD vsdb 48 -
> > +	 * 180 yet Monitor Ranges 55 - 180). After setting the lower boundary
> > +	 * from AMD vsdb, such monitors start having blanking issues.
> > +	 *
> > +	 * Work around that by not touching VRR min if it still supports LFC.
> > +	 */
> > +	if (vsdb->min_refresh_rate_hz < vrr_min && (vrr_min * 2 >= vrr_max))
> > +		vrr_min = vsdb->min_refresh_rate_hz;
> > +
> > +	display->monitor_range.min_vfreq = vrr_min;
> > +	display->monitor_range.max_vfreq = vrr_max;
> 
> Random driver code should *not* modify struct drm_display_info,
> especially the fields that drm_edid.c parses. Drivers should cease to
> parse EDID and DisplayID altogether.
> 
> I'm on the verge of NAKing, to the extent that I have control over this,
> any further improvements on driver EDID/DisplayID parsing, with the
> expectation that everything's moved to shared EDID parser in drm_edid.c
> first, and improved there.
> 
> BR,
> Jani.

Hi!

So I prepared a version which completely removes the drm_display_info
modifications from this part of the driver, but I think I know why it
was there in there in the first place. Without changing these fields,
the dri debug information, reports wrong vrr range vs what the driver
decided to do.

Basically, with monitor that have different vrr range in 'monitor
ranges' and AMD vsdb, it only reports the range from monitor ranges,
worse, for monitors that report GTF range (all HDMI TV, even some DP
monitors) the vrr_range for the connector in dri debug is reported to be
0 - 0 even though the correct range was picked up from other places like
DisplayID, AMD vsdb, HDMI Forum vsdb or nvidia specific vsdb.

Moving all this into generic edid handling could be quite a problem as
well since different manufacturers might want to handle VRR differenty.
For example, Intel could decide to support FreeSync sinnalling or just
getting the range from there, or not. HDMI VRR is another issue where
one brand could decide to prioritize their own solution over the generic
one (FreeSync over HDMI vs HDMI VRR).

The vrr_range in debug ony exposes display_info.monitor_range but it's
name suggests that it does something different. Maybe it needs a
dedicated place for drivers to show what they ended up deciding upon?
monitr_range alrady is only used to expose the range if it has 'range
limits only' flag so only for VRR. My concern is that we would have bad
debug info in there now.

I'm working on adding a drp connector property that would expose the vrr
range so compositors could easily parse it instead of relying on edid
parsing, that will be impossible to match.

And again, I'm just a guy providing fixes, I think bigger changes should
be taken with AMD. The functionality is already in the driver and my
patches do not change what it does with it. I'm not even changing how it
parses anything when it comes to edid, just using what's already there
to decide how to handle VRR.

Tomasz

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v4 07/27] drm/amd/display: Use bigger VRR range if found in AMD vsdb
  2026-03-17 10:03     ` [PATCH v4 07/27] drm/amd/display: Use bigger VRR range if found in AMD vsdb Tomasz Pakuła
@ 2026-03-20 14:48       ` Leo Li
  0 siblings, 0 replies; 2+ messages in thread
From: Leo Li @ 2026-03-20 14:48 UTC (permalink / raw)
  To: Tomasz Pakuła, Jani Nikula, alexander.deucher,
	harry.wentland
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, simona,
	siqueira, dri-devel, amd-gfx, linux-kernel, bernhard.berger,
	michel.daenzer, daniel



On 2026-03-17 06:03, Tomasz Pakuła wrote:
> Hi!
> 
> So I prepared a version which completely removes the drm_display_info
> modifications from this part of the driver, but I think I know why it
> was there in there in the first place. Without changing these fields,
> the dri debug information, reports wrong vrr range vs what the driver
> decided to do.
> 
> Basically, with monitor that have different vrr range in 'monitor
> ranges' and AMD vsdb, it only reports the range from monitor ranges,
> worse, for monitors that report GTF range (all HDMI TV, even some DP
> monitors) the vrr_range for the connector in dri debug is reported to be
> 0 - 0 even though the correct range was picked up from other places like
> DisplayID, AMD vsdb, HDMI Forum vsdb or nvidia specific vsdb.
> 
> Moving all this into generic edid handling could be quite a problem as
> well since different manufacturers might want to handle VRR differenty.
> For example, Intel could decide to support FreeSync sinnalling or just
> getting the range from there, or not. HDMI VRR is another issue where
> one brand could decide to prioritize their own solution over the generic
> one (FreeSync over HDMI vs HDMI VRR).
> 
> The vrr_range in debug ony exposes display_info.monitor_range but it's
> name suggests that it does something different. Maybe it needs a
> dedicated place for drivers to show what they ended up deciding upon?
> monitr_range alrady is only used to expose the range if it has 'range
> limits only' flag so only for VRR. My concern is that we would have bad
> debug info in there now.
> 
> I'm working on adding a drp connector property that would expose the vrr
> range so compositors could easily parse it instead of relying on edid
> parsing, that will be impossible to match.
> 
> And again, I'm just a guy providing fixes, I think bigger changes should
> be taken with AMD. The functionality is already in the driver and my
> patches do not change what it does with it. I'm not even changing how it
> parses anything when it comes to edid, just using what's already there
> to decide how to handle VRR.
> 
> Tomasz

Hi Tomasz,

First of all, thanks for creating and testing these patches. I can't
_currently_ comment on the HDMI related things. But regarding VSDB parsing,
there's work in progress (not on the mailing lists yet) to move it to
drm_edid.c, looking to store everything in a "struct amd_vsdb_info"
within drm_display_info.

There will be duplicated vrr_min/max values stored in different places in
display_info, but I think that's OK, since they're also duplicated across
EDID/DID/VSDB? Drivers can then make their own selection for the final vrr
min/max, and reported in a new read-only property like you suggested.

I'm not sure if compositors would prefer to decode the edid themselves, or
rather have driver tell them via new properties. Maybe it's something to
bring up at the display hackfest.

Thanks,
Leo


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-03-20 14:48 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260216164516.36803-1-tomasz.pakula.oficjalny@gmail.com>
     [not found] ` <20260216164516.36803-8-tomasz.pakula.oficjalny@gmail.com>
     [not found]   ` <7da80c4be3e20c126017646f783b80136bb0700e@intel.com>
2026-03-17 10:03     ` [PATCH v4 07/27] drm/amd/display: Use bigger VRR range if found in AMD vsdb Tomasz Pakuła
2026-03-20 14:48       ` Leo Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox