Re: [PATCH v3 1/2] drm/i915/display: Add block_dc6_needed variable into intel_crtc

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 17, 2024 at 08:58:07PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 17, 2024 at 09:35:59AM +0300, Jouni Högander wrote:
> > We need to block DC6 entry in case of Panel Replay as enabling VBI doesn't
> > prevent DC6 in case of Panel Replay. This causes problems if user-space is
> > polling for vblank events. For this purpose add new block_dc6_needed
> > variable into intel_crtc. Check if eDP Panel Replay is possible and set the
> > variable accordingly.
> > 
> > v3: check that encoder is dp
> > v2: set/clear block_dc6_needed in intel_crtc_vblank_on/off
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crtc.c       | 17 +++++++++++++++++
> >  .../gpu/drm/i915/display/intel_display_types.h  |  7 +++++++
> >  drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index aed3853952be8..34a60b5b1e55b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -24,6 +24,7 @@
> >  #include "intel_display_irq.h"
> >  #include "intel_display_trace.h"
> >  #include "intel_display_types.h"
> > +#include "intel_dp.h"
> >  #include "intel_drrs.h"
> >  #include "intel_dsi.h"
> >  #include "intel_fifo_underrun.h"
> > @@ -123,6 +124,20 @@ u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state)
> >  void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct intel_encoder *encoder;
> > +
> > +	for_each_encoder_on_crtc(crtc->base.dev, &crtc->base, encoder) {
> > +		struct intel_dp *intel_dp;
> > +
> > +		if (!intel_encoder_is_dp(encoder))
> > +			continue;
> > +
> > +		intel_dp = enc_to_intel_dp(encoder);
> > +
> > +		if (intel_dp_is_edp(intel_dp) &&
> > +		    CAN_PANEL_REPLAY(intel_dp))
> > +			crtc->block_dc6_needed = true;
> > +	}
> 
> This could just a function provided by intel_psr.c so that
> we don't have to to see any of the details.
> 
> Is there some reason this isn't simply looking at
> crtc_state->has_panel_replay?
> 
> >  
> >  	assert_vblank_disabled(&crtc->base);
> >  	drm_crtc_set_max_vblank_count(&crtc->base,
> > @@ -150,6 +165,8 @@ void intel_crtc_vblank_off(const struct intel_crtc_state *crtc_state)
> >  
> >  	drm_crtc_vblank_off(&crtc->base);
> >  	assert_vblank_disabled(&crtc->base);
> > +
> > +	crtc->block_dc6_needed = false;
> >  }
> >  
> >  struct intel_crtc_state *intel_crtc_state_alloc(struct intel_crtc *crtc)
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 000ab373c8879..df0c3eb750809 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1413,6 +1413,13 @@ struct intel_crtc {
> >  #ifdef CONFIG_DEBUG_FS
> >  	struct intel_pipe_crc pipe_crc;
> >  #endif
> > +
> > +	/*
> > +	 * We need to block DC6 entry in case of Panel Replay as enabling VBI doesn't
> > +	 * prevent DC6 in case of Panel Replay. This causes problems if user-space is
> > +	 * polling for vblank events.
> > +	 */
> 
> We should point out the fact that panel replay turns the
> link off only while in DC states. Otherwise I'm sure to
> get confused by this again in the future.

This comment should probably live in the function that the PSR
code provides to determine if we need to block DC states or not.
We could have other reasons for using this same mechanism in 
the future.

> 
> > +	u8 block_dc6_needed;
> 
> That sounds a bit too generic perhaps. block_dc_for_vblank or something?
> 
> >  };
> >  
> >  struct intel_plane {
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 4f29ac32ff85b..957f470b08fe8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -35,6 +35,7 @@
> >  #include "intel_cursor_regs.h"
> >  #include "intel_ddi.h"
> >  #include "intel_de.h"
> > +#include "intel_display_irq.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp.h"
> >  #include "intel_dp_aux.h"
> > -- 
> > 2.34.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel