Re: [Intel-gfx] [PATCH v3] drm/i915/display/misc: use intel_de_rmw if possible

2023-01-10 Thread Rodrigo Vivi 
On Tue, Jan 10, 2023 at 12:36:56PM +0100, Andrzej Hajda wrote:
> The helper makes the code more compact and readable.

next time, please add the revision comments here so we know what
changed from one version to the next.

> 
> Signed-off-by: Andrzej Hajda 
> ---
>  drivers/gpu/drm/i915/display/g4x_dp.c | 12 
>  drivers/gpu/drm/i915/display/intel_drrs.c | 12 +++-
>  drivers/gpu/drm/i915/display/intel_dvo.c  |  7 ++-
>  drivers/gpu/drm/i915/display/intel_lvds.c | 12 
>  drivers/gpu/drm/i915/display/intel_tv.c   |  6 ++
>  5 files changed, 15 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c 
> b/drivers/gpu/drm/i915/display/g4x_dp.c
> index 24ef36ec2d3d3c..9629b174ec5d2c 100644
> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> @@ -136,16 +136,12 @@ static void intel_dp_prepare(struct intel_encoder 
> *encoder,
>  
>   intel_dp->DP |= DP_PIPE_SEL_IVB(crtc->pipe);
>   } else if (HAS_PCH_CPT(dev_priv) && port != PORT_A) {
> - u32 trans_dp;
> -
>   intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
>  
> - trans_dp = intel_de_read(dev_priv, TRANS_DP_CTL(crtc->pipe));
> - if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> - trans_dp |= TRANS_DP_ENH_FRAMING;
> - else
> - trans_dp &= ~TRANS_DP_ENH_FRAMING;
> - intel_de_write(dev_priv, TRANS_DP_CTL(crtc->pipe), trans_dp);
> + intel_de_rmw(dev_priv, TRANS_DP_CTL(crtc->pipe),
> +  TRANS_DP_ENH_FRAMING,
> +  drm_dp_enhanced_frame_cap(intel_dp->dpcd) ?
> +  TRANS_DP_ENH_FRAMING : 0);

yet another case with risk of change in behavior, but here again I believe
this is the right thing.

Reviewed-by: Rodrigo Vivi 

>   } else {
>   if (IS_G4X(dev_priv) && pipe_config->limited_color_range)
>   intel_dp->DP |= DP_COLOR_RANGE_16_235;
> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c 
> b/drivers/gpu/drm/i915/display/intel_drrs.c
> index 5b9e3814e9..a52974f5f66042 100644
> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> @@ -68,21 +68,15 @@ intel_drrs_set_refresh_rate_pipeconf(struct intel_crtc 
> *crtc,
>  {
>   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   enum transcoder cpu_transcoder = crtc->drrs.cpu_transcoder;
> - u32 val, bit;
> + u32 bit;
>  
>   if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>   bit = PIPECONF_REFRESH_RATE_ALT_VLV;
>   else
>   bit = PIPECONF_REFRESH_RATE_ALT_ILK;
>  
> - val = intel_de_read(dev_priv, PIPECONF(cpu_transcoder));
> -
> - if (refresh_rate == DRRS_REFRESH_RATE_LOW)
> - val |= bit;
> - else
> - val &= ~bit;
> -
> - intel_de_write(dev_priv, PIPECONF(cpu_transcoder), val);
> + intel_de_rmw(dev_priv, PIPECONF(cpu_transcoder),
> +  bit, refresh_rate == DRRS_REFRESH_RATE_LOW ? bit : 0);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c 
> b/drivers/gpu/drm/i915/display/intel_dvo.c
> index 4aeae0f3ac9172..77d413781020de 100644
> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> @@ -444,11 +444,8 @@ static bool intel_dvo_init_dev(struct drm_i915_private 
> *dev_priv,
>* the clock enabled before we attempt to initialize
>* the device.
>*/
> - for_each_pipe(dev_priv, pipe) {
> - dpll[pipe] = intel_de_read(dev_priv, DPLL(pipe));
> - intel_de_write(dev_priv, DPLL(pipe),
> -dpll[pipe] | DPLL_DVO_2X_MODE);
> - }
> + for_each_pipe(dev_priv, pipe)
> + dpll[pipe] = intel_de_rmw(dev_priv, DPLL(pipe), 0, 
> DPLL_DVO_2X_MODE);
>  
>   ret = dvo->dev_ops->init(_dvo->dev, i2c);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c 
> b/drivers/gpu/drm/i915/display/intel_lvds.c
> index aecec992cd0d2d..e8f47b7ef87649 100644
> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
> @@ -316,11 +316,9 @@ static void intel_enable_lvds(struct intel_atomic_state 
> *state,
>   struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
>   struct drm_i915_private *dev_priv = to_i915(dev);
>  
> - intel_de_write(dev_priv, lvds_encoder->reg,
> -intel_de_read(dev_priv, lvds_encoder->reg) | 
> LVDS_PORT_EN);
> + intel_de_rmw(dev_priv, lvds_encoder->reg, 0, LVDS_PORT_EN);
>  
> - intel_de_write(dev_priv, PP_CONTROL(0),
> -intel_de_read(dev_priv, PP_CONTROL(0)) | PANEL_POWER_ON);
> + intel_de_rmw(dev_priv, PP_CONTROL(0), 0, PANEL_POWER_ON);
>   intel_de_posting_read(dev_priv, lvds_encoder->reg);
>  
>   if

[Intel-gfx] [PATCH v3] drm/i915/display/misc: use intel_de_rmw if possible

2023-01-10 Thread Andrzej Hajda 
The helper makes the code more compact and readable.

Signed-off-by: Andrzej Hajda 
---
 drivers/gpu/drm/i915/display/g4x_dp.c | 12 
 drivers/gpu/drm/i915/display/intel_drrs.c | 12 +++-
 drivers/gpu/drm/i915/display/intel_dvo.c  |  7 ++-
 drivers/gpu/drm/i915/display/intel_lvds.c | 12 
 drivers/gpu/drm/i915/display/intel_tv.c   |  6 ++
 5 files changed, 15 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c 
b/drivers/gpu/drm/i915/display/g4x_dp.c
index 24ef36ec2d3d3c..9629b174ec5d2c 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -136,16 +136,12 @@ static void intel_dp_prepare(struct intel_encoder 
*encoder,
 
intel_dp->DP |= DP_PIPE_SEL_IVB(crtc->pipe);
} else if (HAS_PCH_CPT(dev_priv) && port != PORT_A) {
-   u32 trans_dp;
-
intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
 
-   trans_dp = intel_de_read(dev_priv, TRANS_DP_CTL(crtc->pipe));
-   if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
-   trans_dp |= TRANS_DP_ENH_FRAMING;
-   else
-   trans_dp &= ~TRANS_DP_ENH_FRAMING;
-   intel_de_write(dev_priv, TRANS_DP_CTL(crtc->pipe), trans_dp);
+   intel_de_rmw(dev_priv, TRANS_DP_CTL(crtc->pipe),
+TRANS_DP_ENH_FRAMING,
+drm_dp_enhanced_frame_cap(intel_dp->dpcd) ?
+TRANS_DP_ENH_FRAMING : 0);
} else {
if (IS_G4X(dev_priv) && pipe_config->limited_color_range)
intel_dp->DP |= DP_COLOR_RANGE_16_235;
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c 
b/drivers/gpu/drm/i915/display/intel_drrs.c
index 5b9e3814e9..a52974f5f66042 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.c
+++ b/drivers/gpu/drm/i915/display/intel_drrs.c
@@ -68,21 +68,15 @@ intel_drrs_set_refresh_rate_pipeconf(struct intel_crtc 
*crtc,
 {
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum transcoder cpu_transcoder = crtc->drrs.cpu_transcoder;
-   u32 val, bit;
+   u32 bit;
 
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
bit = PIPECONF_REFRESH_RATE_ALT_VLV;
else
bit = PIPECONF_REFRESH_RATE_ALT_ILK;
 
-   val = intel_de_read(dev_priv, PIPECONF(cpu_transcoder));
-
-   if (refresh_rate == DRRS_REFRESH_RATE_LOW)
-   val |= bit;
-   else
-   val &= ~bit;
-
-   intel_de_write(dev_priv, PIPECONF(cpu_transcoder), val);
+   intel_de_rmw(dev_priv, PIPECONF(cpu_transcoder),
+bit, refresh_rate == DRRS_REFRESH_RATE_LOW ? bit : 0);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c 
b/drivers/gpu/drm/i915/display/intel_dvo.c
index 4aeae0f3ac9172..77d413781020de 100644
--- a/drivers/gpu/drm/i915/display/intel_dvo.c
+++ b/drivers/gpu/drm/i915/display/intel_dvo.c
@@ -444,11 +444,8 @@ static bool intel_dvo_init_dev(struct drm_i915_private 
*dev_priv,
 * the clock enabled before we attempt to initialize
 * the device.
 */
-   for_each_pipe(dev_priv, pipe) {
-   dpll[pipe] = intel_de_read(dev_priv, DPLL(pipe));
-   intel_de_write(dev_priv, DPLL(pipe),
-  dpll[pipe] | DPLL_DVO_2X_MODE);
-   }
+   for_each_pipe(dev_priv, pipe)
+   dpll[pipe] = intel_de_rmw(dev_priv, DPLL(pipe), 0, 
DPLL_DVO_2X_MODE);
 
ret = dvo->dev_ops->init(_dvo->dev, i2c);
 
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c 
b/drivers/gpu/drm/i915/display/intel_lvds.c
index aecec992cd0d2d..e8f47b7ef87649 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -316,11 +316,9 @@ static void intel_enable_lvds(struct intel_atomic_state 
*state,
struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
struct drm_i915_private *dev_priv = to_i915(dev);
 
-   intel_de_write(dev_priv, lvds_encoder->reg,
-  intel_de_read(dev_priv, lvds_encoder->reg) | 
LVDS_PORT_EN);
+   intel_de_rmw(dev_priv, lvds_encoder->reg, 0, LVDS_PORT_EN);
 
-   intel_de_write(dev_priv, PP_CONTROL(0),
-  intel_de_read(dev_priv, PP_CONTROL(0)) | PANEL_POWER_ON);
+   intel_de_rmw(dev_priv, PP_CONTROL(0), 0, PANEL_POWER_ON);
intel_de_posting_read(dev_priv, lvds_encoder->reg);
 
if (intel_de_wait_for_set(dev_priv, PP_STATUS(0), PP_ON, 5000))
@@ -338,14 +336,12 @@ static void intel_disable_lvds(struct intel_atomic_state 
*state,
struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
-   intel_de_write(dev_priv, PP_CONTROL(0),
-  intel_de_read(dev_priv, PP_CONTROL(0)) &