Re: [RFC 1/2] drm/i915: add display uncore helpers

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 10/29/19 3:51 AM, Jani Nikula wrote:
> Add convenience helpers for the most common uncore operations with
> struct drm_i915_private * as context rather than struct intel_uncore *.
> 
> The goal is to replace all instances of I915_READ(),
> I915_POSTING_READ(), and I915_WRITE() in display/ with these, to finally
> be able to get rid of the implicit dev_priv local parameter use.
> 
> The idea is that any non-u32 reads or writes are special enough that
> they can use the intel_uncore_* functions directly.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Let the name bikeshedding commence! Some options are:
> 
> - intel_de_read, intel_de_write (this patch, de for display engine)

+1 for this. it also matches the intel_de_wait_for_register/set/clear 
macros, which we could probably move in this file as well.

Daniele

> - intel_display_read, intel_display_write (too long I think)
> - intel_read, intel_write
> - i915_read, i915_write
> - display_read, display_write
> - ?
> 
> Here's a draft cocci patch that could be used on display/ subdir, en
> masse:
> 
> @@
> expression REG, OFFSET;
> @@
> - I915_READ(REG)
> + intel_de_read(dev_priv, REG)
> 
> @@
> expression REG, OFFSET;
> @@
> - I915_POSTING_READ(REG)
> + intel_de_posting_read(dev_priv, REG)
> 
> @@
> expression REG, OFFSET;
> @@
> - I915_WRITE(REG, OFFSET)
> + intel_de_write(dev_priv, REG, OFFSET)
> 
> Conveniently, we *know* dev_priv is everywhere the change is needed.
> 
> This enables us to make the s/dev_priv/i915/ rename on top. We also need
> to discuss whether it's better to separate the steps to two (even if in
> the same patch series) or if it's better to add the dev_priv parameter
> as i915 directly.
> ---
>   .../drm/i915/display/intel_display_uncore.h   | 36 +++++++++++++++++++
>   1 file changed, 36 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/display/intel_display_uncore.h
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_uncore.h b/drivers/gpu/drm/i915/display/intel_display_uncore.h
> new file mode 100644
> index 000000000000..e6c8c56fb0f9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_display_uncore.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __INTEL_DISPLAY_UNCORE_H__
> +#define __INTEL_DISPLAY_UNCORE_H__
> +
> +#include "i915_drv.h"
> +#include "i915_reg.h"
> +#include "intel_uncore.h"
> +
> +static inline u32 intel_de_read(struct drm_i915_private *i915, i915_reg_t reg)
> +{
> +	return intel_uncore_read(&i915->uncore, reg);
> +}
> +
> +static inline void intel_de_posting_read(struct drm_i915_private *i915,
> +					 i915_reg_t reg)
> +{
> +	intel_uncore_posting_read(&i915->uncore, reg);
> +}
> +
> +static inline void intel_de_write(struct drm_i915_private *i915, i915_reg_t reg,
> +				  u32 val)
> +{
> +	intel_uncore_write(&i915->uncore, reg, val);
> +}
> +
> +static inline void intel_de_rmw(struct drm_i915_private *i915, i915_reg_t reg,
> +				u32 clear, u32 set)
> +{
> +	intel_uncore_rmw(&i915->uncore, reg, clear, set);
> +}
> +
> +#endif /* __INTEL_DISPLAY_UNCORE_H__ */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx