* [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup. @ 2014-02-07 18:09 Rodrigo Vivi 2014-02-07 18:09 ` [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite Rodrigo Vivi 2014-02-07 19:14 ` [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup Ville Syrjälä 0 siblings, 2 replies; 7+ messages in thread From: Rodrigo Vivi @ 2014-02-07 18:09 UTC (permalink / raw) To: intel-gfx As pointed out by Ville we were using inverted logic here. According to spec: For link standby mode set 170h[1] = 1. For full link disabling set 170h[1] = 0. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 50381f7..4ecda72 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1661,12 +1661,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) /* Enable PSR in sink */ if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, - DP_PSR_ENABLE & - ~DP_PSR_MAIN_LINK_ACTIVE); - else - intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE); + else + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, + DP_PSR_ENABLE & + ~DP_PSR_MAIN_LINK_ACTIVE); /* Setup AUX registers */ I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND); -- 1.7.11.7 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite. 2014-02-07 18:09 [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup Rodrigo Vivi @ 2014-02-07 18:09 ` Rodrigo Vivi 2014-02-07 19:00 ` Daniel Vetter 2014-02-07 19:17 ` Ville Syrjälä 2014-02-07 19:14 ` [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup Ville Syrjälä 1 sibling, 2 replies; 7+ messages in thread From: Rodrigo Vivi @ 2014-02-07 18:09 UTC (permalink / raw) To: intel-gfx On the current structure HSW doesn't support PSR with sprites enabled but sprites can be enabled after PSR was enabled what would cause user to miss screen updates. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/intel_sprite.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 336ae6c..3132686 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, I915_WRITE(SPRSURF(pipe), i915_gem_obj_ggtt_offset(obj) + sprsurf_offset); POSTING_READ(SPRSURF(pipe)); + + intel_edp_psr_update(dev); } static void -- 1.7.11.7 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite. 2014-02-07 18:09 ` [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite Rodrigo Vivi @ 2014-02-07 19:00 ` Daniel Vetter 2014-02-07 19:17 ` Ville Syrjälä 1 sibling, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2014-02-07 19:00 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx On Fri, Feb 07, 2014 at 04:09:48PM -0200, Rodrigo Vivi wrote: > On the current structure HSW doesn't support PSR with sprites enabled > but sprites can be enabled after PSR was enabled what would cause > user to miss screen updates. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> Have you checked whether your current psr testcase hits this issue? If so please add the Testcase: tag with the right subtest to this patch. -Daniel > --- > drivers/gpu/drm/i915/intel_sprite.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 336ae6c..3132686 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > I915_WRITE(SPRSURF(pipe), > i915_gem_obj_ggtt_offset(obj) + sprsurf_offset); > POSTING_READ(SPRSURF(pipe)); > + > + intel_edp_psr_update(dev); > } > > static void > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite. 2014-02-07 18:09 ` [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite Rodrigo Vivi 2014-02-07 19:00 ` Daniel Vetter @ 2014-02-07 19:17 ` Ville Syrjälä 2014-02-07 19:24 ` Rodrigo Vivi 1 sibling, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2014-02-07 19:17 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx On Fri, Feb 07, 2014 at 04:09:48PM -0200, Rodrigo Vivi wrote: > On the current structure HSW doesn't support PSR with sprites enabled > but sprites can be enabled after PSR was enabled what would cause > user to miss screen updates. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 336ae6c..3132686 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > I915_WRITE(SPRSURF(pipe), > i915_gem_obj_ggtt_offset(obj) + sprsurf_offset); > POSTING_READ(SPRSURF(pipe)); > + > + intel_edp_psr_update(dev); I was thinking this might be better placed in intel_update_plane() since the fbc/ips stuff is there, but I can live with it being here too. But should if have a HSW check on it? BDW doesn't have this restriction anymore, right? > } > > static void > -- > 1.7.11.7 -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite. 2014-02-07 19:17 ` Ville Syrjälä @ 2014-02-07 19:24 ` Rodrigo Vivi 0 siblings, 0 replies; 7+ messages in thread From: Rodrigo Vivi @ 2014-02-07 19:24 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, Feb 7, 2014 at 5:17 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Feb 07, 2014 at 04:09:48PM -0200, Rodrigo Vivi wrote: >> On the current structure HSW doesn't support PSR with sprites enabled >> but sprites can be enabled after PSR was enabled what would cause >> user to miss screen updates. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index 336ae6c..3132686 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, >> I915_WRITE(SPRSURF(pipe), >> i915_gem_obj_ggtt_offset(obj) + sprsurf_offset); >> POSTING_READ(SPRSURF(pipe)); >> + >> + intel_edp_psr_update(dev); > > I was thinking this might be better placed in intel_update_plane() > since the fbc/ips stuff is there, but I can live with it being here too. I agree there is better. > > But should if have a HSW check on it? BDW doesn't have this restriction > anymore, right? yes, it should... you are right! > >> } >> >> static void >> -- >> 1.7.11.7 > > -- > Ville Syrjälä > Intel OTC I also agree with Daniel we need a test case for that... so, will provide this and a new patch version later -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup. 2014-02-07 18:09 [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup Rodrigo Vivi 2014-02-07 18:09 ` [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite Rodrigo Vivi @ 2014-02-07 19:14 ` Ville Syrjälä 2014-02-07 19:29 ` Rodrigo Vivi 1 sibling, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2014-02-07 19:14 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx On Fri, Feb 07, 2014 at 04:09:47PM -0200, Rodrigo Vivi wrote: > As pointed out by Ville we were using inverted logic here. > According to spec: > For link standby mode set 170h[1] = 1. > For full link disabling set 170h[1] = 0. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 50381f7..4ecda72 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1661,12 +1661,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) > /* Enable PSR in sink */ > if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) > intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, > - DP_PSR_ENABLE & > - ~DP_PSR_MAIN_LINK_ACTIVE); > - else > - intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, > DP_PSR_ENABLE | > DP_PSR_MAIN_LINK_ACTIVE); > + else > + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, > + DP_PSR_ENABLE & > + ~DP_PSR_MAIN_LINK_ACTIVE); I think this is now the opposite of what we want. Ie. if the sink doesn't require training we should disable the main link. Otherwise we should keep the main link on, and that way avoid the need to train on PSR exit. Actually I'm not sure that's really what we want. I think the hardware can do the training on its own, so in theory we should just always disable the main link. Although the PM guide has a comment indicating that the hardware training can fail, in which case software must repeat it. We don't have code to do that, so I guess leaving the main link on is the safer option. Would be nice to have a comment in the code stating as much, if this is indeed the reason why the code was written this way. > > /* Setup AUX registers */ > I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND); > -- > 1.7.11.7 -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup. 2014-02-07 19:14 ` [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup Ville Syrjälä @ 2014-02-07 19:29 ` Rodrigo Vivi 0 siblings, 0 replies; 7+ messages in thread From: Rodrigo Vivi @ 2014-02-07 19:29 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, Feb 7, 2014 at 5:14 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Feb 07, 2014 at 04:09:47PM -0200, Rodrigo Vivi wrote: >> As pointed out by Ville we were using inverted logic here. >> According to spec: >> For link standby mode set 170h[1] = 1. >> For full link disabling set 170h[1] = 0. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 50381f7..4ecda72 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1661,12 +1661,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) >> /* Enable PSR in sink */ >> if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) >> intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, >> - DP_PSR_ENABLE & >> - ~DP_PSR_MAIN_LINK_ACTIVE); >> - else >> - intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, >> DP_PSR_ENABLE | >> DP_PSR_MAIN_LINK_ACTIVE); >> + else >> + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, >> + DP_PSR_ENABLE & >> + ~DP_PSR_MAIN_LINK_ACTIVE); > > I think this is now the opposite of what we want. Ie. if the sink > doesn't require training we should disable the main link. Otherwise we > should keep the main link on, and that way avoid the need to train on > PSR exit. To be honest, I think I agree with you, but apparently performance counter inc improved on this way... > > Actually I'm not sure that's really what we want. I think the hardware > can do the training on its own, so in theory we should just always disable > the main link. Although the PM guide has a comment indicating that the > hardware training can fail, in which case software must repeat it. We > don't have code to do that, so I guess leaving the main link on is the > safer option. Would be nice to have a comment in the code stating as > much, if this is indeed the reason why the code was written this way. I'll do a carefull check and local tests and send new version fixed or with good comments. > >> >> /* Setup AUX registers */ >> I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND); >> -- >> 1.7.11.7 > > -- > Ville Syrjälä > Intel OTC -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-02-07 19:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-07 18:09 [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup Rodrigo Vivi 2014-02-07 18:09 ` [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite Rodrigo Vivi 2014-02-07 19:00 ` Daniel Vetter 2014-02-07 19:17 ` Ville Syrjälä 2014-02-07 19:24 ` Rodrigo Vivi 2014-02-07 19:14 ` [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup Ville Syrjälä 2014-02-07 19:29 ` Rodrigo Vivi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox