| Message ID | 20190214020206.26046-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset | expand |
On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote: > Forcing a specific CRTC to the eDP connector was causing the > intel_psr_fastset_force() to mark mode_chaged in the wrong and > disabled CRTC causing no update in the PSR state. > > Looks like our internal state track do not clear output_types and > has_psr in the disabled CRTCs, not sure if this is the expected > behavior or not but in the mean time this fix the issue. Is there an IGT that's failing because of this? Looks like the state would have got cleared only if the crtc was enabled to drive another encoder. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index 75c1a5deebf5..08967836b48e 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -986,7 +986,8 @@ static int intel_psr_fastset_force(struct > drm_i915_private *dev_priv) > > intel_crtc_state = to_intel_crtc_state(crtc_state); > > - if (intel_crtc_has_type(intel_crtc_state, > INTEL_OUTPUT_EDP) && > + if (crtc_state->enable && > + intel_crtc_has_type(intel_crtc_state, > INTEL_OUTPUT_EDP) && > intel_crtc_state->has_psr) { > /* Mark mode as changed to trigger a pipe- > >update() */ > crtc_state->mode_changed = true; I was wondering if we should remove the 'break;' that follows instead and let ->update_pipe take care of the rest. However, checking crtc_state->enable makes sense as there is not much point in triggering a fastset on a disabled crtc. Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> There might be a better way to do this, so please double check with someone :)
On Fri, 2019-02-22 at 16:27 -0800, Pandiyan, Dhinakaran wrote: > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote: > > Forcing a specific CRTC to the eDP connector was causing the > > intel_psr_fastset_force() to mark mode_chaged in the wrong and > > disabled CRTC causing no update in the PSR state. > > > > Looks like our internal state track do not clear output_types and > > has_psr in the disabled CRTCs, not sure if this is the expected > > behavior or not but in the mean time this fix the issue. > > Is there an IGT that's failing because of this? Looks like the state > would have got cleared only if the crtc was enabled to drive another > encoder. When PSR2 is enabled by default tests like kms_pipe_crc_basic@read-crc-pipe-b are failling even with the patch that disable PSR2 when getting CRC. > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 75c1a5deebf5..08967836b48e 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -986,7 +986,8 @@ static int intel_psr_fastset_force(struct > > drm_i915_private *dev_priv) > > > > intel_crtc_state = to_intel_crtc_state(crtc_state); > > > > - if (intel_crtc_has_type(intel_crtc_state, > > INTEL_OUTPUT_EDP) && > > + if (crtc_state->enable && > > + intel_crtc_has_type(intel_crtc_state, > > INTEL_OUTPUT_EDP) && > > intel_crtc_state->has_psr) { > > /* Mark mode as changed to trigger a pipe- > > > update() */ > > crtc_state->mode_changed = true; > I was wondering if we should remove the 'break;' that follows instead > and let ->update_pipe take care of the rest. However, checking > crtc_state->enable makes sense as there is not much point in > triggering > a fastset on a disabled crtc. > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > There might be a better way to do this, so please double check with > someone :) >
On Fri, 2019-02-22 at 16:33 -0800, Souza, Jose wrote: > On Fri, 2019-02-22 at 16:27 -0800, Pandiyan, Dhinakaran wrote: > > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote: > > > Forcing a specific CRTC to the eDP connector was causing the > > > intel_psr_fastset_force() to mark mode_chaged in the wrong and > > > disabled CRTC causing no update in the PSR state. > > > > > > Looks like our internal state track do not clear output_types and > > > has_psr in the disabled CRTCs, not sure if this is the expected > > > behavior or not but in the mean time this fix the issue. > > > > Is there an IGT that's failing because of this? Looks like the > > state > > would have got cleared only if the crtc was enabled to drive > > another > > encoder. > > When PSR2 is enabled by default tests like > kms_pipe_crc_basic@read-crc-pipe-b are failling even with the patch > that disable PSR2 when getting CRC. Thanks! > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 75c1a5deebf5..08967836b48e 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -986,7 +986,8 @@ static int intel_psr_fastset_force(struct > > > drm_i915_private *dev_priv) > > > > > > intel_crtc_state = to_intel_crtc_state(crtc_state); > > > > > > - if (intel_crtc_has_type(intel_crtc_state, > > > INTEL_OUTPUT_EDP) && > > > + if (crtc_state->enable && > > > + intel_crtc_has_type(intel_crtc_state, > > > INTEL_OUTPUT_EDP) && > > > intel_crtc_state->has_psr) { if (crtc_state->enable && intel_crtc_state->has_psr) should cover all the cases, no? And also add a WARN() in case we somehow end up with more than once crtc with the above condition being true. > > > /* Mark mode as changed to trigger a pipe- > > > > update() */ > > > > > > crtc_state->mode_changed = true; > > > > I was wondering if we should remove the 'break;' that follows > > instead > > and let ->update_pipe take care of the rest. However, checking > > crtc_state->enable makes sense as there is not much point in > > triggering > > a fastset on a disabled crtc. > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > There might be a better way to do this, so please double check with > > someone :) > >
Op 23-02-2019 om 03:02 schreef Dhinakaran Pandiyan: > On Fri, 2019-02-22 at 16:33 -0800, Souza, Jose wrote: >> On Fri, 2019-02-22 at 16:27 -0800, Pandiyan, Dhinakaran wrote: >>> On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote: >>>> Forcing a specific CRTC to the eDP connector was causing the >>>> intel_psr_fastset_force() to mark mode_chaged in the wrong and >>>> disabled CRTC causing no update in the PSR state. >>>> >>>> Looks like our internal state track do not clear output_types and >>>> has_psr in the disabled CRTCs, not sure if this is the expected >>>> behavior or not but in the mean time this fix the issue. >>> Is there an IGT that's failing because of this? Looks like the >>> state >>> would have got cleared only if the crtc was enabled to drive >>> another >>> encoder. >> When PSR2 is enabled by default tests like >> kms_pipe_crc_basic@read-crc-pipe-b are failling even with the patch >> that disable PSR2 when getting CRC. > Thanks! > >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_psr.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c >>>> b/drivers/gpu/drm/i915/intel_psr.c >>>> index 75c1a5deebf5..08967836b48e 100644 >>>> --- a/drivers/gpu/drm/i915/intel_psr.c >>>> +++ b/drivers/gpu/drm/i915/intel_psr.c >>>> @@ -986,7 +986,8 @@ static int intel_psr_fastset_force(struct >>>> drm_i915_private *dev_priv) >>>> >>>> intel_crtc_state = to_intel_crtc_state(crtc_state); >>>> >>>> - if (intel_crtc_has_type(intel_crtc_state, >>>> INTEL_OUTPUT_EDP) && >>>> + if (crtc_state->enable && >>>> + intel_crtc_has_type(intel_crtc_state, >>>> INTEL_OUTPUT_EDP) && >>>> intel_crtc_state->has_psr) { > > if (crtc_state->enable && intel_crtc_state->has_psr) > should cover all the cases, no? > > And also add a WARN() in case we somehow end up with more than once > crtc with the above condition being true. Please use crtc_state->active crtc_state->enable only tells if the crtc is configured. It doesn't mean thatthe pipe is running. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 75c1a5deebf5..08967836b48e 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -986,7 +986,8 @@ static int intel_psr_fastset_force(struct drm_i915_private *dev_priv) intel_crtc_state = to_intel_crtc_state(crtc_state); - if (intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) && + if (crtc_state->enable && + intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) && intel_crtc_state->has_psr) { /* Mark mode as changed to trigger a pipe->update() */ crtc_state->mode_changed = true;
Forcing a specific CRTC to the eDP connector was causing the intel_psr_fastset_force() to mark mode_chaged in the wrong and disabled CRTC causing no update in the PSR state. Looks like our internal state track do not clear output_types and has_psr in the disabled CRTCs, not sure if this is the expected behavior or not but in the mean time this fix the issue. Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)