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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
