Re: [PATCH 1/8] drm/i915/skl+: Prepare for removing data rate from skl watermark state

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Oct 12, 2016 at 03:28:14PM +0200, Maarten Lankhorst wrote:
> Caching is not required, drm_atomic_crtc_state_for_each_plane_state
> can be used to inspect all plane_states that are assigned to the
> current crtc_state, so we can just recalculate every time.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6af1587e9d84..b96a899c899d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -31,6 +31,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  /**
>   * DOC: RC6
> @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
>  	struct drm_crtc *crtc = cstate->crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	const struct drm_plane *plane;
> +	struct drm_plane *plane;
>  	const struct intel_plane *intel_plane;
> -	struct drm_plane_state *pstate;
> +	const struct drm_plane_state *pstate;
>  	unsigned int rate, total_data_rate = 0;
>  	int id;
> -	int i;
>  
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	/* Calculate and cache data rate for each plane */
> -	for_each_plane_in_state(state, plane, pstate, i) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
>  		id = skl_wm_plane_id(to_intel_plane(plane));
>  		intel_plane = to_intel_plane(plane);
>  
> @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane;
>  	struct drm_plane *plane;
> -	struct drm_plane_state *pstate;
> +	const struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
>  	uint16_t alloc_size, start, cursor_blocks;
> @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	alloc_size -= cursor_blocks;
>  
>  	/* 1. Allocate the mininum required blocks for each active plane */
> -	for_each_plane_in_state(state, plane, pstate, i) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
>  		intel_plane = to_intel_plane(plane);
>  		id = skl_wm_plane_id(intel_plane);
>  
>  		if (intel_plane->pipe != pipe)
>  			continue;
>  
> -		if (!to_intel_plane_state(pstate)->base.visible) {
> +		if (!pstate->visible) {
>  			minimum[id] = 0;
>  			y_minimum[id] = 0;
>  			continue;
> @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
>  
>  	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
>  
> -	drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) {
> +	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {

Is this change necessary?  Any plane that differs between the two masks
should already be part of our state, so I don't think this changes the
behavior at all.  The original 'crtc->state->plane_mask' form is closer
to the drm_atomic_add_affected_planes() that this function is modeled
after so my slight preference would be to leave it alone for
consistency.

Aside from that, this patch is

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I remember when I first wrote an early version of this code it was just
doing a drm_atomic_get_plane_state() on each plane unconditionally,
which was non-optimal.  When I reworked it, I wasn't aware of
drm_atomic_crtc_state_for_each_plane_state (or maybe it didn't exist
yet), so I used caching as an alternative.  But the new approach is much
better so I'm glad we can get rid of the caching.


Matt

>  		id = skl_wm_plane_id(to_intel_plane(plane));
>  
>  		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id],
> @@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
>  		to_intel_atomic_state(state);
>  	const struct drm_crtc *crtc;
>  	const struct drm_crtc_state *cstate;
> -	const struct drm_plane *plane;
>  	const struct intel_plane *intel_plane;
> -	const struct drm_plane_state *pstate;
>  	const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
>  	const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
>  	enum pipe pipe;
>  	int id;
> -	int i, j;
> +	int i;
>  
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		if (!crtc->state)
> @@ -4078,10 +4076,9 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
>  
>  		pipe = to_intel_crtc(crtc)->pipe;
>  
> -		for_each_plane_in_state(state, plane, pstate, j) {
> +		for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), intel_plane) {
>  			const struct skl_ddb_entry *old, *new;
>  
> -			intel_plane = to_intel_plane(plane);
>  			id = skl_wm_plane_id(intel_plane);
>  			old = &old_ddb->plane[pipe][id];
>  			new = &new_ddb->plane[pipe][id];
> @@ -4094,13 +4091,13 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
>  
>  			if (id != PLANE_CURSOR) {
>  				DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id, id + 1,
> +						 intel_plane->base.base.id, id + 1,
>  						 pipe_name(pipe),
>  						 old->start, old->end,
>  						 new->start, new->end);
>  			} else {
>  				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor %c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id,
> +						 intel_plane->base.base.id,
>  						 pipe_name(pipe),
>  						 old->start, old->end,
>  						 new->start, new->end);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx