Re: [PATCH v3] drm/i915: Eliminate vmap overhead for cmd parser

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

On Wed, Nov 25, 2015 at 08:13:43PM +0000, Chris Wilson wrote:
> On Wed, Nov 25, 2015 at 09:51:08PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 20, 2015 at 03:31:23PM +0000, Chris Wilson wrote:
> > > @@ -1097,6 +1003,7 @@ static bool check_cmd(const struct intel_engine_cs *ring,
> > >  }
> > >  
> > >  #define LENGTH_BIAS 2
> > > +#define MAX_PARTIAL 256
> > 
> > There seems to some confusion whether this is bytes or dwords.
> 
> Indeed, I can't remember of the top of my head.
> 
> (Double checked that the set of commands that I was thinking were 132
> bytes.)
>  
> > Also I guess we already end up allocating two pages anyway, so
> > maybe MAX_PARTIAL should just be one page? It's still not big
> > enough to cover the max legal cmd length AFAICS, so I think
> > the WARN in the check needs to be removed.
> 
> Sure, rounding up the next 8192 byte slab cache doesn't seem like it
> will bite us.
> 
> So #define MAX_PARTIAL_BYTES PAGE_SIZE
> 
> > > +	in = offset_in_page(batch_start_offset);
> > > +	partial = 0;
> > > +	for (src_iter = batch_start_offset >> PAGE_SHIFT;
> > > +	     src_iter < batch_obj->base.size >> PAGE_SHIFT;
> > > +	     src_iter++) {
> > 
> > So we're iterating over all the pages. Should be enough to iterate
> > until batch_start_offset+batch_len I suppose, but as long as we bail
> > out when we run out of batch it should be fine.
> 
> Right, this was mostly convenience for writing the loop bounds - it was
> more or less a simple conversion from the old iterator.
> 
> > I see there's a batch_len check at the end, but I don't see us handling
> > the case when the user already gives us something with batch_len==0.
> > Maybe that should be rejected somewhere higher up?
> 
> batch_len = 0 is filtered out in the caller...

Oh yeah, see it now.

>  
> > Also what happens if we don't find MI_BATCH_BUFFER_END before running
> > out of batch? Oh, I see, we set ret=-EINVAL, and clear it to 0 when we
> > find MI_BATCH_BUFFER_END. So that part seems to be fine.
> > 
> > > +		u32 *cmd, *page_end, *batch_end;
> > > +		u32 this;
> > > +
> > > +		this = batch_len;
> > 
> > I was a bit concerned about batch_len & 3, but we already check for
> > batch_len&7==0 in i915_gem_check_execbuffer(), so it should be good here.
> 
> cmdparser_assert(batch_len > 0 && (batch_len & 3) == 0);
> 
> as documentation for the contract?

I won't insist, but feel free to add something like that if you
wish.

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