Re: [PATCH RFC 06/25] drm/connector: hdmi: Add VTEM EMP generation

[Vidith Madhu <vmadhu@nvidia.com>]

On Sat, 26 Sep 2026, Nicolas Frattaroli wrote:

> On Friday, 25 September 2026 05:48:54 Central European Summer Time Vidith Madhu wrote:
> > 
> > On Mon, 21 Sep 2026, Nicolas Frattaroli wrote:
> > 
> > > From: Derek Foreman <derek.foreman@collabora.com>
> > > 
> > > Add VTEM EMP generation to enable variable refresh rate signalling over
> > > HDMI.
> > > 
> > > These infoframes are only generated if the sink supports VRR.
> > > 
> > > Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 60 +++++++++++++++++++++++++
> > >  include/drm/drm_connector.h                     |  5 +++
> > >  2 files changed, 65 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > index d55548399687..33d0c9491643 100644
> > > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > @@ -853,6 +853,56 @@ static int hdmi_generate_hdmi_vendor_infoframe(const struct drm_connector *conne
> > >  	return 0;
> > >  }
> > >  
> > > +static int hdmi_generate_emp_infoframe_vtem(const struct drm_connector *connector,
> > > +					    struct drm_connector_state *conn_state)
> > > +{
> > > +	const struct drm_display_info *info = &connector->display_info;
> > > +	const struct drm_crtc_state *crtc_state =
> > > +		drm_atomic_get_crtc_state(conn_state->state, conn_state->crtc);
> > > +	struct drm_connector_hdmi_infoframe *infoframe =
> > > +		&conn_state->hdmi.infoframes.vtem;
> > > +	struct hdmi_emp_infoframe_vtem *vtem =
> > > +		&infoframe->data.vtem;
> > > +	const struct drm_crtc_vrr_state *vrr = &crtc_state->vrr_state;
> > > +	int vfront;
> > > +
> > > +	infoframe->set = false;
> > > +
> > > +	if (!connector->hdmi.funcs->vtem.write_infoframe)
> > > +		return 0;
> > > +
> > > +	if (!info->hdmi.vrr_capable)
> > > +		return 0;
> > > +
> > > +	hdmi_emp_infoframe_vtem_init(vtem);
> > > +	if (!crtc_state->vrr_enabled || vrr->vic) {
> > > +		vtem->base_refresh_rate = 0;
> > > +		vtem->base_vfront = 0;
> > It shouldn't hurt to always populate base_refresh_rate and base_vfront, 
> > might be cleaner to skip this check.
> 
> Hm, I thought they had to be blank when the vic was non-zero. If that's more
> of a "they can be blank" then yeah I'm fine with skipping it. But that makes
> me question the purpose of vrr->vic, which is then no longer needed as it's
> not used anywhere else.
That's how I interpreted what's written in the HDMI spec - populating 
these fields is only required in the VRR_EN=1 case, there is nothing 
stated for VRR_EN=0. And, for VRR_EN=1 + (VIC != 0 or RID != 0), it is 
explicitly stated the source may choose whether to populate them.
> 
> > > +	} else {
> > > +		vtem->base_refresh_rate = drm_mode_vrefresh(&crtc_state->mode);
> > > +		vfront = crtc_state->adjusted_mode.crtc_vsync_start -
> > > +			 crtc_state->adjusted_mode.crtc_vdisplay;
> > > +		if (vfront > U8_MAX || vfront < 0)
> > > +			return -EINVAL;
> > > +
> > > +		vtem->base_vfront = vfront;
> > > +	}
> > > +	vtem->fva_factor_m1 = 0;
> > > +	infoframe->set = true;
> > > +
> > > +	if (!crtc_state->vrr_enabled) {
> > I don't think we should use the vrr_enabled CRTC property to determine 
> > VRR_EN in the VTEM EMP. Transitioning the VRR mode sink-side typically causes
> > blanking, and it was discussed in patch [03/25] that drivers should be 
> > free to handle vrr_enabled changes as a seamless switch since it only 
> > concerns source-side VRR state (this is how the NVIDIA driver handles it).
> 
> I'll be honest, this is the first time I've thought about the VRR state
> communicated from userspace to kernel to be different to the VRR state
> communicated from kernel to display.
> 
> > Maybe it would make sense to extend the qms_enabled connector property 
> > introduced in this patchset to an enum of {Off, Gaming, QMS}? This would allow
> > a standard path to control the VRR state on the sink, separately from
> > vrr_enabled.
> 
> An earlier version of the patch series I was working on internally had a
> VRR limiter property that would either be Off (i.e. Game), Game-Limited,
> QMS-Limited, and it self-inflicted some amount of confusion because of
> the way things were named, so I refactored it to the limiter values being
> what determines whether a limiter is used, and the QMS enable to determine
> whether QMS is used to apply said limit. So my initial reaction is to be
> hesitant about expanding the collection of possible states exposed through
> the uAPI.
> 
> To help my understanding: what does
> 
>  vrr_enabled=true
>  $new_property=Off
> 
> mean for presentation?
In this case, it should be acceptable to disable VRR on both the source 
and sink and effectively ignore vrr_enabled=true. From reading
https://elixir.bootlin.com/linux/v7.2.8/source/drivers/gpu/drm/drm_connector.c#L2370
vrr_enabled is essentially a hint and there is no need to reject commits 
because of its value (at least, that is how we handle it in the NVIDIA 
driver).
> Another state I'm curious about is
> 
>  vrr_enabled=false
>  $new_property=Game
> 
> which I assume is the case you're interested in, where the compositor does
> not want VRR presentation but we're keeping the sink in Game mode to avoid
> having the display go blank.
That's right, in this case VRR is enabled on the sink but deactivated 
source-side. For NVIDIA, there is a distinct HW "deactivated" state of 
VRR, but we could achieve the same result by setting the frame range 
limits to a fixed value as you suggested.
> 
> is that correct, and something that does need an expanded property? I feel
> like userspace could be smart enough to do that by keeping vrr_enabled=true
> and then setting a fixed target, if the goal is to have non-VRR but with the
> display still in VRR mode.
I agree, I am a little concerned though about the transition period for 
compositors to adopt this new model. I know for example KDE is using the 
vrr_enabled property for their "AdaptiveSync" option, and it seems like it 
would be a regression if all of a sudden toggling it caused display 
blanking on newer kernels.
If we're going to move towards this model, it may make sense to deprecate 
the current vrr_enabled property and create a new one for connectors 
rather than CRTCs (e.g. functionally similar to qms_enabled). This new 
vrr_enabled property would then be used for the sink-side VRR mode 
signalling. We could map it as follows:

qms_enabled=true, vrr_enabled={true, false} --> QMS-VRR
qms_enabled=false, vrr_enabled=true         --> Gaming-VRR
qms_enabled=false, vrr_enabled=false        --> Off

The semantics of the old vrr_enabled property would be implied by the 
settings of vrr_{min_n, min_d, max_n, max_d} (e.g. if max=0, no VRR, else 
hint the driver to use VRR).

Thanks,
Vidith
> 
> Kind regards,
> Nicolas Frattaroli
> 
> > > +		vtem->m_const = false;
> > > +		vtem->game_vrr_en = false;
> > > +		return 0;
> > > +	}
> > > +
> > > +	vtem->game_vrr_en = true;
> > > +
> > > +	vtem->m_const = !vrr->dynamic;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int
> > >  hdmi_generate_infoframes(const struct drm_connector *connector,
> > >  			 struct drm_connector_state *conn_state)
> > > @@ -884,6 +934,10 @@ hdmi_generate_infoframes(const struct drm_connector *connector,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	ret = hdmi_generate_emp_infoframe_vtem(connector, conn_state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > @@ -1494,6 +1548,12 @@ int drm_atomic_helper_connector_hdmi_update_infoframes(struct drm_connector *con
> > >  			goto out;
> > >  	}
> > >  
> > > +	if (info->hdmi.vrr_capable)
> > > +		ret = write_or_clear_infoframe(connector,
> > > +					&funcs->vtem, "VTEM",
> > > +					&old_conn_state->hdmi.infoframes.vtem,
> > > +					&new_conn_state->hdmi.infoframes.vtem);
> > > +
> > >  out:
> > >  	mutex_unlock(&connector->hdmi.infoframes.lock);
> > >  	return ret;
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index e561a444515f..6e431eb81705 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -1180,6 +1180,11 @@ struct drm_connector_hdmi_state {
> > >  		 * matching our state.
> > >  		 */
> > >  		struct drm_connector_hdmi_infoframe hdmi;
> > > +
> > > +		/**
> > > +		 * @vtem: VTEM EMP infoframes structure matching our state.
> > > +		 */
> > > +		struct drm_connector_hdmi_infoframe vtem;
> > >  	} infoframes;
> > >  
> > >  	/**
> > > 
> > 
> 
> 
> 
> 
>