Re: [Intel-gfx] [PATCH 09/13] drm/i915/pps: rename intel_dp_check_edp to intel_pps_check_power_unlocked

2021-01-08 Thread Gupta, Anshuman 



> -Original Message-
> From: Jani Nikula 
> Sent: Friday, January 8, 2021 4:04 PM
> To: Gupta, Anshuman 
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 09/13] drm/i915/pps: rename
> intel_dp_check_edp to intel_pps_check_power_unlocked
> 
> On Tue, 29 Dec 2020, Anshuman Gupta 
> wrote:
> > On 2020-12-22 at 20:19:49 +0530, Jani Nikula wrote:
> >> Follow the usual naming pattern for functions.
> >>
> >> Signed-off-by: Jani Nikula 
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_dp.c  | 2 +-
> >> drivers/gpu/drm/i915/display/intel_pps.c | 2 +-
> >> drivers/gpu/drm/i915/display/intel_pps.h | 2 +-
> >>  3 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> >> b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index 334ba1775cd3..65406d4ccdbe 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -1046,7 +1046,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >> */
> >>cpu_latency_qos_update_request(&i915->pm_qos, 0);
> >>
> >> -  intel_dp_check_edp(intel_dp);
> >> +  intel_pps_check_power_unlocked(intel_dp);
> >>
> >>/* Try to wait for any previous AUX channel activity */
> >>for (try = 0; try < 3; try++) {
> >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> >> b/drivers/gpu/drm/i915/display/intel_pps.c
> >> index 3e62d1450682..dfd6722bc40e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> >> @@ -431,7 +431,7 @@ static bool edp_have_panel_vdd(struct intel_dp
> *intel_dp)
> >>return intel_de_read(dev_priv, _pp_ctrl_reg(intel_dp)) &
> >> EDP_FORCE_VDD;  }
> >>
> >> -void intel_dp_check_edp(struct intel_dp *intel_dp)
> >> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp)
> > IMHO comment to take pps_lock would be useful here.
> 
> Part of the point of this change is to name it _unlocked to highlight it does
> not take the lock, i.e. you should be aware of locking. You see this pattern
> all over the kernel. It's self-documenting code.
> 
> Moreover, after the edp check, the calls here have:
> 
>   lockdep_assert_held(&dev_priv->pps_mutex);
> 
> which both documents the requirement as well as ensures the proper usage
> in lockdep builds. I don't think a comment adds any value.
Agreeing with you on this.
Patch looks good to me.
Reviewed-by: Anshuman Gupta  
> 
> BR,
> Jani.
> 
> 
> > Thanks,
> > Anshuman.
> >>  {
> >>struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h
> >> b/drivers/gpu/drm/i915/display/intel_pps.h
> >> index 4780b59a59df..8dda282abd42 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_pps.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
> >> @@ -22,7 +22,6 @@ intel_wakeref_t intel_pps_unlock(struct intel_dp
> *intel_dp, intel_wakeref_t wake
> >>  #define with_intel_pps_lock(dp, wf)
>   \
> >>for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp),
> >> (wf)))
> >>
> >> -void intel_dp_check_edp(struct intel_dp *intel_dp);  void
> >> intel_pps_backlight_on(struct intel_dp *intel_dp);  void
> >> intel_pps_backlight_off(struct intel_dp *intel_dp);  void
> >> intel_pps_backlight_power(struct intel_connector *connector, bool
> >> enable); @@ -31,6 +30,7 @@ bool intel_pps_vdd_on_unlocked(struct
> >> intel_dp *intel_dp);  void intel_pps_vdd_off_unlocked(struct intel_dp
> >> *intel_dp, bool sync);  void intel_pps_on_unlocked(struct intel_dp
> >> *intel_dp);  void intel_pps_off_unlocked(struct intel_dp *intel_dp);
> >> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp);
> >>
> >>  void intel_pps_vdd_on(struct intel_dp *intel_dp);  void
> >> intel_pps_on(struct intel_dp *intel_dp);
> >> --
> >> 2.20.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
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/13] drm/i915/pps: rename intel_dp_check_edp to intel_pps_check_power_unlocked

2021-01-08 Thread Jani Nikula 
On Tue, 29 Dec 2020, Anshuman Gupta  wrote:
> On 2020-12-22 at 20:19:49 +0530, Jani Nikula wrote:
>> Follow the usual naming pattern for functions.
>> 
>> Signed-off-by: Jani Nikula 
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c  | 2 +-
>>  drivers/gpu/drm/i915/display/intel_pps.c | 2 +-
>>  drivers/gpu/drm/i915/display/intel_pps.h | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 334ba1775cd3..65406d4ccdbe 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -1046,7 +1046,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>>   */
>>  cpu_latency_qos_update_request(&i915->pm_qos, 0);
>>  
>> -intel_dp_check_edp(intel_dp);
>> +intel_pps_check_power_unlocked(intel_dp);
>>  
>>  /* Try to wait for any previous AUX channel activity */
>>  for (try = 0; try < 3; try++) {
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c 
>> b/drivers/gpu/drm/i915/display/intel_pps.c
>> index 3e62d1450682..dfd6722bc40e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> @@ -431,7 +431,7 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>  return intel_de_read(dev_priv, _pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
>>  }
>>  
>> -void intel_dp_check_edp(struct intel_dp *intel_dp)
>> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp)
> IMHO comment to take pps_lock would be useful here.

Part of the point of this change is to name it _unlocked to highlight it
does not take the lock, i.e. you should be aware of locking. You see
this pattern all over the kernel. It's self-documenting code.

Moreover, after the edp check, the calls here have:

lockdep_assert_held(&dev_priv->pps_mutex);

which both documents the requirement as well as ensures the proper usage
in lockdep builds. I don't think a comment adds any value.

BR,
Jani.


> Thanks,
> Anshuman.
>>  {
>>  struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h 
>> b/drivers/gpu/drm/i915/display/intel_pps.h
>> index 4780b59a59df..8dda282abd42 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.h
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
>> @@ -22,7 +22,6 @@ intel_wakeref_t intel_pps_unlock(struct intel_dp 
>> *intel_dp, intel_wakeref_t wake
>>  #define with_intel_pps_lock(dp, wf) 
>> \
>>  for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), 
>> (wf)))
>>  
>> -void intel_dp_check_edp(struct intel_dp *intel_dp);
>>  void intel_pps_backlight_on(struct intel_dp *intel_dp);
>>  void intel_pps_backlight_off(struct intel_dp *intel_dp);
>>  void intel_pps_backlight_power(struct intel_connector *connector, bool 
>> enable);
>> @@ -31,6 +30,7 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp);
>>  void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync);
>>  void intel_pps_on_unlocked(struct intel_dp *intel_dp);
>>  void intel_pps_off_unlocked(struct intel_dp *intel_dp);
>> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp);
>>  
>>  void intel_pps_vdd_on(struct intel_dp *intel_dp);
>>  void intel_pps_on(struct intel_dp *intel_dp);
>> -- 
>> 2.20.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

-- 
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/13] drm/i915/pps: rename intel_dp_check_edp to intel_pps_check_power_unlocked

2020-12-28 Thread Anshuman Gupta 
On 2020-12-22 at 20:19:49 +0530, Jani Nikula wrote:
> Follow the usual naming pattern for functions.
> 
> Signed-off-by: Jani Nikula 
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c  | 2 +-
>  drivers/gpu/drm/i915/display/intel_pps.c | 2 +-
>  drivers/gpu/drm/i915/display/intel_pps.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 334ba1775cd3..65406d4ccdbe 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1046,7 +1046,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>*/
>   cpu_latency_qos_update_request(&i915->pm_qos, 0);
>  
> - intel_dp_check_edp(intel_dp);
> + intel_pps_check_power_unlocked(intel_dp);
>  
>   /* Try to wait for any previous AUX channel activity */
>   for (try = 0; try < 3; try++) {
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c 
> b/drivers/gpu/drm/i915/display/intel_pps.c
> index 3e62d1450682..dfd6722bc40e 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -431,7 +431,7 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>   return intel_de_read(dev_priv, _pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
>  }
>  
> -void intel_dp_check_edp(struct intel_dp *intel_dp)
> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp)
IMHO comment to take pps_lock would be useful here.
Thanks,
Anshuman.
>  {
>   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h 
> b/drivers/gpu/drm/i915/display/intel_pps.h
> index 4780b59a59df..8dda282abd42 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.h
> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
> @@ -22,7 +22,6 @@ intel_wakeref_t intel_pps_unlock(struct intel_dp *intel_dp, 
> intel_wakeref_t wake
>  #define with_intel_pps_lock(dp, wf)  
> \
>   for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), 
> (wf)))
>  
> -void intel_dp_check_edp(struct intel_dp *intel_dp);
>  void intel_pps_backlight_on(struct intel_dp *intel_dp);
>  void intel_pps_backlight_off(struct intel_dp *intel_dp);
>  void intel_pps_backlight_power(struct intel_connector *connector, bool 
> enable);
> @@ -31,6 +30,7 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp);
>  void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync);
>  void intel_pps_on_unlocked(struct intel_dp *intel_dp);
>  void intel_pps_off_unlocked(struct intel_dp *intel_dp);
> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp);
>  
>  void intel_pps_vdd_on(struct intel_dp *intel_dp);
>  void intel_pps_on(struct intel_dp *intel_dp);
> -- 
> 2.20.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

[Intel-gfx] [PATCH 09/13] drm/i915/pps: rename intel_dp_check_edp to intel_pps_check_power_unlocked

2020-12-22 Thread Jani Nikula 
Follow the usual naming pattern for functions.

Signed-off-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_dp.c  | 2 +-
 drivers/gpu/drm/i915/display/intel_pps.c | 2 +-
 drivers/gpu/drm/i915/display/intel_pps.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index 334ba1775cd3..65406d4ccdbe 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1046,7 +1046,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 */
cpu_latency_qos_update_request(&i915->pm_qos, 0);
 
-   intel_dp_check_edp(intel_dp);
+   intel_pps_check_power_unlocked(intel_dp);
 
/* Try to wait for any previous AUX channel activity */
for (try = 0; try < 3; try++) {
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c 
b/drivers/gpu/drm/i915/display/intel_pps.c
index 3e62d1450682..dfd6722bc40e 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -431,7 +431,7 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
return intel_de_read(dev_priv, _pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
 }
 
-void intel_dp_check_edp(struct intel_dp *intel_dp)
+void intel_pps_check_power_unlocked(struct intel_dp *intel_dp)
 {
struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/display/intel_pps.h 
b/drivers/gpu/drm/i915/display/intel_pps.h
index 4780b59a59df..8dda282abd42 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.h
+++ b/drivers/gpu/drm/i915/display/intel_pps.h
@@ -22,7 +22,6 @@ intel_wakeref_t intel_pps_unlock(struct intel_dp *intel_dp, 
intel_wakeref_t wake
 #define with_intel_pps_lock(dp, wf)
\
for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), 
(wf)))
 
-void intel_dp_check_edp(struct intel_dp *intel_dp);
 void intel_pps_backlight_on(struct intel_dp *intel_dp);
 void intel_pps_backlight_off(struct intel_dp *intel_dp);
 void intel_pps_backlight_power(struct intel_connector *connector, bool enable);
@@ -31,6 +30,7 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp);
 void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync);
 void intel_pps_on_unlocked(struct intel_dp *intel_dp);
 void intel_pps_off_unlocked(struct intel_dp *intel_dp);
+void intel_pps_check_power_unlocked(struct intel_dp *intel_dp);
 
 void intel_pps_vdd_on(struct intel_dp *intel_dp);
 void intel_pps_on(struct intel_dp *intel_dp);
-- 
2.20.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx