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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
> 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.

But can't the current for_each_plane_in_state() do the same thing? Why
is the new macro better? What's the real point here?

As someone who just downloaded the series and started looking at patch
1 without looking at the others, this commit message makes zero sense.
I'd really like if you could explain how the paragraph above is
connected with the patch below. What does the macro really have to do
with caching? Perhaps you could elaborate more on the plans of the next
patches and explain how the changes below enable the grand plan. Please
do this in the commit message instead of just an email reply.

> 
> 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) {


Now we can cut the "if (intel_plane->pipe != intel_crtc->pipe)" check
this code has.

>  		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;

Same thing here: cut the check above?

>  
> -		if (!to_intel_plane_state(pstate)->base.visible) {
> +		if (!pstate->visible) {

I lol'd :)
I'd probably have done this in a separate patch, since it doesn't seem
to match the commit message.

>  			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) 


This should be in a separate patch with a separate commit message
explaining what exactly changes and why the current code is bad. And as
Matt pointed, this code is completely based
on drm_atomic_add_affected_planes(), so if there's something to fix
here, there's probably something to fix there.


> {
>  		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);

This chunk is another chunk that looks like it belongs to a separate
patch. What does it have to do with the commit message above? It
doesn't even look at the macro you mention.

Thanks,
Paulo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx