On Mon, Feb 23, 2015 at 03:56:00PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Acked-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
> Acked-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++++++-----
> drivers/gpu/drm/i915/intel_sprite.c | 6 ++++++
> 4 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c622b11..74d4923 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11985,6 +11985,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;
Imo this is fragile. We should unconditionally recomupte watermarks imo
and just ellide the hw update if nothing changed. Spending a few cpu
cycles on this per frame shouldn't hurt, and it will avoid duplicated
logic and checks for watermark recomputation splattered all over the code.
I know that it's kinda there already, but still minimal enough to patch
up. But if Matt knows that this will get removed again with the two-stage
wm code then I'm ok with leaving it in, with a FIXME comment added.
-Daniel
> }
>
> 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 f7c9938..006e635 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2662,6 +2662,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);
> @@ -2677,6 +2678,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;
> @@ -2702,6 +2711,7 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
> {
> uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
> uint32_t result_bytes;
> + uint32_t y_tile_minimum;
>
> if (mem_value == 0 || !p->active || !p_params->enabled)
> return false;
> @@ -2718,11 +2728,16 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
> plane_bytes_per_line = p_params->horiz_pixels *
> p_params->bytes_per_pixel;
>
> - /* For now xtile and linear */
> - if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> - result_bytes = min(method1, method2);
> - else
> - result_bytes = method1;
> + if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> + p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
> + y_tile_minimum = plane_bytes_per_line * 4;
> + result_bytes = max(method2, y_tile_minimum);
> + } else {
> + if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> + result_bytes = min(method1, method2);
> + else
> + result_bytes = method1;
> + }
>
> res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
> res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
> @@ -3153,12 +3168,20 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
> int pixel_size, bool enabled, bool scaled)
> {
> struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct drm_framebuffer *fb = plane->fb;
>
> intel_plane->wm.enabled = enabled;
> intel_plane->wm.scaled = scaled;
> intel_plane->wm.horiz_pixels = sprite_width;
> intel_plane->wm.vert_pixels = sprite_height;
> intel_plane->wm.bytes_per_pixel = pixel_size;
> + intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
> + /*
> + * Framebuffer can be NULL on plane disable, but it does not
> + * matter for watermarks if we assume no tiling in that case.
> + */
> + if (fb)
> + intel_plane->wm.tiling = fb->modifier[0];
>
> skl_update_wm(crtc);
> }
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index b7103bd..29ec206 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1256,6 +1256,12 @@ finish:
>
> if (!intel_crtc->primary_enabled && !state->hides_primary)
> intel_crtc->atomic.post_enable_primary = 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;
> --
> 2.3.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
