Re: [PATCH v3 3/6] drm/i915: Clean up fbc vs. plane checks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Feb 22, 2018 at 04:07:53PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 21, 2018 at 09:59:27PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-02-21 17:31:01)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Let's record the information whether a plane can do fbc or not under
> > > struct inte_plane.
> > > 
> > > v2: Rebase due to i9xx_plane_id
> > >     Handle BDW/HSW correctly
> > > v3: Move inte_fbc_init() back since we depend on it happening
> > >     even with i915.disable_display, and populate
> > >     fbc->possible_framebuffer_bits directly from the
> > >     plane init code instead
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_fbc.c     | 26 ++---------------------
> > >  3 files changed, 44 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index d2a66704e6f5..c60d2215b377 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13238,6 +13238,32 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> > >         .format_mod_supported = intel_cursor_plane_format_mod_supported,
> > >  };
> > >  
> > > +static bool i9xx_plane_has_fbc(struct drm_i915_private *dev_priv,
> > > +                              enum i9xx_plane_id i9xx_plane)
> > > +{
> > > +       if (!HAS_FBC(dev_priv))
> > > +               return false;
> > 
> > > -static inline bool fbc_on_pipe_a_only(struct drm_i915_private *dev_priv)
> > > -{
> > > -       return IS_HASWELL(dev_priv) || INTEL_GEN(dev_priv) >= 8;
> > > -}
> > > -
> > > -static inline bool fbc_on_plane_a_only(struct drm_i915_private *dev_priv)
> > > -{
> > > -       return INTEL_GEN(dev_priv) < 4;
> > > -}
> > 
> > > +
> > > +       if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> > > +               return i9xx_plane == PLANE_A;
> > 
> > Where PLANE_A is tied to PIPE_A by construction. Is that worth a quick
> > comment?
> 
> Wouldn't hurt to spell it out I suppose.

Note added and series pushed to dinq. Thanks for the review.

I'll think a bit more about the need_fence thing and try to come up
a followup patch/series.

> 
> > 
> > > +       else if (IS_IVYBRIDGE(dev_priv))
> > > +               return i9xx_plane == PLANE_A || i9xx_plane == PLANE_B ||
> > > +                       i9xx_plane == PLANE_C;
> > > +       else if (INTEL_GEN(dev_priv) >= 4)
> > > +               return i9xx_plane == PLANE_A || i9xx_plane == PLANE_B;
> > > +       else
> > > +               return i9xx_plane == PLANE_A;
> > 
> > Ok, looks a bit more complete than before.
> > 
> > > +}
> > > +
> > > +static bool skl_plane_has_fbc(struct drm_i915_private *dev_priv,
> > > +                             enum pipe pipe, enum plane_id plane_id)
> > > +{
> > > +       if (!HAS_FBC(dev_priv))
> > > +               return false;
> > > +
> > > +       return pipe == PIPE_A && plane_id == PLANE_PRIMARY;
> > > +}
> > > +
> > >  static struct intel_plane *
> > >  intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> > >  {
> > > @@ -13280,6 +13306,21 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> > >                 primary->i9xx_plane = (enum i9xx_plane_id) pipe;
> > >         primary->id = PLANE_PRIMARY;
> > >         primary->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, primary->id);
> > > +
> > > +       if (INTEL_GEN(dev_priv) >= 9)
> > > +               primary->has_fbc = skl_plane_has_fbc(dev_priv,
> > > +                                                    primary->pipe,
> > > +                                                    primary->id);
> > > +       else
> > > +               primary->has_fbc = i9xx_plane_has_fbc(dev_priv,
> > > +                                                     primary->i9xx_plane);
> > > +
> > > +       if (primary->has_fbc) {
> > > +               struct intel_fbc *fbc = &dev_priv->fbc;
> > > +
> > > +               fbc->possible_framebuffer_bits |= primary->frontbuffer_bit;
> > > +       }
> > 
> > So an equivalent init loop to
> > 
> > > -       for_each_pipe(dev_priv, pipe) {
> > > -               fbc->possible_framebuffer_bits |=
> > > -                       INTEL_FRONTBUFFER(pipe, PLANE_PRIMARY);
> > > -
> > > -               if (fbc_on_pipe_a_only(dev_priv))
> > > -                       break;
> > > -       }
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > -Chris
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx