[Intel-gfx] [PATCH 46/89] drm/i915/skl: Add DDB allocation management structures

[Ville Syrjälä]

On Mon, Sep 22, 2014 at 03:08:27PM +0100, Damien Lespiau wrote:
> On Wed, Sep 17, 2014 at 01:47:54PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 04, 2014 at 12:27:12PM +0100, Damien Lespiau wrote:
> > > We now need to allocate space in the DDB for planes being scanned out
> > > ourselves. The data structure to represent an allocation mirrors what
> > > we'll need to write in the registers later on: (start, end).
> > > 
> > > We add that allocation datat to the skl_wm_values structure as part of
> > > the values to program the hardware with.
> > > 
> > > v2: Split planes and cursor for consistency.
> > > 
> > > v3: Make the skl_ddb_entry_size() parameter const
> > > 
> > > Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 3764ad5..de278a5 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1381,8 +1381,27 @@ struct ilk_wm_values {
> > >  	enum intel_ddb_partitioning partitioning;
> > >  };
> > >  
> > > +struct skl_ddb_entry {
> > > +	uint16_t start, end;	/* in number of blocks */
> > > +};
> > > +
> > > +static inline uint16_t skl_ddb_entry_size(const struct skl_ddb_entry *entry)
> > > +{
> > > +	/* end not set, clearly no allocation here. start can be 0 though */
> > > +	if (entry->end == 0)
> > > +		return 0;
> > > +
> > > +	return entry->end - entry->start + 1;
> > 
> > I would make 'end' exclusive as that allows you to represent an empty
> > block more naturally, and you don't have to worry about the +/-1 adjustments
> > when going between start/end and start/size representation.
> 
> The values we program in the registers are inclusive (I had a bug where
> the end of one plane and the start of another plane had the same value
> and artefacts could be seen, due to the pipes fetching from overlapping
> DDB space). I took the option of having those values mirror what we need
> to program. Hopefully the small functions around the skl_ddb_entry
> structure doesn't make it too bad?

I'd still change to exclusive. Inclusive is just too troublesome to use IMO,
you're bound to forget the magic +/-1 somewhere eventually.

-- 
Ville Syrjälä
Intel OTC