diff mbox

[1/6] drm/i915/psr: Avoid DPCD reads when panel does not support PSR

Message ID 20180511195145.3829-1-dhinakaran.pandiyan@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dhinakaran Pandiyan May 11, 2018, 7:51 p.m. UTC 
Ville noticed that we are unncessarily reading DPCD's after knowing
panel did not support PSR. Looks like this check that was present
earlier got removed unintentionally, let's put it back.

While we do this, add the PSR version number in the debug print.

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 | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Tarun Vyas May 18, 2018, 12:27 a.m. UTC | #1 
On Fri, May 11, 2018 at 12:51:40PM -0700, Dhinakaran Pandiyan wrote:
> Ville noticed that we are unncessarily reading DPCD's after knowing
> panel did not support PSR. Looks like this check that was present
> earlier got removed unintentionally, let's put it back.
> 
> While we do this, add the PSR version number in the debug print.
> 
> 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 | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index db27f2faa1de..8fe6d2f9ab2b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -250,10 +250,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
>  			 sizeof(intel_dp->psr_dpcd));
>  
> -	if (intel_dp->psr_dpcd[0]) {
> -		dev_priv->psr.sink_support = true;
> -		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> -	}
> +	if (!intel_dp->psr_dpcd[0])
> +		return;
> +
> +	DRM_DEBUG_KMS("eDP panel supports PSR version %x\n",
> +		      intel_dp->psr_dpcd[0]);
> +	dev_priv->psr.sink_support = true;
>  
>  	if (INTEL_GEN(dev_priv) >= 9 &&
>  	    (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> @@ -270,8 +272,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  		 */
>  		dev_priv->psr.sink_psr2_support =
>  				intel_dp_get_y_coord_required(intel_dp);
> -		DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.sink_psr2_support
> -			      ? "supported" : "not supported");
> +		DRM_DEBUG_KMS("PSR2 %ssupported\n",
> +			      dev_priv->psr.sink_psr2_support ? "" : "not ");
Would it make sense to make it clearer that PSR2 is not supported b/c of lack of y-coordinate support on the sink ?

Reviewed-by: Tarun Vyas <tarun.vyas@intel.com>
>  
>  		if (dev_priv->psr.sink_psr2_support) {
>  			dev_priv->psr.colorimetry_support =
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan May 21, 2018, 11:40 p.m. UTC | #2 
On Thu, 2018-05-17 at 17:27 -0700, Tarun Vyas wrote:
> On Fri, May 11, 2018 at 12:51:40PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > Ville noticed that we are unncessarily reading DPCD's after knowing
> > panel did not support PSR. Looks like this check that was present
> > earlier got removed unintentionally, let's put it back.
> > 
> > While we do this, add the PSR version number in the debug print.
> > 
> > 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 | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index db27f2faa1de..8fe6d2f9ab2b 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -250,10 +250,12 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp-
> > >psr_dpcd,
> >  			 sizeof(intel_dp->psr_dpcd));
> >  
> > -	if (intel_dp->psr_dpcd[0]) {
> > -		dev_priv->psr.sink_support = true;
> > -		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > -	}
> > +	if (!intel_dp->psr_dpcd[0])
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("eDP panel supports PSR version %x\n",
> > +		      intel_dp->psr_dpcd[0]);
> > +	dev_priv->psr.sink_support = true;
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> >  	    (intel_dp->psr_dpcd[0] ==
> > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > @@ -270,8 +272,8 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  		 */
> >  		dev_priv->psr.sink_psr2_support =
> >  				intel_dp_get_y_coord_required(inte
> > l_dp);
> > -		DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv-
> > >psr.sink_psr2_support
> > -			      ? "supported" : "not supported");
> > +		DRM_DEBUG_KMS("PSR2 %ssupported\n",
> > +			      dev_priv->psr.sink_psr2_support ? ""
> > : "not ");
> Would it make sense to make it clearer that PSR2 is not supported b/c
> of lack of y-coordinate support on the sink ?

We could do something like 

dev_priv->psr.sink_psr2_support = y_req && alpm;
DRM_DEBUG_KMS("PSR2 %ssupported ALPM %d Y-req %d\n",
              dev_priv->psr.sink_psr2_support ? "" : "not ",
              alpm, y_req);

But this would need the code movement done in patch 6/6.


> 
> Reviewed-by: Tarun Vyas <tarun.vyas@intel.com>
> > 
> >  
> >  		if (dev_priv->psr.sink_psr2_support) {
> >  			dev_priv->psr.colorimetry_support =
vathsala nagaraju May 22, 2018, 12:29 p.m. UTC | #3 
On 5/12/2018 1:21 AM, Dhinakaran Pandiyan wrote:
> Ville noticed that we are unncessarily reading DPCD's after knowing
> panel did not support PSR. Looks like this check that was present
> earlier got removed unintentionally, let's put it back.
>
> While we do this, add the PSR version number in the debug print.
>
> 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 | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
Reviewed-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index db27f2faa1de..8fe6d2f9ab2b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -250,10 +250,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>   	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
>   			 sizeof(intel_dp->psr_dpcd));
>   
> -	if (intel_dp->psr_dpcd[0]) {
> -		dev_priv->psr.sink_support = true;
> -		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> -	}
> +	if (!intel_dp->psr_dpcd[0])
> +		return;
> +
> +	DRM_DEBUG_KMS("eDP panel supports PSR version %x\n",
> +		      intel_dp->psr_dpcd[0]);
> +	dev_priv->psr.sink_support = true;
>   
>   	if (INTEL_GEN(dev_priv) >= 9 &&
>   	    (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> @@ -270,8 +272,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>   		 */
>   		dev_priv->psr.sink_psr2_support =
>   				intel_dp_get_y_coord_required(intel_dp);
> -		DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.sink_psr2_support
> -			      ? "supported" : "not supported");
> +		DRM_DEBUG_KMS("PSR2 %ssupported\n",
> +			      dev_priv->psr.sink_psr2_support ? "" : "not ");
>   
>   		if (dev_priv->psr.sink_psr2_support) {
>   			dev_priv->psr.colorimetry_support =
Jani Nikula May 24, 2018, 1:29 p.m. UTC | #4 
On Thu, 17 May 2018, Tarun Vyas <tarun.vyas@intel.com> wrote:
> On Fri, May 11, 2018 at 12:51:40PM -0700, Dhinakaran Pandiyan wrote:
>> Ville noticed that we are unncessarily reading DPCD's after knowing
>> panel did not support PSR. Looks like this check that was present
>> earlier got removed unintentionally, let's put it back.
>> 
>> While we do this, add the PSR version number in the debug print.
>> 
>> 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 | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index db27f2faa1de..8fe6d2f9ab2b 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -250,10 +250,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>>  	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
>>  			 sizeof(intel_dp->psr_dpcd));
>>  
>> -	if (intel_dp->psr_dpcd[0]) {
>> -		dev_priv->psr.sink_support = true;
>> -		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> -	}
>> +	if (!intel_dp->psr_dpcd[0])
>> +		return;
>> +
>> +	DRM_DEBUG_KMS("eDP panel supports PSR version %x\n",
>> +		      intel_dp->psr_dpcd[0]);
>> +	dev_priv->psr.sink_support = true;
>>  
>>  	if (INTEL_GEN(dev_priv) >= 9 &&
>>  	    (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
>> @@ -270,8 +272,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>>  		 */
>>  		dev_priv->psr.sink_psr2_support =
>>  				intel_dp_get_y_coord_required(intel_dp);
>> -		DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.sink_psr2_support
>> -			      ? "supported" : "not supported");
>> +		DRM_DEBUG_KMS("PSR2 %ssupported\n",
>> +			      dev_priv->psr.sink_psr2_support ? "" : "not ");
> Would it make sense to make it clearer that PSR2 is not supported b/c of lack of y-coordinate support on the sink ?
>
> Reviewed-by: Tarun Vyas <tarun.vyas@intel.com>

Pushed to dinq, thanks for the patch and review.

BR,
Jani.


>>  
>>  		if (dev_priv->psr.sink_psr2_support) {
>>  			dev_priv->psr.colorimetry_support =
>> -- 
>> 2.14.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index db27f2faa1de..8fe6d2f9ab2b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -250,10 +250,12 @@  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 	drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
 			 sizeof(intel_dp->psr_dpcd));
 
-	if (intel_dp->psr_dpcd[0]) {
-		dev_priv->psr.sink_support = true;
-		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
-	}
+	if (!intel_dp->psr_dpcd[0])
+		return;
+
+	DRM_DEBUG_KMS("eDP panel supports PSR version %x\n",
+		      intel_dp->psr_dpcd[0]);
+	dev_priv->psr.sink_support = true;
 
 	if (INTEL_GEN(dev_priv) >= 9 &&
 	    (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
@@ -270,8 +272,8 @@  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 		 */
 		dev_priv->psr.sink_psr2_support =
 				intel_dp_get_y_coord_required(intel_dp);
-		DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.sink_psr2_support
-			      ? "supported" : "not supported");
+		DRM_DEBUG_KMS("PSR2 %ssupported\n",
+			      dev_priv->psr.sink_psr2_support ? "" : "not ");
 
 		if (dev_priv->psr.sink_psr2_support) {
 			dev_priv->psr.colorimetry_support =