Re: [PATCH 05/10] drm/i915/psr: Ensure SFF/CFF bits are not written at their sample point

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Fri, 2024-12-20 at 11:34 +0200, Ville Syrjälä wrote:
> On Fri, Dec 13, 2024 at 08:35:23AM +0200, Jouni Högander wrote:
> > Bspec says this for SFF/CFF:
> > 
> > "Hardware samples this bit on the start of every V. Blank Guardband
> > region. For deterministic behavior, Software should ensure it is
> > not
> > changing this bit around the sample point."
> > 
> > This can be achieved by using intel_vblank_evade.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index c074de6b6c12..8aa2574ba5c9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -26,6 +26,7 @@
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_damage_helper.h>
> >  #include <drm/drm_debugfs.h>
> > +#include <drm/drm_vblank.h>
> >  
> >  #include "i915_drv.h"
> >  #include "i915_reg.h"
> > @@ -44,6 +45,7 @@
> >  #include "intel_psr.h"
> >  #include "intel_psr_regs.h"
> >  #include "intel_snps_phy.h"
> > +#include "intel_vblank.h"
> >  #include "skl_universal_plane.h"
> >  
> >  /**
> > @@ -3114,16 +3116,31 @@ static void
> > intel_psr_configure_full_frame_update(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> > +	struct intel_crtc *crtc = intel_crtc_for_pipe(display,
> > intel_dp->psr.pipe);
> > +	struct intel_crtc_state *crtc_state =
> > to_intel_crtc_state(crtc->base.state);
> 
> You can't use that without proper locking.

I did some experiments with this and started thinking I will drop this
patch from the series. frontbuffer callbacks in gem code are quite a
rabbit hole what comes to locking. This is anyways addressing existing
issue not introduced by this patch series. One idea I have is to
implement evasion only for ORIGIN_DIRTYFB and ORIGIN_CURSOR_UPDATE. Do
you have any better ideas?

BR,

Jouni Högander

> 
> > +	struct intel_vblank_evade_ctx evade;
> >  
> >  	if (!intel_dp->psr.psr2_sel_fetch_enabled)
> >  		return;
> >  
> > +	intel_vblank_evade_init(crtc_state, crtc_state, &evade);
> > +
> > +	drm_crtc_vblank_get(&crtc->base);
> > +
> > +	local_irq_disable();
> > +
> > +	intel_vblank_evade(&evade);
> > +
> > +	drm_crtc_vblank_put(&crtc->base);
> > +
> >  	intel_de_write(display,
> >  		       PSR2_MAN_TRK_CTL(display, cpu_transcoder),
> >  		       man_trk_ctl_enable_bit_get(display) |
> >  		       man_trk_ctl_partial_frame_bit_get(display)
> > |
> >  		      
> > man_trk_ctl_single_full_frame_bit_get(display) |
> >  		       man_trk_ctl_continuos_full_frame(display));
> > +
> > +	local_irq_enable();
> >  }
> >  
> >  static void _psr_invalidate_handle(struct intel_dp *intel_dp)
> > -- 
> > 2.34.1
>