| Message ID | 20170228113143.8280-6-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-02-28 at 17:01 +0530, Mahesh Kumar wrote: > DDB minimum requirement may also exceed the allocated DDB for CRTC. > Instead of directly deducting from alloc_size, check against > total_min_ddb requirement. if exceeding fail the flip. Instead of doing a low level description of the code change, including variable names and whatnot, use this space to describe in a high level what the patch does and why it is necessary. For instance, in this particular case the patch changes the modeset to fail when there isn't enough ddb for the given configuration. Previously it succeeded, but the plane ddb allocations would be bogus because of the negative value of alloc_size, leading to screen corruption and system hangs. Do I understand correctly that this patch is independent from the rest of the series? Might make sense to merge it earlier, since the bug is there with the old ddb allocation algorithm too. > > 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 76c986166664..24e9e5d69833 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3380,6 +3380,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])); > @@ -3407,10 +3408,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)) { One '(' is enough. > + 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; Space between return and -EINVAL please. Ander > + } > + > + alloc_size -= total_min_blocks; > ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR]; > ddb->plane[pipe][PLANE_CURSOR].end = alloc->end; >
Hi Ander, Thanks for review On Wednesday 12 April 2017 02:47 PM, Ander Conselvan De Oliveira wrote: > On Tue, 2017-02-28 at 17:01 +0530, Mahesh Kumar wrote: >> DDB minimum requirement may also exceed the allocated DDB for CRTC. >> Instead of directly deducting from alloc_size, check against >> total_min_ddb requirement. if exceeding fail the flip. > Instead of doing a low level description of the code change, including variable > names and whatnot, use this space to describe in a high level what the patch > does and why it is necessary. For instance, in this particular case the patch > changes the modeset to fail when there isn't enough ddb for the given > configuration. Previously it succeeded, but the plane ddb allocations would be > bogus because of the negative value of alloc_size, leading to screen corruption > and system hangs. Ok, will make changes, Not only modeset, any flip whose requirement is exceeding available ddb allocation, we should fail that ioctl. > > Do I understand correctly that this patch is independent from the rest of the > series? Might make sense to merge it earlier, since the bug is there with the > old ddb allocation algorithm too. Yes, patch 3,4 as well are independent of the series & applicable for old algorithm too. > >> 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 76c986166664..24e9e5d69833 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3380,6 +3380,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])); >> @@ -3407,10 +3408,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)) { > One '(' is enough. will do :) > >> + 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; > Space between return and -EINVAL please. will update. -Mahesh > > Ander > >> + } >> + >> + alloc_size -= total_min_blocks; >> ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR]; >> ddb->plane[pipe][PLANE_CURSOR].end = alloc->end; >>
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 76c986166664..24e9e5d69833 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3380,6 +3380,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])); @@ -3407,10 +3408,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 also exceed the allocated DDB for CRTC. Instead of directly deducting from alloc_size, check against total_min_ddb requirement. if exceeding fail the flip. Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)