On Mon, May 08, 2017 at 05:18:58PM +0530, Mahesh Kumar wrote:
> DDB minimum requirement may exceed the allocated DDB for CRTC/Pipe. This
> patch make changes to fail the flip if minimum requirement for pipe
> exceeds the total ddb allocated to the pipe.
> Previously it succeeded but making alloc_size a negative value. Which
> will make later calculations for plane ddb allocation bogus & may lead
> to screen corruption or system hang.
>
I think originally the bspec defined the initial minimum DDB allocation
for each plane to just be '8' in all cases. So even with 4 planes on a
pipe, your initial pipe minimum would only be 32, so the code here never
had a risk of overflowing (note that the initial minimum is different
than the WM0 minimum requirement we ultimately wind up calculating).
But then the bspec was updated to suggest a higher initial minimum for
y-tiled buffers (calculated based on the width of the buffer), so our
initial assumptions here don't really hold anymore. We should still be
safe if we're not using y-tile, but even a single 4k y-tiled plane would
have an initial minimum of something like 250 blocks iirc., so we can't
assume we're always safe anymore.
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2a4e9d89cd6f..0ace94d67432 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3591,6 +3591,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> int num_active;
> unsigned plane_data_rate[I915_MAX_PLANES] = {};
> unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
> + uint16_t total_min_blocks = 0;
>
> /* Clear the partitioning for disabled planes. */
> memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> @@ -3618,10 +3619,18 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> */
>
> for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> - alloc_size -= minimum[plane_id];
> - alloc_size -= y_minimum[plane_id];
> + total_min_blocks += minimum[plane_id];
> + total_min_blocks += y_minimum[plane_id];
> }
>
> + if ((total_min_blocks > alloc_size)) {
> + DRM_DEBUG_KMS("Requested display configuration exceeds system DDB limitations");
> + DRM_DEBUG_KMS("minimum required %d/%d\n", total_min_blocks,
> + alloc_size);
> + return-EINVAL;
I think you're missing a space here?
With that fixed,
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> + }
> +
> + alloc_size -= total_min_blocks;
> ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
>
> --
> 2.11.0
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
