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

[Chris Wilson]

On Fri, Nov 20, 2015 at 04:41:53PM +0200, Ville Syrjälä wrote:
> > +	if (batch_len > shadow_batch_obj->base.size ||
> 
> AFAIK that can't actaully happen since we allocate the shadow batch
> based on batch_len. But I see it was already like this in the old code.
> 
> > +	    batch_len + batch_start_offset > batch_obj->base.size)
> 
> This worries me more. Shouldn't we also have this check somewhere for
> the non-cmd_parser cases? Nor can I see any overflow check for
> 'batch_len+batch_start_offset'.

True, that is worthy of being moved to execbuf validation.

> > +		return -E2BIG;
> > +
> > +	if (WARN_ON(shadow_batch_obj->pages_pin_count == 0))
> > +		return -ENODEV;
> > +
> > +	ret = i915_gem_obj_prepare_shmem_read(batch_obj, &needs_clflush);
> > +	if (ret) {
> > +		DRM_DEBUG_DRIVER("CMD: failed to prepare shadow batch\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = i915_gem_object_set_to_cpu_domain(shadow_batch_obj, true);
> > +	if (ret) {
> > +		DRM_DEBUG_DRIVER("CMD: Failed to set shadow batch to CPU\n");
> > +		goto unpin;
> >  	}
> >  
> > +	tmp = kmalloc(PAGE_SIZE + MAX_PARTIAL, GFP_KERNEL);
> 
> GFP_TEMPORARY perhaps?

Ok.

> > +	if (tmp == NULL)
> > +		return -ENOMEM;
> > +
> >  	/*
> >  	 * We use the batch length as size because the shadow object is as
> >  	 * large or larger and copy_batch() will write MI_NOPs to the extra
> 
> copy_batch() is gone.

I guess the whole comment is now misleading. Comments, never trust them!

> >  	 * space. Parsing should be faster in some cases this way.
> >  	 */
> > -	batch_end = batch_base + (batch_len / sizeof(*batch_end));
> > +	ret = -EINVAL;
> > +	rewind = batch_obj->get_page;
> 
> Shouldn't the get_page work just fine without this rewind trick? As in if
> some other user wants one of the previous pages it restarts iterating
> from 0?

Yes, it works fine, it is just a trick to keep the cache of the
last location intact for other paths (basically trying to keep the cache
for the direct user paths).

> > -		if (*cmd == MI_BATCH_BUFFER_END)
> > -			break;
> > +			desc = find_cmd(ring, *cmd, &default_desc);
> > +			if (!desc) {
> > +				DRM_DEBUG_DRIVER("CMD: Unrecognized command: 0x%08X\n",
> > +						 *cmd);
> > +				goto unmap;
> > +			}
> >  
> > -		desc = find_cmd(ring, *cmd, &default_desc);
> > -		if (!desc) {
> > -			DRM_DEBUG_DRIVER("CMD: Unrecognized command: 0x%08X\n",
> > -					 *cmd);
> > -			ret = -EINVAL;
> > -			break;
> > -		}
> 
> It's quite hard to see what's changed due to the reindent. Any chance
> you could do the reindent in a prep patch just help my poor brain a bit?

Or maybe the ignore-whitespace option for send-email?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre