Re: [PATCH 45/89] drm/i915/skl: Definition of SKL WM param structs for pipe/plane

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

On Thu, Sep 04, 2014 at 12:27:11PM +0100, Damien Lespiau wrote:
> From: Pradeep Bhat <pradeep.bhat@intel.com>
> 
> This patch defines the structures needed for computation of
> watermarks of pipes and planes for SKL.
> 
> v2: Incorporated Damien's review comments and removed unused fields
>     in structs for future features like rotation, drrs and scaling.
>     The skl_wm_values struct is now made more generic across planes
>     and cursor planes for all pipes.
> 
> v3: implemented the plane/cursor split.
> 
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_pm.c  |  8 ++++++++
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 32be299..3764ad5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1381,6 +1381,24 @@ struct ilk_wm_values {
>  	enum intel_ddb_partitioning partitioning;
>  };
>  
> +struct skl_wm_values {
> +	bool dirty[I915_MAX_PIPES];
> +	uint32_t wm_linetime[I915_MAX_PIPES];
> +	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> +	uint32_t cursor[I915_MAX_PIPES][8];
> +	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
> +	uint32_t cursor_trans[I915_MAX_PIPES];
> +};

These multi dimensional arrays hurt my eyes. Maybe we should restructure
this a bit to eg:

struct skl_wm_values {
	struct {
		wm_linetime;
		plane[MAX_PLANES][8];
		...
	} pipe[MAX_PIPES];
};

The two dimensional plane[][] array is still a bit nasty, but maybe we
can live with it.

We could also do the same operatiob for the ilk version to keep stuff
similar.

> +
> +struct skl_wm_level {
> +	bool plane_en[I915_MAX_PLANES];
> +	uint16_t plane_res_b[I915_MAX_PLANES];
> +	uint8_t plane_res_l[I915_MAX_PLANES];

This stuff could also look better as an array of struct of some sort.
Also should probably put the bool and uint8_t next to each other in case
gcc is smart enough to pack things more tightly.

> +	bool cursor_en;
> +	uint16_t cursor_res_b;
> +	uint8_t cursor_res_l;

And this could also be an instance of the same struct use for the proper
planes.

I'm actually wondering if we want this cursor stuff around at all.
I was under the impression using the cursor eats one of the planes,
so we could just as well use the plane always. But if the current
code actually implements stuff using the legacy cursor thing I guess we
need it for now at least.

> +};
> +
>  /*
>   * This struct helps tracking the state needed for runtime PM, which puts the
>   * device in PCI D3 state. Notice that when this happens, nothing on the
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3239e58..268087f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -387,6 +387,12 @@ struct intel_mmio_flip {
>  	u32 ring_id;
>  };
>  
> +struct skl_pipe_wm {
> +	struct skl_wm_level wm[8];
> +	struct skl_wm_level trans_wm;
> +	uint32_t linetime;
> +};
> +
>  struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
> @@ -431,9 +437,11 @@ struct intel_crtc {
>  	bool pch_fifo_underrun_disabled;
>  
>  	/* per-pipe watermark state */
> -	struct {
> +	union {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
> +		/* SKL wm values currently in use */
> +		struct skl_pipe_wm skl_active;
>  	} wm;
>  
>  	int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d8c8531..2503ab9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1928,6 +1928,14 @@ static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t horiz_pixels,
>  	return DIV_ROUND_UP(pri_val * 64, horiz_pixels * bytes_per_pixel) + 2;
>  }
>  
> +struct skl_pipe_wm_parameters {
> +	bool active;
> +	uint32_t pipe_htotal;
> +	uint32_t pixel_rate; /* in KHz */
> +	struct intel_plane_wm_parameters plane[I915_MAX_PLANES];
> +	struct intel_plane_wm_parameters cursor;
> +};

I suppose we just need to start using some kind of named indexes for the
planes on all platforms so we can unify all this stuff. But that can be
done when we have all the code merged so we can better see how to unify
things.

> +
>  struct ilk_pipe_wm_parameters {
>  	bool active;
>  	uint32_t pipe_htotal;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC