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

From: "Gupta, Anshuman" <anshuman.gupta@intel.com>
To: "Nikula, Jani" <jani.nikula@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <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
Date: Fri, 8 Jan 2021 10:59:40 +0000	[thread overview]
Message-ID: <ad00a6fc05104df0b2441e9644926ed6@intel.com> (raw)
In-Reply-To: <87a6tjzpiz.fsf@intel.com>


> -----Original Message-----
> From: Jani Nikula <jani.nikula@intel.com>
> Sent: Friday, January 8, 2021 4:04 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> 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 <anshuman.gupta@intel.com>
> 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 <jani.nikula@intel.com>
> >> ---
> >>  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 <anshuman.gupta@intel.com> 
> 
> 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