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 Wed, Sep 18, 2024 at 01:58:44PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 18, 2024 at 05:53:37AM +0000, Hogander, Jouni wrote:
> > On Tue, 2024-09-17 at 20:58 +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?
> > 
> > Is there intel_crtc_vblank_off/on cycle always when doing full mode
> > set? If that is the case, then I think we can rely on crtc_state-
> > >has_panel_replay: changes in Panel Replay mode always mean full mode
> > set currently. How about fast mode set? Do we have vblank off/on cycle
> > there?
> 
> No. vblank_off()/on() is only around full modesets.
> 
> > 
> > Later if we move into activating/de-activating Panel Replay without
> > full mode set I think we need to do something else.
> 
> I think we need a clear separation of the "logically enabled/possible"
> vs. "currently active" states of PSR and panel replay. With that
> we can just always enable this workaround whenever panel replay
> was selected during the full modeset. Fastsets/plane updates
> can then just activate/deactivate panel replay/PSR (*) as needed
> due to more dynamic constraints (eg. planes going on/off) without
> having to worry about this stuff.
> 
> (*) the activate/deactive should only toggle the single enable
>     bit in the appropriate registers, nothing more

Just to clarify, I'm fine with going with this logic for now
if the has_panel_replay/etc isn't suitable rigth now (as in
can change during fastsets/etc), as long as it's neatly 
buried in the psr code.

So if this code reads something along the lines of:
 crtc->block_dc_for_vblank = intel_psr_block_dc_for_vblank(...); 

then I can just turn a blind eye to the details
and keep on reading past it ;)

-- 
Ville Syrjälä
Intel