Re: [PATCH v2 7/8] drm/i915/display/skl+: Drop frontbuffer rendering support

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

 



On Thu, 2021-09-02 at 21:42 +0300, Gwan-gyeong Mun wrote:
> 
> On 8/25/21 3:58 AM, José Roberto de Souza wrote:
> > By now all the userspace applications should have migrated to atomic
> > or at least be calling DRM_IOCTL_MODE_DIRTYFB.
> > 
> > With that we can kill frontbuffer rendering support in i915 for
> > modern platforms.
> > 
> > So here converting legacy APIs into atomic commits so it can be
> > properly handled by driver i915.
> > 
> > Several IGT tests will fail with this changes, because some tests
> > were stressing those frontbuffer rendering scenarios that no userspace
> > should be using by now, fixes to IGT should be sent soon.
> > 
> > v2:
> > - return earlier to not set fb_tracking.busy/flip_bits
> > - added a warn on to make sure we are not setting the busy/flip_bits
> > 
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/display/intel_cursor.c    |  6 ++----
> >   drivers/gpu/drm/i915/display/intel_fb.c        |  8 +++++++-
> >   .../gpu/drm/i915/display/intel_frontbuffer.c   | 18 ++++++++++++++++++
> >   drivers/gpu/drm/i915/i915_drv.h                |  2 ++
> >   4 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index c7618fef01439..5aa996c3b7980 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> >   			   u32 src_w, u32 src_h,
> >   			   struct drm_modeset_acquire_ctx *ctx)
> >   {
> > +	struct drm_i915_private *i915 = to_i915(_crtc->dev);
> >   	struct intel_plane *plane = to_intel_plane(_plane);
> >   	struct intel_crtc *crtc = to_intel_crtc(_crtc);
> >   	struct intel_plane_state *old_plane_state =
> > @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> >   	 * PSR2 selective fetch also requires the slow path as
> >   	 * PSR2 plane and transcoder registers can only be updated during
> >   	 * vblank.
> > -	 *
> > -	 * FIXME bigjoiner fastpath would be good
> >   	 */
> >   	if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
> > -	    crtc_state->update_pipe || crtc_state->bigjoiner ||
> > -	    crtc_state->enable_psr2_sel_fetch)
> > +	    crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915))
> >   		goto slow;
> >   
> >   	/*
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index e4b8602ec0cd2..3eb60785c9f29 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -3,6 +3,7 @@
> >    * Copyright © 2021 Intel Corporation
> >    */
> >   
> > +#include <drm/drm_damage_helper.h>
> >   #include <drm/drm_framebuffer.h>
> >   #include <drm/drm_modeset_helper.h>
> >   
> > @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> >   					unsigned int num_clips)
> >   {
> >   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> >   
> >   	i915_gem_object_flush_if_display(obj);
> > -	intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
> >   
> > +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> > +		return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips,
> > +						 num_clips);
> Hi,
> Even if the userspace informs us of the dirty (damage) region of the 
> front buffer being used, GEN9 to GEN12 still uses the HW Tracking 
> function for PSR and FBC.
> And if you look at the description of the intel_psr_flush() function, 
> you can see that there are the following restrictions.
> 
> "Since the hardware frontbuffer tracking has gaps we need to integrate 
> with the software frontbuffer tracking."
> 
> If this restriction is still valid from GEN9 to GEN12, even if the 
> existing frontbuffer tracking function is not used, when 
> intel_user_framebuffer_dirty() is called, in the case of PSR, 
> psr_force_hw_tracking_exit() is called or intel_psr_exit() and 
> schedule_work(psr.work) seems to be required.

As this will trigger calls to the functions that write the plane registers PSR HW tracking and FBC tracking will understand a page flip happened even
if going from and to the same surface.

But will double check it.

> 
> In the case of FBC, it seems that calls to FBC deactive / FBC activate 
> should be added.
> 
> If GEN9 to GEN12 do not have the above restrictions, please ignore this 
> comment.
> 
> G.G.
> > +
> > +	intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
> >   	return 0;
> >   }
> >   
> > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > index 0492446cd04ad..3860f87dac31c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915,
> >   void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915,
> >   				    unsigned frontbuffer_bits)
> >   {
> > +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> > +		return;
> > +
> >   	spin_lock(&i915->fb_tracking.lock);
> >   	i915->fb_tracking.flip_bits |= frontbuffer_bits;
> >   	/* Remove stale busy bits due to the old buffer. */
> > @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915,
> >   void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
> >   				     unsigned frontbuffer_bits)
> >   {
> > +	if (!HAS_FRONTBUFFER_RENDERING(i915)) {
> > +		drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits |
> > +					     i915->fb_tracking.busy_bits);
> > +		return;
> > +	}
> > +
> >   	spin_lock(&i915->fb_tracking.lock);
> >   	/* Mask any cancelled flips. */
> >   	frontbuffer_bits &= i915->fb_tracking.flip_bits;
> > @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
> >   void intel_frontbuffer_flip(struct drm_i915_private *i915,
> >   			    unsigned frontbuffer_bits)
> >   {
> > +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> > +		return;
> > +
> >   	spin_lock(&i915->fb_tracking.lock);
> >   	/* Remove stale busy bits due to the old buffer. */
> >   	i915->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front,
> >   {
> >   	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> >   
> > +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> > +		return;
> > +
> >   	if (origin == ORIGIN_CS) {
> >   		spin_lock(&i915->fb_tracking.lock);
> >   		i915->fb_tracking.busy_bits |= frontbuffer_bits;
> > @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
> >   {
> >   	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> >   
> > +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> > +		return;
> > +
> >   	if (origin == ORIGIN_CS) {
> >   		spin_lock(&i915->fb_tracking.lock);
> >   		/* Filter out new bits since rendering started. */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 91453f7dbd656..20c135a2bba33 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >   
> >   #define HAS_ASYNC_FLIPS(i915)		(DISPLAY_VER(i915) >= 5)
> >   
> > +#define HAS_FRONTBUFFER_RENDERING(i915)	(DISPLAY_VER(i915) < 9)
> > +
> >   /* Only valid when HAS_DISPLAY() is true */
> >   #define INTEL_DISPLAY_ENABLED(dev_priv) \
> >   	(drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display)
> > 





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux