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

Mon Oct 24 07:00:10 UTC 2016

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