diff mbox

[4/4] drm/i915/dp: read DPCD PSR capability only on eDP

Message ID 2841535a1c9a9822f989e8e1674225f5a2fecf34.1379684248.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jani Nikula Sept. 20, 2013, 1:42 p.m. UTC 
Reduce AUX transactions for non-eDP.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Todd Previte Sept. 20, 2013, 8:50 p.m. UTC | #1 
On 09/20/2013 06:42 AM, Jani Nikula wrote:
> Reduce AUX transactions for non-eDP.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c |   13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dd780bf..f2e16a1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2691,11 +2691,14 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   
>   	/* Check if the panel supports PSR */
>   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> -	intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> -				       intel_dp->psr_dpcd,
> -				       sizeof(intel_dp->psr_dpcd));
> -	if (is_edp_psr(intel_dp))
> -		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> +	if (is_edp(intel_dp)) {
> +		intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> +					       intel_dp->psr_dpcd,
> +					       sizeof(intel_dp->psr_dpcd));
> +		if (is_edp_psr(intel_dp))
> +			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> +	}
> +
>   	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>   	      DP_DWN_STRM_PORT_PRESENT))
>   		return true; /* native DP sink */

Couldn't you just use is_edp_psr() at the top and not need the second 
is_edp() inside the if() statement? As in:

+	if (is_edp_psr(intel_dp)) {
+		intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
+					       intel_dp->psr_dpcd,
+					       sizeof(intel_dp->psr_dpcd));
+		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
+	}


-T
Paulo Zanoni Sept. 20, 2013, 8:57 p.m. UTC | #2 
2013/9/20 Todd Previte <tprevite@gmail.com>:
> On 09/20/2013 06:42 AM, Jani Nikula wrote:
>>
>> Reduce AUX transactions for non-eDP.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index dd780bf..f2e16a1 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2691,11 +2691,14 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>         /* Check if the panel supports PSR */
>>         memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>> -       intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> -                                      intel_dp->psr_dpcd,
>> -                                      sizeof(intel_dp->psr_dpcd));
>> -       if (is_edp_psr(intel_dp))
>> -               DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> +       if (is_edp(intel_dp)) {
>> +               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> +                                              intel_dp->psr_dpcd,
>> +
>> sizeof(intel_dp->psr_dpcd));
>> +               if (is_edp_psr(intel_dp))
>> +                       DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> +       }
>> +
>>         if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>>               DP_DWN_STRM_PORT_PRESENT))
>>                 return true; /* native DP sink */
>
>
> Couldn't you just use is_edp_psr() at the top and not need the second
> is_edp() inside the if() statement? As in:

No, because is_edp_psr() only makes sense after we do the DPCD read
that checks the PSR bit.


>
> +       if (is_edp_psr(intel_dp)) {
>
> +               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> +                                              intel_dp->psr_dpcd,
> +                                              sizeof(intel_dp->psr_dpcd));
> +               DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> +       }
>
>
> -T
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Todd Previte Sept. 20, 2013, 10:31 p.m. UTC | #3 
On 09/20/2013 01:57 PM, Paulo Zanoni wrote:
> 2013/9/20 Todd Previte <tprevite@gmail.com>:
>> On 09/20/2013 06:42 AM, Jani Nikula wrote:
>>> Reduce AUX transactions for non-eDP.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dp.c |   13 ++++++++-----
>>>    1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index dd780bf..f2e16a1 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2691,11 +2691,14 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>          /* Check if the panel supports PSR */
>>>          memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>> -       intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>>> -                                      intel_dp->psr_dpcd,
>>> -                                      sizeof(intel_dp->psr_dpcd));
>>> -       if (is_edp_psr(intel_dp))
>>> -               DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>>> +       if (is_edp(intel_dp)) {
>>> +               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>>> +                                              intel_dp->psr_dpcd,
>>> +
>>> sizeof(intel_dp->psr_dpcd));
>>> +               if (is_edp_psr(intel_dp))
>>> +                       DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>>> +       }
>>> +
>>>          if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>>>                DP_DWN_STRM_PORT_PRESENT))
>>>                  return true; /* native DP sink */
>>
>> Couldn't you just use is_edp_psr() at the top and not need the second
>> is_edp() inside the if() statement? As in:
> No, because is_edp_psr() only makes sense after we do the DPCD read
> that checks the PSR bit.

Yep, I see that now. And thus...

[Reviewed-by]: Todd Previte <tprevite@gmail.com>

>
>> +       if (is_edp_psr(intel_dp)) {
>>
>> +               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> +                                              intel_dp->psr_dpcd,
>> +                                              sizeof(intel_dp->psr_dpcd));
>> +               DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> +       }
>>
>>
>> -T
>>
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
Daniel Vetter Sept. 23, 2013, 7:29 a.m. UTC | #4 
On Fri, Sep 20, 2013 at 03:31:42PM -0700, Todd Previte wrote:
> On 09/20/2013 01:57 PM, Paulo Zanoni wrote:
> >2013/9/20 Todd Previte <tprevite@gmail.com>:
> >>On 09/20/2013 06:42 AM, Jani Nikula wrote:
> >>>Reduce AUX transactions for non-eDP.
> >>>
> >>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>>---
> >>>   drivers/gpu/drm/i915/intel_dp.c |   13 ++++++++-----
> >>>   1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >>>b/drivers/gpu/drm/i915/intel_dp.c
> >>>index dd780bf..f2e16a1 100644
> >>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>@@ -2691,11 +2691,14 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >>>         /* Check if the panel supports PSR */
> >>>         memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> >>>-       intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> >>>-                                      intel_dp->psr_dpcd,
> >>>-                                      sizeof(intel_dp->psr_dpcd));
> >>>-       if (is_edp_psr(intel_dp))
> >>>-               DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> >>>+       if (is_edp(intel_dp)) {
> >>>+               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> >>>+                                              intel_dp->psr_dpcd,
> >>>+
> >>>sizeof(intel_dp->psr_dpcd));
> >>>+               if (is_edp_psr(intel_dp))
> >>>+                       DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> >>>+       }
> >>>+
> >>>         if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> >>>               DP_DWN_STRM_PORT_PRESENT))
> >>>                 return true; /* native DP sink */
> >>
> >>Couldn't you just use is_edp_psr() at the top and not need the second
> >>is_edp() inside the if() statement? As in:
> >No, because is_edp_psr() only makes sense after we do the DPCD read
> >that checks the PSR bit.
> 
> Yep, I see that now. And thus...
> 
> [Reviewed-by]: Todd Previte <tprevite@gmail.com>

Queued for -next, thanks for the patch.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dd780bf..f2e16a1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2691,11 +2691,14 @@  intel_dp_get_dpcd(struct intel_dp *intel_dp)
 
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
-	intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
-				       intel_dp->psr_dpcd,
-				       sizeof(intel_dp->psr_dpcd));
-	if (is_edp_psr(intel_dp))
-		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
+	if (is_edp(intel_dp)) {
+		intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
+					       intel_dp->psr_dpcd,
+					       sizeof(intel_dp->psr_dpcd));
+		if (is_edp_psr(intel_dp))
+			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
+	}
+
 	if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
 	      DP_DWN_STRM_PORT_PRESENT))
 		return true; /* native DP sink */