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

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

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?

Later if we move into activating/de-activating Panel Replay without
full mode set I think we need to do something else. E.g. reference
count I had in previous version was trying to address this. To my
opinion relying on CAN_PANEL_REPLAY could be good enough. That would
cover all cases where blocking is needed. Drawback is that we are
unnecessarily blocking it on certain cases. But that shouldn't matter
as in these cases DC6 is blocked anyways by the HW. One example is
panel supporting Panel Replay, but only PSR2 is allowed and VBI is
enabled. What do you think?

BR,

Jouni Högander

> 
> >  
> >         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.
> 
> > +       u8 block_dc6_needed;
> 
> That sounds a bit too generic perhaps. block_dc_for_vblank or
> something?

Ok, I will change these.

BR,

Jouni Högander

> 
> >  };
> >  
> >  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
>