Re: [PATCH RFCv2 0/4] Etnaviv DRM driver again

From: Daniel Vetter <daniel@ffwll.ch>
To: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux@arm.linux.org.uk>,
	kernel@pengutronix.de, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH RFCv2 0/4] Etnaviv DRM driver again
Date: Tue, 20 Oct 2015 11:36:27 +0200	[thread overview]
Message-ID: <20151020093627.GV13786@phenom.ffwll.local> (raw)
In-Reply-To: <1441980614-19216-1-git-send-email-l.stach@pengutronix.de>

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.

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

- similar input validation issue for op in gem_cpu_prep

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

- gem_submit_bo->flags gets it rigth, yay!

- 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)

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

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

- flags for gem_wait_fence perhaps? Probably not needed ever.

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

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've kept things in staging for now, as that's the place where Christian started
> this driver, but would really like to move it to DRM proper _before_ merging. So
> please review stuff with that in mind.

Yeah, staging is the place where drm drivers get all forgotten about. Imo
reasonable good drivers should land directly in drm and it's better to
apply the last polish there.

Cheers, Daniel

> Since the last posting a lot of cleanups and bugfixes have landed, but also a major
> rewrite of the userspace interface. The UAPI is now considerably simpler as a lot
> of things that turned out to be not useful have been cut out. Also a pretty big
> security issue has been fixed where the userspace could abuse the still mapped
> command buffer to change the command stream after the kernel validated and patched
> it up, but before actual GPU execution.
> 
> Thanks to Russell King GPU power management with proper state reinitialization is
> now in place, which allows the GPU to be completely power gated when not in use,
> but is also the foundation for GPU recovery after a hanging submit.
> 
> A modified version of Russell Kings xf86-video-armada driver driver that works on
> top of the new UAPI is available at
> 
> git://git.pengutronix.de/git/lst/xf86-video-armada.git for-rmk
> 
> Regards,
> Lucas
> 
> Christian Gmeiner (1):
>   staging: etnaviv: add drm driver
> 
> Lucas Stach (2):
>   staging: etnaviv: add devicetree bindings
>   ARM: imx6: add Vivante GPU nodes
> 
> Philipp Zabel (1):
>   of: Add vendor prefix for Vivante Corporation
> 
>  .../bindings/drm/etnaviv/etnaviv-drm.txt           |   44 +
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  arch/arm/boot/dts/imx6dl.dtsi                      |    5 +
>  arch/arm/boot/dts/imx6q.dtsi                       |   15 +
>  arch/arm/boot/dts/imx6qdl.dtsi                     |   21 +
>  drivers/staging/Kconfig                            |    2 +
>  drivers/staging/Makefile                           |    1 +
>  drivers/staging/etnaviv/Kconfig                    |   20 +
>  drivers/staging/etnaviv/Makefile                   |   18 +
>  drivers/staging/etnaviv/cmdstream.xml.h            |  218 +++
>  drivers/staging/etnaviv/common.xml.h               |  249 ++++
>  drivers/staging/etnaviv/etnaviv_buffer.c           |  271 ++++
>  drivers/staging/etnaviv/etnaviv_cmd_parser.c       |  119 ++
>  drivers/staging/etnaviv/etnaviv_drv.c              |  705 ++++++++++
>  drivers/staging/etnaviv/etnaviv_drv.h              |  138 ++
>  drivers/staging/etnaviv/etnaviv_gem.c              |  887 ++++++++++++
>  drivers/staging/etnaviv/etnaviv_gem.h              |  141 ++
>  drivers/staging/etnaviv/etnaviv_gem_prime.c        |  121 ++
>  drivers/staging/etnaviv/etnaviv_gem_submit.c       |  421 ++++++
>  drivers/staging/etnaviv/etnaviv_gpu.c              | 1468 ++++++++++++++++++++
>  drivers/staging/etnaviv/etnaviv_gpu.h              |  198 +++
>  drivers/staging/etnaviv/etnaviv_iommu.c            |  221 +++
>  drivers/staging/etnaviv/etnaviv_iommu.h            |   28 +
>  drivers/staging/etnaviv/etnaviv_iommu_v2.c         |   33 +
>  drivers/staging/etnaviv/etnaviv_iommu_v2.h         |   25 +
>  drivers/staging/etnaviv/etnaviv_mmu.c              |  282 ++++
>  drivers/staging/etnaviv/etnaviv_mmu.h              |   58 +
>  drivers/staging/etnaviv/state.xml.h                |  351 +++++
>  drivers/staging/etnaviv/state_hi.xml.h             |  407 ++++++
>  include/uapi/drm/etnaviv_drm.h                     |  215 +++
>  30 files changed, 6683 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/etnaviv/etnaviv-drm.txt
>  create mode 100644 drivers/staging/etnaviv/Kconfig
>  create mode 100644 drivers/staging/etnaviv/Makefile
>  create mode 100644 drivers/staging/etnaviv/cmdstream.xml.h
>  create mode 100644 drivers/staging/etnaviv/common.xml.h
>  create mode 100644 drivers/staging/etnaviv/etnaviv_buffer.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_cmd_parser.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_drv.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_drv.h
>  create mode 100644 drivers/staging/etnaviv/etnaviv_gem.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_gem.h
>  create mode 100644 drivers/staging/etnaviv/etnaviv_gem_prime.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_gem_submit.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_gpu.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_gpu.h
>  create mode 100644 drivers/staging/etnaviv/etnaviv_iommu.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_iommu.h
>  create mode 100644 drivers/staging/etnaviv/etnaviv_iommu_v2.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_iommu_v2.h
>  create mode 100644 drivers/staging/etnaviv/etnaviv_mmu.c
>  create mode 100644 drivers/staging/etnaviv/etnaviv_mmu.h
>  create mode 100644 drivers/staging/etnaviv/state.xml.h
>  create mode 100644 drivers/staging/etnaviv/state_hi.xml.h
>  create mode 100644 include/uapi/drm/etnaviv_drm.h
> 
> -- 
> 2.5.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel