| Message ID | 1423500395-1787-6-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 09, 2015 at 02:46:31PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We need this for FBC, and possibly for PSR too. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 3d198f8..15910fa 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1111,6 +1111,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, > ret = i915_gem_phys_pwrite(obj, args, file); > else > ret = i915_gem_shmem_pwrite(dev, obj, args, file); > + > + intel_fb_obj_flush(obj, false, ORIGIN_CPU); > + } else { > + intel_fb_obj_flush(obj, false, ORIGIN_GTT); A flush alone does nothing. Well should, but you're kinda not quite using it correctly in the next patch to convert fbc over to frontbuffer tracking. I guess the docs aren't perfect, so let me try again. There are two kinds of events the frontbuffer tracking code supplies to tell its consumers that screen changes are happening: - invalidate/flush: Invalidate denotes the start of the frontbuffer rendering, from that point on psr/fbc/drrs must update the screen with the usual refresh rate and not cache anything anywhere. When the flush happens (which could easily be after a _very_ long time, e.g. fbcon) then only can caching recomence. Caching = enable fbc, allow psr or reduce refresh rate. - flip events: That's an instantenous event (well at least for consumer, internally we need to track it as prepare/complete for async flips), and mostly just interesting when the hw doesn't notice flips (some psr modes and drrs). So if you want to add frontbuffer tracking to pwrite then we need both an invalidate (before the actual pwrite) and a flush (after the pwrite, like you've added here). The other issue is that there's a bug with the origin assignemnt: phys_pwrite also goes through the gtt. I think it would be best if we push the fb_obj_invalidate/flush into the relevant pwrite functions. That should make it easier to review, since the invalidate/flush will be next to the write op. -Daniel > } > > out: > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Feb 09, 2015 at 07:41:17PM +0100, Daniel Vetter wrote: > On Mon, Feb 09, 2015 at 02:46:31PM -0200, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > We need this for FBC, and possibly for PSR too. > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 3d198f8..15910fa 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1111,6 +1111,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, > > ret = i915_gem_phys_pwrite(obj, args, file); > > else > > ret = i915_gem_shmem_pwrite(dev, obj, args, file); > > + > > + intel_fb_obj_flush(obj, false, ORIGIN_CPU); > > + } else { > > + intel_fb_obj_flush(obj, false, ORIGIN_GTT); > > A flush alone does nothing. Well should, but you're kinda not quite using > it correctly in the next patch to convert fbc over to frontbuffer > tracking. > > I guess the docs aren't perfect, so let me try again. There are two kinds > of events the frontbuffer tracking code supplies to tell its consumers > that screen changes are happening: > - invalidate/flush: Invalidate denotes the start of the frontbuffer > rendering, from that point on psr/fbc/drrs must update the screen with > the usual refresh rate and not cache anything anywhere. When the flush > happens (which could easily be after a _very_ long time, e.g. fbcon) > then only can caching recomence. Caching = enable fbc, allow psr or > reduce refresh rate. > - flip events: That's an instantenous event (well at least for consumer, > internally we need to track it as prepare/complete for async flips), and > mostly just interesting when the hw doesn't notice flips (some psr modes > and drrs). > > So if you want to add frontbuffer tracking to pwrite then we need both an > invalidate (before the actual pwrite) and a flush (after the pwrite, like > you've added here). > > The other issue is that there's a bug with the origin assignemnt: > phys_pwrite also goes through the gtt. I think it would be best if we push > the fb_obj_invalidate/flush into the relevant pwrite functions. That > should make it easier to review, since the invalidate/flush will be next > to the write op. btw what's the use-case here? We don't upload stuff to X-tiled buffers with pwrite, so this isn't really relevant for fbc I think. It is a real gap for psr though, since cursor updates are done with pwrite. But that probably gets papered over by X also updating the position when the image changes, which means we'll get another (hw) flip combo. But that's just X, and we indeed don't have a pwrite cursor case yet in the psr testcase. -Daniel
On Wed, 11 Feb 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Feb 09, 2015 at 07:41:17PM +0100, Daniel Vetter wrote: >> On Mon, Feb 09, 2015 at 02:46:31PM -0200, Paulo Zanoni wrote: >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> > We need this for FBC, and possibly for PSR too. >> > >> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_gem.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> > index 3d198f8..15910fa 100644 >> > --- a/drivers/gpu/drm/i915/i915_gem.c >> > +++ b/drivers/gpu/drm/i915/i915_gem.c >> > @@ -1111,6 +1111,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, >> > ret = i915_gem_phys_pwrite(obj, args, file); >> > else >> > ret = i915_gem_shmem_pwrite(dev, obj, args, file); >> > + >> > + intel_fb_obj_flush(obj, false, ORIGIN_CPU); >> > + } else { >> > + intel_fb_obj_flush(obj, false, ORIGIN_GTT); >> >> A flush alone does nothing. Well should, but you're kinda not quite using >> it correctly in the next patch to convert fbc over to frontbuffer >> tracking. >> >> I guess the docs aren't perfect, so let me try again. There are two kinds >> of events the frontbuffer tracking code supplies to tell its consumers >> that screen changes are happening: >> - invalidate/flush: Invalidate denotes the start of the frontbuffer >> rendering, from that point on psr/fbc/drrs must update the screen with >> the usual refresh rate and not cache anything anywhere. When the flush >> happens (which could easily be after a _very_ long time, e.g. fbcon) >> then only can caching recomence. Caching = enable fbc, allow psr or >> reduce refresh rate. >> - flip events: That's an instantenous event (well at least for consumer, >> internally we need to track it as prepare/complete for async flips), and >> mostly just interesting when the hw doesn't notice flips (some psr modes >> and drrs). >> >> So if you want to add frontbuffer tracking to pwrite then we need both an >> invalidate (before the actual pwrite) and a flush (after the pwrite, like >> you've added here). >> >> The other issue is that there's a bug with the origin assignemnt: >> phys_pwrite also goes through the gtt. I think it would be best if we push >> the fb_obj_invalidate/flush into the relevant pwrite functions. That >> should make it easier to review, since the invalidate/flush will be next >> to the write op. > > btw what's the use-case here? We don't upload stuff to X-tiled buffers > with pwrite, so this isn't really relevant for fbc I think. > > It is a real gap for psr though, since cursor updates are done with > pwrite. But that probably gets papered over by X also updating the > position when the image changes, which means we'll get another (hw) > flip combo. But that's just X, and we indeed don't have a pwrite cursor > case yet in the psr testcase. FYI, you asked to test an earlier version of this patch in https://bugs.freedesktop.org/show_bug.cgi?id=87143 BR, Jani > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-02-11 6:30 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Feb 09, 2015 at 07:41:17PM +0100, Daniel Vetter wrote: >> On Mon, Feb 09, 2015 at 02:46:31PM -0200, Paulo Zanoni wrote: >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> > We need this for FBC, and possibly for PSR too. >> > >> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_gem.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> > index 3d198f8..15910fa 100644 >> > --- a/drivers/gpu/drm/i915/i915_gem.c >> > +++ b/drivers/gpu/drm/i915/i915_gem.c >> > @@ -1111,6 +1111,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, >> > ret = i915_gem_phys_pwrite(obj, args, file); >> > else >> > ret = i915_gem_shmem_pwrite(dev, obj, args, file); >> > + >> > + intel_fb_obj_flush(obj, false, ORIGIN_CPU); >> > + } else { >> > + intel_fb_obj_flush(obj, false, ORIGIN_GTT); >> >> A flush alone does nothing. Well should, but you're kinda not quite using >> it correctly in the next patch to convert fbc over to frontbuffer >> tracking. >> >> I guess the docs aren't perfect, so let me try again. There are two kinds >> of events the frontbuffer tracking code supplies to tell its consumers >> that screen changes are happening: >> - invalidate/flush: Invalidate denotes the start of the frontbuffer >> rendering, from that point on psr/fbc/drrs must update the screen with >> the usual refresh rate and not cache anything anywhere. When the flush >> happens (which could easily be after a _very_ long time, e.g. fbcon) >> then only can caching recomence. Caching = enable fbc, allow psr or >> reduce refresh rate. >> - flip events: That's an instantenous event (well at least for consumer, >> internally we need to track it as prepare/complete for async flips), and >> mostly just interesting when the hw doesn't notice flips (some psr modes >> and drrs). >> >> So if you want to add frontbuffer tracking to pwrite then we need both an >> invalidate (before the actual pwrite) and a flush (after the pwrite, like >> you've added here). >> >> The other issue is that there's a bug with the origin assignemnt: >> phys_pwrite also goes through the gtt. I think it would be best if we push >> the fb_obj_invalidate/flush into the relevant pwrite functions. That >> should make it easier to review, since the invalidate/flush will be next >> to the write op. > > btw what's the use-case here? We don't upload stuff to X-tiled buffers > with pwrite, so this isn't really relevant for fbc I think. We can do this, I wrote a testcase for that. Of course, user space needs to be aware of the tiling, but that's documented on the PRM. > > It is a real gap for psr though, since cursor updates are done with > pwrite. A lot of the tests I wrote can be used by PSR, I should plan on doing this later. > But that probably gets papered over by X also updating the > position when the image changes, which means we'll get another (hw) > flip combo. But that's just X, and we indeed don't have a pwrite cursor > case yet in the psr testcase. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3d198f8..15910fa 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1111,6 +1111,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, ret = i915_gem_phys_pwrite(obj, args, file); else ret = i915_gem_shmem_pwrite(dev, obj, args, file); + + intel_fb_obj_flush(obj, false, ORIGIN_CPU); + } else { + intel_fb_obj_flush(obj, false, ORIGIN_GTT); } out: