Re: [Intel-gfx] [PATCH v2 6/9] drm/i915/display/hdmi: use intel_de_rmw if possible

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jan 05, 2023 at 02:10:43PM +0100, Andrzej Hajda wrote:
> The helper makes the code more compact and readable.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>  drivers/gpu/drm/i915/display/g4x_hdmi.c   |  8 ++---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 15 ++++-----
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 40 +++++++----------------
>  3 files changed, 22 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/g4x_hdmi.c b/drivers/gpu/drm/i915/display/g4x_hdmi.c
> index c3580d96765c6c..f58849b416ea89 100644
> --- a/drivers/gpu/drm/i915/display/g4x_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/g4x_hdmi.c
> @@ -271,8 +271,8 @@ static void cpt_enable_hdmi(struct intel_atomic_state *state,
>  	 */
>  
>  	if (pipe_config->pipe_bpp > 24) {
> -		intel_de_write(dev_priv, TRANS_CHICKEN1(pipe),
> -			       intel_de_read(dev_priv, TRANS_CHICKEN1(pipe)) | TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +		intel_de_rmw(dev_priv, TRANS_CHICKEN1(pipe),
> +			     0, TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
>  
>  		temp &= ~SDVO_COLOR_FORMAT_MASK;
>  		temp |= SDVO_COLOR_FORMAT_8bpc;
> @@ -288,8 +288,8 @@ static void cpt_enable_hdmi(struct intel_atomic_state *state,
>  		intel_de_write(dev_priv, intel_hdmi->hdmi_reg, temp);
>  		intel_de_posting_read(dev_priv, intel_hdmi->hdmi_reg);
>  
> -		intel_de_write(dev_priv, TRANS_CHICKEN1(pipe),
> -			       intel_de_read(dev_priv, TRANS_CHICKEN1(pipe)) & ~TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE);
> +		intel_de_rmw(dev_priv, TRANS_CHICKEN1(pipe),
> +			     TRANS_CHICKEN1_HDMIUNIT_GC_DISABLE, 0);
>  	}
>  
>  	drm_WARN_ON(&dev_priv->drm, pipe_config->has_audio &&
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index 6406fd487ee524..2984d2810e42cc 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -943,8 +943,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  
>  	repeater_ctl = intel_hdcp_get_repeater_ctl(dev_priv, cpu_transcoder,
>  						   port);
> -	intel_de_write(dev_priv, HDCP_REP_CTL,
> -		       intel_de_read(dev_priv, HDCP_REP_CTL) & ~repeater_ctl);
> +	intel_de_rmw(dev_priv, HDCP_REP_CTL, repeater_ctl, 0);
>  
>  	ret = hdcp->shim->toggle_signalling(dig_port, cpu_transcoder, false);
>  	if (ret) {
> @@ -1819,12 +1818,10 @@ static int hdcp2_enable_encryption(struct intel_connector *connector)
>  	}
>  
>  	if (intel_de_read(dev_priv, HDCP2_STATUS(dev_priv, cpu_transcoder, port)) &
> -	    LINK_AUTH_STATUS) {
> +	    LINK_AUTH_STATUS)
>  		/* Link is Authenticated. Now set for Encryption */
> -		intel_de_write(dev_priv,
> -			       HDCP2_CTL(dev_priv, cpu_transcoder, port),
> -			       intel_de_read(dev_priv, HDCP2_CTL(dev_priv, cpu_transcoder, port)) | CTL_LINK_ENCRYPTION_REQ);
> -	}
> +		intel_de_rmw(dev_priv, HDCP2_CTL(dev_priv, cpu_transcoder, port),
> +			     0, CTL_LINK_ENCRYPTION_REQ);
>  
>  	ret = intel_de_wait_for_set(dev_priv,
>  				    HDCP2_STATUS(dev_priv, cpu_transcoder,
> @@ -1848,8 +1845,8 @@ static int hdcp2_disable_encryption(struct intel_connector *connector)
>  	drm_WARN_ON(&dev_priv->drm, !(intel_de_read(dev_priv, HDCP2_STATUS(dev_priv, cpu_transcoder, port)) &
>  				      LINK_ENCRYPTION_STATUS));
>  
> -	intel_de_write(dev_priv, HDCP2_CTL(dev_priv, cpu_transcoder, port),
> -		       intel_de_read(dev_priv, HDCP2_CTL(dev_priv, cpu_transcoder, port)) & ~CTL_LINK_ENCRYPTION_REQ);
> +	intel_de_rmw(dev_priv, HDCP2_CTL(dev_priv, cpu_transcoder, port),
> +		     CTL_LINK_ENCRYPTION_REQ, 0);
>  
>  	ret = intel_de_wait_for_clear(dev_priv,
>  				      HDCP2_STATUS(dev_priv, cpu_transcoder,
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index efa2da080f62d4..4b09f17aa4b23b 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -237,15 +237,11 @@ static void g4x_read_infoframe(struct intel_encoder *encoder,
>  			       void *frame, ssize_t len)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	u32 val, *data = frame;
> +	u32 *data = frame;
>  	int i;
>  
> -	val = intel_de_read(dev_priv, VIDEO_DIP_CTL);
> -
> -	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */

A probably good follow-up clean up would be to define the missing masks
and remove the hardcoded things like this 0xf.

And also something that I had noticed on the previous patches but I forgot
to mention: it would be good as a followup to replace the local value << shift
per FIELD_PREP() helpers and remove the shift definitions...

But really nothing related directly with this patch. For this:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Oh, and I also noticed that CI didn't return yet for these patches...
https://patchwork.freedesktop.org/series/112438/

a strange delay... I will probably hit the retest if we don't get
anything by the end of the day today.

> -	val |= g4x_infoframe_index(type);
> -
> -	intel_de_write(dev_priv, VIDEO_DIP_CTL, val);
> +	intel_de_rmw(dev_priv, VIDEO_DIP_CTL,
> +		     VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));
>  
>  	for (i = 0; i < len; i += 4)
>  		*data++ = intel_de_read(dev_priv, VIDEO_DIP_DATA);
> @@ -313,15 +309,11 @@ static void ibx_read_infoframe(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	u32 val, *data = frame;
> +	u32 *data = frame;
>  	int i;
>  
> -	val = intel_de_read(dev_priv, TVIDEO_DIP_CTL(crtc->pipe));
> -
> -	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> -	val |= g4x_infoframe_index(type);
> -
> -	intel_de_write(dev_priv, TVIDEO_DIP_CTL(crtc->pipe), val);
> +	intel_de_rmw(dev_priv, TVIDEO_DIP_CTL(crtc->pipe),
> +		     VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));
>  
>  	for (i = 0; i < len; i += 4)
>  		*data++ = intel_de_read(dev_priv, TVIDEO_DIP_DATA(crtc->pipe));
> @@ -395,15 +387,11 @@ static void cpt_read_infoframe(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	u32 val, *data = frame;
> +	u32 *data = frame;
>  	int i;
>  
> -	val = intel_de_read(dev_priv, TVIDEO_DIP_CTL(crtc->pipe));
> -
> -	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> -	val |= g4x_infoframe_index(type);
> -
> -	intel_de_write(dev_priv, TVIDEO_DIP_CTL(crtc->pipe), val);
> +	intel_de_rmw(dev_priv, TVIDEO_DIP_CTL(crtc->pipe),
> +		     VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));
>  
>  	for (i = 0; i < len; i += 4)
>  		*data++ = intel_de_read(dev_priv, TVIDEO_DIP_DATA(crtc->pipe));
> @@ -471,15 +459,11 @@ static void vlv_read_infoframe(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	u32 val, *data = frame;
> +	u32 *data = frame;
>  	int i;
>  
> -	val = intel_de_read(dev_priv, VLV_TVIDEO_DIP_CTL(crtc->pipe));
> -
> -	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> -	val |= g4x_infoframe_index(type);
> -
> -	intel_de_write(dev_priv, VLV_TVIDEO_DIP_CTL(crtc->pipe), val);
> +	intel_de_rmw(dev_priv, VLV_TVIDEO_DIP_CTL(crtc->pipe),
> +		     VIDEO_DIP_SELECT_MASK | 0xf, g4x_infoframe_index(type));
>  
>  	for (i = 0; i < len; i += 4)
>  		*data++ = intel_de_read(dev_priv,
> -- 
> 2.34.1
>