| Message ID | 1464010661-32674-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 23, 2016 at 03:37:41PM +0200, Maarten Lankhorst wrote: > With nonblocking unpin there can be many cursor pins before they're > cleared by the next page flip. > > Fix this by extending pin_count to the full 32-bit to prevent a > WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT) This is a hack that affects non-KMS paths. Being able to process binding in a single operation on all architectures is something we want to preserve. Why is every cursor movement generating an unpin work? Should I just start poking registers from userspace to avoid a silly kerenl? -Chris
Op 23-05-16 om 16:25 schreef Chris Wilson: > On Mon, May 23, 2016 at 03:37:41PM +0200, Maarten Lankhorst wrote: >> With nonblocking unpin there can be many cursor pins before they're >> cleared by the next page flip. >> >> Fix this by extending pin_count to the full 32-bit to prevent a >> WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT) > This is a hack that affects non-KMS paths. Being able to process binding > in a single operation on all architectures is something we want to > preserve. > > Why is every cursor movement generating an unpin work? Should I just > start poking registers from userspace to avoid a silly kerenl? > -Chris > All the unpin work gets batched till after the next vblank, it's not very efficient but if you want to fix it you should just add the vma to plane state already. According to Ville unpin count would still be too low on BXT/SKL, so it wouldn't remove the need for this patch anyway..
On Mon, May 23, 2016 at 05:09:05PM +0200, Maarten Lankhorst wrote: > Op 23-05-16 om 16:25 schreef Chris Wilson: > > On Mon, May 23, 2016 at 03:37:41PM +0200, Maarten Lankhorst wrote: > >> With nonblocking unpin there can be many cursor pins before they're > >> cleared by the next page flip. > >> > >> Fix this by extending pin_count to the full 32-bit to prevent a > >> WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT) > > This is a hack that affects non-KMS paths. Being able to process binding > > in a single operation on all architectures is something we want to > > preserve. > > > > Why is every cursor movement generating an unpin work? Should I just > > start poking registers from userspace to avoid a silly kerenl? > > -Chris > > > All the unpin work gets batched till after the next vblank, it's not very efficient > but if you want to fix it you should just add the vma to plane state already. I still don't see where the flush will occur, or why vblanks would be running at all for cursor updates. > According to Ville unpin count would still be too low on BXT/SKL, so it wouldn't > remove the need for this patch anyway.. Increasing the count is one thing, taking all 32bits as a workaround for poor behaviour in the cursor update is another. -Chris
Op 23-05-16 om 17:43 schreef Chris Wilson: > On Mon, May 23, 2016 at 05:09:05PM +0200, Maarten Lankhorst wrote: >> Op 23-05-16 om 16:25 schreef Chris Wilson: >>> On Mon, May 23, 2016 at 03:37:41PM +0200, Maarten Lankhorst wrote: >>>> With nonblocking unpin there can be many cursor pins before they're >>>> cleared by the next page flip. >>>> >>>> Fix this by extending pin_count to the full 32-bit to prevent a >>>> WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT) >>> This is a hack that affects non-KMS paths. Being able to process binding >>> in a single operation on all architectures is something we want to >>> preserve. >>> >>> Why is every cursor movement generating an unpin work? Should I just >>> start poking registers from userspace to avoid a silly kerenl? >>> -Chris >>> >> All the unpin work gets batched till after the next vblank, it's not very efficient >> but if you want to fix it you should just add the vma to plane state already. > I still don't see where the flush will occur, or why vblanks would be > running at all for cursor updates. Next page flip. All cursor updates are added to flip_work list and are cleared on vblank. >> According to Ville unpin count would still be too low on BXT/SKL, so it wouldn't >> remove the need for this patch anyway.. > Increasing the count is one thing, taking all 32bits as a workaround for > poor behaviour in the cursor update is another. > -Chris >
On Mon, May 23, 2016 at 06:09:31PM +0200, Maarten Lankhorst wrote: > Op 23-05-16 om 17:43 schreef Chris Wilson: > > On Mon, May 23, 2016 at 05:09:05PM +0200, Maarten Lankhorst wrote: > >> Op 23-05-16 om 16:25 schreef Chris Wilson: > >>> On Mon, May 23, 2016 at 03:37:41PM +0200, Maarten Lankhorst wrote: > >>>> With nonblocking unpin there can be many cursor pins before they're > >>>> cleared by the next page flip. > >>>> > >>>> Fix this by extending pin_count to the full 32-bit to prevent a > >>>> WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT) > >>> This is a hack that affects non-KMS paths. Being able to process binding > >>> in a single operation on all architectures is something we want to > >>> preserve. > >>> > >>> Why is every cursor movement generating an unpin work? Should I just > >>> start poking registers from userspace to avoid a silly kerenl? > >>> -Chris > >>> > >> All the unpin work gets batched till after the next vblank, it's not very efficient > >> but if you want to fix it you should just add the vma to plane state already. > > I still don't see where the flush will occur, or why vblanks would be > > running at all for cursor updates. > Next page flip. All cursor updates are added to flip_work list and are cleared on vblank. Now knowing the trigger, I've written a reproducer: igt/kms_cursor_legacy -Chris
On Mon, May 23, 2016 at 03:37:41PM +0200, Maarten Lankhorst wrote: > With nonblocking unpin there can be many cursor pins before they're > cleared by the next page flip. > > Fix this by extending pin_count to the full 32-bit to prevent a > WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reported-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: a6747b7304a9 ("drm/i915: Make unpin async.") > --- > drivers/gpu/drm/i915/i915_gem_gtt.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 62be77cac5cd..1d43cc290f71 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -218,8 +218,8 @@ struct i915_vma { > * > * In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3 > * bits with absolutely no headroom. So use 4 bits. */ > - unsigned int pin_count:4; > -#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf > + unsigned int pin_count; > +#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT UINT_MAX You need to read up on some of the history of this. The problem is that some peeps have too many rt threads and managed to sufficiently stall our unpin worker until we ran out of memory. Well, we would have if not for this check here. The real fix should be to eventually stall for the unpin workers to complete (like we do/did in the old pageflip code), or to have an explicit (per-crtc probably) list of to-be-unpinned stuff, so that we can process old unpins synchronously once they're completed. Just bumping the limit so that no one notices that we leak pin counts like bad ain't a fix ;-) -Daniel > }; > > struct i915_page_dma { > -- > 2.5.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 62be77cac5cd..1d43cc290f71 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -218,8 +218,8 @@ struct i915_vma { * * In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3 * bits with absolutely no headroom. So use 4 bits. */ - unsigned int pin_count:4; -#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf + unsigned int pin_count; +#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT UINT_MAX }; struct i915_page_dma {
With nonblocking unpin there can be many cursor pins before they're cleared by the next page flip. Fix this by extending pin_count to the full 32-bit to prevent a WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reported-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Fixes: a6747b7304a9 ("drm/i915: Make unpin async.") --- drivers/gpu/drm/i915/i915_gem_gtt.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)