Re: [PATCH 2/2] drm/i915: Crop cursor image for CHV pipe C cursor issue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 13, 2016 at 7:52 PM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx>
>> > > ---
>> > >  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@xxxxxxxxxxxxxxxxxxxxx
>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux