[Intel-gfx] [PATCH v3] drm/i915: Eliminate vmap overhead for cmd parser

[Chris Wilson]

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...
 
> 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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre