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@xxxxxxxxx>
> > > > ---
> > > > 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
