Re: [PATCH] drm/915/psr: Set psr.source_ok during atomic_check

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Thu, 2017-12-14 at 17:06 +0200, Ville Syrjälä wrote:
> On Tue, Dec 12, 2017 at 04:59:34PM -0800, Dhinakaran Pandiyan wrote:
> > Since commit 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc
> > state"), we check whether PSR can be enabled or not in
> > psr_compute_config(). Given that the psr.source_ok field is supposed to
> > track this check, set the field in psr_compute_config() as well.
> 
> NAK. compute_config() isn't allowed to change global state since the
> modeset can still fail later, or this might in fact be a
> TEST_ONLY operation.

I thought of it, but the only purpose of this flag is in debugfs to
indicate PSR requirements were met. It is not checked anywhere in the
driver. Setting this flag during commit (in intel_psr_enable) is
redundant because psr.enabled, exposed via debugfs, already provides
that information. By moving this flag to where PSR conditions are
checked, this could give a hint that something went wrong if PSR was not
enabled when PSR conditions were met.


Basically, I don't see any use for this flag the way it is set now. The
other idea I was considering is killing this flag and exposing a
no_fbc_reason type string.


> 
> > 
> > Now tests can distinguish between PSR not enabled due to unmet mode
> > requirements and something going wrong during commit.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index a1ad85fa5c1a..b59a956047ff 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -358,6 +358,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  		&crtc_state->base.adjusted_mode;
> >  	int psr_setup_time;
> >  
> > +	dev_priv->psr.source_ok = false;
> > +
> >  	if (!HAS_PSR(dev_priv))
> >  		return;
> >  
> > @@ -420,7 +422,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  	 * caps during eDP detection.
> >  	 */
> >  	if (!dev_priv->psr.psr2_support) {
> > -		crtc_state->has_psr = true;
> > +		dev_priv->psr.source_ok = (crtc_state->has_psr = true);
> >  		return;
> >  	}
> >  
> > @@ -440,7 +442,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  		return;
> >  	}
> >  
> > -	crtc_state->has_psr = true;
> > +	dev_priv->psr.source_ok = (crtc_state->has_psr = true);
> >  	crtc_state->has_psr2 = true;
> >  }
> >  
> > @@ -522,8 +524,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> >  	}
> >  
> >  	dev_priv->psr.psr2_support = crtc_state->has_psr2;
> > -	dev_priv->psr.source_ok = true;
> > -
> >  	dev_priv->psr.busy_frontbuffer_bits = 0;
> >  
> >  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
> > @@ -657,7 +657,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >  	/* Disable PSR on Sink */
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> >  
> > -	dev_priv->psr.enabled = NULL;
> > +	dev_priv->psr.source_ok = (dev_priv->psr.enabled = NULL);
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  
> >  	cancel_delayed_work_sync(&dev_priv->psr.work);
> > -- 
> > 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx