| Message ID | 1465374464-7473-2-git-send-email-akshu.agrawal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 08, 2016 at 01:57:44PM +0530, Akshu Agrawal wrote: > CHV pipe C hits underrun when we get -ve X values of cursor. To avoid > this we crop the cursor image for by -ve X value and thus use '0' as > least X value. You're talking about "-ve" here and there's absolutely no "-ve" anywhere in your patch. That makes your commit message non-understandable. But I think I get what you're doing here, and nope, you can't override the cursor image like that. If we go with this w/a then you need to allocate a new cursor gem bo instead. This is userspace-visible. For similar reasons you're also not allowed to change crtc->state. -Daniel > Signed-off-by: Akshu Agrawal <akshu.agrawal@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 113 +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 3 + > 2 files changed, 116 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index bca9245..e6e6568 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14279,6 +14279,81 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane * > plane->base.state->rotation); > } > > +static void vlv_unpin_buffer_obj(struct drm_i915_gem_object *obj, > + char __iomem *buffer_start) > +{ > + iounmap(buffer_start); > + i915_gem_object_ggtt_unpin(obj); > +} > + > +static char __iomem *vlv_pin_and_map_buffer_obj > + (struct drm_i915_gem_object *obj) > +{ > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > + char __iomem *buffer_start; > + int ret; > + > + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); > + if (ret) > + return NULL; > + > + ret = i915_gem_object_set_to_gtt_domain(obj, true); > + if (ret) { > + i915_gem_object_ggtt_unpin(obj); > + return NULL; > + } > + > + buffer_start = ioremap_wc(dev_priv->gtt.mappable_base + > + i915_gem_obj_ggtt_offset(obj), obj->base.size); > + if (buffer_start == NULL) { > + i915_gem_object_ggtt_unpin(obj); > + return NULL; > + } > + > + return buffer_start; > +} > + > +static int vlv_cursor_crop(struct intel_plane_state *state, > + int prev_x) > +{ > + struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); > + struct drm_framebuffer *fb = state->base.fb; > + char __iomem *buffer = state->vlv_cursor_image; > + char __iomem *base, *cursor_base; > + int size = obj->base.size; > + int i, x = state->base.crtc_x; > + int bytes_per_pixel = fb->bits_per_pixel / 8; > + > + base = vlv_pin_and_map_buffer_obj(obj); > + if (base == NULL) > + return -ENOMEM; > + > + if (prev_x >= 0) { > + if (buffer != NULL) > + kfree(buffer); > + buffer = kzalloc(size, GFP_KERNEL); > + state->vlv_cursor_image = buffer; > + if (buffer == NULL) > + return -ENOMEM; > + memcpy(buffer, base, size); > + } > + cursor_base = buffer; > + x = -x; > + for (i = 0; i < state->base.crtc_h; i++) { > + cursor_base += x * bytes_per_pixel; > + memcpy(base, cursor_base, > + (state->base.crtc_w - x) * bytes_per_pixel); > + base += (state->base.crtc_w - x) * bytes_per_pixel; > + memset(base, 0, x * bytes_per_pixel); > + base += x * bytes_per_pixel; > + cursor_base += (state->base.crtc_w - x) * bytes_per_pixel; > + } > + > + vlv_unpin_buffer_obj(obj, base); > + > + return 0; > +} > + > static int > intel_check_cursor_plane(struct drm_plane *plane, > struct intel_crtc_state *crtc_state, > @@ -14287,8 +14362,10 @@ intel_check_cursor_plane(struct drm_plane *plane, > struct drm_crtc *crtc = crtc_state->base.crtc; > struct drm_framebuffer *fb = state->base.fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > + enum pipe pipe = to_intel_plane(plane)->pipe; > unsigned stride; > int ret; > + int crtc_prev_x = state->vlv_cursor_prev_x; > > ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src, > &state->dst, &state->clip, > @@ -14309,6 +14386,32 @@ intel_check_cursor_plane(struct drm_plane *plane, > return -EINVAL; > } > > + /* > + * There is an issue in CHV PIPE C where we hit underrun on > + * -ve value of cursor. To avoid this we are cropping the > + * image for all PIPE C -ve values. > + */ > + if (IS_CHERRYVIEW(plane->dev)) { > + if (pipe == PIPE_C && state->visible && > + state->base.crtc_x < 0) { > + ret = vlv_cursor_crop(state, crtc_prev_x); > + if (ret) > + return -ENOMEM; > + } else if (crtc_prev_x < 0) { /* Restore the image back */ > + char __iomem *base; > + char __iomem *org_image = state->vlv_cursor_image; > + int size = obj->base.size; > + > + if (org_image == NULL) > + return -ENOMEM; > + base = vlv_pin_and_map_buffer_obj(obj); > + if (base == NULL) > + return -ENOMEM; > + memcpy(base, org_image, size); > + vlv_unpin_buffer_obj(obj, base); > + } > + } > + > stride = roundup_pow_of_two(state->base.crtc_w) * 4; > if (obj->base.size < stride * state->base.crtc_h) { > DRM_DEBUG_KMS("buffer is too small\n"); > @@ -14320,6 +14423,16 @@ intel_check_cursor_plane(struct drm_plane *plane, > return -EINVAL; > } > > + if (IS_CHERRYVIEW(plane->dev)) { > + if (pipe == PIPE_C && > + state->visible && state->base.crtc_x < 0) { > + state->vlv_cursor_prev_x = state->base.crtc_x; > + state->base.crtc_x = 0; > + } else { > + state->vlv_cursor_prev_x = state->base.crtc_x; > + } > + } > + > return 0; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index ebe7b34..0c406d1 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -350,6 +350,9 @@ struct intel_plane_state { > > /* async flip related structures */ > struct drm_i915_gem_request *wait_req; > + > + char __iomem *vlv_cursor_image; > + int vlv_cursor_prev_x; > }; > > struct intel_initial_plane_config { > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.7-rc2 next-20160608]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Akshu-Agrawal/Revert-drm-i915-Workaround-CHV-pipe-C-cursor-fail/20160608-163007
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-defconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_display.c: In function 'vlv_pin_and_map_buffer_obj':
>> drivers/gpu/drm/i915/intel_display.c:14306:36: error: 'struct drm_i915_private' has no member named 'gtt'; did you mean 'ggtt'?
buffer_start = ioremap_wc(dev_priv->gtt.mappable_base +
^~
vim +14306 drivers/gpu/drm/i915/intel_display.c
14300 ret = i915_gem_object_set_to_gtt_domain(obj, true);
14301 if (ret) {
14302 i915_gem_object_ggtt_unpin(obj);
14303 return NULL;
14304 }
14305
14306 buffer_start = ioremap_wc(dev_priv->gtt.mappable_base +
14307 i915_gem_obj_ggtt_offset(obj), obj->base.size);
14308 if (buffer_start == NULL) {
14309 i915_gem_object_ggtt_unpin(obj);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 08/06/16 09:40, Daniel Vetter wrote: > On Wed, Jun 08, 2016 at 01:57:44PM +0530, Akshu Agrawal wrote: >> CHV pipe C hits underrun when we get -ve X values of cursor. To avoid >> this we crop the cursor image for by -ve X value and thus use '0' as >> least X value. > > You're talking about "-ve" here and there's absolutely no "-ve" anywhere > in your patch. That makes your commit message non-understandable. That's shorthand for "negative", and some of the code below is indeed testing for a negative X coordinate, e.g: [snip] >> + /* >> + * There is an issue in CHV PIPE C where we hit underrun on >> + * -ve value of cursor. To avoid this we are cropping the >> + * image for all PIPE C -ve values. >> + */ >> + if (IS_CHERRYVIEW(plane->dev)) { >> + if (pipe == PIPE_C && state->visible && >> + state->base.crtc_x < 0) { >> + ret = vlv_cursor_crop(state, crtc_prev_x); >> + if (ret) >> + return -ENOMEM; >> + } else if (crtc_prev_x < 0) { /* Restore the image back */ >> + char __iomem *base; >> + char __iomem *org_image = state->vlv_cursor_image; >> + int size = obj->base.size; >> + >> + if (org_image == NULL) >> + return -ENOMEM; >> + base = vlv_pin_and_map_buffer_obj(obj); >> + if (base == NULL) >> + return -ENOMEM; >> + memcpy(base, org_image, size); >> + vlv_unpin_buffer_obj(obj, base); >> + } >> + } HTH, .Dave.
Hi,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.7-rc2 next-20160608]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Akshu-Agrawal/Revert-drm-i915-Workaround-CHV-pipe-C-cursor-fail/20160608-163007
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-4.9 (Debian 4.9.3-14) 4.9.3
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_display.c: In function 'vlv_pin_and_map_buffer_obj':
>> drivers/gpu/drm/i915/intel_display.c:14306:36: error: 'struct drm_i915_private' has no member named 'gtt'
buffer_start = ioremap_wc(dev_priv->gtt.mappable_base +
^
vim +14306 drivers/gpu/drm/i915/intel_display.c
14300 ret = i915_gem_object_set_to_gtt_domain(obj, true);
14301 if (ret) {
14302 i915_gem_object_ggtt_unpin(obj);
14303 return NULL;
14304 }
14305
14306 buffer_start = ioremap_wc(dev_priv->gtt.mappable_base +
14307 i915_gem_obj_ggtt_offset(obj), obj->base.size);
14308 if (buffer_start == NULL) {
14309 i915_gem_object_ggtt_unpin(obj);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Jun 8, 2016 at 12:18 PM, Dave Gordon <david.s.gordon@intel.com> wrote: > On 08/06/16 09:40, Daniel Vetter wrote: >> >> On Wed, Jun 08, 2016 at 01:57:44PM +0530, Akshu Agrawal wrote: >>> >>> CHV pipe C hits underrun when we get -ve X values of cursor. To avoid >>> this we crop the cursor image for by -ve X value and thus use '0' as >>> least X value. >> >> >> You're talking about "-ve" here and there's absolutely no "-ve" anywhere >> in your patch. That makes your commit message non-understandable. > > > That's shorthand for "negative", and some of the code below is indeed > testing for a negative X coordinate, e.g: Just type it out, thanks. Not everyone is a native speaker and knows all this stuff. -Daniel
On 6/8/2016 2:10 PM, Daniel Vetter wrote: > On Wed, Jun 08, 2016 at 01:57:44PM +0530, Akshu Agrawal wrote: >> CHV pipe C hits underrun when we get -ve X values of cursor. To avoid >> this we crop the cursor image for by -ve X value and thus use '0' as >> least X value. > > You're talking about "-ve" here and there's absolutely no "-ve" anywhere > in your patch. That makes your commit message non-understandable. Will change -ve to "negative" for better readability. > > But I think I get what you're doing here, and nope, you can't override the > cursor image like that. If we go with this w/a then you need to allocate a > new cursor gem bo instead. This is userspace-visible. Can't we use the same gem bo? As we are calling "i915_gem_object_set_to_gtt_domain" to get write access for CPU after unbinding from GTT. Regards, Akshu > > For similar reasons you're also not allowed to change crtc->state. > -Daniel > >> Signed-off-by: Akshu Agrawal <akshu.agrawal@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 113 +++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 3 + >> 2 files changed, 116 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index bca9245..e6e6568 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -14279,6 +14279,81 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane * >> plane->base.state->rotation); >> } >> >> +static void vlv_unpin_buffer_obj(struct drm_i915_gem_object *obj, >> + char __iomem *buffer_start) >> +{ >> + iounmap(buffer_start); >> + i915_gem_object_ggtt_unpin(obj); >> +} >> + >> +static char __iomem *vlv_pin_and_map_buffer_obj >> + (struct drm_i915_gem_object *obj) >> +{ >> + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; >> + char __iomem *buffer_start; >> + int ret; >> + >> + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); >> + if (ret) >> + return NULL; >> + >> + ret = i915_gem_object_set_to_gtt_domain(obj, true); >> + if (ret) { >> + i915_gem_object_ggtt_unpin(obj); >> + return NULL; >> + } >> + >> + buffer_start = ioremap_wc(dev_priv->gtt.mappable_base + >> + i915_gem_obj_ggtt_offset(obj), obj->base.size); >> + if (buffer_start == NULL) { >> + i915_gem_object_ggtt_unpin(obj); >> + return NULL; >> + } >> + >> + return buffer_start; >> +} >> + >> +static int vlv_cursor_crop(struct intel_plane_state *state, >> + int prev_x) >> +{ >> + struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); >> + struct drm_framebuffer *fb = state->base.fb; >> + char __iomem *buffer = state->vlv_cursor_image; >> + char __iomem *base, *cursor_base; >> + int size = obj->base.size; >> + int i, x = state->base.crtc_x; >> + int bytes_per_pixel = fb->bits_per_pixel / 8; >> + >> + base = vlv_pin_and_map_buffer_obj(obj); >> + if (base == NULL) >> + return -ENOMEM; >> + >> + if (prev_x >= 0) { >> + if (buffer != NULL) >> + kfree(buffer); >> + buffer = kzalloc(size, GFP_KERNEL); >> + state->vlv_cursor_image = buffer; >> + if (buffer == NULL) >> + return -ENOMEM; >> + memcpy(buffer, base, size); >> + } >> + cursor_base = buffer; >> + x = -x; >> + for (i = 0; i < state->base.crtc_h; i++) { >> + cursor_base += x * bytes_per_pixel; >> + memcpy(base, cursor_base, >> + (state->base.crtc_w - x) * bytes_per_pixel); >> + base += (state->base.crtc_w - x) * bytes_per_pixel; >> + memset(base, 0, x * bytes_per_pixel); >> + base += x * bytes_per_pixel; >> + cursor_base += (state->base.crtc_w - x) * bytes_per_pixel; >> + } >> + >> + vlv_unpin_buffer_obj(obj, base); >> + >> + return 0; >> +} >> + >> static int >> intel_check_cursor_plane(struct drm_plane *plane, >> struct intel_crtc_state *crtc_state, >> @@ -14287,8 +14362,10 @@ intel_check_cursor_plane(struct drm_plane *plane, >> struct drm_crtc *crtc = crtc_state->base.crtc; >> struct drm_framebuffer *fb = state->base.fb; >> struct drm_i915_gem_object *obj = intel_fb_obj(fb); >> + enum pipe pipe = to_intel_plane(plane)->pipe; >> unsigned stride; >> int ret; >> + int crtc_prev_x = state->vlv_cursor_prev_x; >> >> ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src, >> &state->dst, &state->clip, >> @@ -14309,6 +14386,32 @@ intel_check_cursor_plane(struct drm_plane *plane, >> return -EINVAL; >> } >> >> + /* >> + * There is an issue in CHV PIPE C where we hit underrun on >> + * -ve value of cursor. To avoid this we are cropping the >> + * image for all PIPE C -ve values. >> + */ >> + if (IS_CHERRYVIEW(plane->dev)) { >> + if (pipe == PIPE_C && state->visible && >> + state->base.crtc_x < 0) { >> + ret = vlv_cursor_crop(state, crtc_prev_x); >> + if (ret) >> + return -ENOMEM; >> + } else if (crtc_prev_x < 0) { /* Restore the image back */ >> + char __iomem *base; >> + char __iomem *org_image = state->vlv_cursor_image; >> + int size = obj->base.size; >> + >> + if (org_image == NULL) >> + return -ENOMEM; >> + base = vlv_pin_and_map_buffer_obj(obj); >> + if (base == NULL) >> + return -ENOMEM; >> + memcpy(base, org_image, size); >> + vlv_unpin_buffer_obj(obj, base); >> + } >> + } >> + >> stride = roundup_pow_of_two(state->base.crtc_w) * 4; >> if (obj->base.size < stride * state->base.crtc_h) { >> DRM_DEBUG_KMS("buffer is too small\n"); >> @@ -14320,6 +14423,16 @@ intel_check_cursor_plane(struct drm_plane *plane, >> return -EINVAL; >> } >> >> + if (IS_CHERRYVIEW(plane->dev)) { >> + if (pipe == PIPE_C && >> + state->visible && state->base.crtc_x < 0) { >> + state->vlv_cursor_prev_x = state->base.crtc_x; >> + state->base.crtc_x = 0; >> + } else { >> + state->vlv_cursor_prev_x = state->base.crtc_x; >> + } >> + } >> + >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index ebe7b34..0c406d1 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -350,6 +350,9 @@ struct intel_plane_state { >> >> /* async flip related structures */ >> struct drm_i915_gem_request *wait_req; >> + >> + char __iomem *vlv_cursor_image; >> + int vlv_cursor_prev_x; >> }; >> >> struct intel_initial_plane_config { >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Fri, Jun 10, 2016 at 03:14:36PM +0530, Agrawal, Akshu wrote: > On 6/8/2016 2:10 PM, Daniel Vetter wrote: > > On Wed, Jun 08, 2016 at 01:57:44PM +0530, Akshu Agrawal wrote: > > > CHV pipe C hits underrun when we get -ve X values of cursor. To avoid > > > this we crop the cursor image for by -ve X value and thus use '0' as > > > least X value. > > > > You're talking about "-ve" here and there's absolutely no "-ve" anywhere > > in your patch. That makes your commit message non-understandable. > > Will change -ve to "negative" for better readability. > > > > > But I think I get what you're doing here, and nope, you can't override the > > cursor image like that. If we go with this w/a then you need to allocate a > > new cursor gem bo instead. This is userspace-visible. > > Can't we use the same gem bo? As we are calling > "i915_gem_object_set_to_gtt_domain" to get write access for CPU after > unbinding from GTT. Userspace can write to the underlying bo without telling us. That'll cause tearing. And it could also read from it, which would result in even more serious bugs. We can't just change userspace-visible state for a w/a. Same reasoning applies to the crtc_state stuff. Again we need copies, to avoid accidentally confusion userspace. With all those copies it's probably best to write a special atomic_update_plane function for cursor pipe C, to avoid spreading all of them all over the driver. -Daniel > > Regards, > Akshu > > > > For similar reasons you're also not allowed to change crtc->state. > > -Daniel > > > > > Signed-off-by: Akshu Agrawal <akshu.agrawal@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 113 +++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 3 + > > > 2 files changed, 116 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index bca9245..e6e6568 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -14279,6 +14279,81 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane * > > > plane->base.state->rotation); > > > } > > > > > > +static void vlv_unpin_buffer_obj(struct drm_i915_gem_object *obj, > > > + char __iomem *buffer_start) > > > +{ > > > + iounmap(buffer_start); > > > + i915_gem_object_ggtt_unpin(obj); > > > +} > > > + > > > +static char __iomem *vlv_pin_and_map_buffer_obj > > > + (struct drm_i915_gem_object *obj) > > > +{ > > > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > > > + char __iomem *buffer_start; > > > + int ret; > > > + > > > + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); > > > + if (ret) > > > + return NULL; > > > + > > > + ret = i915_gem_object_set_to_gtt_domain(obj, true); > > > + if (ret) { > > > + i915_gem_object_ggtt_unpin(obj); > > > + return NULL; > > > + } > > > + > > > + buffer_start = ioremap_wc(dev_priv->gtt.mappable_base + > > > + i915_gem_obj_ggtt_offset(obj), obj->base.size); > > > + if (buffer_start == NULL) { > > > + i915_gem_object_ggtt_unpin(obj); > > > + return NULL; > > > + } > > > + > > > + return buffer_start; > > > +} > > > + > > > +static int vlv_cursor_crop(struct intel_plane_state *state, > > > + int prev_x) > > > +{ > > > + struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); > > > + struct drm_framebuffer *fb = state->base.fb; > > > + char __iomem *buffer = state->vlv_cursor_image; > > > + char __iomem *base, *cursor_base; > > > + int size = obj->base.size; > > > + int i, x = state->base.crtc_x; > > > + int bytes_per_pixel = fb->bits_per_pixel / 8; > > > + > > > + base = vlv_pin_and_map_buffer_obj(obj); > > > + if (base == NULL) > > > + return -ENOMEM; > > > + > > > + if (prev_x >= 0) { > > > + if (buffer != NULL) > > > + kfree(buffer); > > > + buffer = kzalloc(size, GFP_KERNEL); > > > + state->vlv_cursor_image = buffer; > > > + if (buffer == NULL) > > > + return -ENOMEM; > > > + memcpy(buffer, base, size); > > > + } > > > + cursor_base = buffer; > > > + x = -x; > > > + for (i = 0; i < state->base.crtc_h; i++) { > > > + cursor_base += x * bytes_per_pixel; > > > + memcpy(base, cursor_base, > > > + (state->base.crtc_w - x) * bytes_per_pixel); > > > + base += (state->base.crtc_w - x) * bytes_per_pixel; > > > + memset(base, 0, x * bytes_per_pixel); > > > + base += x * bytes_per_pixel; > > > + cursor_base += (state->base.crtc_w - x) * bytes_per_pixel; > > > + } > > > + > > > + vlv_unpin_buffer_obj(obj, base); > > > + > > > + return 0; > > > +} > > > + > > > static int > > > intel_check_cursor_plane(struct drm_plane *plane, > > > struct intel_crtc_state *crtc_state, > > > @@ -14287,8 +14362,10 @@ intel_check_cursor_plane(struct drm_plane *plane, > > > struct drm_crtc *crtc = crtc_state->base.crtc; > > > struct drm_framebuffer *fb = state->base.fb; > > > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > > > + enum pipe pipe = to_intel_plane(plane)->pipe; > > > unsigned stride; > > > int ret; > > > + int crtc_prev_x = state->vlv_cursor_prev_x; > > > > > > ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src, > > > &state->dst, &state->clip, > > > @@ -14309,6 +14386,32 @@ intel_check_cursor_plane(struct drm_plane *plane, > > > return -EINVAL; > > > } > > > > > > + /* > > > + * There is an issue in CHV PIPE C where we hit underrun on > > > + * -ve value of cursor. To avoid this we are cropping the > > > + * image for all PIPE C -ve values. > > > + */ > > > + if (IS_CHERRYVIEW(plane->dev)) { > > > + if (pipe == PIPE_C && state->visible && > > > + state->base.crtc_x < 0) { > > > + ret = vlv_cursor_crop(state, crtc_prev_x); > > > + if (ret) > > > + return -ENOMEM; > > > + } else if (crtc_prev_x < 0) { /* Restore the image back */ > > > + char __iomem *base; > > > + char __iomem *org_image = state->vlv_cursor_image; > > > + int size = obj->base.size; > > > + > > > + if (org_image == NULL) > > > + return -ENOMEM; > > > + base = vlv_pin_and_map_buffer_obj(obj); > > > + if (base == NULL) > > > + return -ENOMEM; > > > + memcpy(base, org_image, size); > > > + vlv_unpin_buffer_obj(obj, base); > > > + } > > > + } > > > + > > > stride = roundup_pow_of_two(state->base.crtc_w) * 4; > > > if (obj->base.size < stride * state->base.crtc_h) { > > > DRM_DEBUG_KMS("buffer is too small\n"); > > > @@ -14320,6 +14423,16 @@ intel_check_cursor_plane(struct drm_plane *plane, > > > return -EINVAL; > > > } > > > > > > + if (IS_CHERRYVIEW(plane->dev)) { > > > + if (pipe == PIPE_C && > > > + state->visible && state->base.crtc_x < 0) { > > > + state->vlv_cursor_prev_x = state->base.crtc_x; > > > + state->base.crtc_x = 0; > > > + } else { > > > + state->vlv_cursor_prev_x = state->base.crtc_x; > > > + } > > > + } > > > + > > > return 0; > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index ebe7b34..0c406d1 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -350,6 +350,9 @@ struct intel_plane_state { > > > > > > /* async flip related structures */ > > > struct drm_i915_gem_request *wait_req; > > > + > > > + char __iomem *vlv_cursor_image; > > > + int vlv_cursor_prev_x; > > > }; > > > > > > struct intel_initial_plane_config { > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Mon, Jun 13, 2016 at 7:52 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jun 10, 2016 at 03:14:36PM +0530, Agrawal, Akshu wrote: >> On 6/8/2016 2:10 PM, Daniel Vetter wrote: >> > On Wed, Jun 08, 2016 at 01:57:44PM +0530, Akshu Agrawal wrote: >> > > CHV pipe C hits underrun when we get -ve X values of cursor. To avoid >> > > this we crop the cursor image for by -ve X value and thus use '0' as >> > > least X value. >> > >> > You're talking about "-ve" here and there's absolutely no "-ve" anywhere >> > in your patch. That makes your commit message non-understandable. >> >> Will change -ve to "negative" for better readability. >> >> > >> > But I think I get what you're doing here, and nope, you can't override the >> > cursor image like that. If we go with this w/a then you need to allocate a >> > new cursor gem bo instead. This is userspace-visible. >> >> Can't we use the same gem bo? As we are calling >> "i915_gem_object_set_to_gtt_domain" to get write access for CPU after >> unbinding from GTT. > > Userspace can write to the underlying bo without telling us. That'll cause > tearing. And it could also read from it, which would result in even more > serious bugs. > > We can't just change userspace-visible state for a w/a. Same reasoning > applies to the crtc_state stuff. Again we need copies, to avoid > accidentally confusion userspace. > Agreed Daniel. Though chances user space doing something like this are less, but nevertheless it can happen. > With all those copies it's probably best to write a special > atomic_update_plane function for cursor pipe C, to avoid spreading all of > them all over the driver. Is implementing a intel_chv_update_pipe_c_cursor_plane and initializing that as the update_plane callback in case of PIPE C on CHV during cursor plane creation is what you have in mind ? We can then implement the copies inside this function. No too sure on the atomic path so just want to confirm if this will work well. Regards Shobhit > -Daniel > >> >> Regards, >> Akshu >> > >> > For similar reasons you're also not allowed to change crtc->state. >> > -Daniel >> > >> > > Signed-off-by: Akshu Agrawal <akshu.agrawal@intel.com> >> > > --- >> > > drivers/gpu/drm/i915/intel_display.c | 113 +++++++++++++++++++++++++++++++++++ >> > > drivers/gpu/drm/i915/intel_drv.h | 3 + >> > > 2 files changed, 116 insertions(+) >> > > >> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > > index bca9245..e6e6568 100644 >> > > --- a/drivers/gpu/drm/i915/intel_display.c >> > > +++ b/drivers/gpu/drm/i915/intel_display.c >> > > @@ -14279,6 +14279,81 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane * >> > > plane->base.state->rotation); >> > > } >> > > >> > > +static void vlv_unpin_buffer_obj(struct drm_i915_gem_object *obj, >> > > + char __iomem *buffer_start) >> > > +{ >> > > + iounmap(buffer_start); >> > > + i915_gem_object_ggtt_unpin(obj); >> > > +} >> > > + >> > > +static char __iomem *vlv_pin_and_map_buffer_obj >> > > + (struct drm_i915_gem_object *obj) >> > > +{ >> > > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; >> > > + char __iomem *buffer_start; >> > > + int ret; >> > > + >> > > + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); >> > > + if (ret) >> > > + return NULL; >> > > + >> > > + ret = i915_gem_object_set_to_gtt_domain(obj, true); >> > > + if (ret) { >> > > + i915_gem_object_ggtt_unpin(obj); >> > > + return NULL; >> > > + } >> > > + >> > > + buffer_start = ioremap_wc(dev_priv->gtt.mappable_base + >> > > + i915_gem_obj_ggtt_offset(obj), obj->base.size); >> > > + if (buffer_start == NULL) { >> > > + i915_gem_object_ggtt_unpin(obj); >> > > + return NULL; >> > > + } >> > > + >> > > + return buffer_start; >> > > +} >> > > + >> > > +static int vlv_cursor_crop(struct intel_plane_state *state, >> > > + int prev_x) >> > > +{ >> > > + struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); >> > > + struct drm_framebuffer *fb = state->base.fb; >> > > + char __iomem *buffer = state->vlv_cursor_image; >> > > + char __iomem *base, *cursor_base; >> > > + int size = obj->base.size; >> > > + int i, x = state->base.crtc_x; >> > > + int bytes_per_pixel = fb->bits_per_pixel / 8; >> > > + >> > > + base = vlv_pin_and_map_buffer_obj(obj); >> > > + if (base == NULL) >> > > + return -ENOMEM; >> > > + >> > > + if (prev_x >= 0) { >> > > + if (buffer != NULL) >> > > + kfree(buffer); >> > > + buffer = kzalloc(size, GFP_KERNEL); >> > > + state->vlv_cursor_image = buffer; >> > > + if (buffer == NULL) >> > > + return -ENOMEM; >> > > + memcpy(buffer, base, size); >> > > + } >> > > + cursor_base = buffer; >> > > + x = -x; >> > > + for (i = 0; i < state->base.crtc_h; i++) { >> > > + cursor_base += x * bytes_per_pixel; >> > > + memcpy(base, cursor_base, >> > > + (state->base.crtc_w - x) * bytes_per_pixel); >> > > + base += (state->base.crtc_w - x) * bytes_per_pixel; >> > > + memset(base, 0, x * bytes_per_pixel); >> > > + base += x * bytes_per_pixel; >> > > + cursor_base += (state->base.crtc_w - x) * bytes_per_pixel; >> > > + } >> > > + >> > > + vlv_unpin_buffer_obj(obj, base); >> > > + >> > > + return 0; >> > > +} >> > > + >> > > static int >> > > intel_check_cursor_plane(struct drm_plane *plane, >> > > struct intel_crtc_state *crtc_state, >> > > @@ -14287,8 +14362,10 @@ intel_check_cursor_plane(struct drm_plane *plane, >> > > struct drm_crtc *crtc = crtc_state->base.crtc; >> > > struct drm_framebuffer *fb = state->base.fb; >> > > struct drm_i915_gem_object *obj = intel_fb_obj(fb); >> > > + enum pipe pipe = to_intel_plane(plane)->pipe; >> > > unsigned stride; >> > > int ret; >> > > + int crtc_prev_x = state->vlv_cursor_prev_x; >> > > >> > > ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src, >> > > &state->dst, &state->clip, >> > > @@ -14309,6 +14386,32 @@ intel_check_cursor_plane(struct drm_plane *plane, >> > > return -EINVAL; >> > > } >> > > >> > > + /* >> > > + * There is an issue in CHV PIPE C where we hit underrun on >> > > + * -ve value of cursor. To avoid this we are cropping the >> > > + * image for all PIPE C -ve values. >> > > + */ >> > > + if (IS_CHERRYVIEW(plane->dev)) { >> > > + if (pipe == PIPE_C && state->visible && >> > > + state->base.crtc_x < 0) { >> > > + ret = vlv_cursor_crop(state, crtc_prev_x); >> > > + if (ret) >> > > + return -ENOMEM; >> > > + } else if (crtc_prev_x < 0) { /* Restore the image back */ >> > > + char __iomem *base; >> > > + char __iomem *org_image = state->vlv_cursor_image; >> > > + int size = obj->base.size; >> > > + >> > > + if (org_image == NULL) >> > > + return -ENOMEM; >> > > + base = vlv_pin_and_map_buffer_obj(obj); >> > > + if (base == NULL) >> > > + return -ENOMEM; >> > > + memcpy(base, org_image, size); >> > > + vlv_unpin_buffer_obj(obj, base); >> > > + } >> > > + } >> > > + >> > > stride = roundup_pow_of_two(state->base.crtc_w) * 4; >> > > if (obj->base.size < stride * state->base.crtc_h) { >> > > DRM_DEBUG_KMS("buffer is too small\n"); >> > > @@ -14320,6 +14423,16 @@ intel_check_cursor_plane(struct drm_plane *plane, >> > > return -EINVAL; >> > > } >> > > >> > > + if (IS_CHERRYVIEW(plane->dev)) { >> > > + if (pipe == PIPE_C && >> > > + state->visible && state->base.crtc_x < 0) { >> > > + state->vlv_cursor_prev_x = state->base.crtc_x; >> > > + state->base.crtc_x = 0; >> > > + } else { >> > > + state->vlv_cursor_prev_x = state->base.crtc_x; >> > > + } >> > > + } >> > > + >> > > return 0; >> > > } >> > > >> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> > > index ebe7b34..0c406d1 100644 >> > > --- a/drivers/gpu/drm/i915/intel_drv.h >> > > +++ b/drivers/gpu/drm/i915/intel_drv.h >> > > @@ -350,6 +350,9 @@ struct intel_plane_state { >> > > >> > > /* async flip related structures */ >> > > struct drm_i915_gem_request *wait_req; >> > > + >> > > + char __iomem *vlv_cursor_image; >> > > + int vlv_cursor_prev_x; >> > > }; >> > > >> > > struct intel_initial_plane_config { >> > > -- >> > > 1.9.1 >> > > >> > > _______________________________________________ >> > > Intel-gfx mailing list >> > > Intel-gfx@lists.freedesktop.org >> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bca9245..e6e6568 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14279,6 +14279,81 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane * plane->base.state->rotation); } +static void vlv_unpin_buffer_obj(struct drm_i915_gem_object *obj, + char __iomem *buffer_start) +{ + iounmap(buffer_start); + i915_gem_object_ggtt_unpin(obj); +} + +static char __iomem *vlv_pin_and_map_buffer_obj + (struct drm_i915_gem_object *obj) +{ + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; + char __iomem *buffer_start; + int ret; + + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); + if (ret) + return NULL; + + ret = i915_gem_object_set_to_gtt_domain(obj, true); + if (ret) { + i915_gem_object_ggtt_unpin(obj); + return NULL; + } + + buffer_start = ioremap_wc(dev_priv->gtt.mappable_base + + i915_gem_obj_ggtt_offset(obj), obj->base.size); + if (buffer_start == NULL) { + i915_gem_object_ggtt_unpin(obj); + return NULL; + } + + return buffer_start; +} + +static int vlv_cursor_crop(struct intel_plane_state *state, + int prev_x) +{ + struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); + struct drm_framebuffer *fb = state->base.fb; + char __iomem *buffer = state->vlv_cursor_image; + char __iomem *base, *cursor_base; + int size = obj->base.size; + int i, x = state->base.crtc_x; + int bytes_per_pixel = fb->bits_per_pixel / 8; + + base = vlv_pin_and_map_buffer_obj(obj); + if (base == NULL) + return -ENOMEM; + + if (prev_x >= 0) { + if (buffer != NULL) + kfree(buffer); + buffer = kzalloc(size, GFP_KERNEL); + state->vlv_cursor_image = buffer; + if (buffer == NULL) + return -ENOMEM; + memcpy(buffer, base, size); + } + cursor_base = buffer; + x = -x; + for (i = 0; i < state->base.crtc_h; i++) { + cursor_base += x * bytes_per_pixel; + memcpy(base, cursor_base, + (state->base.crtc_w - x) * bytes_per_pixel); + base += (state->base.crtc_w - x) * bytes_per_pixel; + memset(base, 0, x * bytes_per_pixel); + base += x * bytes_per_pixel; + cursor_base += (state->base.crtc_w - x) * bytes_per_pixel; + } + + vlv_unpin_buffer_obj(obj, base); + + return 0; +} + static int intel_check_cursor_plane(struct drm_plane *plane, struct intel_crtc_state *crtc_state, @@ -14287,8 +14362,10 @@ intel_check_cursor_plane(struct drm_plane *plane, struct drm_crtc *crtc = crtc_state->base.crtc; struct drm_framebuffer *fb = state->base.fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); + enum pipe pipe = to_intel_plane(plane)->pipe; unsigned stride; int ret; + int crtc_prev_x = state->vlv_cursor_prev_x; ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src, &state->dst, &state->clip, @@ -14309,6 +14386,32 @@ intel_check_cursor_plane(struct drm_plane *plane, return -EINVAL; } + /* + * There is an issue in CHV PIPE C where we hit underrun on + * -ve value of cursor. To avoid this we are cropping the + * image for all PIPE C -ve values. + */ + if (IS_CHERRYVIEW(plane->dev)) { + if (pipe == PIPE_C && state->visible && + state->base.crtc_x < 0) { + ret = vlv_cursor_crop(state, crtc_prev_x); + if (ret) + return -ENOMEM; + } else if (crtc_prev_x < 0) { /* Restore the image back */ + char __iomem *base; + char __iomem *org_image = state->vlv_cursor_image; + int size = obj->base.size; + + if (org_image == NULL) + return -ENOMEM; + base = vlv_pin_and_map_buffer_obj(obj); + if (base == NULL) + return -ENOMEM; + memcpy(base, org_image, size); + vlv_unpin_buffer_obj(obj, base); + } + } + stride = roundup_pow_of_two(state->base.crtc_w) * 4; if (obj->base.size < stride * state->base.crtc_h) { DRM_DEBUG_KMS("buffer is too small\n"); @@ -14320,6 +14423,16 @@ intel_check_cursor_plane(struct drm_plane *plane, return -EINVAL; } + if (IS_CHERRYVIEW(plane->dev)) { + if (pipe == PIPE_C && + state->visible && state->base.crtc_x < 0) { + state->vlv_cursor_prev_x = state->base.crtc_x; + state->base.crtc_x = 0; + } else { + state->vlv_cursor_prev_x = state->base.crtc_x; + } + } + return 0; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ebe7b34..0c406d1 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -350,6 +350,9 @@ struct intel_plane_state { /* async flip related structures */ struct drm_i915_gem_request *wait_req; + + char __iomem *vlv_cursor_image; + int vlv_cursor_prev_x; }; struct intel_initial_plane_config {
CHV pipe C hits underrun when we get -ve X values of cursor. To avoid this we crop the cursor image for by -ve X value and thus use '0' as least X value. Signed-off-by: Akshu Agrawal <akshu.agrawal@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 113 +++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 3 + 2 files changed, 116 insertions(+)