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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 16 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> The helper makes the code more compact and readable.
>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>

[snip]

> @@ -649,23 +611,18 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder,
>  	enum port port;
>  
>  	if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK) {
> -		u32 temp;
> +		u32 temp = intel_dsi->pixel_overlap;
>  		if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) {
> -			for_each_dsi_port(port, intel_dsi->ports) {
> -				temp = intel_de_read(dev_priv,
> -						     MIPI_CTRL(port));
> -				temp &= ~BXT_PIXEL_OVERLAP_CNT_MASK |
> -					intel_dsi->pixel_overlap <<
> -					BXT_PIXEL_OVERLAP_CNT_SHIFT;

This can't possibly be correct to begin with.

I think it's supposed to be the logical

	temp &= ~BXT_PIXEL_OVERLAP_CNT_MASK;
        temp |= intel_dsi->pixel_overlap << BXT_PIXEL_OVERLAP_CNT_SHIFT;

Nothing else makes sense, really.

I think I'd just fix that in a separate patch first, and then do the rmw
changes on top.

> -				intel_de_write(dev_priv, MIPI_CTRL(port),
> -					       temp);
> -			}
> +			for_each_dsi_port(port, intel_dsi->ports)
> +				intel_de_rmw(dev_priv, MIPI_CTRL(port),
> +					     BXT_PIXEL_OVERLAP_CNT_MASK &
> +					     ~(temp << BXT_PIXEL_OVERLAP_CNT_SHIFT),
> +					     0);
>  		} else {
> -			temp = intel_de_read(dev_priv, VLV_CHICKEN_3);
> -			temp &= ~PIXEL_OVERLAP_CNT_MASK |
> -					intel_dsi->pixel_overlap <<
> -					PIXEL_OVERLAP_CNT_SHIFT;
> -			intel_de_write(dev_priv, VLV_CHICKEN_3, temp);

Ditto here.

> +			intel_de_rmw(dev_priv, VLV_CHICKEN_3,
> +				     PIXEL_OVERLAP_CNT_MASK &
> +				     ~(temp << PIXEL_OVERLAP_CNT_SHIFT),
> +				     0);
>  		}
>  	}
>  

Everything else in the patch checks out. With the above split out and
the rmw rebased,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


-- 
Jani Nikula, Intel Open Source Graphics Center