[PATCH RFCv2 0/4] Etnaviv DRM driver again

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 11:36:27AM +0200, Daniel Vetter wrote:
> On Fri, Sep 11, 2015 at 04:10:10PM +0200, Lucas Stach wrote:
> > Hey all,
> > 
> > this is a new posting of the Etnaviv DRM driver for Vivante embedded GPUs.
> > This time I've squashed all patches to the DRM driver itself into a single commit
> > to make it easier for people to look at and review this stuff.
> > 
> > Aside from squashing of some of the trivial bugfixes I intend to keep all the
> > individual commits around to retain the authorship of people working on this
> > driver. If you want to look at the stream of commits please fetch
> > 
> > git://git.pengutronix.de/git/lst/linux.git etnaviv-for-upstream
> 
> Finally unlazied and looked at this, assuming it's v2. Piles of comments:
> 
> - Please don't use dev->struct_mutex in new driver code. With latest
>   linux-next there's no reason at all for driver's to use it, and doing so
>   will massively encumber your driver with everything else.
> 
>   There's a bit a trick involved since as-is struct_mutex allows you to do
>   horrible leaky locking designs around gem_free_object. On a quick look
>   to fix this you probably need a mutex for the drm_mm and various lists,
>   plus per object reservations.  Tricky part is the eviction logic in
>   etnaviv_iommu_map_gem, where you need to trylock eviction candidates. If
>   you can't lock them you can't evict them anyway, so no harm done.
> 
>   If that's too much just replace it with your own lock and trylock in
>   gem_free_object, punting to a worker if that fails (ttm has a
>   deferred_free list for that, protected by a spinlock).
> 
> - ->load and ->unload are deprecated hooks becaus fundamentally racy (and
>   we can't fix it since it would break dri 1 crap). Please use instead:
> 
> 	drm_dev_alloc();
> 	/* load code */
> 	drm_dev_register();
> 
>   and
> 
> 	drm_dev_unregister();
> 	/* unload code */
> 	drm_dev_unref();
> 
>   Laurent just sent a nice patch for rcar to do that conversion. That will
>   also get rid of the deprecated drm_platform_init and drm_dev_put.
> 
> - check pad for 0 in gem_info ioctl.

My tree already does this, and afaik it was part of Christian's patches.
I'm not sure whether Lucas' patches are missing something.

+static int etnaviv_ioctl_gem_info(struct drm_device *dev, void *data,
+               struct drm_file *file)
+{
+       struct drm_etnaviv_gem_info *args = data;
+       struct drm_gem_object *obj;
+       int ret = 0;
+
+       if (args->pad)
+               return -EINVAL;

Did you miss it?

> - the flags check for gem_new seems leaky since you only check for flags &
>   ETNA_BO_CACHE_MASK.

Fixed in etnaviv_ioctl_gem_new().

> - similar input validation issue for op in gem_cpu_prep

Fixed in etnaviv_ioctl_gem_cpu_prep().

> - maybe add a flags/op to future-proof gem_cpu_fini just in case? Might be
>   useful to optimize cache flushing.

Having just merged Lucas' patch, which carries the op from gem_cpu_prep()
over to gem_cpu_fini(), I'm wondering if this is hiding a potential
problem - what if two threads call gem_cpu_prep() but with different 'op'
arguments.  Seems rather fragile, as 'etnaviv_obj->last_cpu_prep_op'
becomes rather indeterminant.

> - the naming in gem_submit_reloc confuses me a bit, but that's just a
>   bikeshed ;-)
> 
> - gem_submit seems to miss a flag, ime definitely needed (just to be able
>   to extend to future hw iterations)

Grumble, another API revision I'll need to maintain in the DDX driver.
(Even though this isn't in mainline, I'm already up to three different
kernel APIs for etnadrm.)  If we do this, I'll want to lump it together
with other API changes (like the one below for flags) so I'll wait until
we've got an answer to the gem_wait_fence question.

> - gem_submit->exec_state doesn't seem to be validated (it's just an if
>   (exec_state == 2D) ... else ... in cmd_select_pipe)

Fixed.

> - all the array allocations aren't checked for integer overflows in
>   gem_submit. Just use kmalloc_array or similar to get this right. That
>   means you need to allocations in submit_create, but imo better safe than
>   security-buggy. Similar problem in submit_reloc, but there
>   copy_from_user will protect you since you only copy individual structs.
>   Still a bit fragile.

I'm not sure kmalloc_array() is the right answer there, but I'll look
into it - I'd really like to avoid doing lots of small kmalloc()s all
over the place as each one has a non-zero cost.  The more we can lump
together, the better - but it has to be done safely.

> - flags for gem_wait_fence perhaps? Probably not needed ever.

We could, to be on the safe side, add some padding to the struct, and
not call it "flags" until we need flags.  Christian, Lucas, any thoughts
from the 3D and VG point of view on this?

> - gem_userptr should probably check for page alignment since that's the
>   only unit you can map into the iommu. At least I didn't spot that check
>   anywhere.

Added (even though it'll probably break Xvideo...)  I'll give it a test
later this afternoon/evening.

> Random reading all around and looks pretty overall.
> 
> One more question: Do you have validation tests for the basic ioctl
> interfaces? I'd like to push igt as the general drm gpu tests suite, and
> we now have support to run testcases on non-i915 drivers. Some are generic
> and run everywhere (lots more need to be converted to be generic), but I'd
> also welcome driver-specific tests, maybe in an etnaviv subdirectory.

I don't think we have - my validation is "does it work with my DDX
driver without rendering errors" which so far has proven to be good
at finding bugs.  This doesn't use libdrm-etnaviv as I need to maintain
compatibility with libetnaviv's API (the original open source library
that talks to Vivante's open source kernel space) to avoid having to
maintain two near-identical GPU backends.

Christian uses his libdrm-etnaviv library plugged into mesa.  So we
have two independently created and maintained code bases talking to
this kernel interface.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.