| Message ID | 20230901130440.2085-5-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: VRR, LRR, and M/N stuff | expand |
Looks good to me, Reviewed-by: Manasi Navare <navaremanasi@chromium.org> Manasi On Fri, Sep 1, 2023 at 6:05 AM Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > In order to reconcile seamless M/N updates with VRR we'll > need to defer the fastset VRR enable to happen after the > seamless M/N update (which happens during the vblank evade > critical section). So just push the VRR enable to be the last > thing during the update. > > This will also affect the vblank evasion as the transcoder > will now still be running with the old VRR state during > the vblank evasion. So just grab the timings always from the > old crtc state during any non-modeset commit, and also grab > the current state of VRR from the active timings (as we disable > VRR before vblank evasion during fastsets). > > This also fixes vblank evasion for seamless M/N updates as > we now properly account for the fact that the M/N update > happens after vblank evasion. > > Cc: Manasi Navare <navaremanasi@chromium.org> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_crtc.c | 35 ++++++++++++-------- > drivers/gpu/drm/i915/display/intel_display.c | 21 ++++++++---- > 2 files changed, 36 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c > index e46a15d59d79..1992e7060263 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -472,15 +472,31 @@ static void intel_crtc_vblank_evade_scanlines(struct intel_atomic_state *state, > struct intel_crtc *crtc, > int *min, int *max, int *vblank_start) > { > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > - const struct drm_display_mode *adjusted_mode = &new_crtc_state->hw.adjusted_mode; > + const struct intel_crtc_state *crtc_state; > + const struct drm_display_mode *adjusted_mode; > > - if (new_crtc_state->vrr.enable) { > - if (intel_vrr_is_push_sent(new_crtc_state)) > - *vblank_start = intel_vrr_vmin_vblank_start(new_crtc_state); > + /* > + * During fastsets/etc. the transcoder is still > + * running with the old timings at this point. > + * > + * TODO: maybe just use the active timings here? > + */ > + if (intel_crtc_needs_modeset(new_crtc_state)) > + crtc_state = new_crtc_state; > + else > + crtc_state = old_crtc_state; > + > + adjusted_mode = &crtc_state->hw.adjusted_mode; > + > + if (crtc->mode_flags & I915_MODE_FLAG_VRR) { > + if (intel_vrr_is_push_sent(crtc_state)) > + *vblank_start = intel_vrr_vmin_vblank_start(crtc_state); > else > - *vblank_start = intel_vrr_vmax_vblank_start(new_crtc_state); > + *vblank_start = intel_vrr_vmax_vblank_start(crtc_state); > } else { > *vblank_start = intel_mode_vblank_start(adjusted_mode); > } > @@ -710,15 +726,6 @@ void intel_pipe_update_end(struct intel_atomic_state *state, > */ > intel_vrr_send_push(new_crtc_state); > > - /* > - * Seamless M/N update may need to update frame timings. > - * > - * FIXME Should be synchronized with the start of vblank somehow... > - */ > - if (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state)) > - intel_crtc_update_active_timings(new_crtc_state, > - new_crtc_state->vrr.enable); > - > local_irq_enable(); > > if (intel_vgpu_active(dev_priv)) > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index cfad967b5684..632f1f58df9e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -6476,6 +6476,8 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state, > struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > > @@ -6487,6 +6489,9 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state, > if (DISPLAY_VER(dev_priv) >= 9 && > !intel_crtc_needs_modeset(new_crtc_state)) > skl_detach_scalers(new_crtc_state); > + > + if (vrr_enabling(old_crtc_state, new_crtc_state)) > + intel_vrr_enable(new_crtc_state); > } > > static void intel_enable_crtc(struct intel_atomic_state *state, > @@ -6527,12 +6532,6 @@ static void intel_update_crtc(struct intel_atomic_state *state, > intel_dpt_configure(crtc); > } > > - if (vrr_enabling(old_crtc_state, new_crtc_state)) { > - intel_vrr_enable(new_crtc_state); > - intel_crtc_update_active_timings(new_crtc_state, > - new_crtc_state->vrr.enable); > - } > - > if (!modeset) { > if (new_crtc_state->preload_luts && > intel_crtc_needs_color_update(new_crtc_state)) > @@ -6569,6 +6568,16 @@ static void intel_update_crtc(struct intel_atomic_state *state, > > intel_pipe_update_end(state, crtc); > > + /* > + * VRR/Seamless M/N update may need to update frame timings. > + * > + * FIXME Should be synchronized with the start of vblank somehow... > + */ > + if (vrr_enabling(old_crtc_state, new_crtc_state) || > + (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state))) > + intel_crtc_update_active_timings(new_crtc_state, > + new_crtc_state->vrr.enable); > + > /* > * We usually enable FIFO underrun interrupts as part of the > * CRTC enable sequence during modesets. But when we inherit a > -- > 2.41.0 >
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > Syrjala > Sent: 01 September 2023 18:35 > To: intel-gfx@lists.freedesktop.org > Subject: [Intel-gfx] [PATCH 04/12] drm/i915: Enable VRR later during fastsets > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > In order to reconcile seamless M/N updates with VRR we'll need to defer the > fastset VRR enable to happen after the seamless M/N update (which > happens during the vblank evade critical section). So just push the VRR > enable to be the last thing during the update. > > This will also affect the vblank evasion as the transcoder will now still be > running with the old VRR state during the vblank evasion. So just grab the > timings always from the old crtc state during any non-modeset commit, and > also grab the current state of VRR from the active timings (as we disable VRR > before vblank evasion during fastsets). > > This also fixes vblank evasion for seamless M/N updates as we now properly > account for the fact that the M/N update happens after vblank evasion. > > Cc: Manasi Navare <navaremanasi@chromium.org> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_crtc.c | 35 ++++++++++++-------- > drivers/gpu/drm/i915/display/intel_display.c | 21 ++++++++---- > 2 files changed, 36 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > b/drivers/gpu/drm/i915/display/intel_crtc.c > index e46a15d59d79..1992e7060263 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -472,15 +472,31 @@ static void > intel_crtc_vblank_evade_scanlines(struct intel_atomic_state *state, > struct intel_crtc *crtc, > int *min, int *max, int > *vblank_start) { > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > - const struct drm_display_mode *adjusted_mode = &new_crtc_state- > >hw.adjusted_mode; > + const struct intel_crtc_state *crtc_state; > + const struct drm_display_mode *adjusted_mode; > > - if (new_crtc_state->vrr.enable) { > - if (intel_vrr_is_push_sent(new_crtc_state)) > - *vblank_start = > intel_vrr_vmin_vblank_start(new_crtc_state); > + /* > + * During fastsets/etc. the transcoder is still > + * running with the old timings at this point. > + * > + * TODO: maybe just use the active timings here? > + */ > + if (intel_crtc_needs_modeset(new_crtc_state)) > + crtc_state = new_crtc_state; > + else > + crtc_state = old_crtc_state; > + > + adjusted_mode = &crtc_state->hw.adjusted_mode; > + > + if (crtc->mode_flags & I915_MODE_FLAG_VRR) { > + if (intel_vrr_is_push_sent(crtc_state)) > + *vblank_start = > intel_vrr_vmin_vblank_start(crtc_state); > else > - *vblank_start = > intel_vrr_vmax_vblank_start(new_crtc_state); > + *vblank_start = > intel_vrr_vmax_vblank_start(crtc_state); > } else { > *vblank_start = intel_mode_vblank_start(adjusted_mode); > } > @@ -710,15 +726,6 @@ void intel_pipe_update_end(struct > intel_atomic_state *state, > */ > intel_vrr_send_push(new_crtc_state); > > - /* > - * Seamless M/N update may need to update frame timings. > - * > - * FIXME Should be synchronized with the start of vblank somehow... > - */ > - if (new_crtc_state->seamless_m_n && > intel_crtc_needs_fastset(new_crtc_state)) > - intel_crtc_update_active_timings(new_crtc_state, > - new_crtc_state->vrr.enable); > - > local_irq_enable(); > > if (intel_vgpu_active(dev_priv)) > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index cfad967b5684..632f1f58df9e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -6476,6 +6476,8 @@ static void commit_pipe_post_planes(struct > intel_atomic_state *state, > struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > > @@ -6487,6 +6489,9 @@ static void commit_pipe_post_planes(struct > intel_atomic_state *state, > if (DISPLAY_VER(dev_priv) >= 9 && > !intel_crtc_needs_modeset(new_crtc_state)) > skl_detach_scalers(new_crtc_state); > + > + if (vrr_enabling(old_crtc_state, new_crtc_state)) > + intel_vrr_enable(new_crtc_state); > } > > static void intel_enable_crtc(struct intel_atomic_state *state, @@ -6527,12 > +6532,6 @@ static void intel_update_crtc(struct intel_atomic_state *state, > intel_dpt_configure(crtc); > } > > - if (vrr_enabling(old_crtc_state, new_crtc_state)) { > - intel_vrr_enable(new_crtc_state); > - intel_crtc_update_active_timings(new_crtc_state, > - new_crtc_state->vrr.enable); > - } > - > if (!modeset) { > if (new_crtc_state->preload_luts && > intel_crtc_needs_color_update(new_crtc_state)) > @@ -6569,6 +6568,16 @@ static void intel_update_crtc(struct > intel_atomic_state *state, > > intel_pipe_update_end(state, crtc); > > + /* > + * VRR/Seamless M/N update may need to update frame timings. > + * > + * FIXME Should be synchronized with the start of vblank somehow... > + */ > + if (vrr_enabling(old_crtc_state, new_crtc_state) || > + (new_crtc_state->seamless_m_n && > intel_crtc_needs_fastset(new_crtc_state))) > + intel_crtc_update_active_timings(new_crtc_state, > + new_crtc_state->vrr.enable); > + changes looks good to me. Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> > /* > * We usually enable FIFO underrun interrupts as part of the > * CRTC enable sequence during modesets. But when we inherit a > -- > 2.41.0
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index e46a15d59d79..1992e7060263 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -472,15 +472,31 @@ static void intel_crtc_vblank_evade_scanlines(struct intel_atomic_state *state, struct intel_crtc *crtc, int *min, int *max, int *vblank_start) { + const struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); const struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); - const struct drm_display_mode *adjusted_mode = &new_crtc_state->hw.adjusted_mode; + const struct intel_crtc_state *crtc_state; + const struct drm_display_mode *adjusted_mode; - if (new_crtc_state->vrr.enable) { - if (intel_vrr_is_push_sent(new_crtc_state)) - *vblank_start = intel_vrr_vmin_vblank_start(new_crtc_state); + /* + * During fastsets/etc. the transcoder is still + * running with the old timings at this point. + * + * TODO: maybe just use the active timings here? + */ + if (intel_crtc_needs_modeset(new_crtc_state)) + crtc_state = new_crtc_state; + else + crtc_state = old_crtc_state; + + adjusted_mode = &crtc_state->hw.adjusted_mode; + + if (crtc->mode_flags & I915_MODE_FLAG_VRR) { + if (intel_vrr_is_push_sent(crtc_state)) + *vblank_start = intel_vrr_vmin_vblank_start(crtc_state); else - *vblank_start = intel_vrr_vmax_vblank_start(new_crtc_state); + *vblank_start = intel_vrr_vmax_vblank_start(crtc_state); } else { *vblank_start = intel_mode_vblank_start(adjusted_mode); } @@ -710,15 +726,6 @@ void intel_pipe_update_end(struct intel_atomic_state *state, */ intel_vrr_send_push(new_crtc_state); - /* - * Seamless M/N update may need to update frame timings. - * - * FIXME Should be synchronized with the start of vblank somehow... - */ - if (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state)) - intel_crtc_update_active_timings(new_crtc_state, - new_crtc_state->vrr.enable); - local_irq_enable(); if (intel_vgpu_active(dev_priv)) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index cfad967b5684..632f1f58df9e 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6476,6 +6476,8 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state, struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); + const struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); const struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc); @@ -6487,6 +6489,9 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state, if (DISPLAY_VER(dev_priv) >= 9 && !intel_crtc_needs_modeset(new_crtc_state)) skl_detach_scalers(new_crtc_state); + + if (vrr_enabling(old_crtc_state, new_crtc_state)) + intel_vrr_enable(new_crtc_state); } static void intel_enable_crtc(struct intel_atomic_state *state, @@ -6527,12 +6532,6 @@ static void intel_update_crtc(struct intel_atomic_state *state, intel_dpt_configure(crtc); } - if (vrr_enabling(old_crtc_state, new_crtc_state)) { - intel_vrr_enable(new_crtc_state); - intel_crtc_update_active_timings(new_crtc_state, - new_crtc_state->vrr.enable); - } - if (!modeset) { if (new_crtc_state->preload_luts && intel_crtc_needs_color_update(new_crtc_state)) @@ -6569,6 +6568,16 @@ static void intel_update_crtc(struct intel_atomic_state *state, intel_pipe_update_end(state, crtc); + /* + * VRR/Seamless M/N update may need to update frame timings. + * + * FIXME Should be synchronized with the start of vblank somehow... + */ + if (vrr_enabling(old_crtc_state, new_crtc_state) || + (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state))) + intel_crtc_update_active_timings(new_crtc_state, + new_crtc_state->vrr.enable); + /* * We usually enable FIFO underrun interrupts as part of the * CRTC enable sequence during modesets. But when we inherit a