| Message ID | 20240917063600.3086259-2-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Block DC6 on Vblank enable for Panel Replay | expand |
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. > + 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
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
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 >
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 > 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 > > >
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 ;)
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; + } 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. + */ + u8 block_dc6_needed; }; 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"
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(+)