Re: [PATCH 7/8] drm/i915/skl: Update watermarks for Y tiling

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/26/2015 04:59 PM, Damien Lespiau wrote:
> On Wed, Feb 25, 2015 at 04:47:23PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Display watermarks need different programming for different tiling
>> modes.
>>
>> Set the relevant flag so this happens during the plane commit and
>> add relevant data into a structure made available to the watermark
>> computation code.
>>
>> v2: Pass in tiling info to sprite plane updates as well.
>> v3: Rebased for plane handling changes.
>> v4: Handle fb == NULL when plane is disabled.
>> v5: Refactored for addfb2 interface.
>> v6: Refactored for fb modifier changes.
>> v7: Updated for atomic commit by only updating watermarks when tiling changes.
>> v8: BSpec watermark calculation updates.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Acked-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> Acked-by: Matt Roper <matthew.d.roper@intel.com>
>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c |  6 ++++
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   drivers/gpu/drm/i915/intel_pm.c      | 56 ++++++++++++++++++++++++++++++------
>>   drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++
>>   4 files changed, 60 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 626e6538..1d50934 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11994,6 +11994,12 @@ intel_check_primary_plane(struct drm_plane *plane,
>>   			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>>
>>   		intel_crtc->atomic.update_fbc = true;
>> +
>> +		/* Update watermarks on tiling changes. */
>> +		if (!plane->state->fb || !state->base.fb ||
>> +		    plane->state->fb->modifier[0] !=
>> +		    state->base.fb->modifier[0])
>> +			intel_crtc->atomic.update_wm = true;
>>   	}
>>
>>   	return 0;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 399d2b2..b124548 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -501,6 +501,7 @@ struct intel_plane_wm_parameters {
>>   	uint8_t bytes_per_pixel;
>>   	bool enabled;
>>   	bool scaled;
>> +	u64 tiling;
>>   };
>>
>>   struct intel_plane {
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 626c3c2..e0d6ebc 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2603,7 +2603,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
>>
>>   static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>>   			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
>> -			       uint32_t latency)
>> +			       uint64_t tiling, uint32_t latency)
>>   {
>
> Hum, does this compile? I'm seeing an extra argument to skl_wm_method2()
> but no update at the calling site?

Not only that, but it even works! :) (Extra argument is there, you must 
have missed it!)

>>   	uint32_t ret;
>>   	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>> @@ -2613,7 +2613,16 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>>   		return UINT_MAX;
>>
>>   	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
>> -	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>> +
>> +	if (tiling == I915_FORMAT_MOD_Y_TILED ||
>> +	    tiling == I915_FORMAT_MOD_Yf_TILED) {
>> +		plane_bytes_per_line *= 4;
>> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>> +		plane_blocks_per_line /= 4;
>> +	} else {
>> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>> +	}
>> +
>>   	wm_intermediate_val = latency * pixel_rate;
>>   	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
>>   				plane_blocks_per_line;
>> @@ -2665,6 +2674,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>   	enum pipe pipe = intel_crtc->pipe;
>>   	struct drm_plane *plane;
>> +	struct drm_framebuffer *fb;
>>   	int i = 1; /* Index for sprite planes start */
>>
>>   	p->active = intel_crtc_active(crtc);
>> @@ -2680,6 +2690,14 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>>   			crtc->primary->fb->bits_per_pixel / 8;
>>   		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
>>   		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
>> +		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
>> +		fb = crtc->primary->fb;
>> +		/*
>> +		 * Framebuffer can be NULL on plane disable, but it does not
>> +		 * matter for watermarks if we assume no tiling in that case.
>> +		 */
>> +		if (fb)
>> +			p->plane[0].tiling = fb->modifier[0];
>>
>>   		p->cursor.enabled = true;
>>   		p->cursor.bytes_per_pixel = 4;
>> @@ -2709,6 +2727,7 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>>   	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>>   	uint32_t res_blocks, res_lines;
>>   	uint32_t result_blocks;
>> +	uint32_t y_tile_minimum;
>
> The scope of the y_tile_minimum variable could be restricted further,
> but well...

Okay, okay, I'm in the old C standard for kernel still.

Regards,

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