Re: [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

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

On Wed, Jan 24, 2018 at 07:36:01PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2018 at 09:11:23PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-01-23 18:33:43)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Replace the ad-hoc plane indexing scheme used by the frontbuffer
> > > tracking with enum plane_id.
> > > 
> > > The old video overlay not being part of the plane_id namespace
> > > will just be given the high bit.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      | 11 +++--------
> > >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> > >  drivers/gpu/drm/i915/intel_fbc.c     |  2 +-
> > >  drivers/gpu/drm/i915/intel_sprite.c  |  4 +++-
> > >  4 files changed, 9 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 8333692dac5a..bd545b1c9546 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2404,16 +2404,11 @@ enum hdmi_force_audio {
> > >   *
> > >   * We have one bit per pipe and per scanout plane type.
> > >   */
> > > -#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
> > >  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> > 
> > I would feel safer with a BUILD_BUG_ON to catch plane_id overflowing
> > INTEL_FRONTBUFFER_BITS_PER_PIPE (-1 for the overlay reservation).
> 
> Actaully we'll be going past that BITS_PER_PIPE-1 limit real soon
> now, but the BITS_PER_PIPE will be sufficient for some time I think.
> So I think I'd just rather ignore the overlap between the plane_id
> and the overlay because that's not going happen on actual hardware.
> 
> But I think asserting that BITS_PER_PIPE is sufficient is a good idea.
> Just need to figure out where to put it. There's no init function
> or anything like that in the frontbuffer tracking code, so that's
> not going to work. I guess one option would be to include the
> assert in the INTEL_FRONTBUFFER() macro itself.

Pushed this as is to dinq. Thanks for the review.

I'll send that BUILD_BUG_ON() as a followup.

> 
> > 
> > > -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> > > -       (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > > -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> > > -       (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> > > -#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> > > -       (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> > > +#define INTEL_FRONTBUFFER(pipe, plane_id) \
> > > +       (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > >  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> > > -       (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))))
> > > +       (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > >  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> > >         (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > 
> > The conversion looks straightforward nevertheless, and indeed no reason
> > to have move than one indexing id for a plane.
> > 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