| Message ID | 20250224061717.1095226-19-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use VRR timing generator for fixed refresh rate modes | expand |
On Mon, Feb 24, 2025 at 11:47:15AM +0530, Ankit Nautiyal wrote: > For fixed refresh rate use fixed timings for all platforms that support > VRR. For this add checks to avoid computing and reading VRR for > platforms that do not support VRR. > For platforms that do support VRR, readback vrr timings whether or not > VRR_CTL_FLIP_LINE_EN is set in VRR_CTL or not. > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_vrr.c | 43 ++++++++++++------------ > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c > index 551dcc16f0d4..975fed9930c1 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > @@ -344,6 +344,9 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, > struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; > int vmin = 0, vmax = 0; > > + if (!HAS_VRR(display)) > + return; > + > if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) > return; > > @@ -358,9 +361,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, > > vmin = intel_vrr_compute_vmin(crtc_state); > > - if (vmin >= vmax) > - return; > - > crtc_state->vrr.vmin = vmin; > crtc_state->vrr.vmax = vmax; I think your earlier pathc left vmax==0 here for the !in_range so this looks a bit wrong. But if you change the earlier patch like I suggested to set vmax=vmin then this would be fine. > > @@ -373,7 +373,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, > */ > crtc_state->vrr.vmin -= intel_vrr_flipline_offset(display); > > - if (crtc_state->uapi.vrr_enabled) > + if (crtc_state->uapi.vrr_enabled && vmin < vmax) > intel_vrr_compute_vrr_timings(crtc_state); > else if (is_cmrr_frac_required(crtc_state) && is_edp) > intel_vrr_compute_cmrr_timings(crtc_state); > @@ -640,6 +640,9 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > u32 trans_vrr_ctl, trans_vrr_vsync; > > + if (!HAS_VRR(display)) > + return; I think the caller is already checking that. But I suppose we could move the checks into the VRR code. > + > trans_vrr_ctl = intel_de_read(display, > TRANS_VRR_CTL(display, cpu_transcoder)); > > @@ -663,23 +666,21 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > crtc_state->vrr.pipeline_full = > REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, trans_vrr_ctl); > > - if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) { > - crtc_state->vrr.flipline = intel_de_read(display, > - TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; > - crtc_state->vrr.vmax = intel_de_read(display, > - TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; > - crtc_state->vrr.vmin = intel_de_read(display, > - TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; > - > - if (HAS_AS_SDP(display)) { > - trans_vrr_vsync = > - intel_de_read(display, > - TRANS_VRR_VSYNC(display, cpu_transcoder)); > - crtc_state->vrr.vsync_start = > - REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); > - crtc_state->vrr.vsync_end = > - REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); > - } I think you want to keep the VRR_CTL_FLIP_LINE_EN check around the TRANS_VRR_FLIPLINE read at least, because we want the state checker to catch any misprogrammng of VRR_CTL_FLIP_LINE_EN. > + crtc_state->vrr.flipline = intel_de_read(display, > + TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; > + crtc_state->vrr.vmax = intel_de_read(display, > + TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; > + crtc_state->vrr.vmin = intel_de_read(display, > + TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; > + > + if (HAS_AS_SDP(display)) { > + trans_vrr_vsync = > + intel_de_read(display, > + TRANS_VRR_VSYNC(display, cpu_transcoder)); > + crtc_state->vrr.vsync_start = > + REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); > + crtc_state->vrr.vsync_end = > + REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); > } > > crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE && > -- > 2.45.2
On 2/26/2025 8:41 PM, Ville Syrjälä wrote: > On Mon, Feb 24, 2025 at 11:47:15AM +0530, Ankit Nautiyal wrote: >> For fixed refresh rate use fixed timings for all platforms that support >> VRR. For this add checks to avoid computing and reading VRR for >> platforms that do not support VRR. >> For platforms that do support VRR, readback vrr timings whether or not >> VRR_CTL_FLIP_LINE_EN is set in VRR_CTL or not. >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_vrr.c | 43 ++++++++++++------------ >> 1 file changed, 22 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c >> index 551dcc16f0d4..975fed9930c1 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vrr.c >> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c >> @@ -344,6 +344,9 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, >> struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; >> int vmin = 0, vmax = 0; >> >> + if (!HAS_VRR(display)) >> + return; >> + >> if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) >> return; >> >> @@ -358,9 +361,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, >> >> vmin = intel_vrr_compute_vmin(crtc_state); >> >> - if (vmin >= vmax) >> - return; >> - >> crtc_state->vrr.vmin = vmin; >> crtc_state->vrr.vmax = vmax; > I think your earlier pathc left vmax==0 here for the !in_range so > this looks a bit wrong. But if you change the earlier patch like I > suggested to set vmax=vmin then this would be fine. Right, will set the vmax=vmin, as discussed in the earlier patch. > >> >> @@ -373,7 +373,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, >> */ >> crtc_state->vrr.vmin -= intel_vrr_flipline_offset(display); >> >> - if (crtc_state->uapi.vrr_enabled) >> + if (crtc_state->uapi.vrr_enabled && vmin < vmax) >> intel_vrr_compute_vrr_timings(crtc_state); >> else if (is_cmrr_frac_required(crtc_state) && is_edp) >> intel_vrr_compute_cmrr_timings(crtc_state); >> @@ -640,6 +640,9 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) >> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; >> u32 trans_vrr_ctl, trans_vrr_vsync; >> >> + if (!HAS_VRR(display)) >> + return; > I think the caller is already checking that. But I suppose we could > move the checks into the VRR code. > >> + >> trans_vrr_ctl = intel_de_read(display, >> TRANS_VRR_CTL(display, cpu_transcoder)); >> >> @@ -663,23 +666,21 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) >> crtc_state->vrr.pipeline_full = >> REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, trans_vrr_ctl); >> >> - if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) { >> - crtc_state->vrr.flipline = intel_de_read(display, >> - TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; >> - crtc_state->vrr.vmax = intel_de_read(display, >> - TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; >> - crtc_state->vrr.vmin = intel_de_read(display, >> - TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; >> - >> - if (HAS_AS_SDP(display)) { >> - trans_vrr_vsync = >> - intel_de_read(display, >> - TRANS_VRR_VSYNC(display, cpu_transcoder)); >> - crtc_state->vrr.vsync_start = >> - REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); >> - crtc_state->vrr.vsync_end = >> - REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); >> - } > I think you want to keep the VRR_CTL_FLIP_LINE_EN check around the > TRANS_VRR_FLIPLINE read at least, because we want the state checker > to catch any misprogrammng of VRR_CTL_FLIP_LINE_EN. Alright, will remove this change. There is one more thing I wanted your opinion on: For PTL+, the support for TRANS_VTOTAL.Vtotal bits is going away. I can skip writing it based on the intel_vrr_always_use_vrr_tg(), but how should I fill adjusted_mode->crtc_vtotal during readout? Can we use vrr.vmin for that? Or should we just remove the state checker for crtc_vtotal for platforms where this applies? I am intending to include this change as the last patch of the series. Thanks again for all the reviews and suggestions. Regards, Ankit > >> + crtc_state->vrr.flipline = intel_de_read(display, >> + TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; >> + crtc_state->vrr.vmax = intel_de_read(display, >> + TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; >> + crtc_state->vrr.vmin = intel_de_read(display, >> + TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; >> + >> + if (HAS_AS_SDP(display)) { >> + trans_vrr_vsync = >> + intel_de_read(display, >> + TRANS_VRR_VSYNC(display, cpu_transcoder)); >> + crtc_state->vrr.vsync_start = >> + REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); >> + crtc_state->vrr.vsync_end = >> + REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); >> } >> >> crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE && >> -- >> 2.45.2
On Thu, Feb 27, 2025 at 04:31:28PM +0530, Nautiyal, Ankit K wrote: > > On 2/26/2025 8:41 PM, Ville Syrjälä wrote: > > On Mon, Feb 24, 2025 at 11:47:15AM +0530, Ankit Nautiyal wrote: > >> For fixed refresh rate use fixed timings for all platforms that support > >> VRR. For this add checks to avoid computing and reading VRR for > >> platforms that do not support VRR. > >> For platforms that do support VRR, readback vrr timings whether or not > >> VRR_CTL_FLIP_LINE_EN is set in VRR_CTL or not. > >> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_vrr.c | 43 ++++++++++++------------ > >> 1 file changed, 22 insertions(+), 21 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c > >> index 551dcc16f0d4..975fed9930c1 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_vrr.c > >> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > >> @@ -344,6 +344,9 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, > >> struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; > >> int vmin = 0, vmax = 0; > >> > >> + if (!HAS_VRR(display)) > >> + return; > >> + > >> if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) > >> return; > >> > >> @@ -358,9 +361,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, > >> > >> vmin = intel_vrr_compute_vmin(crtc_state); > >> > >> - if (vmin >= vmax) > >> - return; > >> - > >> crtc_state->vrr.vmin = vmin; > >> crtc_state->vrr.vmax = vmax; > > I think your earlier pathc left vmax==0 here for the !in_range so > > this looks a bit wrong. But if you change the earlier patch like I > > suggested to set vmax=vmin then this would be fine. > > Right, will set the vmax=vmin, as discussed in the earlier patch. > > > > > >> > >> @@ -373,7 +373,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, > >> */ > >> crtc_state->vrr.vmin -= intel_vrr_flipline_offset(display); > >> > >> - if (crtc_state->uapi.vrr_enabled) > >> + if (crtc_state->uapi.vrr_enabled && vmin < vmax) > >> intel_vrr_compute_vrr_timings(crtc_state); > >> else if (is_cmrr_frac_required(crtc_state) && is_edp) > >> intel_vrr_compute_cmrr_timings(crtc_state); > >> @@ -640,6 +640,9 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > >> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > >> u32 trans_vrr_ctl, trans_vrr_vsync; > >> > >> + if (!HAS_VRR(display)) > >> + return; > > I think the caller is already checking that. But I suppose we could > > move the checks into the VRR code. > > > >> + > >> trans_vrr_ctl = intel_de_read(display, > >> TRANS_VRR_CTL(display, cpu_transcoder)); > >> > >> @@ -663,23 +666,21 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) > >> crtc_state->vrr.pipeline_full = > >> REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, trans_vrr_ctl); > >> > >> - if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) { > >> - crtc_state->vrr.flipline = intel_de_read(display, > >> - TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; > >> - crtc_state->vrr.vmax = intel_de_read(display, > >> - TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; > >> - crtc_state->vrr.vmin = intel_de_read(display, > >> - TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; > >> - > >> - if (HAS_AS_SDP(display)) { > >> - trans_vrr_vsync = > >> - intel_de_read(display, > >> - TRANS_VRR_VSYNC(display, cpu_transcoder)); > >> - crtc_state->vrr.vsync_start = > >> - REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); > >> - crtc_state->vrr.vsync_end = > >> - REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); > >> - } > > I think you want to keep the VRR_CTL_FLIP_LINE_EN check around the > > TRANS_VRR_FLIPLINE read at least, because we want the state checker > > to catch any misprogrammng of VRR_CTL_FLIP_LINE_EN. > > Alright, will remove this change. > > > There is one more thing I wanted your opinion on: > > For PTL+, the support for TRANS_VTOTAL.Vtotal bits is going away. Aren't we still using the legacy timing gnerator currently on ptl? I think I did manage to run upstream code on a ptl somehwat succesfully, and I don't remeber any state checker warns or other real problems (which I would expect to happen if we don't have a working vtotal programmed in). > I can > skip writing it based on the intel_vrr_always_use_vrr_tg(), but how > should I fill adjusted_mode->crtc_vtotal during readout? > > Can we use vrr.vmin for that? Or should we just remove the state checker > for crtc_vtotal for platforms where this applies? I think reading it out from VMIN would be the way to go. Otherwise we'd have to somehow stop using crtc_vtotal everywhere and that sounds like a lot of work. > > I am intending to include this change as the last patch of the series. > > Thanks again for all the reviews and suggestions. > > > Regards, > > Ankit > > > > > >> + crtc_state->vrr.flipline = intel_de_read(display, > >> + TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; > >> + crtc_state->vrr.vmax = intel_de_read(display, > >> + TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; > >> + crtc_state->vrr.vmin = intel_de_read(display, > >> + TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; > >> + > >> + if (HAS_AS_SDP(display)) { > >> + trans_vrr_vsync = > >> + intel_de_read(display, > >> + TRANS_VRR_VSYNC(display, cpu_transcoder)); > >> + crtc_state->vrr.vsync_start = > >> + REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); > >> + crtc_state->vrr.vsync_end = > >> + REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); > >> } > >> > >> crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE && > >> -- > >> 2.45.2
On 2/28/2025 2:35 AM, Ville Syrjälä wrote: > On Thu, Feb 27, 2025 at 04:31:28PM +0530, Nautiyal, Ankit K wrote: >> On 2/26/2025 8:41 PM, Ville Syrjälä wrote: >>> On Mon, Feb 24, 2025 at 11:47:15AM +0530, Ankit Nautiyal wrote: >>>> For fixed refresh rate use fixed timings for all platforms that support >>>> VRR. For this add checks to avoid computing and reading VRR for >>>> platforms that do not support VRR. >>>> For platforms that do support VRR, readback vrr timings whether or not >>>> VRR_CTL_FLIP_LINE_EN is set in VRR_CTL or not. >>>> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_vrr.c | 43 ++++++++++++------------ >>>> 1 file changed, 22 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c >>>> index 551dcc16f0d4..975fed9930c1 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c >>>> @@ -344,6 +344,9 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, >>>> struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; >>>> int vmin = 0, vmax = 0; >>>> >>>> + if (!HAS_VRR(display)) >>>> + return; >>>> + >>>> if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) >>>> return; >>>> >>>> @@ -358,9 +361,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, >>>> >>>> vmin = intel_vrr_compute_vmin(crtc_state); >>>> >>>> - if (vmin >= vmax) >>>> - return; >>>> - >>>> crtc_state->vrr.vmin = vmin; >>>> crtc_state->vrr.vmax = vmax; >>> I think your earlier pathc left vmax==0 here for the !in_range so >>> this looks a bit wrong. But if you change the earlier patch like I >>> suggested to set vmax=vmin then this would be fine. >> Right, will set the vmax=vmin, as discussed in the earlier patch. >> >> >>>> >>>> @@ -373,7 +373,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, >>>> */ >>>> crtc_state->vrr.vmin -= intel_vrr_flipline_offset(display); >>>> >>>> - if (crtc_state->uapi.vrr_enabled) >>>> + if (crtc_state->uapi.vrr_enabled && vmin < vmax) >>>> intel_vrr_compute_vrr_timings(crtc_state); >>>> else if (is_cmrr_frac_required(crtc_state) && is_edp) >>>> intel_vrr_compute_cmrr_timings(crtc_state); >>>> @@ -640,6 +640,9 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) >>>> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; >>>> u32 trans_vrr_ctl, trans_vrr_vsync; >>>> >>>> + if (!HAS_VRR(display)) >>>> + return; >>> I think the caller is already checking that. But I suppose we could >>> move the checks into the VRR code. >>> >>>> + >>>> trans_vrr_ctl = intel_de_read(display, >>>> TRANS_VRR_CTL(display, cpu_transcoder)); >>>> >>>> @@ -663,23 +666,21 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) >>>> crtc_state->vrr.pipeline_full = >>>> REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, trans_vrr_ctl); >>>> >>>> - if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) { >>>> - crtc_state->vrr.flipline = intel_de_read(display, >>>> - TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; >>>> - crtc_state->vrr.vmax = intel_de_read(display, >>>> - TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; >>>> - crtc_state->vrr.vmin = intel_de_read(display, >>>> - TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; >>>> - >>>> - if (HAS_AS_SDP(display)) { >>>> - trans_vrr_vsync = >>>> - intel_de_read(display, >>>> - TRANS_VRR_VSYNC(display, cpu_transcoder)); >>>> - crtc_state->vrr.vsync_start = >>>> - REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); >>>> - crtc_state->vrr.vsync_end = >>>> - REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); >>>> - } >>> I think you want to keep the VRR_CTL_FLIP_LINE_EN check around the >>> TRANS_VRR_FLIPLINE read at least, because we want the state checker >>> to catch any misprogrammng of VRR_CTL_FLIP_LINE_EN. >> Alright, will remove this change. >> >> >> There is one more thing I wanted your opinion on: >> >> For PTL+, the support for TRANS_VTOTAL.Vtotal bits is going away. > Aren't we still using the legacy timing gnerator currently on ptl? > I think I did manage to run upstream code on a ptl somehwat > succesfully, and I don't remeber any state checker warns or other > real problems (which I would expect to happen if we don't have a > working vtotal programmed in). For PTL the support for legacy timing generator is still there but its recommended to switch to VRR Timing generator. GOP I think has already moved in that direction. > >> I can >> skip writing it based on the intel_vrr_always_use_vrr_tg(), but how >> should I fill adjusted_mode->crtc_vtotal during readout? >> >> Can we use vrr.vmin for that? Or should we just remove the state checker >> for crtc_vtotal for platforms where this applies? > I think reading it out from VMIN would be the way to go. Otherwise > we'd have to somehow stop using crtc_vtotal everywhere and that sounds > like a lot of work. Will try to have this read from VRR.Vmin. Regards, Ankit > >> I am intending to include this change as the last patch of the series. >> >> Thanks again for all the reviews and suggestions. >> >> >> Regards, >> >> Ankit >> >> >>>> + crtc_state->vrr.flipline = intel_de_read(display, >>>> + TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; >>>> + crtc_state->vrr.vmax = intel_de_read(display, >>>> + TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; >>>> + crtc_state->vrr.vmin = intel_de_read(display, >>>> + TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; >>>> + >>>> + if (HAS_AS_SDP(display)) { >>>> + trans_vrr_vsync = >>>> + intel_de_read(display, >>>> + TRANS_VRR_VSYNC(display, cpu_transcoder)); >>>> + crtc_state->vrr.vsync_start = >>>> + REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); >>>> + crtc_state->vrr.vsync_end = >>>> + REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); >>>> } >>>> >>>> crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE && >>>> -- >>>> 2.45.2
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c index 551dcc16f0d4..975fed9930c1 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.c +++ b/drivers/gpu/drm/i915/display/intel_vrr.c @@ -344,6 +344,9 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; int vmin = 0, vmax = 0; + if (!HAS_VRR(display)) + return; + if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) return; @@ -358,9 +361,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, vmin = intel_vrr_compute_vmin(crtc_state); - if (vmin >= vmax) - return; - crtc_state->vrr.vmin = vmin; crtc_state->vrr.vmax = vmax; @@ -373,7 +373,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state, */ crtc_state->vrr.vmin -= intel_vrr_flipline_offset(display); - if (crtc_state->uapi.vrr_enabled) + if (crtc_state->uapi.vrr_enabled && vmin < vmax) intel_vrr_compute_vrr_timings(crtc_state); else if (is_cmrr_frac_required(crtc_state) && is_edp) intel_vrr_compute_cmrr_timings(crtc_state); @@ -640,6 +640,9 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; u32 trans_vrr_ctl, trans_vrr_vsync; + if (!HAS_VRR(display)) + return; + trans_vrr_ctl = intel_de_read(display, TRANS_VRR_CTL(display, cpu_transcoder)); @@ -663,23 +666,21 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state) crtc_state->vrr.pipeline_full = REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, trans_vrr_ctl); - if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) { - crtc_state->vrr.flipline = intel_de_read(display, - TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; - crtc_state->vrr.vmax = intel_de_read(display, - TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; - crtc_state->vrr.vmin = intel_de_read(display, - TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; - - if (HAS_AS_SDP(display)) { - trans_vrr_vsync = - intel_de_read(display, - TRANS_VRR_VSYNC(display, cpu_transcoder)); - crtc_state->vrr.vsync_start = - REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); - crtc_state->vrr.vsync_end = - REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); - } + crtc_state->vrr.flipline = intel_de_read(display, + TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1; + crtc_state->vrr.vmax = intel_de_read(display, + TRANS_VRR_VMAX(display, cpu_transcoder)) + 1; + crtc_state->vrr.vmin = intel_de_read(display, + TRANS_VRR_VMIN(display, cpu_transcoder)) + 1; + + if (HAS_AS_SDP(display)) { + trans_vrr_vsync = + intel_de_read(display, + TRANS_VRR_VSYNC(display, cpu_transcoder)); + crtc_state->vrr.vsync_start = + REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync); + crtc_state->vrr.vsync_end = + REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync); } crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE &&
For fixed refresh rate use fixed timings for all platforms that support VRR. For this add checks to avoid computing and reading VRR for platforms that do not support VRR. For platforms that do support VRR, readback vrr timings whether or not VRR_CTL_FLIP_LINE_EN is set in VRR_CTL or not. Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> --- drivers/gpu/drm/i915/display/intel_vrr.c | 43 ++++++++++++------------ 1 file changed, 22 insertions(+), 21 deletions(-)