| Message ID | ba4558149d3c81305c975817b3972363c3b310cd.1447378621.git.agoins@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote: > If a buffer is backed by dmabuf, wait on its reservation object's fences > before flipping. > > Signed-off-by: Alex Goins <agoins@nvidia.com> > --- > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index b2270d5..acec108a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -44,6 +44,8 @@ > #include <drm/drm_plane_helper.h> > #include <drm/drm_rect.h> > #include <linux/dma_remapping.h> > +#include <linux/reservation.h> > +#include <linux/dma-buf.h> > > /* Primary plane formats for gen <= 3 */ > static const uint32_t i8xx_primary_formats[] = { > @@ -11170,10 +11172,19 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc) > static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) > { > struct drm_device *dev = intel_crtc->base.dev; > + struct drm_i915_gem_object *pending_flip_obj = > + intel_crtc->unpin_work->pending_flip_obj; > u32 start_vbl_count; > > intel_mark_page_flip_active(intel_crtc); > > + /* For framebuffer backed by dmabuf, wait for fence */ > + if (pending_flip_obj->base.dma_buf) { > + reservation_object_wait_timeout_rcu( > + pending_flip_obj->base.dma_buf->resv, > + true, false, msecs_to_jiffies(96)); > + } This wait should be prior to marking the flip as waiting for the flip-completion interrupt. My personal preference (aside from putting this next to the other wait) would to have been to use crtc->primary->fb to match the do_mmip_flips funcs (I expect that we will eliminate the pending_flip_obj in the near future). Also we are missing the addition of if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl) return true; to use_mmio_flip(). -Chris
Hi, On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote: >> static const uint32_t i8xx_primary_formats[] = { >> @@ -11170,10 +11172,19 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc) >> static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) >> { >> struct drm_device *dev = intel_crtc->base.dev; >> + struct drm_i915_gem_object *pending_flip_obj = >> + intel_crtc->unpin_work->pending_flip_obj; >> u32 start_vbl_count; >> >> intel_mark_page_flip_active(intel_crtc); >> >> + /* For framebuffer backed by dmabuf, wait for fence */ >> + if (pending_flip_obj->base.dma_buf) { >> + reservation_object_wait_timeout_rcu( >> + pending_flip_obj->base.dma_buf->resv, >> + true, false, msecs_to_jiffies(96)); >> + } > > This wait should be prior to marking the flip as waiting for the > flip-completion interrupt. My personal preference (aside from putting > this next to the other wait) would to have been to use crtc->primary->fb > to match the do_mmip_flips funcs (I expect that we will eliminate the > pending_flip_obj in the near future). No, don't use crtc->primary->fb for anything. Cheers, Daniel
On Fri, Nov 13, 2015 at 10:38:07AM +0000, Daniel Stone wrote: > Hi, > > On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote: > >> static const uint32_t i8xx_primary_formats[] = { > >> @@ -11170,10 +11172,19 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc) > >> static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) > >> { > >> struct drm_device *dev = intel_crtc->base.dev; > >> + struct drm_i915_gem_object *pending_flip_obj = > >> + intel_crtc->unpin_work->pending_flip_obj; > >> u32 start_vbl_count; > >> > >> intel_mark_page_flip_active(intel_crtc); > >> > >> + /* For framebuffer backed by dmabuf, wait for fence */ > >> + if (pending_flip_obj->base.dma_buf) { > >> + reservation_object_wait_timeout_rcu( > >> + pending_flip_obj->base.dma_buf->resv, > >> + true, false, msecs_to_jiffies(96)); > >> + } > > > > This wait should be prior to marking the flip as waiting for the > > flip-completion interrupt. My personal preference (aside from putting > > this next to the other wait) would to have been to use crtc->primary->fb > > to match the do_mmip_flips funcs (I expect that we will eliminate the > > pending_flip_obj in the near future). > > No, don't use crtc->primary->fb for anything. s/crtc->primary->fb/whatever is actually used by the ilk_do_mmio_flip and co which is certainly not pending_flip_obj/ -Chris
Sorry; needless to say I'm not super familiar with the Intel driver. ilk_do_mmio_flip() uses crtc->primary->fb to fetch the gem object:
struct intel_framebuffer *intel_fb =
to_intel_framebuffer(intel_crtc->base.primary->fb);
struct drm_i915_gem_object *obj = intel_fb->obj;
Given that, would it be okay for me to do the same?
Thanks,
Alex
-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
Sent: Friday, November 13, 2015 2:46 AM
To: Daniel Stone
Cc: Alexander Goins; dri-devel; Daniel Vetter; Maarten Lankhorst
Subject: Re: [PATCH i915 v3 1/2] i915: wait for fences in mmio_flip()
On Fri, Nov 13, 2015 at 10:38:07AM +0000, Daniel Stone wrote:
> Hi,
>
> On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote:
> >> static const uint32_t i8xx_primary_formats[] = { @@ -11170,10
> >> +11172,19 @@ static void ilk_do_mmio_flip(struct intel_crtc
> >> *intel_crtc) static void intel_do_mmio_flip(struct intel_crtc
> >> *intel_crtc) {
> >> struct drm_device *dev = intel_crtc->base.dev;
> >> + struct drm_i915_gem_object *pending_flip_obj =
> >> + intel_crtc->unpin_work->pending_flip_obj;
> >> u32 start_vbl_count;
> >>
> >> intel_mark_page_flip_active(intel_crtc);
> >>
> >> + /* For framebuffer backed by dmabuf, wait for fence */
> >> + if (pending_flip_obj->base.dma_buf) {
> >> + reservation_object_wait_timeout_rcu(
> >> + pending_flip_obj->base.dma_buf->resv,
> >> + true, false, msecs_to_jiffies(96));
> >> + }
> >
> > This wait should be prior to marking the flip as waiting for the
> > flip-completion interrupt. My personal preference (aside from
> > putting this next to the other wait) would to have been to use
> > crtc->primary->fb to match the do_mmip_flips funcs (I expect that we
> > will eliminate the pending_flip_obj in the near future).
>
> No, don't use crtc->primary->fb for anything.
s/crtc->primary->fb/whatever is actually used by the ilk_do_mmio_flip and co which is certainly not pending_flip_obj/ -Chris
--
Chris Wilson, Intel Open Source Technology Centre
Okay, implemented these suggestions. Will send out v4 patch set once the crtc->primary->fb thing is cleared up. -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Friday, November 13, 2015 2:08 AM To: Alexander Goins Cc: dri-devel@lists.freedesktop.org; daniel@fooishbar.org; daniel@ffwll.ch; maarten.lankhorst@linux.intel.com Subject: Re: [PATCH i915 v3 1/2] i915: wait for fences in mmio_flip() On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote: > If a buffer is backed by dmabuf, wait on its reservation object's > fences before flipping. > > Signed-off-by: Alex Goins <agoins@nvidia.com> > --- > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index b2270d5..acec108a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -44,6 +44,8 @@ > #include <drm/drm_plane_helper.h> > #include <drm/drm_rect.h> > #include <linux/dma_remapping.h> > +#include <linux/reservation.h> > +#include <linux/dma-buf.h> > > /* Primary plane formats for gen <= 3 */ static const uint32_t > i8xx_primary_formats[] = { @@ -11170,10 +11172,19 @@ static void > ilk_do_mmio_flip(struct intel_crtc *intel_crtc) static void > intel_do_mmio_flip(struct intel_crtc *intel_crtc) { > struct drm_device *dev = intel_crtc->base.dev; > + struct drm_i915_gem_object *pending_flip_obj = > + intel_crtc->unpin_work->pending_flip_obj; > u32 start_vbl_count; > > intel_mark_page_flip_active(intel_crtc); > > + /* For framebuffer backed by dmabuf, wait for fence */ > + if (pending_flip_obj->base.dma_buf) { > + reservation_object_wait_timeout_rcu( > + pending_flip_obj->base.dma_buf->resv, > + true, false, msecs_to_jiffies(96)); > + } This wait should be prior to marking the flip as waiting for the flip-completion interrupt. My personal preference (aside from putting this next to the other wait) would to have been to use crtc->primary->fb to match the do_mmip_flips funcs (I expect that we will eliminate the pending_flip_obj in the near future). Also we are missing the addition of if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl) return true; to use_mmio_flip(). -Chris -- Chris Wilson, Intel Open Source Technology Centre
On Fri, Nov 13, 2015 at 07:38:21PM +0000, Alexander Goins wrote: > Sorry; needless to say I'm not super familiar with the Intel driver. ilk_do_mmio_flip() uses crtc->primary->fb to fetch the gem object: > > struct intel_framebuffer *intel_fb = > to_intel_framebuffer(intel_crtc->base.primary->fb); > struct drm_i915_gem_object *obj = intel_fb->obj; > > Given that, would it be okay for me to do the same? I think so, since this is the legacy page_flip. Which will hopefully disappear real soon now ... -Daniel > > Thanks, > Alex > > -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Friday, November 13, 2015 2:46 AM > To: Daniel Stone > Cc: Alexander Goins; dri-devel; Daniel Vetter; Maarten Lankhorst > Subject: Re: [PATCH i915 v3 1/2] i915: wait for fences in mmio_flip() > > On Fri, Nov 13, 2015 at 10:38:07AM +0000, Daniel Stone wrote: > > Hi, > > > > On 13 November 2015 at 10:08, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > On Thu, Nov 12, 2015 at 05:49:28PM -0800, Alex Goins wrote: > > >> static const uint32_t i8xx_primary_formats[] = { @@ -11170,10 > > >> +11172,19 @@ static void ilk_do_mmio_flip(struct intel_crtc > > >> *intel_crtc) static void intel_do_mmio_flip(struct intel_crtc > > >> *intel_crtc) { > > >> struct drm_device *dev = intel_crtc->base.dev; > > >> + struct drm_i915_gem_object *pending_flip_obj = > > >> + intel_crtc->unpin_work->pending_flip_obj; > > >> u32 start_vbl_count; > > >> > > >> intel_mark_page_flip_active(intel_crtc); > > >> > > >> + /* For framebuffer backed by dmabuf, wait for fence */ > > >> + if (pending_flip_obj->base.dma_buf) { > > >> + reservation_object_wait_timeout_rcu( > > >> + pending_flip_obj->base.dma_buf->resv, > > >> + true, false, msecs_to_jiffies(96)); > > >> + } > > > > > > This wait should be prior to marking the flip as waiting for the > > > flip-completion interrupt. My personal preference (aside from > > > putting this next to the other wait) would to have been to use > > > crtc->primary->fb to match the do_mmip_flips funcs (I expect that we > > > will eliminate the pending_flip_obj in the near future). > > > > No, don't use crtc->primary->fb for anything. > > s/crtc->primary->fb/whatever is actually used by the ilk_do_mmio_flip and co which is certainly not pending_flip_obj/ -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > ----------------------------------------------------------------------------------- > This email message is for the sole use of the intended recipient(s) and may contain > confidential information. Any unauthorized review, use, disclosure or distribution > is prohibited. If you are not the intended recipient, please contact the sender by > reply email and destroy all copies of the original message. > -----------------------------------------------------------------------------------
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b2270d5..acec108a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -44,6 +44,8 @@ #include <drm/drm_plane_helper.h> #include <drm/drm_rect.h> #include <linux/dma_remapping.h> +#include <linux/reservation.h> +#include <linux/dma-buf.h> /* Primary plane formats for gen <= 3 */ static const uint32_t i8xx_primary_formats[] = { @@ -11170,10 +11172,19 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc) static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) { struct drm_device *dev = intel_crtc->base.dev; + struct drm_i915_gem_object *pending_flip_obj = + intel_crtc->unpin_work->pending_flip_obj; u32 start_vbl_count; intel_mark_page_flip_active(intel_crtc); + /* For framebuffer backed by dmabuf, wait for fence */ + if (pending_flip_obj->base.dma_buf) { + reservation_object_wait_timeout_rcu( + pending_flip_obj->base.dma_buf->resv, + true, false, msecs_to_jiffies(96)); + } + intel_pipe_update_start(intel_crtc, &start_vbl_count); if (INTEL_INFO(dev)->gen >= 9)
If a buffer is backed by dmabuf, wait on its reservation object's fences before flipping. Signed-off-by: Alex Goins <agoins@nvidia.com> --- drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ 1 file changed, 11 insertions(+)