Re: [PATCH 06/11] drm/i915: refactor skl_write_plane_wm()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 16, 2018 at 03:01:28PM -0700, Paulo Zanoni wrote:
> Its control flow is not as easy to follow as it could be. We recently
> even had a double register write that went unnoticed until commit
> 9e44b180f81b ("drm/i915: don't write PLANE_BUF_CFG twice every time")
> fixed it. The return statement in the middle along with the fact that
> it's on a void function really doesn't help readability IMHO.
> 
> Refactor the function so that the first level of checks is per
> platform and the second level is for planar planes. IMHO that makes
> the code much more readable.
> 
> Requested-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7fd344b81d66..f388bfa99a97 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5033,21 +5033,28 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
>  	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
>  			   &wm->trans_wm);
>  
> -	/* FIXME: add proper NV12 support for ICL. */
> -	if (INTEL_GEN(dev_priv) >= 11)
> -		return skl_ddb_entry_write(dev_priv,
> -					   PLANE_BUF_CFG(pipe, plane_id),
> -					   &ddb->plane[pipe][plane_id]);
> -	if (wm->is_planar) {
> -		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> -				    &ddb->uv_plane[pipe][plane_id]);
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		/* FIXME: add proper NV12 support for ICL. */
> +		if (wm->is_planar)
> +			DRM_ERROR("No DDB support for planar formats yet\n");
> +
>  		skl_ddb_entry_write(dev_priv,
> -				    PLANE_NV12_BUF_CFG(pipe, plane_id),
> -				    &ddb->plane[pipe][plane_id]);
> +				   PLANE_BUF_CFG(pipe, plane_id),
> +				   &ddb->plane[pipe][plane_id]);
>  	} else {
> -		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> -				    &ddb->plane[pipe][plane_id]);
> -		I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
> +		if (wm->is_planar) {
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_BUF_CFG(pipe, plane_id),
> +					    &ddb->uv_plane[pipe][plane_id]);
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_NV12_BUF_CFG(pipe, plane_id),
> +					    &ddb->plane[pipe][plane_id]);
> +		} else {
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_BUF_CFG(pipe, plane_id),
> +					    &ddb->plane[pipe][plane_id]);
> +			I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
> +		}
>  	}
>  }
>  
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx