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

[Nicolas Frattaroli]

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@xxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxx>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@xxxxxxxxxxxxx>
> > ---
> >  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.

> > +	} 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? 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.

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.

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;
> >  
> >  	/**
> > 
>