[Intel-xe] [RFC PATCH 17/20] drm/xe/display: Use frontbuffer tracking for Xe as well

[Hogander, Jouni]

On Fri, 2023-05-05 at 10:50 +0200, Maarten Lankhorst wrote:
> 
> On 2023-05-05 10:29, Jouni Högander wrote:
> > Use frontbuffer tracking for Xe as well as needed. We still need to
> > perform flush for PSR, FBC and DRRS. For this purpose use
> > frontbuffer
> > tracking for Xe as well.
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_drrs.c          | 1 +
> >   drivers/gpu/drm/i915/display/intel_fb.c            | 3 +++
> >   drivers/gpu/drm/i915/display/intel_psr.c           | 1 +
> >   drivers/gpu/drm/i915/display/skl_universal_plane.c | 1 +
> >   drivers/gpu/drm/xe/xe_bo_types.h                   | 2 ++
> >   5 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c
> > b/drivers/gpu/drm/i915/display/intel_drrs.c
> > index 760e63cdc0c8..59527a4667f4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> > @@ -9,6 +9,7 @@
> >   #include "intel_de.h"
> >   #include "intel_display_types.h"
> >   #include "intel_drrs.h"
> > +#include "intel_frontbuffer.h"
> >   #include "intel_panel.h"
> >   
> >   /**
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> > b/drivers/gpu/drm/i915/display/intel_fb.c
> > index e62e1e12758d..fa4464d433b7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -13,6 +13,7 @@
> >   #include "intel_display_types.h"
> >   #include "intel_dpt.h"
> >   #include "intel_fb.h"
> > +#include "intel_frontbuffer.h"
> >   
> >   #ifdef I915
> >   /*
> > @@ -1923,11 +1924,13 @@ int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> >         int i;
> >   #ifdef I915
> >         unsigned tiling, stride;
> > +#endif
> >   
> >         intel_fb->frontbuffer = intel_frontbuffer_get(obj);
> >         if (!intel_fb->frontbuffer)
> >                 return -ENOMEM;
> >   
> > +#ifdef I915
> >         i915_gem_object_lock(obj, NULL);
> >         tiling = i915_gem_object_get_tiling(obj);
> >         stride = i915_gem_object_get_stride(obj);
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index ea0389c5f656..2ecc902a85a2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -32,6 +32,7 @@
> >   #include "intel_display_types.h"
> >   #include "intel_dp.h"
> >   #include "intel_dp_aux.h"
> > +#include "intel_frontbuffer.h"
> >   #include "intel_hdmi.h"
> >   #include "intel_psr.h"
> >   #include "intel_psr_regs.h"
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 71ba544eda71..0f1e4e61d32e 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -16,6 +16,7 @@
> >   #include "intel_display_types.h"
> >   #include "intel_fb.h"
> >   #include "intel_fbc.h"
> > +#include "intel_frontbuffer.h"
> >   #include "intel_psr.h"
> >   #include "skl_scaler.h"
> >   #include "skl_universal_plane.h"
> > diff --git a/drivers/gpu/drm/xe/xe_bo_types.h
> > b/drivers/gpu/drm/xe/xe_bo_types.h
> > index 229fedc8ea72..df5be030991a 100644
> > --- a/drivers/gpu/drm/xe/xe_bo_types.h
> > +++ b/drivers/gpu/drm/xe/xe_bo_types.h
> > @@ -68,6 +68,8 @@ struct xe_bo {
> >         struct llist_node freed;
> >         /** @created: Whether the bo has passed initial creation */
> >         bool created;
> > +
> > +       struct intel_frontbuffer *frontbuffer;
> 
> Instead of putting this in xe_bo, could this be put inside intel_fb
> as 
> well? In that case, for Xe each framebuffer could have its own 
> frontbuffer tracking, and an invalidate on 1 fb would not invalidate 
> another FB

intel_fb already has a pointer to frontbuffer. The frontbuffer object
is same as the one in underlying xe_bo. See
drivers/gpu/drm/i915/display/intel_fb.c:intel_framebuffer_init.
Frontbuffer_invalidate/flush are effective only when the framebuffer is
used for a plane. See
drivers/gpu/drm/i915/intel_display.c:intel_atomic_track_fbs. Now if the
intel_fb and it's underlying xe_bo are used for some plane (i.e. it's
frontbuffer). Then e.g. dirtyfb is called for that framebuffer. Do you
see some benefit if the logic would not invalidate/flush some other
framebuffer using different intel_fb but same xe_bo for another plane.

Did I understood your comment correctly?

> 
> I think we don't want to copy i915 semantics here 1:1.
> 
> >   };
> >   
> >   #define intel_bo_to_drm_bo(bo) ((bo)->ttm.base)