[PATCH 0/8] drm/i915/gen9+: Atomic wm fixes.

[PATCH 0/8] drm/i915/gen9+: Atomic wm fixes.

[Maarten Lankhorst]

This patch series applies on top of Lyude's patches.
They clean up the remainder of SKL style wm's, and finally makes
SKL watermarks ready for nonblocking modeset by using the crtc_state
for watermarks as much as possible.

Maarten Lankhorst (8):
  drm/i915/skl+: Prepare for removing data rate from skl watermark state
  drm/i915/skl+: Remove data_rate from watermark struct.
  drm/i915/skl+: Remove minimum block allocation from crtc state.
  drm/i915/skl+: Clean up minimum allocations.
  drm/i915: Add a atomic evasion step to watermark programming.
  drm/i915/gen9+: Use the watermarks from crtc_state for everything.
  drm/i915/gen9+: Program watermarks as a separate step during evasion
  drm/i915/gen9+: Preserve old allocation from crtc_state.

 drivers/gpu/drm/i915/i915_drv.h      |  13 +--
 drivers/gpu/drm/i915/intel_display.c |  58 ++++------
 drivers/gpu/drm/i915/intel_drv.h     |  16 ---
 drivers/gpu/drm/i915/intel_pm.c      | 206 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_sprite.c  |  18 ---
 5 files changed, 136 insertions(+), 175 deletions(-)

-- 
2.7.4

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

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

[Maarten Lankhorst]

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

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

[Matt Roper]

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

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

[Maarten Lankhorst]

Op 20-10-16 om 00:13 schreef Matt Roper:
> 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
Not completely, but I was removing it since I'm trying to get rid of all pointers to obj->state as much as I can.
All accesses should be through some wrapper functions, still working out the specifics.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Paulo Zanoni]

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

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

[Maarten Lankhorst]

Op 20-10-16 om 15:11 schreef Paulo Zanoni:
> 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?
for_each_plane_in_state looks at all planes in the current atomic state. It doesn't
enumerate planes on a crtc that are not part of it.

This macro takes a crtc_state, and enumerates all plane_states assigned to it,
whether they are part of the atomic state or not. This can be done because acquiring
a plane state also requires acquiring crtc_state.

Updating the plane state with this macro is not allowed, because it requires that the
plane has to be part of the atomic state. This is why a const drm_plane_state is returned.
> 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.
The subset that we care about here is crtc->state->plane_mask & cstate->base.plane_mask.
Nothing to fix here, either way is correct. But using cstate instead of crtc->state is nice for removing
obj->state later on.

>
>> {
>>  		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.
Agreed, should be split out.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Maarten Lankhorst]

It's only used in one function, and can be calculated without caching it
in the global struct by using drm_atomic_crtc_state_for_each_plane_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ----
 drivers/gpu/drm/i915/intel_pm.c  | 44 +++++++++++++++++++---------------------
 2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bb468c974e14..888054518f3c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
 			struct skl_pipe_wm optimal;
 			struct skl_ddb_entry ddb;
 
-			/* cached plane data rate */
-			unsigned plane_data_rate[I915_MAX_PLANES];
-			unsigned plane_y_data_rate[I915_MAX_PLANES];
-
 			/* minimum block allocation */
 			uint16_t minimum_blocks[I915_MAX_PLANES];
 			uint16_t minimum_y_blocks[I915_MAX_PLANES];
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b96a899c899d..97b6202c4097 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
  *   3 * 4096 * 8192  * 4 < 2^32
  */
 static unsigned int
-skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
+skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
+				 unsigned *plane_data_rate,
+				 unsigned *plane_y_data_rate)
 {
 	struct drm_crtc_state *cstate = &intel_cstate->base;
 	struct drm_atomic_state *state = cstate->state;
 	struct drm_crtc *crtc = cstate->crtc;
-	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane;
 	const struct intel_plane *intel_plane;
@@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 		/* packed/uv */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 0);
-		intel_cstate->wm.skl.plane_data_rate[id] = rate;
+		plane_data_rate[id] = rate;
+
+		total_data_rate += rate;
 
 		/* y-plane */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 1);
-		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
-	}
-
-	/* Calculate CRTC's total data rate from cached values */
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		int id = skl_wm_plane_id(intel_plane);
+		plane_y_data_rate[id] = rate;
 
-		/* packed/uv */
-		total_data_rate += intel_cstate->wm.skl.plane_data_rate[id];
-		total_data_rate += intel_cstate->wm.skl.plane_y_data_rate[id];
+		total_data_rate += rate;
 	}
 
 	return total_data_rate;
@@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	int num_active;
 	int id, i;
 
+	unsigned data_rate[I915_MAX_PLANES] = {};
+	unsigned y_data_rate[I915_MAX_PLANES] = {};
+
 	/* Clear the partitioning for disabled planes. */
 	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
 	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
@@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	 *
 	 * FIXME: we may not allocate every single block here.
 	 */
-	total_data_rate = skl_get_total_relative_data_rate(cstate);
+	total_data_rate = skl_get_total_relative_data_rate(cstate, data_rate, y_data_rate);
 	if (total_data_rate == 0)
 		return 0;
 
 	start = alloc->start;
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		unsigned int data_rate, y_data_rate;
+	for (id = 0; id < I915_MAX_PLANES; id++) {
+		unsigned rate;
 		uint16_t plane_blocks, y_plane_blocks = 0;
-		int id = skl_wm_plane_id(intel_plane);
 
-		data_rate = cstate->wm.skl.plane_data_rate[id];
+		rate = data_rate[id];
 
 		/*
 		 * allocation for (packed formats) or (uv-plane part of planar format):
 		 * promote the expression to 64 bits to avoid overflowing, the
-		 * result is < available as data_rate / total_data_rate < 1
+		 * result is < available as rate / total_data_rate < 1
 		 */
 		plane_blocks = minimum[id];
-		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
+		plane_blocks += div_u64((uint64_t)alloc_size * rate,
 					total_data_rate);
 
 		/* Leave disabled planes at (0,0) */
-		if (data_rate) {
+		if (rate) {
 			ddb->plane[pipe][id].start = start;
 			ddb->plane[pipe][id].end = start + plane_blocks;
 		}
@@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		/*
 		 * allocation for y_plane part of planar format:
 		 */
-		y_data_rate = cstate->wm.skl.plane_y_data_rate[id];
+		rate = y_data_rate[id];
 
 		y_plane_blocks = y_minimum[id];
-		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
+		y_plane_blocks += div_u64((uint64_t)alloc_size * rate,
 					total_data_rate);
 
-		if (y_data_rate) {
+		if (rate) {
 			ddb->y_plane[pipe][id].start = start;
 			ddb->y_plane[pipe][id].end = start + y_plane_blocks;
 		}
-- 
2.7.4

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

Re: [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote:
> It's only used in one function, and can be calculated without caching it
> in the global struct by using drm_atomic_crtc_state_for_each_plane_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 ----
>  drivers/gpu/drm/i915/intel_pm.c  | 44 +++++++++++++++++++---------------------
>  2 files changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bb468c974e14..888054518f3c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
>  			struct skl_pipe_wm optimal;
>  			struct skl_ddb_entry ddb;
>  
> -			/* cached plane data rate */
> -			unsigned plane_data_rate[I915_MAX_PLANES];
> -			unsigned plane_y_data_rate[I915_MAX_PLANES];
> -
>  			/* minimum block allocation */
>  			uint16_t minimum_blocks[I915_MAX_PLANES];
>  			uint16_t minimum_y_blocks[I915_MAX_PLANES];
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b96a899c899d..97b6202c4097 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>   *   3 * 4096 * 8192  * 4 < 2^32
>   */
>  static unsigned int
> -skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
> +skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
> +				 unsigned *plane_data_rate,
> +				 unsigned *plane_y_data_rate)
>  {
>  	struct drm_crtc_state *cstate = &intel_cstate->base;
>  	struct drm_atomic_state *state = cstate->state;
>  	struct drm_crtc *crtc = cstate->crtc;
> -	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_plane *plane;
>  	const struct intel_plane *intel_plane;
> @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
>  		/* packed/uv */
>  		rate = skl_plane_relative_data_rate(intel_cstate,
>  						    pstate, 0);
> -		intel_cstate->wm.skl.plane_data_rate[id] = rate;
> +		plane_data_rate[id] = rate;
> +
> +		total_data_rate += rate;
>  
>  		/* y-plane */
>  		rate = skl_plane_relative_data_rate(intel_cstate,
>  						    pstate, 1);
> -		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> -	}
> -
> -	/* Calculate CRTC's total data rate from cached values */
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		int id = skl_wm_plane_id(intel_plane);
> +		plane_y_data_rate[id] = rate;
>  
> -		/* packed/uv */
> -		total_data_rate += intel_cstate->wm.skl.plane_data_rate[id];
> -		total_data_rate += intel_cstate->wm.skl.plane_y_data_rate[id];
> +		total_data_rate += rate;
>  	}
>  
>  	return total_data_rate;
> @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	int num_active;
>  	int id, i;
>  
> +	unsigned data_rate[I915_MAX_PLANES] = {};
> +	unsigned y_data_rate[I915_MAX_PLANES] = {};
> +

Minor nitpick; if you picked a different names here (e.g.,
plane_data_rate[]) then you could leave the local variables farther down
named 'data_rate' and 'y_data_rate' which would reduce the diff changes
and result in a slightly smaller patch.

Whether or not you feel like making that change, killing the caching is
good so,

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


>  	/* Clear the partitioning for disabled planes. */
>  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>  	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	 *
>  	 * FIXME: we may not allocate every single block here.
>  	 */
> -	total_data_rate = skl_get_total_relative_data_rate(cstate);
> +	total_data_rate = skl_get_total_relative_data_rate(cstate, data_rate, y_data_rate);
>  	if (total_data_rate == 0)
>  		return 0;
>  
>  	start = alloc->start;
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		unsigned int data_rate, y_data_rate;
> +	for (id = 0; id < I915_MAX_PLANES; id++) {
> +		unsigned rate;
>  		uint16_t plane_blocks, y_plane_blocks = 0;
> -		int id = skl_wm_plane_id(intel_plane);
>  
> -		data_rate = cstate->wm.skl.plane_data_rate[id];
> +		rate = data_rate[id];
>  
>  		/*
>  		 * allocation for (packed formats) or (uv-plane part of planar format):
>  		 * promote the expression to 64 bits to avoid overflowing, the
> -		 * result is < available as data_rate / total_data_rate < 1
> +		 * result is < available as rate / total_data_rate < 1
>  		 */
>  		plane_blocks = minimum[id];
> -		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
> +		plane_blocks += div_u64((uint64_t)alloc_size * rate,
>  					total_data_rate);
>  
>  		/* Leave disabled planes at (0,0) */
> -		if (data_rate) {
> +		if (rate) {
>  			ddb->plane[pipe][id].start = start;
>  			ddb->plane[pipe][id].end = start + plane_blocks;
>  		}
> @@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		/*
>  		 * allocation for y_plane part of planar format:
>  		 */
> -		y_data_rate = cstate->wm.skl.plane_y_data_rate[id];
> +		rate = y_data_rate[id];
>  
>  		y_plane_blocks = y_minimum[id];
> -		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
> +		y_plane_blocks += div_u64((uint64_t)alloc_size * rate,
>  					total_data_rate);
>  
> -		if (y_data_rate) {
> +		if (rate) {
>  			ddb->y_plane[pipe][id].start = start;
>  			ddb->y_plane[pipe][id].end = start + y_plane_blocks;
>  		}
> -- 
> 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

Re: [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Paulo Zanoni]

Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu:
> On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote:
> > 
> > It's only used in one function, and can be calculated without
> > caching it
> > in the global struct by using
> > drm_atomic_crtc_state_for_each_plane_state.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> > >
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  4 ----
> >  drivers/gpu/drm/i915/intel_pm.c  | 44 +++++++++++++++++++---------
> > ------------
> >  2 files changed, 21 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index bb468c974e14..888054518f3c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
> >  			struct skl_pipe_wm optimal;
> >  			struct skl_ddb_entry ddb;
> >  
> > -			/* cached plane data rate */
> > -			unsigned plane_data_rate[I915_MAX_PLANES];
> > -			unsigned
> > plane_y_data_rate[I915_MAX_PLANES];
> > -
> >  			/* minimum block allocation */
> >  			uint16_t minimum_blocks[I915_MAX_PLANES];
> >  			uint16_t
> > minimum_y_blocks[I915_MAX_PLANES];
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index b96a899c899d..97b6202c4097 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct
> > intel_crtc_state *cstate,
> >   *   3 * 4096 * 8192  * 4 < 2^32
> >   */
> >  static unsigned int
> > -skl_get_total_relative_data_rate(struct intel_crtc_state
> > *intel_cstate)
> > +skl_get_total_relative_data_rate(struct intel_crtc_state
> > *intel_cstate,
> > +				 unsigned *plane_data_rate,
> > +				 unsigned *plane_y_data_rate)
> >  {
> >  	struct drm_crtc_state *cstate = &intel_cstate->base;
> >  	struct drm_atomic_state *state = cstate->state;
> >  	struct drm_crtc *crtc = cstate->crtc;
> > -	struct drm_device *dev = crtc->dev;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct drm_plane *plane;
> >  	const struct intel_plane *intel_plane;
> > @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct
> > intel_crtc_state *intel_cstate)
> >  		/* packed/uv */
> >  		rate = skl_plane_relative_data_rate(intel_cstate,
> >  						    pstate, 0);
> > -		intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > +		plane_data_rate[id] = rate;
> > +
> > +		total_data_rate += rate;
> >  
> >  		/* y-plane */
> >  		rate = skl_plane_relative_data_rate(intel_cstate,
> >  						    pstate, 1);
> > -		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > -	}
> > -
> > -	/* Calculate CRTC's total data rate from cached values */
> > -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > {
> > -		int id = skl_wm_plane_id(intel_plane);
> > +		plane_y_data_rate[id] = rate;
> >  
> > -		/* packed/uv */
> > -		total_data_rate += intel_cstate-
> > >wm.skl.plane_data_rate[id];
> > -		total_data_rate += intel_cstate-
> > >wm.skl.plane_y_data_rate[id];
> > +		total_data_rate += rate;
> >  	}
> >  
> >  	return total_data_rate;
> > @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > *cstate,
> >  	int num_active;
> >  	int id, i;
> >  
> > +	unsigned data_rate[I915_MAX_PLANES] = {};
> > +	unsigned y_data_rate[I915_MAX_PLANES] = {};
> > +
> 
> Minor nitpick; if you picked a different names here (e.g.,
> plane_data_rate[]) then you could leave the local variables farther
> down
> named 'data_rate' and 'y_data_rate' which would reduce the diff
> changes
> and result in a slightly smaller patch.
> 
> Whether or not you feel like making that change, killing the caching
> is
> good so,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> > 
> >  	/* Clear the partitioning for disabled planes. */
> >  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> >  	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> > @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct
> > intel_crtc_state *cstate,
> >  	 *
> >  	 * FIXME: we may not allocate every single block here.
> >  	 */
> > -	total_data_rate =
> > skl_get_total_relative_data_rate(cstate);
> > +	total_data_rate = skl_get_total_relative_data_rate(cstate,
> > data_rate, y_data_rate);
> >  	if (total_data_rate == 0)
> >  		return 0;
> >  
> >  	start = alloc->start;
> > -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > {
> > -		unsigned int data_rate, y_data_rate;
> > +	for (id = 0; id < I915_MAX_PLANES; id++) {

Can we please use a different kind of iteration? Although this is
correct today, history shows that the number of planes increases over
time and the code may suddenly break when if we ever introduce PLANE_D.

Perhaps:
for_each_intel_plane_on_crtc(...) {
	id = skl_wm_plane_id(intel_plane);
	...
}

With that fixed (and, in case you want, Matt's suggestion):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> > +		unsigned rate;
> >  		uint16_t plane_blocks, y_plane_blocks = 0;
> > -		int id = skl_wm_plane_id(intel_plane);
> >  
> > -		data_rate = cstate->wm.skl.plane_data_rate[id];
> > +		rate = data_rate[id];
> >  
> >  		/*
> >  		 * allocation for (packed formats) or (uv-plane
> > part of planar format):
> >  		 * promote the expression to 64 bits to avoid
> > overflowing, the
> > -		 * result is < available as data_rate /
> > total_data_rate < 1
> > +		 * result is < available as rate / total_data_rate
> > < 1
> >  		 */
> >  		plane_blocks = minimum[id];
> > -		plane_blocks += div_u64((uint64_t)alloc_size *
> > data_rate,
> > +		plane_blocks += div_u64((uint64_t)alloc_size *
> > rate,
> >  					total_data_rate);
> >  
> >  		/* Leave disabled planes at (0,0) */
> > -		if (data_rate) {
> > +		if (rate) {
> >  			ddb->plane[pipe][id].start = start;
> >  			ddb->plane[pipe][id].end = start +
> > plane_blocks;
> >  		}
> > @@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct
> > intel_crtc_state *cstate,
> >  		/*
> >  		 * allocation for y_plane part of planar format:
> >  		 */
> > -		y_data_rate = cstate-
> > >wm.skl.plane_y_data_rate[id];
> > +		rate = y_data_rate[id];
> >  
> >  		y_plane_blocks = y_minimum[id];
> > -		y_plane_blocks += div_u64((uint64_t)alloc_size *
> > y_data_rate,
> > +		y_plane_blocks += div_u64((uint64_t)alloc_size *
> > rate,
> >  					total_data_rate);
> >  
> > -		if (y_data_rate) {
> > +		if (rate) {
> >  			ddb->y_plane[pipe][id].start = start;
> >  			ddb->y_plane[pipe][id].end = start +
> > y_plane_blocks;
> >  		}
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Paulo Zanoni]

Em Qui, 2016-10-20 às 15:18 -0200, Paulo Zanoni escreveu:
> Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu:
> > 
> > On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote:
> > > 
> > > 
> > > It's only used in one function, and can be calculated without
> > > caching it
> > > in the global struct by using
> > > drm_atomic_crtc_state_for_each_plane_state.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om
> > > > 
> > > > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_drv.h |  4 ----
> > >  drivers/gpu/drm/i915/intel_pm.c  | 44 +++++++++++++++++++-------
> > > --
> > > ------------
> > >  2 files changed, 21 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index bb468c974e14..888054518f3c 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
> > >  			struct skl_pipe_wm optimal;
> > >  			struct skl_ddb_entry ddb;
> > >  
> > > -			/* cached plane data rate */
> > > -			unsigned
> > > plane_data_rate[I915_MAX_PLANES];
> > > -			unsigned
> > > plane_y_data_rate[I915_MAX_PLANES];
> > > -
> > >  			/* minimum block allocation */
> > >  			uint16_t
> > > minimum_blocks[I915_MAX_PLANES];
> > >  			uint16_t
> > > minimum_y_blocks[I915_MAX_PLANES];
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index b96a899c899d..97b6202c4097 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct
> > > intel_crtc_state *cstate,
> > >   *   3 * 4096 * 8192  * 4 < 2^32
> > >   */
> > >  static unsigned int
> > > -skl_get_total_relative_data_rate(struct intel_crtc_state
> > > *intel_cstate)
> > > +skl_get_total_relative_data_rate(struct intel_crtc_state
> > > *intel_cstate,
> > > +				 unsigned *plane_data_rate,
> > > +				 unsigned *plane_y_data_rate)
> > >  {
> > >  	struct drm_crtc_state *cstate = &intel_cstate->base;
> > >  	struct drm_atomic_state *state = cstate->state;
> > >  	struct drm_crtc *crtc = cstate->crtc;
> > > -	struct drm_device *dev = crtc->dev;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	struct drm_plane *plane;
> > >  	const struct intel_plane *intel_plane;
> > > @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct
> > > intel_crtc_state *intel_cstate)
> > >  		/* packed/uv */
> > >  		rate =
> > > skl_plane_relative_data_rate(intel_cstate,
> > >  						    pstate, 0);
> > > -		intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > > +		plane_data_rate[id] = rate;
> > > +
> > > +		total_data_rate += rate;
> > >  
> > >  		/* y-plane */
> > >  		rate =
> > > skl_plane_relative_data_rate(intel_cstate,
> > >  						    pstate, 1);
> > > -		intel_cstate->wm.skl.plane_y_data_rate[id] =
> > > rate;
> > > -	}
> > > -
> > > -	/* Calculate CRTC's total data rate from cached values
> > > */
> > > -	for_each_intel_plane_on_crtc(dev, intel_crtc,
> > > intel_plane)
> > > {
> > > -		int id = skl_wm_plane_id(intel_plane);
> > > +		plane_y_data_rate[id] = rate;
> > >  
> > > -		/* packed/uv */
> > > -		total_data_rate += intel_cstate-
> > > > 
> > > > wm.skl.plane_data_rate[id];
> > > -		total_data_rate += intel_cstate-
> > > > 
> > > > wm.skl.plane_y_data_rate[id];
> > > +		total_data_rate += rate;
> > >  	}
> > >  
> > >  	return total_data_rate;
> > > @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct
> > > intel_crtc_state
> > > *cstate,
> > >  	int num_active;
> > >  	int id, i;
> > >  

Also obligatory bikeshed to remove the ugly blank line above :)

> > > +	unsigned data_rate[I915_MAX_PLANES] = {};
> > > +	unsigned y_data_rate[I915_MAX_PLANES] = {};
> > > +
> > 
> > Minor nitpick; if you picked a different names here (e.g.,
> > plane_data_rate[]) then you could leave the local variables farther
> > down
> > named 'data_rate' and 'y_data_rate' which would reduce the diff
> > changes
> > and result in a slightly smaller patch.
> > 
> > Whether or not you feel like making that change, killing the
> > caching
> > is
> > good so,
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > 
> > > 
> > > 
> > >  	/* Clear the partitioning for disabled planes. */
> > >  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > >  	memset(ddb->y_plane[pipe], 0, sizeof(ddb-
> > > >y_plane[pipe]));
> > > @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct
> > > intel_crtc_state *cstate,
> > >  	 *
> > >  	 * FIXME: we may not allocate every single block here.
> > >  	 */
> > > -	total_data_rate =
> > > skl_get_total_relative_data_rate(cstate);
> > > +	total_data_rate =
> > > skl_get_total_relative_data_rate(cstate,
> > > data_rate, y_data_rate);
> > >  	if (total_data_rate == 0)
> > >  		return 0;
> > >  
> > >  	start = alloc->start;
> > > -	for_each_intel_plane_on_crtc(dev, intel_crtc,
> > > intel_plane)
> > > {
> > > -		unsigned int data_rate, y_data_rate;
> > > +	for (id = 0; id < I915_MAX_PLANES; id++) {
> 
> Can we please use a different kind of iteration? Although this is
> correct today, history shows that the number of planes increases over
> time and the code may suddenly break when if we ever introduce
> PLANE_D.
> 
> Perhaps:
> for_each_intel_plane_on_crtc(...) {
> 	id = skl_wm_plane_id(intel_plane);
> 	...
> }
> 
> With that fixed (and, in case you want, Matt's suggestion):
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > 
> > > 
> > > +		unsigned rate;
> > >  		uint16_t plane_blocks, y_plane_blocks = 0;
> > > -		int id = skl_wm_plane_id(intel_plane);
> > >  
> > > -		data_rate = cstate->wm.skl.plane_data_rate[id];
> > > +		rate = data_rate[id];
> > >  
> > >  		/*
> > >  		 * allocation for (packed formats) or (uv-plane
> > > part of planar format):
> > >  		 * promote the expression to 64 bits to avoid
> > > overflowing, the
> > > -		 * result is < available as data_rate /
> > > total_data_rate < 1
> > > +		 * result is < available as rate /
> > > total_data_rate
> > > < 1
> > >  		 */
> > >  		plane_blocks = minimum[id];
> > > -		plane_blocks += div_u64((uint64_t)alloc_size *
> > > data_rate,
> > > +		plane_blocks += div_u64((uint64_t)alloc_size *
> > > rate,
> > >  					total_data_rate);
> > >  
> > >  		/* Leave disabled planes at (0,0) */
> > > -		if (data_rate) {
> > > +		if (rate) {
> > >  			ddb->plane[pipe][id].start = start;
> > >  			ddb->plane[pipe][id].end = start +
> > > plane_blocks;
> > >  		}
> > > @@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct
> > > intel_crtc_state *cstate,
> > >  		/*
> > >  		 * allocation for y_plane part of planar format:
> > >  		 */
> > > -		y_data_rate = cstate-
> > > > 
> > > > wm.skl.plane_y_data_rate[id];
> > > +		rate = y_data_rate[id];
> > >  
> > >  		y_plane_blocks = y_minimum[id];
> > > -		y_plane_blocks += div_u64((uint64_t)alloc_size *
> > > y_data_rate,
> > > +		y_plane_blocks += div_u64((uint64_t)alloc_size *
> > > rate,
> > >  					total_data_rate);
> > >  
> > > -		if (y_data_rate) {
> > > +		if (rate) {
> > >  			ddb->y_plane[pipe][id].start = start;
> > >  			ddb->y_plane[pipe][id].end = start +
> > > y_plane_blocks;
> > >  		}
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Maarten Lankhorst]

Op 20-10-16 om 19:18 schreef Paulo Zanoni:
> Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu:
>> On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote:
>>> It's only used in one function, and can be calculated without
>>> caching it
>>> in the global struct by using
>>> drm_atomic_crtc_state_for_each_plane_state.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
>>> ---
>>>  drivers/gpu/drm/i915/intel_drv.h |  4 ----
>>>  drivers/gpu/drm/i915/intel_pm.c  | 44 +++++++++++++++++++---------
>>> ------------
>>>  2 files changed, 21 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index bb468c974e14..888054518f3c 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
>>>  			struct skl_pipe_wm optimal;
>>>  			struct skl_ddb_entry ddb;
>>>  
>>> -			/* cached plane data rate */
>>> -			unsigned plane_data_rate[I915_MAX_PLANES];
>>> -			unsigned
>>> plane_y_data_rate[I915_MAX_PLANES];
>>> -
>>>  			/* minimum block allocation */
>>>  			uint16_t minimum_blocks[I915_MAX_PLANES];
>>>  			uint16_t
>>> minimum_y_blocks[I915_MAX_PLANES];
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>> b/drivers/gpu/drm/i915/intel_pm.c
>>> index b96a899c899d..97b6202c4097 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct
>>> intel_crtc_state *cstate,
>>>   *   3 * 4096 * 8192  * 4 < 2^32
>>>   */
>>>  static unsigned int
>>> -skl_get_total_relative_data_rate(struct intel_crtc_state
>>> *intel_cstate)
>>> +skl_get_total_relative_data_rate(struct intel_crtc_state
>>> *intel_cstate,
>>> +				 unsigned *plane_data_rate,
>>> +				 unsigned *plane_y_data_rate)
>>>  {
>>>  	struct drm_crtc_state *cstate = &intel_cstate->base;
>>>  	struct drm_atomic_state *state = cstate->state;
>>>  	struct drm_crtc *crtc = cstate->crtc;
>>> -	struct drm_device *dev = crtc->dev;
>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>  	struct drm_plane *plane;
>>>  	const struct intel_plane *intel_plane;
>>> @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct
>>> intel_crtc_state *intel_cstate)
>>>  		/* packed/uv */
>>>  		rate = skl_plane_relative_data_rate(intel_cstate,
>>>  						    pstate, 0);
>>> -		intel_cstate->wm.skl.plane_data_rate[id] = rate;
>>> +		plane_data_rate[id] = rate;
>>> +
>>> +		total_data_rate += rate;
>>>  
>>>  		/* y-plane */
>>>  		rate = skl_plane_relative_data_rate(intel_cstate,
>>>  						    pstate, 1);
>>> -		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
>>> -	}
>>> -
>>> -	/* Calculate CRTC's total data rate from cached values */
>>> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
>>> {
>>> -		int id = skl_wm_plane_id(intel_plane);
>>> +		plane_y_data_rate[id] = rate;
>>>  
>>> -		/* packed/uv */
>>> -		total_data_rate += intel_cstate-
>>>> wm.skl.plane_data_rate[id];
>>> -		total_data_rate += intel_cstate-
>>>> wm.skl.plane_y_data_rate[id];
>>> +		total_data_rate += rate;
>>>  	}
>>>  
>>>  	return total_data_rate;
>>> @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
>>> *cstate,
>>>  	int num_active;
>>>  	int id, i;
>>>  
>>> +	unsigned data_rate[I915_MAX_PLANES] = {};
>>> +	unsigned y_data_rate[I915_MAX_PLANES] = {};
>>> +
>> Minor nitpick; if you picked a different names here (e.g.,
>> plane_data_rate[]) then you could leave the local variables farther
>> down
>> named 'data_rate' and 'y_data_rate' which would reduce the diff
>> changes
>> and result in a slightly smaller patch.
>>
>> Whether or not you feel like making that change, killing the caching
>> is
>> good so,
>>
>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>
>>
>>>  	/* Clear the partitioning for disabled planes. */
>>>  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>>>  	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>>> @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct
>>> intel_crtc_state *cstate,
>>>  	 *
>>>  	 * FIXME: we may not allocate every single block here.
>>>  	 */
>>> -	total_data_rate =
>>> skl_get_total_relative_data_rate(cstate);
>>> +	total_data_rate = skl_get_total_relative_data_rate(cstate,
>>> data_rate, y_data_rate);
>>>  	if (total_data_rate == 0)
>>>  		return 0;
>>>  
>>>  	start = alloc->start;
>>> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
>>> {
>>> -		unsigned int data_rate, y_data_rate;
>>> +	for (id = 0; id < I915_MAX_PLANES; id++) {
> Can we please use a different kind of iteration? Although this is
> correct today, history shows that the number of planes increases over
> time and the code may suddenly break when if we ever introduce PLANE_D.
>
> Perhaps:
> for_each_intel_plane_on_crtc(...) {
> 	id = skl_wm_plane_id(intel_plane);
> 	...
> }
>
> With that fixed (and, in case you want, Matt's suggestion):
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Why would that break with PLANE_D? in that case rate = 0 and nothing happens. The hooks for it will never be called anyway, since it only updates up to max_planes.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/8] drm/i915/skl+: Remove minimum block allocation from crtc state.

[Maarten Lankhorst]

This is not required any more now that we get fresh state from
drm_atomic_crtc_state_for_each_plane_state. Zero all state
in advance.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ----
 drivers/gpu/drm/i915/intel_pm.c  | 15 +++++----------
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 888054518f3c..a176e6cebab3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -501,10 +501,6 @@ struct intel_crtc_wm_state {
 			/* gen9+ only needs 1-step wm programming */
 			struct skl_pipe_wm optimal;
 			struct skl_ddb_entry ddb;
-
-			/* minimum block allocation */
-			uint16_t minimum_blocks[I915_MAX_PLANES];
-			uint16_t minimum_y_blocks[I915_MAX_PLANES];
 		} skl;
 	};
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 97b6202c4097..83c1b0acef38 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3356,8 +3356,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
 	uint16_t alloc_size, start, cursor_blocks;
-	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
-	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
+	uint16_t minimum[I915_MAX_PLANES] = {};
+	uint16_t y_minimum[I915_MAX_PLANES] = {};
 	unsigned int total_data_rate;
 	int num_active;
 	int id, i;
@@ -3398,16 +3398,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		if (intel_plane->pipe != pipe)
 			continue;
 
-		if (!pstate->visible) {
-			minimum[id] = 0;
-			y_minimum[id] = 0;
+		if (!pstate->visible)
 			continue;
-		}
-		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
-			minimum[id] = 0;
-			y_minimum[id] = 0;
+
+		if (plane->type == DRM_PLANE_TYPE_CURSOR)
 			continue;
-		}
 
 		minimum[id] = skl_ddb_min_alloc(pstate, 0);
 		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
-- 
2.7.4

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

Re: [PATCH 3/8] drm/i915/skl+: Remove minimum block allocation from crtc state.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:16PM +0200, Maarten Lankhorst wrote:
> This is not required any more now that we get fresh state from
> drm_atomic_crtc_state_for_each_plane_state. Zero all state
> in advance.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 ----
>  drivers/gpu/drm/i915/intel_pm.c  | 15 +++++----------
>  2 files changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 888054518f3c..a176e6cebab3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -501,10 +501,6 @@ struct intel_crtc_wm_state {
>  			/* gen9+ only needs 1-step wm programming */
>  			struct skl_pipe_wm optimal;
>  			struct skl_ddb_entry ddb;
> -
> -			/* minimum block allocation */
> -			uint16_t minimum_blocks[I915_MAX_PLANES];
> -			uint16_t minimum_y_blocks[I915_MAX_PLANES];
>  		} skl;
>  	};
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 97b6202c4097..83c1b0acef38 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3356,8 +3356,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
>  	uint16_t alloc_size, start, cursor_blocks;
> -	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
> -	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
> +	uint16_t minimum[I915_MAX_PLANES] = {};
> +	uint16_t y_minimum[I915_MAX_PLANES] = {};
>  	unsigned int total_data_rate;
>  	int num_active;
>  	int id, i;
> @@ -3398,16 +3398,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		if (intel_plane->pipe != pipe)
>  			continue;
>  
> -		if (!pstate->visible) {
> -			minimum[id] = 0;
> -			y_minimum[id] = 0;
> +		if (!pstate->visible)
>  			continue;
> -		}
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> -			minimum[id] = 0;
> -			y_minimum[id] = 0;
> +
> +		if (plane->type == DRM_PLANE_TYPE_CURSOR)
>  			continue;
> -		}
>  
>  		minimum[id] = skl_ddb_min_alloc(pstate, 0);
>  		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> -- 
> 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

Re: [PATCH 3/8] drm/i915/skl+: Remove minimum block allocation from crtc state.

[Paulo Zanoni]

Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu:
> On Wed, Oct 12, 2016 at 03:28:16PM +0200, Maarten Lankhorst wrote:
> > 
> > This is not required any more now that we get fresh state from
> > drm_atomic_crtc_state_for_each_plane_state. Zero all state
> > in advance.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> > >
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

You could also get rid of the unsafe loop that computes alloc_size:
just do it in the main loop now that we iterate over everything. But
this can be done in a separate patch.

> 
> > 
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  4 ----
> >  drivers/gpu/drm/i915/intel_pm.c  | 15 +++++----------
> >  2 files changed, 5 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 888054518f3c..a176e6cebab3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -501,10 +501,6 @@ struct intel_crtc_wm_state {
> >  			/* gen9+ only needs 1-step wm programming
> > */
> >  			struct skl_pipe_wm optimal;
> >  			struct skl_ddb_entry ddb;
> > -
> > -			/* minimum block allocation */
> > -			uint16_t minimum_blocks[I915_MAX_PLANES];
> > -			uint16_t
> > minimum_y_blocks[I915_MAX_PLANES];
> >  		} skl;
> >  	};
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 97b6202c4097..83c1b0acef38 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3356,8 +3356,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > *cstate,
> >  	enum pipe pipe = intel_crtc->pipe;
> >  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> >  	uint16_t alloc_size, start, cursor_blocks;
> > -	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
> > -	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
> > +	uint16_t minimum[I915_MAX_PLANES] = {};
> > +	uint16_t y_minimum[I915_MAX_PLANES] = {};
> >  	unsigned int total_data_rate;
> >  	int num_active;
> >  	int id, i;
> > @@ -3398,16 +3398,11 @@ skl_allocate_pipe_ddb(struct
> > intel_crtc_state *cstate,
> >  		if (intel_plane->pipe != pipe)
> >  			continue;
> >  
> > -		if (!pstate->visible) {
> > -			minimum[id] = 0;
> > -			y_minimum[id] = 0;
> > +		if (!pstate->visible)
> >  			continue;
> > -		}
> > -		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > -			minimum[id] = 0;
> > -			y_minimum[id] = 0;
> > +
> > +		if (plane->type == DRM_PLANE_TYPE_CURSOR)
> >  			continue;
> > -		}
> >  
> >  		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> >  		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/8] drm/i915/skl+: Clean up minimum allocations.

[Maarten Lankhorst]

Move calculating minimum allocations to a helper, which cleans up the
code some more. The cursor is still allocated in advance because it
doesn't count towards data rate and should always be reserved.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 66 ++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 83c1b0acef38..45fb8275abea 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3342,6 +3342,32 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
 	return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) * min_scanlines/4 + 3;
 }
 
+static void
+skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
+		 uint16_t *minimum, uint16_t *y_minimum)
+{
+	const struct drm_plane_state *pstate;
+	struct drm_plane *plane;
+	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
+
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
+		struct intel_plane *intel_plane = to_intel_plane(plane);
+		int id = skl_wm_plane_id(intel_plane);
+
+		if (intel_plane->pipe != pipe ||
+		    id == PLANE_CURSOR)
+			continue;
+
+		if (!pstate->visible)
+			continue;
+
+		minimum[id] = skl_ddb_min_alloc(pstate, 0);
+		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
+	}
+
+	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
+}
+
 static int
 skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		      struct skl_ddb_allocation *ddb /* out */)
@@ -3350,12 +3376,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
-	struct drm_plane *plane;
-	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;
+	uint16_t alloc_size, start;
 	uint16_t minimum[I915_MAX_PLANES] = {};
 	uint16_t y_minimum[I915_MAX_PLANES] = {};
 	unsigned int total_data_rate;
@@ -3384,35 +3407,21 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return 0;
 	}
 
-	cursor_blocks = skl_cursor_allocation(num_active);
-	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - cursor_blocks;
-	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
-
-	alloc_size -= cursor_blocks;
-
-	/* 1. Allocate the mininum required blocks for each active plane */
-	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 (!pstate->visible)
-			continue;
+	skl_ddb_calc_min(cstate, num_active, minimum, y_minimum);
 
-		if (plane->type == DRM_PLANE_TYPE_CURSOR)
-			continue;
-
-		minimum[id] = skl_ddb_min_alloc(pstate, 0);
-		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
-	}
+	/* 1. Allocate the mininum required blocks for each active plane
+	 * and allocate the cursor, it doesn't require extra allocation
+	 * proportional to the data rate.
+	 */
 
-	for (i = 0; i < PLANE_CURSOR; i++) {
+	for (i = 0; i < I915_MAX_PLANES; i++) {
 		alloc_size -= minimum[i];
 		alloc_size -= y_minimum[i];
 	}
 
+	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
+	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
+
 	/*
 	 * 2. Distribute the remaining space in proportion to the amount of
 	 * data each plane needs to fetch from memory.
@@ -3428,6 +3437,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		unsigned rate;
 		uint16_t plane_blocks, y_plane_blocks = 0;
 
+		if (id == PLANE_CURSOR)
+			continue;
+
 		rate = data_rate[id];
 
 		/*
-- 
2.7.4

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

Re: [PATCH 4/8] drm/i915/skl+: Clean up minimum allocations.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:17PM +0200, Maarten Lankhorst wrote:
> Move calculating minimum allocations to a helper, which cleans up the
> code some more. The cursor is still allocated in advance because it
> doesn't count towards data rate and should always be reserved.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 66 ++++++++++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 83c1b0acef38..45fb8275abea 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3342,6 +3342,32 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
>  	return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) * min_scanlines/4 + 3;
>  }
>  
> +static void
> +skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
> +		 uint16_t *minimum, uint16_t *y_minimum)
> +{
> +	const struct drm_plane_state *pstate;
> +	struct drm_plane *plane;
> +	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> +
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
> +		struct intel_plane *intel_plane = to_intel_plane(plane);
> +		int id = skl_wm_plane_id(intel_plane);
> +
> +		if (intel_plane->pipe != pipe ||
> +		    id == PLANE_CURSOR)
> +			continue;
> +
> +		if (!pstate->visible)
> +			continue;
> +
> +		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> +		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> +	}
> +
> +	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> +}
> +
>  static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		      struct skl_ddb_allocation *ddb /* out */)
> @@ -3350,12 +3376,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
> -	struct drm_plane *plane;
> -	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;
> +	uint16_t alloc_size, start;
>  	uint16_t minimum[I915_MAX_PLANES] = {};
>  	uint16_t y_minimum[I915_MAX_PLANES] = {};
>  	unsigned int total_data_rate;
> @@ -3384,35 +3407,21 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		return 0;
>  	}
>  
> -	cursor_blocks = skl_cursor_allocation(num_active);
> -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - cursor_blocks;
> -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> -
> -	alloc_size -= cursor_blocks;
> -
> -	/* 1. Allocate the mininum required blocks for each active plane */
> -	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 (!pstate->visible)
> -			continue;
> +	skl_ddb_calc_min(cstate, num_active, minimum, y_minimum);
>  
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR)
> -			continue;
> -
> -		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> -		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> -	}
> +	/* 1. Allocate the mininum required blocks for each active plane

Minor style nitpick; different multi-line comment format than we
typically use (and that we use for #2 below).

Otherwise,

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


> +	 * and allocate the cursor, it doesn't require extra allocation
> +	 * proportional to the data rate.
> +	 */
>  
> -	for (i = 0; i < PLANE_CURSOR; i++) {
> +	for (i = 0; i < I915_MAX_PLANES; i++) {
>  		alloc_size -= minimum[i];
>  		alloc_size -= y_minimum[i];
>  	}
>  
> +	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> +	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> +
>  	/*
>  	 * 2. Distribute the remaining space in proportion to the amount of
>  	 * data each plane needs to fetch from memory.
> @@ -3428,6 +3437,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		unsigned rate;
>  		uint16_t plane_blocks, y_plane_blocks = 0;
>  
> +		if (id == PLANE_CURSOR)
> +			continue;
> +
>  		rate = data_rate[id];
>  
>  		/*
> -- 
> 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

Re: [PATCH 4/8] drm/i915/skl+: Clean up minimum allocations.

[Paulo Zanoni]

Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
> Move calculating minimum allocations to a helper, which cleans up the
> code some more. The cursor is still allocated in advance because it
> doesn't count towards data rate and should always be reserved.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 66 ++++++++++++++++++++++++-------
> ----------
>  1 file changed, 39 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 83c1b0acef38..45fb8275abea 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3342,6 +3342,32 @@ skl_ddb_min_alloc(const struct drm_plane_state
> *pstate,
>  	return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) *
> min_scanlines/4 + 3;
>  }
>  
> +static void
> +skl_ddb_calc_min(const struct intel_crtc_state *cstate, int
> num_active,
> +		 uint16_t *minimum, uint16_t *y_minimum)
> +{
> +	const struct drm_plane_state *pstate;
> +	struct drm_plane *plane;
> +	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> +
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> &cstate->base) {
> +		struct intel_plane *intel_plane =
> to_intel_plane(plane);
> +		int id = skl_wm_plane_id(intel_plane);
> +
> +		if (intel_plane->pipe != pipe ||
> +		    id == PLANE_CURSOR)

You can also remove the check for pipe here.


> +			continue;
> +
> +		if (!pstate->visible)
> +			continue;
> +
> +		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> +		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> +	}
> +
> +	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> +}
> +
>  static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		      struct skl_ddb_allocation *ddb /* out */)
> @@ -3350,12 +3376,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
> -	struct drm_plane *plane;
> -	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;
> +	uint16_t alloc_size, start;
>  	uint16_t minimum[I915_MAX_PLANES] = {};
>  	uint16_t y_minimum[I915_MAX_PLANES] = {};
>  	unsigned int total_data_rate;
> @@ -3384,35 +3407,21 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		return 0;
>  	}
>  
> -	cursor_blocks = skl_cursor_allocation(num_active);
> -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> cursor_blocks;
> -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> -
> -	alloc_size -= cursor_blocks;
> -
> -	/* 1. Allocate the mininum required blocks for each active
> plane */
> -	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 (!pstate->visible)
> -			continue;
> +	skl_ddb_calc_min(cstate, num_active, minimum, y_minimum);
>  
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR)
> -			continue;
> -
> -		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> -		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> -	}
> +	/* 1. Allocate the mininum required blocks for each active
> plane
> +	 * and allocate the cursor, it doesn't require extra
> allocation
> +	 * proportional to the data rate.
> +	 */
>  
> -	for (i = 0; i < PLANE_CURSOR; i++) {
> +	for (i = 0; i < I915_MAX_PLANES; i++) {

As I mentioned earlier, this is also an unsafe loop. I know you didn't
introduce it, so we can fix this in a next patch.

With the pipe check removed (and Matt's requests addressed):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  		alloc_size -= minimum[i];
>  		alloc_size -= y_minimum[i];
>  	}
>  
> +	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> minimum[PLANE_CURSOR];
> +	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> +
>  	/*
>  	 * 2. Distribute the remaining space in proportion to the
> amount of
>  	 * data each plane needs to fetch from memory.
> @@ -3428,6 +3437,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		unsigned rate;
>  		uint16_t plane_blocks, y_plane_blocks = 0;
>  
> +		if (id == PLANE_CURSOR)
> +			continue;
> +
>  		rate = data_rate[id];
>  
>  		/*
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Maarten Lankhorst]

Allow the driver to write watermarks during atomic evasion.
This will make it possible to write the watermarks in a cleaner
way on gen9+.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++----------
 drivers/gpu/drm/i915/intel_pm.c      | 19 +++++++++++++++++--
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f65ccf9b0bea..09588c58148f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -484,6 +484,7 @@ struct sdvo_device_mapping {
 
 struct intel_connector;
 struct intel_encoder;
+struct intel_atomic_state;
 struct intel_crtc_state;
 struct intel_initial_plane_config;
 struct intel_crtc;
@@ -497,8 +498,9 @@ struct drm_i915_display_funcs {
 	int (*compute_intermediate_wm)(struct drm_device *dev,
 				       struct intel_crtc *intel_crtc,
 				       struct intel_crtc_state *newstate);
-	void (*initial_watermarks)(struct intel_crtc_state *cstate);
-	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
+	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
+	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
+	void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
 	int (*compute_global_watermarks)(struct drm_atomic_state *state);
 	void (*update_wm)(struct drm_crtc *crtc);
 	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 55f8ec8c76ae..23d8c72dade3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	 * us to.
 	 */
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(pipe_config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
 	else if (pipe_config->update_wm_pre)
 		intel_update_watermarks(&crtc->base);
 }
@@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_color_load_luts(&pipe_config->base);
 
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(intel_crtc->config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
 	intel_enable_pipe(intel_crtc);
 
 	if (intel_crtc->config->has_pch_encoder)
@@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 		intel_ddi_enable_transcoder_func(crtc);
 
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(pipe_config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
 	else
 		intel_update_watermarks(crtc);
 
@@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_cstate = to_intel_crtc_state(crtc->state);
 
 		if (dev_priv->display.optimize_watermarks)
-			dev_priv->display.optimize_watermarks(intel_cstate);
+			dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
 	}
 
 	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	struct intel_crtc_state *old_intel_state =
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
-	enum pipe pipe = intel_crtc->pipe;
 
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
@@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 
 	if (intel_cstate->update_pipe)
 		intel_update_pipe_config(intel_crtc, old_intel_state);
-	else if (INTEL_GEN(dev_priv) >= 9) {
+	else if (INTEL_GEN(dev_priv) >= 9)
 		skl_detach_scalers(intel_crtc);
 
-		I915_WRITE(PIPE_WM_LINETIME(pipe),
-			   intel_cstate->wm.skl.optimal.linetime);
-	}
+	if (dev_priv->display.atomic_evade_watermarks)
+		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
@@ -16388,7 +16386,7 @@ retry:
 		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
 
 		cs->wm.need_postvbl_update = true;
-		dev_priv->display.optimize_watermarks(cs);
+		dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
 	}
 
 	drm_atomic_state_free(state);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 45fb8275abea..05ccd253fd7a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void skl_evade_crtc_wm(struct intel_atomic_state *state,
+			      struct intel_crtc_state *cstate)
+{
+	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+	enum pipe pipe = crtc->pipe;
+
+	I915_WRITE(PIPE_WM_LINETIME(pipe),
+		   pipe_wm->linetime);
+}
+
 static void skl_update_wm(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 	ilk_write_wm_values(dev_priv, &results);
 }
 
-static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
+static void ilk_initial_watermarks(struct intel_atomic_state *state,
+				   struct intel_crtc_state *cstate)
 {
 	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
-static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
+static void ilk_optimize_watermarks(struct intel_atomic_state *state,
+				    struct intel_crtc_state *cstate)
 {
 	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 9) {
 		skl_setup_wm_latency(dev);
 		dev_priv->display.update_wm = skl_update_wm;
+		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
 		dev_priv->display.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		ilk_setup_wm_latency(dev);
-- 
2.7.4

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

Re: [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:18PM +0200, Maarten Lankhorst wrote:
> Allow the driver to write watermarks during atomic evasion.
> This will make it possible to write the watermarks in a cleaner
> way on gen9+.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++----------
>  drivers/gpu/drm/i915/intel_pm.c      | 19 +++++++++++++++++--
>  3 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f65ccf9b0bea..09588c58148f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -484,6 +484,7 @@ struct sdvo_device_mapping {
>  
>  struct intel_connector;
>  struct intel_encoder;
> +struct intel_atomic_state;
>  struct intel_crtc_state;
>  struct intel_initial_plane_config;
>  struct intel_crtc;
> @@ -497,8 +498,9 @@ struct drm_i915_display_funcs {
>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>  				       struct intel_crtc *intel_crtc,
>  				       struct intel_crtc_state *newstate);
> -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> -	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> +	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> +	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> +	void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);

initial_watermarks() and optimize_watermarks() are currently only used
on ILK (and possibly by in-development VLV/CHV patches that Ville is
working on?).  As far as I can see, the top-level state that we add as a
parameter here doesn't actually get used in the implementations.  Are
you adding it to just make them more similar to the signature of the new
atomic_evade_watermarks vfunc or did you have something else in mind?

I'd also suggest adding a brief comment to your new skl_evade_crtc_wm()
function that indicates that nearly all of the gen9 watermark values are
per-plane values that get written as part of the general plane update in
skylake_update_primary_plane and/or skl_update_plane.  Given that those
two functions are located in other files that may help clarify to future
developers why this function appears so trivial.


Matt

>  	int (*compute_global_watermarks)(struct drm_atomic_state *state);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 55f8ec8c76ae..23d8c72dade3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  	 * us to.
>  	 */
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(pipe_config);
> +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
>  	else if (pipe_config->update_wm_pre)
>  		intel_update_watermarks(&crtc->base);
>  }
> @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
>  	intel_color_load_luts(&pipe_config->base);
>  
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(intel_crtc->config);
> +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  		intel_ddi_enable_transcoder_func(crtc);
>  
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(pipe_config);
> +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
>  	else
>  		intel_update_watermarks(crtc);
>  
> @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_cstate = to_intel_crtc_state(crtc->state);
>  
>  		if (dev_priv->display.optimize_watermarks)
> -			dev_priv->display.optimize_watermarks(intel_cstate);
> +			dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
>  	}
>  
>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
> -	enum pipe pipe = intel_crtc->pipe;
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
> @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  
>  	if (intel_cstate->update_pipe)
>  		intel_update_pipe_config(intel_crtc, old_intel_state);
> -	else if (INTEL_GEN(dev_priv) >= 9) {
> +	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> -		I915_WRITE(PIPE_WM_LINETIME(pipe),
> -			   intel_cstate->wm.skl.optimal.linetime);
> -	}
> +	if (dev_priv->display.atomic_evade_watermarks)
> +		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> @@ -16388,7 +16386,7 @@ retry:
>  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
>  
>  		cs->wm.need_postvbl_update = true;
> -		dev_priv->display.optimize_watermarks(cs);
> +		dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
>  	}
>  
>  	drm_atomic_state_free(state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 45fb8275abea..05ccd253fd7a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> +			      struct intel_crtc_state *cstate)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	enum pipe pipe = crtc->pipe;
> +
> +	I915_WRITE(PIPE_WM_LINETIME(pipe),
> +		   pipe_wm->linetime);
> +}
> +
>  static void skl_update_wm(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  	ilk_write_wm_values(dev_priv, &results);
>  }
>  
> -static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_initial_watermarks(struct intel_atomic_state *state,
> +				   struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  
> -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_optimize_watermarks(struct intel_atomic_state *state,
> +				    struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
>  		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks = skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev)) {
>  		ilk_setup_wm_latency(dev);
> -- 
> 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

Re: [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Matt Roper]

On Wed, Oct 19, 2016 at 04:15:02PM -0700, Matt Roper wrote:
> On Wed, Oct 12, 2016 at 03:28:18PM +0200, Maarten Lankhorst wrote:
> > Allow the driver to write watermarks during atomic evasion.
> > This will make it possible to write the watermarks in a cleaner
> > way on gen9+.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++----------
> >  drivers/gpu/drm/i915/intel_pm.c      | 19 +++++++++++++++++--
> >  3 files changed, 29 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f65ccf9b0bea..09588c58148f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -484,6 +484,7 @@ struct sdvo_device_mapping {
> >  
> >  struct intel_connector;
> >  struct intel_encoder;
> > +struct intel_atomic_state;
> >  struct intel_crtc_state;
> >  struct intel_initial_plane_config;
> >  struct intel_crtc;
> > @@ -497,8 +498,9 @@ struct drm_i915_display_funcs {
> >  	int (*compute_intermediate_wm)(struct drm_device *dev,
> >  				       struct intel_crtc *intel_crtc,
> >  				       struct intel_crtc_state *newstate);
> > -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> > -	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> > +	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> > +	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> > +	void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> 
> initial_watermarks() and optimize_watermarks() are currently only used
> on ILK (and possibly by in-development VLV/CHV patches that Ville is
> working on?).  As far as I can see, the top-level state that we add as a
> parameter here doesn't actually get used in the implementations.  Are
> you adding it to just make them more similar to the signature of the new
> atomic_evade_watermarks vfunc or did you have something else in mind?

For that matter, it doesn't look like it's really used in
atomic_evade_watermarks either except to grab dev_priv (which we can
obtain in other ways).


Matt

> 
> I'd also suggest adding a brief comment to your new skl_evade_crtc_wm()
> function that indicates that nearly all of the gen9 watermark values are
> per-plane values that get written as part of the general plane update in
> skylake_update_primary_plane and/or skl_update_plane.  Given that those
> two functions are located in other files that may help clarify to future
> developers why this function appears so trivial.
> 
> 
> Matt
> 
> >  	int (*compute_global_watermarks)(struct drm_atomic_state *state);
> >  	void (*update_wm)(struct drm_crtc *crtc);
> >  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 55f8ec8c76ae..23d8c72dade3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> >  	 * us to.
> >  	 */
> >  	if (dev_priv->display.initial_watermarks != NULL)
> > -		dev_priv->display.initial_watermarks(pipe_config);
> > +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
> >  	else if (pipe_config->update_wm_pre)
> >  		intel_update_watermarks(&crtc->base);
> >  }
> > @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
> >  	intel_color_load_luts(&pipe_config->base);
> >  
> >  	if (dev_priv->display.initial_watermarks != NULL)
> > -		dev_priv->display.initial_watermarks(intel_crtc->config);
> > +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
> >  	intel_enable_pipe(intel_crtc);
> >  
> >  	if (intel_crtc->config->has_pch_encoder)
> > @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> >  		intel_ddi_enable_transcoder_func(crtc);
> >  
> >  	if (dev_priv->display.initial_watermarks != NULL)
> > -		dev_priv->display.initial_watermarks(pipe_config);
> > +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
> >  	else
> >  		intel_update_watermarks(crtc);
> >  
> > @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  		intel_cstate = to_intel_crtc_state(crtc->state);
> >  
> >  		if (dev_priv->display.optimize_watermarks)
> > -			dev_priv->display.optimize_watermarks(intel_cstate);
> > +			dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
> >  	}
> >  
> >  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> > @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >  	struct intel_crtc_state *old_intel_state =
> >  		to_intel_crtc_state(old_crtc_state);
> >  	bool modeset = needs_modeset(crtc->state);
> > -	enum pipe pipe = intel_crtc->pipe;
> >  
> >  	/* Perform vblank evasion around commit operation */
> >  	intel_pipe_update_start(intel_crtc);
> > @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >  
> >  	if (intel_cstate->update_pipe)
> >  		intel_update_pipe_config(intel_crtc, old_intel_state);
> > -	else if (INTEL_GEN(dev_priv) >= 9) {
> > +	else if (INTEL_GEN(dev_priv) >= 9)
> >  		skl_detach_scalers(intel_crtc);
> >  
> > -		I915_WRITE(PIPE_WM_LINETIME(pipe),
> > -			   intel_cstate->wm.skl.optimal.linetime);
> > -	}
> > +	if (dev_priv->display.atomic_evade_watermarks)
> > +		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
> >  }
> >  
> >  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> > @@ -16388,7 +16386,7 @@ retry:
> >  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
> >  
> >  		cs->wm.need_postvbl_update = true;
> > -		dev_priv->display.optimize_watermarks(cs);
> > +		dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
> >  	}
> >  
> >  	drm_atomic_state_free(state);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 45fb8275abea..05ccd253fd7a 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> > +			      struct intel_crtc_state *cstate)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> > +	enum pipe pipe = crtc->pipe;
> > +
> > +	I915_WRITE(PIPE_WM_LINETIME(pipe),
> > +		   pipe_wm->linetime);
> > +}
> > +
> >  static void skl_update_wm(struct drm_crtc *crtc)
> >  {
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> >  	ilk_write_wm_values(dev_priv, &results);
> >  }
> >  
> > -static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> > +static void ilk_initial_watermarks(struct intel_atomic_state *state,
> > +				   struct intel_crtc_state *cstate)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> > @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> >  	mutex_unlock(&dev_priv->wm.wm_mutex);
> >  }
> >  
> > -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> > +static void ilk_optimize_watermarks(struct intel_atomic_state *state,
> > +				    struct intel_crtc_state *cstate)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> > @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev)
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> >  		skl_setup_wm_latency(dev);
> >  		dev_priv->display.update_wm = skl_update_wm;
> > +		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
> >  		dev_priv->display.compute_global_watermarks = skl_compute_wm;
> >  	} else if (HAS_PCH_SPLIT(dev)) {
> >  		ilk_setup_wm_latency(dev);
> > -- 
> > 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

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

Re: [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Maarten Lankhorst]

Hey,

Op 20-10-16 om 01:15 schreef Matt Roper:
> On Wed, Oct 12, 2016 at 03:28:18PM +0200, Maarten Lankhorst wrote:
>> Allow the driver to write watermarks during atomic evasion.
>> This will make it possible to write the watermarks in a cleaner
>> way on gen9+.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++----------
>>  drivers/gpu/drm/i915/intel_pm.c      | 19 +++++++++++++++++--
>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index f65ccf9b0bea..09588c58148f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -484,6 +484,7 @@ struct sdvo_device_mapping {
>>  
>>  struct intel_connector;
>>  struct intel_encoder;
>> +struct intel_atomic_state;
>>  struct intel_crtc_state;
>>  struct intel_initial_plane_config;
>>  struct intel_crtc;
>> @@ -497,8 +498,9 @@ struct drm_i915_display_funcs {
>>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>>  				       struct intel_crtc *intel_crtc,
>>  				       struct intel_crtc_state *newstate);
>> -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
>> -	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
>> +	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
>> +	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
>> +	void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> initial_watermarks() and optimize_watermarks() are currently only used
> on ILK (and possibly by in-development VLV/CHV patches that Ville is
> working on?).  As far as I can see, the top-level state that we add as a
> parameter here doesn't actually get used in the implementations.  Are
> you adding it to just make them more similar to the signature of the new
> atomic_evade_watermarks vfunc or did you have something else in mind?
>
> I'd also suggest adding a brief comment to your new skl_evade_crtc_wm()
> function that indicates that nearly all of the gen9 watermark values are
> per-plane values that get written as part of the general plane update in
> skylake_update_primary_plane and/or skl_update_plane.  Given that those
> two functions are located in other files that may help clarify to future
> developers why this function appears so trivial.

We don't completely use intel_atomic_state here yet, patch 7 uses it for the
ddb allocation and because initial_watermarks ends up calling
.atomic_evade_watermarks().

I added intel_atomic_state to all callbacks to keep the function signature
identical. It looked better to me to put the function signature in a small
change, and then the behavioral change in patch 7 separately, for easier
bisection.

~Maarten

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

Re: [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Paulo Zanoni]

Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
> Allow the driver to write watermarks during atomic evasion.
> This will make it possible to write the watermarks in a cleaner
> way on gen9+.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++----------
>  drivers/gpu/drm/i915/intel_pm.c      | 19 +++++++++++++++++--
>  3 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index f65ccf9b0bea..09588c58148f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -484,6 +484,7 @@ struct sdvo_device_mapping {
>  
>  struct intel_connector;
>  struct intel_encoder;
> +struct intel_atomic_state;
>  struct intel_crtc_state;
>  struct intel_initial_plane_config;
>  struct intel_crtc;
> @@ -497,8 +498,9 @@ struct drm_i915_display_funcs {
>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>  				       struct intel_crtc
> *intel_crtc,
>  				       struct intel_crtc_state
> *newstate);
> -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> -	void (*optimize_watermarks)(struct intel_crtc_state
> *cstate);
> +	void (*initial_watermarks)(struct intel_atomic_state *state,
> struct intel_crtc_state *cstate);
> +	void (*atomic_evade_watermarks)(struct intel_atomic_state
> *state, struct intel_crtc_state *cstate);
> +	void (*optimize_watermarks)(struct intel_atomic_state
> *state, struct intel_crtc_state *cstate);

Can't we just get intel_atomic_state from intel_crtc_state?  Why pass
both?


>  	int (*compute_global_watermarks)(struct drm_atomic_state
> *state);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 55f8ec8c76ae..23d8c72dade3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  	 * us to.
>  	 */
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(pipe_config);
> +		dev_priv-
> >display.initial_watermarks(to_intel_atomic_state(old_state),
> pipe_config);
>  	else if (pipe_config->update_wm_pre)
>  		intel_update_watermarks(&crtc->base);
>  }
> @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct
> intel_crtc_state *pipe_config,
>  	intel_color_load_luts(&pipe_config->base);
>  
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(intel_crtc-
> >config);
> +		dev_priv-
> >display.initial_watermarks(to_intel_atomic_state(old_state),
> intel_crtc->config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct
> intel_crtc_state *pipe_config,
>  		intel_ddi_enable_transcoder_func(crtc);
>  
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(pipe_config);
> +		dev_priv-
> >display.initial_watermarks(to_intel_atomic_state(old_state),
> pipe_config);
>  	else
>  		intel_update_watermarks(crtc);
>  
> @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  		intel_cstate = to_intel_crtc_state(crtc->state);
>  
>  		if (dev_priv->display.optimize_watermarks)
> -			dev_priv-
> >display.optimize_watermarks(intel_cstate);
> +			dev_priv-
> >display.optimize_watermarks(intel_state, intel_cstate);
>  	}
>  
>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
> -	enum pipe pipe = intel_crtc->pipe;
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
> @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  
>  	if (intel_cstate->update_pipe)
>  		intel_update_pipe_config(intel_crtc,
> old_intel_state);
> -	else if (INTEL_GEN(dev_priv) >= 9) {
> +	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> -		I915_WRITE(PIPE_WM_LINETIME(pipe),
> -			   intel_cstate->wm.skl.optimal.linetime);
> -	}
> +	if (dev_priv->display.atomic_evade_watermarks)
> +		dev_priv-
> >display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state
> ->state), intel_cstate);

This will now also update PIPE_WM_LINETIME even if intel_cstate-
>update_pipe is true. Is this intentional? Is it a bug fix? If yes for
any of those questions, it probably needs some good explanation in the
commit message or maybe a separate patch with the explicit behavior
change and explanation.


>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> @@ -16388,7 +16386,7 @@ retry:
>  		struct intel_crtc_state *cs =
> to_intel_crtc_state(cstate);
>  
>  		cs->wm.need_postvbl_update = true;
> -		dev_priv->display.optimize_watermarks(cs);
> +		dev_priv-
> >display.optimize_watermarks(to_intel_atomic_state(state), cs);
>  	}
>  
>  	drm_atomic_state_free(state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 45fb8275abea..05ccd253fd7a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> +			      struct intel_crtc_state *cstate)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(state-
> >base.dev);
> +	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	enum pipe pipe = crtc->pipe;
> +
> +	I915_WRITE(PIPE_WM_LINETIME(pipe),
> +		   pipe_wm->linetime);
> +}
> +
>  static void skl_update_wm(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct
> drm_i915_private *dev_priv)
>  	ilk_write_wm_values(dev_priv, &results);
>  }
>  
> -static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_initial_watermarks(struct intel_atomic_state *state,
> +				   struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(cstate-
> >base.crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> >base.crtc);
> @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct
> intel_crtc_state *cstate)
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  
> -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_optimize_watermarks(struct intel_atomic_state
> *state,
> +				    struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(cstate-
> >base.crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> >base.crtc);
> @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
>  		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.atomic_evade_watermarks =
> skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks =
> skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev)) {
>  		ilk_setup_wm_latency(dev);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915: Add a atomic evasion step to watermark programming.

[Maarten Lankhorst]

Op 20-10-16 om 19:51 schreef Paulo Zanoni:
> Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
>> Allow the driver to write watermarks during atomic evasion.
>> This will make it possible to write the watermarks in a cleaner
>> way on gen9+.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++----------
>>  drivers/gpu/drm/i915/intel_pm.c      | 19 +++++++++++++++++--
>>  3 files changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index f65ccf9b0bea..09588c58148f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -484,6 +484,7 @@ struct sdvo_device_mapping {
>>  
>>  struct intel_connector;
>>  struct intel_encoder;
>> +struct intel_atomic_state;
>>  struct intel_crtc_state;
>>  struct intel_initial_plane_config;
>>  struct intel_crtc;
>> @@ -497,8 +498,9 @@ struct drm_i915_display_funcs {
>>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>>  				       struct intel_crtc
>> *intel_crtc,
>>  				       struct intel_crtc_state
>> *newstate);
>> -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
>> -	void (*optimize_watermarks)(struct intel_crtc_state
>> *cstate);
>> +	void (*initial_watermarks)(struct intel_atomic_state *state,
>> struct intel_crtc_state *cstate);
>> +	void (*atomic_evade_watermarks)(struct intel_atomic_state
>> *state, struct intel_crtc_state *cstate);
>> +	void (*optimize_watermarks)(struct intel_atomic_state
>> *state, struct intel_crtc_state *cstate);
> Can't we just get intel_atomic_state from intel_crtc_state?  Why pass
> both?
No we can't. After the swap we can only obtain it from old_crtc_state, not the new state passed here.
>
>>  	int (*compute_global_watermarks)(struct drm_atomic_state
>> *state);
>>  	void (*update_wm)(struct drm_crtc *crtc);
>>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 55f8ec8c76ae..23d8c72dade3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>>  	 * us to.
>>  	 */
>>  	if (dev_priv->display.initial_watermarks != NULL)
>> -		dev_priv->display.initial_watermarks(pipe_config);
>> +		dev_priv-
>>> display.initial_watermarks(to_intel_atomic_state(old_state),
>> pipe_config);
>>  	else if (pipe_config->update_wm_pre)
>>  		intel_update_watermarks(&crtc->base);
>>  }
>> @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct
>> intel_crtc_state *pipe_config,
>>  	intel_color_load_luts(&pipe_config->base);
>>  
>>  	if (dev_priv->display.initial_watermarks != NULL)
>> -		dev_priv->display.initial_watermarks(intel_crtc-
>>> config);
>> +		dev_priv-
>>> display.initial_watermarks(to_intel_atomic_state(old_state),
>> intel_crtc->config);
>>  	intel_enable_pipe(intel_crtc);
>>  
>>  	if (intel_crtc->config->has_pch_encoder)
>> @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct
>> intel_crtc_state *pipe_config,
>>  		intel_ddi_enable_transcoder_func(crtc);
>>  
>>  	if (dev_priv->display.initial_watermarks != NULL)
>> -		dev_priv->display.initial_watermarks(pipe_config);
>> +		dev_priv-
>>> display.initial_watermarks(to_intel_atomic_state(old_state),
>> pipe_config);
>>  	else
>>  		intel_update_watermarks(crtc);
>>  
>> @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct
>> drm_atomic_state *state)
>>  		intel_cstate = to_intel_crtc_state(crtc->state);
>>  
>>  		if (dev_priv->display.optimize_watermarks)
>> -			dev_priv-
>>> display.optimize_watermarks(intel_cstate);
>> +			dev_priv-
>>> display.optimize_watermarks(intel_state, intel_cstate);
>>  	}
>>  
>>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct
>> drm_crtc *crtc,
>>  	struct intel_crtc_state *old_intel_state =
>>  		to_intel_crtc_state(old_crtc_state);
>>  	bool modeset = needs_modeset(crtc->state);
>> -	enum pipe pipe = intel_crtc->pipe;
>>  
>>  	/* Perform vblank evasion around commit operation */
>>  	intel_pipe_update_start(intel_crtc);
>> @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct
>> drm_crtc *crtc,
>>  
>>  	if (intel_cstate->update_pipe)
>>  		intel_update_pipe_config(intel_crtc,
>> old_intel_state);
>> -	else if (INTEL_GEN(dev_priv) >= 9) {
>> +	else if (INTEL_GEN(dev_priv) >= 9)
>>  		skl_detach_scalers(intel_crtc);
>>  
>> -		I915_WRITE(PIPE_WM_LINETIME(pipe),
>> -			   intel_cstate->wm.skl.optimal.linetime);
>> -	}
>> +	if (dev_priv->display.atomic_evade_watermarks)
>> +		dev_priv-
>>> display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state
>> ->state), intel_cstate);
> This will now also update PIPE_WM_LINETIME even if intel_cstate-
>> update_pipe is true. Is this intentional? Is it a bug fix? If yes for
> any of those questions, it probably needs some good explanation in the
> commit message or maybe a separate patch with the explicit behavior
> change and explanation.
This was intentional, I'm not sure when linetime needs to be updated. It seems that during a full modeset we don't update it either,
only on the first plane update after. I suspect this might be a bugfix, but not completely sure what the impact is..
>>  }
>>  
>>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>> @@ -16388,7 +16386,7 @@ retry:
>>  		struct intel_crtc_state *cs =
>> to_intel_crtc_state(cstate);
>>  
>>  		cs->wm.need_postvbl_update = true;
>> -		dev_priv->display.optimize_watermarks(cs);
>> +		dev_priv-
>>> display.optimize_watermarks(to_intel_atomic_state(state), cs);
>>  	}
>>  
>>  	drm_atomic_state_free(state);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 45fb8275abea..05ccd253fd7a 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state)
>>  	return 0;
>>  }
>>  
>> +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
>> +			      struct intel_crtc_state *cstate)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>> +	struct drm_i915_private *dev_priv = to_i915(state-
>>> base.dev);
>> +	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>> +	enum pipe pipe = crtc->pipe;
>> +
>> +	I915_WRITE(PIPE_WM_LINETIME(pipe),
>> +		   pipe_wm->linetime);
>> +}
>> +
>>  static void skl_update_wm(struct drm_crtc *crtc)
>>  {
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct
>> drm_i915_private *dev_priv)
>>  	ilk_write_wm_values(dev_priv, &results);
>>  }
>>  
>> -static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
>> +static void ilk_initial_watermarks(struct intel_atomic_state *state,
>> +				   struct intel_crtc_state *cstate)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(cstate-
>>> base.crtc->dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
>>> base.crtc);
>> @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct
>> intel_crtc_state *cstate)
>>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>>  }
>>  
>> -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
>> +static void ilk_optimize_watermarks(struct intel_atomic_state
>> *state,
>> +				    struct intel_crtc_state *cstate)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(cstate-
>>> base.crtc->dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
>>> base.crtc);
>> @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev)
>>  	if (INTEL_INFO(dev)->gen >= 9) {
>>  		skl_setup_wm_latency(dev);
>>  		dev_priv->display.update_wm = skl_update_wm;
>> +		dev_priv->display.atomic_evade_watermarks =
>> skl_evade_crtc_wm;
>>  		dev_priv->display.compute_global_watermarks =
>> skl_compute_wm;
>>  	} else if (HAS_PCH_SPLIT(dev)) {
>>  		ilk_setup_wm_latency(dev);


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

[PATCH 6/8] drm/i915/gen9+: Use the watermarks from crtc_state for everything.

[Maarten Lankhorst]

There's no need to keep a duplicate skl_pipe_wm around any more,
everything can be discovered from crtc_state, which we pass around
correctly now even in case of plane disable.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_pm.c      | 11 +++++------
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 23d8c72dade3..340861826c46 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13455,7 +13455,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
 		return;
 
 	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
-	sw_wm = &intel_crtc->wm.active.skl;
+	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
 
 	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
 	sw_ddb = &dev_priv->wm.skl_hw.ddb;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a176e6cebab3..9f04e26c4365 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -723,7 +723,6 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		union {
 			struct intel_pipe_wm ilk;
-			struct skl_pipe_wm skl;
 		} active;
 
 		/* allow CxSR on this pipe */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 05ccd253fd7a..be3dd8cdc7ae 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3904,9 +3904,9 @@ bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 			      struct skl_ddb_allocation *ddb, /* out */
 			      struct skl_pipe_wm *pipe_wm, /* out */
+			      const struct skl_pipe_wm *old_pipe_wm,
 			      bool *changed /* out */)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
 	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
 	int ret;
 
@@ -3914,7 +3914,7 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 	if (ret)
 		return ret;
 
-	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
+	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
 		*changed = false;
 	else
 		*changed = true;
@@ -4155,10 +4155,12 @@ skl_compute_wm(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, cstate, i) {
 		struct intel_crtc_state *intel_cstate =
 			to_intel_crtc_state(cstate);
+		const struct skl_pipe_wm *old_pipe_wm =
+			&to_intel_crtc_state(crtc->state)->wm.skl.optimal;
 
 		pipe_wm = &intel_cstate->wm.skl.optimal;
 		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
-					 &changed);
+					 old_pipe_wm, &changed);
 		if (ret)
 			return ret;
 
@@ -4203,8 +4205,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
 
-	intel_crtc->wm.active.skl = *pipe_wm;
-
 	mutex_lock(&dev_priv->wm.wm_mutex);
 
 	/*
@@ -4371,7 +4371,6 @@ void skl_wm_get_hw_state(struct drm_device *dev)
 		cstate = to_intel_crtc_state(crtc->state);
 
 		skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal);
-		intel_crtc->wm.active.skl = cstate->wm.skl.optimal;
 
 		if (!intel_crtc->active)
 			hw->dirty_pipes |= drm_crtc_mask(crtc);
-- 
2.7.4

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

Re: [PATCH 6/8] drm/i915/gen9+: Use the watermarks from crtc_state for everything.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:19PM +0200, Maarten Lankhorst wrote:
> There's no need to keep a duplicate skl_pipe_wm around any more,
> everything can be discovered from crtc_state, which we pass around
> correctly now even in case of plane disable.

You might want to add some clarification that
intel(crtc->state)->wm.skl.optimal and intel_crtc->wm.active always hold
the same value by the time we finally drop our CRTC locks, so there's no
need for the duplication on gen9.  The reason we have
intel_crtc->wm.active in general is because the two-step platforms
(ILK-style, VLV-style) need cross-CRTC information during watermark
updates (which are potentially racing if multiple CRTC's are updated
independently but simultaneously) and the intel_crtc data is protected
by a separate wm_mutex.  Watermark calculations triggered by one CRTC's
update need to know which values are actually active on the hardware
(old, intermediate, or final) while another CRTC update is still being
processed.  The types of CRTC updates that are allowed to race on gen9
don't have the same kind of inter-CRTC data dependency.

I think this patch needs some slight rebasing to apply cleanly on top of
Lyude's changes that landed yesterday, but if you expand the commit
message justification a bit,

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


Matt

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_pm.c      | 11 +++++------
>  3 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 23d8c72dade3..340861826c46 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13455,7 +13455,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
>  		return;
>  
>  	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
> -	sw_wm = &intel_crtc->wm.active.skl;
> +	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
>  
>  	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
>  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a176e6cebab3..9f04e26c4365 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -723,7 +723,6 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		union {
>  			struct intel_pipe_wm ilk;
> -			struct skl_pipe_wm skl;
>  		} active;
>  
>  		/* allow CxSR on this pipe */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 05ccd253fd7a..be3dd8cdc7ae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3904,9 +3904,9 @@ bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  			      struct skl_ddb_allocation *ddb, /* out */
>  			      struct skl_pipe_wm *pipe_wm, /* out */
> +			      const struct skl_pipe_wm *old_pipe_wm,
>  			      bool *changed /* out */)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
>  	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
>  	int ret;
>  
> @@ -3914,7 +3914,7 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  	if (ret)
>  		return ret;
>  
> -	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> +	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
>  		*changed = false;
>  	else
>  		*changed = true;
> @@ -4155,10 +4155,12 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *intel_cstate =
>  			to_intel_crtc_state(cstate);
> +		const struct skl_pipe_wm *old_pipe_wm =
> +			&to_intel_crtc_state(crtc->state)->wm.skl.optimal;
>  
>  		pipe_wm = &intel_cstate->wm.skl.optimal;
>  		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
> -					 &changed);
> +					 old_pipe_wm, &changed);
>  		if (ret)
>  			return ret;
>  
> @@ -4203,8 +4205,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
>  
> -	intel_crtc->wm.active.skl = *pipe_wm;
> -
>  	mutex_lock(&dev_priv->wm.wm_mutex);
>  
>  	/*
> @@ -4371,7 +4371,6 @@ void skl_wm_get_hw_state(struct drm_device *dev)
>  		cstate = to_intel_crtc_state(crtc->state);
>  
>  		skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal);
> -		intel_crtc->wm.active.skl = cstate->wm.skl.optimal;
>  
>  		if (!intel_crtc->active)
>  			hw->dirty_pipes |= drm_crtc_mask(crtc);
> -- 
> 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

Re: [PATCH 6/8] drm/i915/gen9+: Use the watermarks from crtc_state for everything.

[Paulo Zanoni]

Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
> There's no need to keep a duplicate skl_pipe_wm around any more,
> everything can be discovered from crtc_state, which we pass around
> correctly now even in case of plane disable.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_pm.c      | 11 +++++------
>  3 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 23d8c72dade3..340861826c46 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13455,7 +13455,7 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  		return;
>  
>  	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
> -	sw_wm = &intel_crtc->wm.active.skl;
> +	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
>  
>  	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
>  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a176e6cebab3..9f04e26c4365 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -723,7 +723,6 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		union {
>  			struct intel_pipe_wm ilk;
> -			struct skl_pipe_wm skl;
>  		} active;
>  
>  		/* allow CxSR on this pipe */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 05ccd253fd7a..be3dd8cdc7ae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3904,9 +3904,9 @@ bool skl_ddb_allocation_overlaps(struct
> drm_atomic_state *state,
>  static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  			      struct skl_ddb_allocation *ddb, /* out
> */
>  			      struct skl_pipe_wm *pipe_wm, /* out */
> +			      const struct skl_pipe_wm *old_pipe_wm,
>  			      bool *changed /* out */)

Bikeshed: this patch adds an "in" parameter in the middle of the "out"
parameters. That's kinda ugly IMHO.

With that maybe fixed:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
>  	struct intel_crtc_state *intel_cstate =
> to_intel_crtc_state(cstate);
>  	int ret;
>  
> @@ -3914,7 +3914,7 @@ static int skl_update_pipe_wm(struct
> drm_crtc_state *cstate,
>  	if (ret)
>  		return ret;
>  
> -	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm,
> sizeof(*pipe_wm)))
> +	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
>  		*changed = false;
>  	else
>  		*changed = true;
> @@ -4155,10 +4155,12 @@ skl_compute_wm(struct drm_atomic_state
> *state)
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *intel_cstate =
>  			to_intel_crtc_state(cstate);
> +		const struct skl_pipe_wm *old_pipe_wm =
> +			&to_intel_crtc_state(crtc->state)-
> >wm.skl.optimal;
>  
>  		pipe_wm = &intel_cstate->wm.skl.optimal;
>  		ret = skl_update_pipe_wm(cstate, &results->ddb,
> pipe_wm,
> -					 &changed);
> +					 old_pipe_wm, &changed);
>  		if (ret)
>  			return ret;
>  
> @@ -4203,8 +4205,6 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
>  
> -	intel_crtc->wm.active.skl = *pipe_wm;
> -
>  	mutex_lock(&dev_priv->wm.wm_mutex);
>  
>  	/*
> @@ -4371,7 +4371,6 @@ void skl_wm_get_hw_state(struct drm_device
> *dev)
>  		cstate = to_intel_crtc_state(crtc->state);
>  
>  		skl_pipe_wm_get_hw_state(crtc, &cstate-
> >wm.skl.optimal);
> -		intel_crtc->wm.active.skl = cstate->wm.skl.optimal;
>  
>  		if (!intel_crtc->active)
>  			hw->dirty_pipes |= drm_crtc_mask(crtc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Maarten Lankhorst]

Instead of running the watermark updates from the callbacks run
them from a separate hook atomic_evade_watermarks.

This also gets rid of the global skl_results, which was required for
keeping track of the current atomic commit.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  7 -------
 drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  7 -------
 drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
 5 files changed, 28 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 09588c58148f..28e44cb611b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2027,13 +2027,6 @@ struct drm_i915_private {
 		 */
 		uint16_t skl_latency[8];
 
-		/*
-		 * The skl_wm_values structure is a bit too big for stack
-		 * allocation, so we keep the staging struct where we store
-		 * intermediate results here instead.
-		 */
-		struct skl_wm_values skl_results;
-
 		/* current hardware state */
 		union {
 			struct ilk_wm_values hw;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 340861826c46..d3d7d9dc14a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3377,9 +3377,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	const struct skl_plane_wm *p_wm =
-		&crtc_state->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl;
 	unsigned int rotation = plane_state->base.rotation;
@@ -3415,9 +3412,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = src_x;
 	intel_crtc->adjusted_y = src_y;
 
-	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
-		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
-
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
 	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
@@ -3450,18 +3444,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 
-	/*
-	 * We only populate skl_results on watermark updates, and if the
-	 * plane's visiblity isn't actually changing neither is its watermarks.
-	 */
-	if (!crtc->primary->state->visible)
-		skl_write_plane_wm(intel_crtc, p_wm,
-				   &dev_priv->wm.skl_results.ddb, 0);
-
 	I915_WRITE(PLANE_CTL(pipe, 0), 0);
 	I915_WRITE(PLANE_SURF(pipe, 0), 0);
 	POSTING_READ(PLANE_SURF(pipe, 0));
@@ -10824,16 +10808,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	const struct skl_plane_wm *p_wm =
-		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
-	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
-
 	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
@@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 			intel_check_cpu_fifo_underruns(dev_priv);
 			intel_check_pch_fifo_underruns(dev_priv);
 
-			if (!crtc->state->active)
-				intel_update_watermarks(crtc);
+			if (!crtc->state->active) {
+				if (dev_priv->display.initial_watermarks)
+					dev_priv->display.initial_watermarks(intel_state,
+									     to_intel_crtc_state(crtc->state));
+				else
+					intel_update_watermarks(crtc);
+			}
 		}
 	}
 
@@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_swap_state(state, true);
 	dev_priv->wm.distrust_bios_wm = false;
-	dev_priv->wm.skl_results = intel_state->wm_results;
 	intel_shared_dpll_commit(state);
 	intel_atomic_track_fbs(state);
 
@@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	intel_pipe_update_start(intel_crtc);
 
 	if (modeset)
-		return;
+		goto out;
 
 	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
 		intel_color_set_csc(crtc->state);
@@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	else if (INTEL_GEN(dev_priv) >= 9)
 		skl_detach_scalers(intel_crtc);
 
+out:
 	if (dev_priv->display.atomic_evade_watermarks)
 		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9f04e26c4365..17cf1ee83bfb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
 			       enum pipe pipe);
 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
 				 struct intel_crtc *intel_crtc);
-void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
-			 const struct skl_plane_wm *wm,
-			 const struct skl_ddb_allocation *ddb);
-void skl_write_plane_wm(struct intel_crtc *intel_crtc,
-			const struct skl_plane_wm *wm,
-			const struct skl_ddb_allocation *ddb,
-			int plane);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index be3dd8cdc7ae..18c62d1eea19 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state *state)
 	return 0;
 }
 
-static void skl_evade_crtc_wm(struct intel_atomic_state *state,
-			      struct intel_crtc_state *cstate)
+static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct intel_crtc_state *cstate)
 {
 	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
 	enum pipe pipe = crtc->pipe;
+	int plane;
+
+	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
+		return;
 
-	I915_WRITE(PIPE_WM_LINETIME(pipe),
-		   pipe_wm->linetime);
+	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+
+	for (plane = 0; plane < intel_num_planes(crtc); plane++)
+		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
+
+	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
 }
 
-static void skl_update_wm(struct drm_crtc *crtc)
+static void skl_initial_wm(struct intel_atomic_state *state,
+			   struct intel_crtc_state *cstate)
 {
+	struct drm_crtc *crtc = cstate->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct skl_wm_values *results = &dev_priv->wm.skl_results;
+	struct skl_wm_values *results = &state->wm_results;
 	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
 	enum pipe pipe = intel_crtc->pipe;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
@@ -4213,16 +4221,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	 * the pipe's shut off, just do so here. Already active pipes will have
 	 * their watermarks updated once we update their planes.
 	 */
-	if (crtc->state->active_changed) {
-		int plane;
-
-		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
-			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
-					   &results->ddb, plane);
-
-		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
-				    &results->ddb);
-	}
+	if (cstate->base.active_changed)
+		skl_evade_crtc_wm(state, cstate);
 
 	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
@@ -7727,7 +7727,7 @@ void intel_init_pm(struct drm_device *dev)
 	/* For FIFO watermark updates */
 	if (INTEL_INFO(dev)->gen >= 9) {
 		skl_setup_wm_latency(dev);
-		dev_priv->display.update_wm = skl_update_wm;
+		dev_priv->display.initial_watermarks = skl_initial_wm;
 		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
 		dev_priv->display.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0fb775b4c93e..366900dcde34 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	struct drm_crtc *crtc = crtc_state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
-	const struct skl_plane_wm *p_wm =
-		&crtc_state->wm.skl.optimal.planes[plane];
 	u32 plane_ctl;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 surf_addr = plane_state->main.offset;
@@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
 
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
-	if (wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
-
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
@@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 
-	/*
-	 * We only populate skl_results on watermark updates, and if the
-	 * plane's visiblity isn't actually changing neither is its watermarks.
-	 */
-	if (!dplane->state->visible)
-		skl_write_plane_wm(to_intel_crtc(crtc),
-				   &cstate->wm.skl.optimal.planes[plane],
-				   &dev_priv->wm.skl_results.ddb, plane);
-
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
 	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
2.7.4

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

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Lyude]

Loving this patch so far! Would it be possible to get this split into
two separate patches though? One for removing skl_results and one for
programming watermarks as a separate step.

On Wed, 2016-10-12 at 15:28 +0200, Maarten Lankhorst wrote:
> Instead of running the watermark updates from the callbacks run
> them from a separate hook atomic_evade_watermarks.
> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-----------------
> --------
>  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------
> ------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
>  5 files changed, 28 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 09588c58148f..28e44cb611b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2027,13 +2027,6 @@ struct drm_i915_private {
>  		 */
>  		uint16_t skl_latency[8];
>  
> -		/*
> -		 * The skl_wm_values structure is a bit too big for
> stack
> -		 * allocation, so we keep the staging struct where
> we store
> -		 * intermediate results here instead.
> -		 */
> -		struct skl_wm_values skl_results;
> -
>  		/* current hardware state */
>  		union {
>  			struct ilk_wm_values hw;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 340861826c46..d3d7d9dc14a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3377,9 +3377,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> >base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3415,9 +3412,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	intel_crtc->adjusted_x = src_x;
>  	intel_crtc->adjusted_y = src_y;
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -3450,18 +3444,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_plane_wm *p_wm = &cstate-
> >wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if
> the
> -	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> -	 */
> -	if (!crtc->primary->state->visible)
> -		skl_write_plane_wm(intel_crtc, p_wm,
> -				   &dev_priv->wm.skl_results.ddb,
> 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
>  	POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -10824,16 +10808,9 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> -
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> @@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  			intel_check_cpu_fifo_underruns(dev_priv);
>  			intel_check_pch_fifo_underruns(dev_priv);
>  
> -			if (!crtc->state->active)
> -				intel_update_watermarks(crtc);
> +			if (!crtc->state->active) {
> +				if (dev_priv-
> >display.initial_watermarks)
> +					dev_priv-
> >display.initial_watermarks(intel_state,
> +									
>      to_intel_crtc_state(crtc->state));
> +				else
> +					intel_update_watermarks(crtc
> );
> +			}
>  		}
>  	}
>  
> @@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(state, true);
>  	dev_priv->wm.distrust_bios_wm = false;
> -	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
>  	intel_atomic_track_fbs(state);
>  
> @@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	intel_pipe_update_start(intel_crtc);
>  
>  	if (modeset)
> -		return;
> +		goto out;
>  
>  	if (crtc->state->color_mgmt_changed ||
> to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
> @@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> +out:
>  	if (dev_priv->display.atomic_evade_watermarks)
>  		dev_priv-
> >display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state
> ->state), intel_cstate);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 9f04e26c4365..17cf1ee83bfb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct
> skl_ddb_allocation *old,
>  			       enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_plane_wm *wm,
> -			const struct skl_ddb_allocation *ddb,
> -			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index be3dd8cdc7ae..18c62d1eea19 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state
> *state)
>  	return 0;
>  }
>  
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> -			      struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(state-
> >base.dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	const struct skl_ddb_allocation *ddb = &state-
> >wm_results.ddb;
>  	enum pipe pipe = crtc->pipe;
> +	int plane;
> +
> +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc-
> >base)))
> +		return;
>  
> -	I915_WRITE(PIPE_WM_LINETIME(pipe),
> -		   pipe_wm->linetime);
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +
> +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
> +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane],
> ddb, plane);
> +
> +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR],
> ddb);
>  }
>  
> -static void skl_update_wm(struct drm_crtc *crtc)
> +static void skl_initial_wm(struct intel_atomic_state *state,
> +			   struct intel_crtc_state *cstate)
>  {
> +	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> +	struct skl_wm_values *results = &state->wm_results;
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> @@ -4213,16 +4221,8 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  	 * the pipe's shut off, just do so here. Already active
> pipes will have
>  	 * their watermarks updated once we update their planes.
>  	 */
> -	if (crtc->state->active_changed) {
> -		int plane;
> -
> -		for (plane = 0; plane <
> intel_num_planes(intel_crtc); plane++)
> -			skl_write_plane_wm(intel_crtc, &pipe_wm-
> >planes[plane],
> -					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> >planes[PLANE_CURSOR],
> -				    &results->ddb);
> -	}
> +	if (cstate->base.active_changed)
> +		skl_evade_crtc_wm(state, cstate);
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> @@ -7727,7 +7727,7 @@ void intel_init_pm(struct drm_device *dev)
>  	/* For FIFO watermark updates */
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
> -		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.initial_watermarks =
> skl_initial_wm;
>  		dev_priv->display.atomic_evade_watermarks =
> skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks =
> skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 0fb775b4c93e..366900dcde34 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	struct drm_crtc *crtc = crtc_state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[plane];
>  	u32 plane_ctl;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state-
> >ckey;
>  	u32 surf_addr = plane_state->main.offset;
> @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> plane);
> -
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key-
> >min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key-
> >max_value);
> @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if
> the
> -	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> -	 */
> -	if (!dplane->state->visible)
> -		skl_write_plane_wm(to_intel_crtc(crtc),
> -				   &cstate-
> >wm.skl.optimal.planes[plane],
> -				   &dev_priv->wm.skl_results.ddb,
> plane);
> -
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Lyude]

Loving this patch so far! Would it be possible to get this split into
two separate patches though? One for removing skl_results and one for
programming watermarks as a separate step.

On Wed, 2016-10-12 at 15:28 +0200, Maarten Lankhorst wrote:
> Instead of running the watermark updates from the callbacks run
> them from a separate hook atomic_evade_watermarks.
> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-----------------
> --------
>  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------
> ------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
>  5 files changed, 28 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 09588c58148f..28e44cb611b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2027,13 +2027,6 @@ struct drm_i915_private {
>  		 */
>  		uint16_t skl_latency[8];
>  
> -		/*
> -		 * The skl_wm_values structure is a bit too big for
> stack
> -		 * allocation, so we keep the staging struct where
> we store
> -		 * intermediate results here instead.
> -		 */
> -		struct skl_wm_values skl_results;
> -
>  		/* current hardware state */
>  		union {
>  			struct ilk_wm_values hw;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 340861826c46..d3d7d9dc14a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3377,9 +3377,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> >base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3415,9 +3412,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	intel_crtc->adjusted_x = src_x;
>  	intel_crtc->adjusted_y = src_y;
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -3450,18 +3444,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_plane_wm *p_wm = &cstate-
> >wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if
> the
> -	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> -	 */
> -	if (!crtc->primary->state->visible)
> -		skl_write_plane_wm(intel_crtc, p_wm,
> -				   &dev_priv->wm.skl_results.ddb,
> 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
>  	POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -10824,16 +10808,9 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> -
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> @@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  			intel_check_cpu_fifo_underruns(dev_priv);
>  			intel_check_pch_fifo_underruns(dev_priv);
>  
> -			if (!crtc->state->active)
> -				intel_update_watermarks(crtc);
> +			if (!crtc->state->active) {
> +				if (dev_priv-
> >display.initial_watermarks)
> +					dev_priv-
> >display.initial_watermarks(intel_state,
> +									
>      to_intel_crtc_state(crtc->state));
> +				else
> +					intel_update_watermarks(crtc
> );
> +			}
>  		}
>  	}
>  
> @@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(state, true);
>  	dev_priv->wm.distrust_bios_wm = false;
> -	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
>  	intel_atomic_track_fbs(state);
>  
> @@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	intel_pipe_update_start(intel_crtc);
>  
>  	if (modeset)
> -		return;
> +		goto out;
>  
>  	if (crtc->state->color_mgmt_changed ||
> to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
> @@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> +out:
>  	if (dev_priv->display.atomic_evade_watermarks)
>  		dev_priv-
> >display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state
> ->state), intel_cstate);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 9f04e26c4365..17cf1ee83bfb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct
> skl_ddb_allocation *old,
>  			       enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_plane_wm *wm,
> -			const struct skl_ddb_allocation *ddb,
> -			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index be3dd8cdc7ae..18c62d1eea19 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state
> *state)
>  	return 0;
>  }
>  
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> -			      struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(state-
> >base.dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	const struct skl_ddb_allocation *ddb = &state-
> >wm_results.ddb;
>  	enum pipe pipe = crtc->pipe;
> +	int plane;
> +
> +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc-
> >base)))
> +		return;
>  
> -	I915_WRITE(PIPE_WM_LINETIME(pipe),
> -		   pipe_wm->linetime);
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +
> +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
> +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane],
> ddb, plane);
> +
> +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR],
> ddb);
>  }
>  
> -static void skl_update_wm(struct drm_crtc *crtc)
> +static void skl_initial_wm(struct intel_atomic_state *state,
> +			   struct intel_crtc_state *cstate)
>  {
> +	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> +	struct skl_wm_values *results = &state->wm_results;
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> @@ -4213,16 +4221,8 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  	 * the pipe's shut off, just do so here. Already active
> pipes will have
>  	 * their watermarks updated once we update their planes.
>  	 */
> -	if (crtc->state->active_changed) {
> -		int plane;
> -
> -		for (plane = 0; plane <
> intel_num_planes(intel_crtc); plane++)
> -			skl_write_plane_wm(intel_crtc, &pipe_wm-
> >planes[plane],
> -					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> >planes[PLANE_CURSOR],
> -				    &results->ddb);
> -	}
> +	if (cstate->base.active_changed)
> +		skl_evade_crtc_wm(state, cstate);
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> @@ -7727,7 +7727,7 @@ void intel_init_pm(struct drm_device *dev)
>  	/* For FIFO watermark updates */
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
> -		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.initial_watermarks =
> skl_initial_wm;
>  		dev_priv->display.atomic_evade_watermarks =
> skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks =
> skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 0fb775b4c93e..366900dcde34 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	struct drm_crtc *crtc = crtc_state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[plane];
>  	u32 plane_ctl;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state-
> >ckey;
>  	u32 surf_addr = plane_state->main.offset;
> @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> plane);
> -
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key-
> >min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key-
> >max_value);
> @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if
> the
> -	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> -	 */
> -	if (!dplane->state->visible)
> -		skl_write_plane_wm(to_intel_crtc(crtc),
> -				   &cstate-
> >wm.skl.optimal.planes[plane],
> -				   &dev_priv->wm.skl_results.ddb,
> plane);
> -
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Lyude]

Accidentally sent original view twice and found one more issue after
looking at the rest of them, sorry about that!

On Wed, 2016-10-12 at 13:04 -0400, Lyude wrote:
> Loving this patch so far! Would it be possible to get this split into
> two separate patches though? One for removing skl_results and one for
> programming watermarks as a separate step.
> 
> On Wed, 2016-10-12 at 15:28 +0200, Maarten Lankhorst wrote:
> > 
> > Instead of running the watermark updates from the callbacks run
> > them from a separate hook atomic_evade_watermarks.
> > 
> > This also gets rid of the global skl_results, which was required
> > for
> > keeping track of the current atomic commit.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> > >
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
> >  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++---------------
> > --
> > --------
> >  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
> >  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------
> > ------------
> >  drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
> >  5 files changed, 28 insertions(+), 78 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 09588c58148f..28e44cb611b8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2027,13 +2027,6 @@ struct drm_i915_private {
> >  		 */
> >  		uint16_t skl_latency[8];
> >  
> > -		/*
> > -		 * The skl_wm_values structure is a bit too big
> > for
> > stack
> > -		 * allocation, so we keep the staging struct where
> > we store
> > -		 * intermediate results here instead.
> > -		 */
> > -		struct skl_wm_values skl_results;
> > -
> >  		/* current hardware state */
> >  		union {
> >  			struct ilk_wm_values hw;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 340861826c46..d3d7d9dc14a8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3377,9 +3377,6 @@ static void
> > skylake_update_primary_plane(struct
> > drm_plane *plane,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> > > 
> > > base.crtc);
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > -	const struct skl_wm_values *wm = &dev_priv-
> > >wm.skl_results;
> > -	const struct skl_plane_wm *p_wm =
> > -		&crtc_state->wm.skl.optimal.planes[0];
> >  	int pipe = intel_crtc->pipe;
> >  	u32 plane_ctl;
> >  	unsigned int rotation = plane_state->base.rotation;
> > @@ -3415,9 +3412,6 @@ static void
> > skylake_update_primary_plane(struct
> > drm_plane *plane,
> >  	intel_crtc->adjusted_x = src_x;
> >  	intel_crtc->adjusted_y = src_y;
> >  
> > -	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> > -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> > -
> >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> >  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> > @@ -3450,18 +3444,8 @@ static void
> > skylake_disable_primary_plane(struct drm_plane *primary,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_crtc_state *cstate =
> > to_intel_crtc_state(crtc-
> > > 
> > > state);
> > -	const struct skl_plane_wm *p_wm = &cstate-
> > > 
> > > wm.skl.optimal.planes[0];
> >  	int pipe = intel_crtc->pipe;
> >  
> > -	/*
> > -	 * We only populate skl_results on watermark updates, and
> > if
> > the
> > -	 * plane's visiblity isn't actually changing neither is
> > its
> > watermarks.
> > -	 */
> > -	if (!crtc->primary->state->visible)
> > -		skl_write_plane_wm(intel_crtc, p_wm,
> > -				   &dev_priv->wm.skl_results.ddb,
> > 0);
> > -
> >  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
> >  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> >  	POSTING_READ(PLANE_SURF(pipe, 0));
> > @@ -10824,16 +10808,9 @@ static void i9xx_update_cursor(struct
> > drm_crtc *crtc, u32 base,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_crtc_state *cstate =
> > to_intel_crtc_state(crtc-
> > > 
> > > state);
> > -	const struct skl_wm_values *wm = &dev_priv-
> > >wm.skl_results;
> > -	const struct skl_plane_wm *p_wm =
> > -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> >  	int pipe = intel_crtc->pipe;
> >  	uint32_t cntl = 0;
> >  
> > -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > drm_crtc_mask(crtc))
> > -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> > -
> >  	if (plane_state && plane_state->base.visible) {
> >  		cntl = MCURSOR_GAMMA_ENABLE;
> >  		switch (plane_state->base.crtc_w) {
> > @@ -14436,8 +14413,13 @@ static void
> > intel_atomic_commit_tail(struct
> > drm_atomic_state *state)
> >  			intel_check_cpu_fifo_underruns(dev_priv);
> >  			intel_check_pch_fifo_underruns(dev_priv);
> >  
> > -			if (!crtc->state->active)
> > -				intel_update_watermarks(crtc);
> > +			if (!crtc->state->active) {
> > +				if (dev_priv-
> > > 
> > > display.initial_watermarks)
> > +					dev_priv-
> > > 
> > > display.initial_watermarks(intel_state,
> > +									
> >      to_intel_crtc_state(crtc->state));
> > +				else
> > +					intel_update_watermarks(cr
> > tc
> > );
> > +			}
> >  		}
> >  	}
> >  
> > @@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct
> > drm_device *dev,
> >  
> >  	drm_atomic_helper_swap_state(state, true);
> >  	dev_priv->wm.distrust_bios_wm = false;
> > -	dev_priv->wm.skl_results = intel_state->wm_results;
> >  	intel_shared_dpll_commit(state);
> >  	intel_atomic_track_fbs(state);
> >  
> > @@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct
> > drm_crtc *crtc,
> >  	intel_pipe_update_start(intel_crtc);
> >  
> >  	if (modeset)
> > -		return;
> > +		goto out;
> >  
> >  	if (crtc->state->color_mgmt_changed ||
> > to_intel_crtc_state(crtc->state)->update_pipe) {
> >  		intel_color_set_csc(crtc->state);
> > @@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct
> > drm_crtc *crtc,
> >  	else if (INTEL_GEN(dev_priv) >= 9)
> >  		skl_detach_scalers(intel_crtc);
> >  
> > +out:
> >  	if (dev_priv->display.atomic_evade_watermarks)
> >  		dev_priv-
> > > 
> > > display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_st
> > > ate
> > ->state), intel_cstate);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 9f04e26c4365..17cf1ee83bfb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct
> > skl_ddb_allocation *old,
> >  			       enum pipe pipe);
> >  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> >  				 struct intel_crtc *intel_crtc);
> > -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > -			 const struct skl_plane_wm *wm,
> > -			 const struct skl_ddb_allocation *ddb);
> > -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > -			const struct skl_plane_wm *wm,
> > -			const struct skl_ddb_allocation *ddb,
> > -			int plane);
> >  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> > *pipe_config);
> >  bool ilk_disable_lp_wm(struct drm_device *dev);
> >  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> > enable_rc6);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index be3dd8cdc7ae..18c62d1eea19 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state
> > *state)
> >  	return 0;
> >  }
> >  
> > -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> > -			      struct intel_crtc_state *cstate)
> > +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> > struct intel_crtc_state *cstate)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(cstate-
> > >base.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(state-
> > > 
> > > base.dev);
> >  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> > +	const struct skl_ddb_allocation *ddb = &state-
> > > 
> > > wm_results.ddb;
> >  	enum pipe pipe = crtc->pipe;
> > +	int plane;
> > +
> > +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc-
> > > 
> > > base)))
> > +		return;
> >  
> > -	I915_WRITE(PIPE_WM_LINETIME(pipe),
> > -		   pipe_wm->linetime);
> > +	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);

Little bit of rebase debris? Might as well just fix this in patch 5.

With the changes I mentioned:

Reviewed-by: Lyude <cpaul@redhat.com>

> > +
> > +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
> > +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane],
> > ddb, plane);
> > +
> > +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR],
> > ddb);
> >  }
> >  
> > -static void skl_update_wm(struct drm_crtc *crtc)
> > +static void skl_initial_wm(struct intel_atomic_state *state,
> > +			   struct intel_crtc_state *cstate)
> >  {
> > +	struct drm_crtc *crtc = cstate->base.crtc;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> > +	struct skl_wm_values *results = &state->wm_results;
> >  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> > -	struct intel_crtc_state *cstate =
> > to_intel_crtc_state(crtc-
> > > 
> > > state);
> > -	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> >  	enum pipe pipe = intel_crtc->pipe;
> >  
> >  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> > @@ -4213,16 +4221,8 @@ static void skl_update_wm(struct drm_crtc
> > *crtc)
> >  	 * the pipe's shut off, just do so here. Already active
> > pipes will have
> >  	 * their watermarks updated once we update their planes.
> >  	 */
> > -	if (crtc->state->active_changed) {
> > -		int plane;
> > -
> > -		for (plane = 0; plane <
> > intel_num_planes(intel_crtc); plane++)
> > -			skl_write_plane_wm(intel_crtc, &pipe_wm-
> > > 
> > > planes[plane],
> > -					   &results->ddb, plane);
> > -
> > -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> > > 
> > > planes[PLANE_CURSOR],
> > -				    &results->ddb);
> > -	}
> > +	if (cstate->base.active_changed)
> > +		skl_evade_crtc_wm(state, cstate);
> >  
> >  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
> >  
> > @@ -7727,7 +7727,7 @@ void intel_init_pm(struct drm_device *dev)
> >  	/* For FIFO watermark updates */
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> >  		skl_setup_wm_latency(dev);
> > -		dev_priv->display.update_wm = skl_update_wm;
> > +		dev_priv->display.initial_watermarks =
> > skl_initial_wm;
> >  		dev_priv->display.atomic_evade_watermarks =
> > skl_evade_crtc_wm;
> >  		dev_priv->display.compute_global_watermarks =
> > skl_compute_wm;
> >  	} else if (HAS_PCH_SPLIT(dev)) {
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0fb775b4c93e..366900dcde34 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_plane *intel_plane =
> > to_intel_plane(drm_plane);
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > -	const struct skl_wm_values *wm = &dev_priv-
> > >wm.skl_results;
> > -	struct drm_crtc *crtc = crtc_state->base.crtc;
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	const int pipe = intel_plane->pipe;
> >  	const int plane = intel_plane->plane + 1;
> > -	const struct skl_plane_wm *p_wm =
> > -		&crtc_state->wm.skl.optimal.planes[plane];
> >  	u32 plane_ctl;
> >  	const struct drm_intel_sprite_colorkey *key =
> > &plane_state-
> > > 
> > > ckey;
> >  	u32 surf_addr = plane_state->main.offset;
> > @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  
> >  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> >  
> > -	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> > -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> > plane);
> > -
> >  	if (key->flags) {
> >  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key-
> > > 
> > > min_value);
> >  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key-
> > > 
> > > max_value);
> > @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane,
> > struct drm_crtc *crtc)
> >  	struct drm_device *dev = dplane->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> > -	struct intel_crtc_state *cstate =
> > to_intel_crtc_state(crtc-
> > > 
> > > state);
> >  	const int pipe = intel_plane->pipe;
> >  	const int plane = intel_plane->plane + 1;
> >  
> > -	/*
> > -	 * We only populate skl_results on watermark updates, and
> > if
> > the
> > -	 * plane's visiblity isn't actually changing neither is
> > its
> > watermarks.
> > -	 */
> > -	if (!dplane->state->visible)
> > -		skl_write_plane_wm(to_intel_crtc(crtc),
> > -				   &cstate-
> > > 
> > > wm.skl.optimal.planes[plane],
> > -				   &dev_priv->wm.skl_results.ddb,
> > plane);
> > -
> >  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
> >  
> >  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Maarten Lankhorst]

Op 12-10-16 om 19:15 schreef Lyude:
> Accidentally sent original view twice and found one more issue after
> looking at the rest of them, sorry about that!
>
> On Wed, 2016-10-12 at 13:04 -0400, Lyude wrote:
>> Loving this patch so far! Would it be possible to get this split into
>> two separate patches though? One for removing skl_results and one for
>> programming watermarks as a separate step.
Yeah the small hunk has to be moved to patch #5.

I can't split this patch up. skl_results becomes useless after moving the
programming to a separate step. The old way of doing it was storing
intel_state->wm_results in dev_priv->wm.skl_results and using it in the
plane callbacks.

With the watermark update in its own function, this becomes useless
because the pointer to watermarks can be retrieved from state directly.

~Maarten

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

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:20PM +0200, Maarten Lankhorst wrote:
> Instead of running the watermark updates from the callbacks run
> them from a separate hook atomic_evade_watermarks.
> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
>  5 files changed, 28 insertions(+), 78 deletions(-)
> 
...
> @@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  			intel_check_cpu_fifo_underruns(dev_priv);
>  			intel_check_pch_fifo_underruns(dev_priv);
>  
> -			if (!crtc->state->active)
> -				intel_update_watermarks(crtc);
> +			if (!crtc->state->active) {
> +				if (dev_priv->display.initial_watermarks)
> +					dev_priv->display.initial_watermarks(intel_state,
> +									     to_intel_crtc_state(crtc->state));
> +				else
> +					intel_update_watermarks(crtc);
> +			}
>  		}

This will change the behavior on ILK-style platforms won't it?
Previously the intel_update_watermarks here was a noop on those
platforms, but now we're calling initial_watermarks after the CRTC is
disabled there (note that there's also a call to it in pre_plane_update
that we purposely skip when doing any kind of modeset).


Matt

>  	}
>  
> @@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(state, true);
>  	dev_priv->wm.distrust_bios_wm = false;
> -	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
>  	intel_atomic_track_fbs(state);
>  
> @@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	intel_pipe_update_start(intel_crtc);
>  
>  	if (modeset)
> -		return;
> +		goto out;
>  
>  	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
> @@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> +out:
>  	if (dev_priv->display.atomic_evade_watermarks)
>  		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9f04e26c4365..17cf1ee83bfb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
>  			       enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_plane_wm *wm,
> -			const struct skl_ddb_allocation *ddb,
> -			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index be3dd8cdc7ae..18c62d1eea19 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> -			      struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
>  	enum pipe pipe = crtc->pipe;
> +	int plane;
> +
> +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
> +		return;
>  
> -	I915_WRITE(PIPE_WM_LINETIME(pipe),
> -		   pipe_wm->linetime);
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +
> +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
> +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
> +
> +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
>  }
>  
> -static void skl_update_wm(struct drm_crtc *crtc)
> +static void skl_initial_wm(struct intel_atomic_state *state,
> +			   struct intel_crtc_state *cstate)
>  {
> +	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> +	struct skl_wm_values *results = &state->wm_results;
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> @@ -4213,16 +4221,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	 * the pipe's shut off, just do so here. Already active pipes will have
>  	 * their watermarks updated once we update their planes.
>  	 */
> -	if (crtc->state->active_changed) {
> -		int plane;
> -
> -		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
> -			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
> -					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
> -				    &results->ddb);
> -	}
> +	if (cstate->base.active_changed)
> +		skl_evade_crtc_wm(state, cstate);
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> @@ -7727,7 +7727,7 @@ void intel_init_pm(struct drm_device *dev)
>  	/* For FIFO watermark updates */
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
> -		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.initial_watermarks = skl_initial_wm;
>  		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks = skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0fb775b4c93e..366900dcde34 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	struct drm_crtc *crtc = crtc_state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[plane];
>  	u32 plane_ctl;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	u32 surf_addr = plane_state->main.offset;
> @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
> -
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if the
> -	 * plane's visiblity isn't actually changing neither is its watermarks.
> -	 */
> -	if (!dplane->state->visible)
> -		skl_write_plane_wm(to_intel_crtc(crtc),
> -				   &cstate->wm.skl.optimal.planes[plane],
> -				   &dev_priv->wm.skl_results.ddb, plane);
> -
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
> -- 
> 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

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Maarten Lankhorst]

Op 20-10-16 om 20:35 schreef Matt Roper:
> On Wed, Oct 12, 2016 at 03:28:20PM +0200, Maarten Lankhorst wrote:
>> Instead of running the watermark updates from the callbacks run
>> them from a separate hook atomic_evade_watermarks.
>>
>> This also gets rid of the global skl_results, which was required for
>> keeping track of the current atomic commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
>>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-------------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
>>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------------------
>>  drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
>>  5 files changed, 28 insertions(+), 78 deletions(-)
>>
> ...
>> @@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  			intel_check_cpu_fifo_underruns(dev_priv);
>>  			intel_check_pch_fifo_underruns(dev_priv);
>>  
>> -			if (!crtc->state->active)
>> -				intel_update_watermarks(crtc);
>> +			if (!crtc->state->active) {
>> +				if (dev_priv->display.initial_watermarks)
>> +					dev_priv->display.initial_watermarks(intel_state,
>> +									     to_intel_crtc_state(crtc->state));
>> +				else
>> +					intel_update_watermarks(crtc);
>> +			}
>>  		}
> This will change the behavior on ILK-style platforms won't it?
> Previously the intel_update_watermarks here was a noop on those
> platforms, but now we're calling initial_watermarks after the CRTC is
> disabled there (note that there's also a call to it in pre_plane_update
> that we purposely skip when doing any kind of modeset).
Yeah, it could be better to change it to if (HAS_DDI(dev_priv)), that way it's not going to matter..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:20PM +0200, Maarten Lankhorst wrote:
> Instead of running the watermark updates from the callbacks run
> them from a separate hook atomic_evade_watermarks.

The commit message here is a bit terse.  I'd clarify that the change
we're making is that watermark register programming is no longer
happening in the same display callbacks that write general plane
registers, but rather in a new independent hook.  The key thing to
emphasize is that despite the refactoring, the watermark values will
still be written under the same vblank evasion that is covering the rest
of the planes' updates, so they'll still take effect on the same vblank.

> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
>  5 files changed, 28 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 09588c58148f..28e44cb611b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2027,13 +2027,6 @@ struct drm_i915_private {
>  		 */
>  		uint16_t skl_latency[8];
>  
> -		/*
> -		 * The skl_wm_values structure is a bit too big for stack
> -		 * allocation, so we keep the staging struct where we store
> -		 * intermediate results here instead.
> -		 */
> -		struct skl_wm_values skl_results;
> -
>  		/* current hardware state */
>  		union {
>  			struct ilk_wm_values hw;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 340861826c46..d3d7d9dc14a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3377,9 +3377,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3415,9 +3412,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = src_x;
>  	intel_crtc->adjusted_y = src_y;
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -3450,18 +3444,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if the
> -	 * plane's visiblity isn't actually changing neither is its watermarks.
> -	 */
> -	if (!crtc->primary->state->visible)
> -		skl_write_plane_wm(intel_crtc, p_wm,
> -				   &dev_priv->wm.skl_results.ddb, 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
>  	POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -10824,16 +10808,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> -
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> @@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  			intel_check_cpu_fifo_underruns(dev_priv);
>  			intel_check_pch_fifo_underruns(dev_priv);
>  
> -			if (!crtc->state->active)
> -				intel_update_watermarks(crtc);
> +			if (!crtc->state->active) {
> +				if (dev_priv->display.initial_watermarks)
> +					dev_priv->display.initial_watermarks(intel_state,
> +									     to_intel_crtc_state(crtc->state));
> +				else
> +					intel_update_watermarks(crtc);
> +			}
>  		}
>  	}
>  
> @@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(state, true);
>  	dev_priv->wm.distrust_bios_wm = false;
> -	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
>  	intel_atomic_track_fbs(state);
>  
> @@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	intel_pipe_update_start(intel_crtc);
>  
>  	if (modeset)
> -		return;
> +		goto out;

Should this change have been in a previous patch?  Were we missing the
programming of linetime watermarks on modeset before?

>  
>  	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
> @@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> +out:
>  	if (dev_priv->display.atomic_evade_watermarks)
>  		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9f04e26c4365..17cf1ee83bfb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
>  			       enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_plane_wm *wm,
> -			const struct skl_ddb_allocation *ddb,
> -			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index be3dd8cdc7ae..18c62d1eea19 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> -			      struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
>  	enum pipe pipe = crtc->pipe;
> +	int plane;
> +
> +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
> +		return;

Should we have had this test even when we were just writing the linetime
watermarks?  I.e., should this move to an earlier patch?

>  
> -	I915_WRITE(PIPE_WM_LINETIME(pipe),
> -		   pipe_wm->linetime);
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +
> +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
> +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
> +
> +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);

Technically this will cause us to (re)write unchanged watermark values
more often than we did previously.  E.g., we previously skipped writing
values for disabled planes that were staying disabled.  Not sure if it
really matters or not, just something I figured I should point out.


Matt

>  }
>  
> -static void skl_update_wm(struct drm_crtc *crtc)
> +static void skl_initial_wm(struct intel_atomic_state *state,
> +			   struct intel_crtc_state *cstate)
>  {
> +	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> +	struct skl_wm_values *results = &state->wm_results;
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> @@ -4213,16 +4221,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	 * the pipe's shut off, just do so here. Already active pipes will have
>  	 * their watermarks updated once we update their planes.
>  	 */
> -	if (crtc->state->active_changed) {
> -		int plane;
> -
> -		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
> -			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
> -					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
> -				    &results->ddb);
> -	}
> +	if (cstate->base.active_changed)
> +		skl_evade_crtc_wm(state, cstate);
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> @@ -7727,7 +7727,7 @@ void intel_init_pm(struct drm_device *dev)
>  	/* For FIFO watermark updates */
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
> -		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.initial_watermarks = skl_initial_wm;
>  		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks = skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0fb775b4c93e..366900dcde34 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	struct drm_crtc *crtc = crtc_state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[plane];
>  	u32 plane_ctl;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	u32 surf_addr = plane_state->main.offset;
> @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
> -
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if the
> -	 * plane's visiblity isn't actually changing neither is its watermarks.
> -	 */
> -	if (!dplane->state->visible)
> -		skl_write_plane_wm(to_intel_crtc(crtc),
> -				   &cstate->wm.skl.optimal.planes[plane],
> -				   &dev_priv->wm.skl_results.ddb, plane);
> -
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
> -- 
> 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

Re: [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion

[Maarten Lankhorst]

Op 20-10-16 om 23:57 schreef Matt Roper:
> On Wed, Oct 12, 2016 at 03:28:20PM +0200, Maarten Lankhorst wrote:
>> Instead of running the watermark updates from the callbacks run
>> them from a separate hook atomic_evade_watermarks.
> The commit message here is a bit terse.  I'd clarify that the change
> we're making is that watermark register programming is no longer
> happening in the same display callbacks that write general plane
> registers, but rather in a new independent hook.  The key thing to
> emphasize is that despite the refactoring, the watermark values will
> still be written under the same vblank evasion that is covering the rest
> of the planes' updates, so they'll still take effect on the same vblank.
>
>> This also gets rid of the global skl_results, which was required for
>> keeping track of the current atomic commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
>>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-------------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
>>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++------------------
>>  drivers/gpu/drm/i915/intel_sprite.c  | 18 -----------------
>>  5 files changed, 28 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 09588c58148f..28e44cb611b8 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2027,13 +2027,6 @@ struct drm_i915_private {
>>  		 */
>>  		uint16_t skl_latency[8];
>>  
>> -		/*
>> -		 * The skl_wm_values structure is a bit too big for stack
>> -		 * allocation, so we keep the staging struct where we store
>> -		 * intermediate results here instead.
>> -		 */
>> -		struct skl_wm_values skl_results;
>> -
>>  		/* current hardware state */
>>  		union {
>>  			struct ilk_wm_values hw;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 340861826c46..d3d7d9dc14a8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3377,9 +3377,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>>  	struct drm_framebuffer *fb = plane_state->base.fb;
>> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
>> -	const struct skl_plane_wm *p_wm =
>> -		&crtc_state->wm.skl.optimal.planes[0];
>>  	int pipe = intel_crtc->pipe;
>>  	u32 plane_ctl;
>>  	unsigned int rotation = plane_state->base.rotation;
>> @@ -3415,9 +3412,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>>  	intel_crtc->adjusted_x = src_x;
>>  	intel_crtc->adjusted_y = src_y;
>>  
>> -	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
>> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
>> -
>>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
>> @@ -3450,18 +3444,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>> -	const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
>>  	int pipe = intel_crtc->pipe;
>>  
>> -	/*
>> -	 * We only populate skl_results on watermark updates, and if the
>> -	 * plane's visiblity isn't actually changing neither is its watermarks.
>> -	 */
>> -	if (!crtc->primary->state->visible)
>> -		skl_write_plane_wm(intel_crtc, p_wm,
>> -				   &dev_priv->wm.skl_results.ddb, 0);
>> -
>>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
>>  	POSTING_READ(PLANE_SURF(pipe, 0));
>> @@ -10824,16 +10808,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
>> -	const struct skl_plane_wm *p_wm =
>> -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>>  	int pipe = intel_crtc->pipe;
>>  	uint32_t cntl = 0;
>>  
>> -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
>> -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
>> -
>>  	if (plane_state && plane_state->base.visible) {
>>  		cntl = MCURSOR_GAMMA_ENABLE;
>>  		switch (plane_state->base.crtc_w) {
>> @@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  			intel_check_cpu_fifo_underruns(dev_priv);
>>  			intel_check_pch_fifo_underruns(dev_priv);
>>  
>> -			if (!crtc->state->active)
>> -				intel_update_watermarks(crtc);
>> +			if (!crtc->state->active) {
>> +				if (dev_priv->display.initial_watermarks)
>> +					dev_priv->display.initial_watermarks(intel_state,
>> +									     to_intel_crtc_state(crtc->state));
>> +				else
>> +					intel_update_watermarks(crtc);
>> +			}
>>  		}
>>  	}
>>  
>> @@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  
>>  	drm_atomic_helper_swap_state(state, true);
>>  	dev_priv->wm.distrust_bios_wm = false;
>> -	dev_priv->wm.skl_results = intel_state->wm_results;
>>  	intel_shared_dpll_commit(state);
>>  	intel_atomic_track_fbs(state);
>>  
>> @@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>>  	intel_pipe_update_start(intel_crtc);
>>  
>>  	if (modeset)
>> -		return;
>> +		goto out;
> Should this change have been in a previous patch?  Were we missing the
> programming of linetime watermarks on modeset before?
>
>>  
>>  	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
>>  		intel_color_set_csc(crtc->state);
>> @@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>>  	else if (INTEL_GEN(dev_priv) >= 9)
>>  		skl_detach_scalers(intel_crtc);
>>  
>> +out:
>>  	if (dev_priv->display.atomic_evade_watermarks)
>>  		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9f04e26c4365..17cf1ee83bfb 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
>>  			       enum pipe pipe);
>>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>>  				 struct intel_crtc *intel_crtc);
>> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
>> -			 const struct skl_plane_wm *wm,
>> -			 const struct skl_ddb_allocation *ddb);
>> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
>> -			const struct skl_plane_wm *wm,
>> -			const struct skl_ddb_allocation *ddb,
>> -			int plane);
>>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>>  bool ilk_disable_lp_wm(struct drm_device *dev);
>>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index be3dd8cdc7ae..18c62d1eea19 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state *state)
>>  	return 0;
>>  }
>>  
>> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
>> -			      struct intel_crtc_state *cstate)
>> +static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct intel_crtc_state *cstate)
>>  {
>>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>> +	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
>>  	enum pipe pipe = crtc->pipe;
>> +	int plane;
>> +
>> +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
>> +		return;
> Should we have had this test even when we were just writing the linetime
> watermarks?  I.e., should this move to an earlier patch?
>
>>  
>> -	I915_WRITE(PIPE_WM_LINETIME(pipe),
>> -		   pipe_wm->linetime);
>> +	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
>> +
>> +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
>> +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
>> +
>> +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
> Technically this will cause us to (re)write unchanged watermark values
> more often than we did previously.  E.g., we previously skipped writing
> values for disabled planes that were staying disabled.  Not sure if it
> really matters or not, just something I figured I should point out.
Should be harmless. Could prevent it if we cared, but I don't see the value. :)

It would be nice if we would start removing planes that were only included because it's wm values change.
Not completely sure how doable it is, depends whether we could trigger updates race free.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/8] drm/i915/gen9+: Preserve old allocation from crtc_state.

[Maarten Lankhorst]

This is the last bit required for making nonblocking modesets work
correctly. The state in intel_crtc->hw_ddb is not updated until
somewhere in atomic commit, while the previous crtc state should be
accurate if the ddb hasn't changed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d3d7d9dc14a8..93e16da0aa51 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14332,7 +14332,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 			 * new ddb allocation to take effect.
 			 */
 			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
-						 &intel_crtc->hw_ddb) &&
+						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
 			    !crtc->state->active_changed &&
 			    intel_state->wm_results.dirty_pipes != updated)
 				vbl_wait = true;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 18c62d1eea19..182e6b30b60a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3091,7 +3091,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	 * we currently hold.
 	 */
 	if (!intel_state->active_pipe_changes) {
-		*alloc = to_intel_crtc(for_crtc)->hw_ddb;
+		/*
+		 * alloc may be cleared by clear_intel_crtc_state,
+		 * copy from old state to be sure
+		 */
+		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
 		return;
 	}
 
-- 
2.7.4

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

Re: [PATCH 8/8] drm/i915/gen9+: Preserve old allocation from crtc_state.

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:21PM +0200, Maarten Lankhorst wrote:
> This is the last bit required for making nonblocking modesets work
> correctly. The state in intel_crtc->hw_ddb is not updated until
> somewhere in atomic commit, while the previous crtc state should be
> accurate if the ddb hasn't changed.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Can we get rid of hw_ddb completely and always pull from old state?  It
looks like the only other place it's still used is
skl_update_crtcs() -> skl_ddb_allocation_overlaps().


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d3d7d9dc14a8..93e16da0aa51 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14332,7 +14332,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  			 * new ddb allocation to take effect.
>  			 */
>  			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
> -						 &intel_crtc->hw_ddb) &&
> +						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
>  			    !crtc->state->active_changed &&
>  			    intel_state->wm_results.dirty_pipes != updated)
>  				vbl_wait = true;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 18c62d1eea19..182e6b30b60a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3091,7 +3091,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  	 * we currently hold.
>  	 */
>  	if (!intel_state->active_pipe_changes) {
> -		*alloc = to_intel_crtc(for_crtc)->hw_ddb;
> +		/*
> +		 * alloc may be cleared by clear_intel_crtc_state,
> +		 * copy from old state to be sure
> +		 */
> +		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
>  		return;
>  	}
>  
> -- 
> 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

Re: [PATCH 8/8] drm/i915/gen9+: Preserve old allocation from crtc_state.

[Maarten Lankhorst]

Op 21-10-16 om 00:09 schreef Matt Roper:
> On Wed, Oct 12, 2016 at 03:28:21PM +0200, Maarten Lankhorst wrote:
>> This is the last bit required for making nonblocking modesets work
>> correctly. The state in intel_crtc->hw_ddb is not updated until
>> somewhere in atomic commit, while the previous crtc state should be
>> accurate if the ddb hasn't changed.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Can we get rid of hw_ddb completely and always pull from old state?  It
> looks like the only other place it's still used is
> skl_update_crtcs() -> skl_ddb_allocation_overlaps().
Probably as a followup patch. I kept it only for that reason indeed.

I suppose it could be changed to if (updated) state = new_state->ddb else state = old_state->ddb.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx