| Message ID | 20170508114902.18965-8-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2017-05-08 at 17:18 +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. When sending a new version of a patch, please list the changes from the previous version here. For example: v2: Improve commit message. v3: ... > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > --- > 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; > + } The comments I made earlier [1] are still valid. With those fixed and a changelog in the commit message, Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> [1] https://lists.freedesktop.org/archives/intel-gfx/2017-April/125764.html > + > + alloc_size -= total_min_blocks; > ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR]; > ddb->plane[pipe][PLANE_CURSOR].end = alloc->end; >
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@intel.com> > --- > 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@intel.com> > + } > + > + 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 >
On Friday 12 May 2017 05:53 AM, Matt Roper wrote: > 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@intel.com> >> --- >> 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, I think I already fixed this in newer version of patch. :), anyway resubmitting complete series again with other comments addressed. thanks, -Mahesh > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >> + } >> + >> + 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 >>
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; + } + + alloc_size -= total_min_blocks; ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR]; ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
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. Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)