Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Wed, Jun 12, 2013 at 05:49:14PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 12, 2013 at 09:56:22AM -0400, Rob Clark wrote:
> > On Wed, Jun 12, 2013 at 9:48 AM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > And having thought about this driver, DRM some more, I'm now of the
> > > opinion that DRM is not suitable for driving hardware where the GPU is
> > > an entirely separate IP block from the display side.
> > >
> > > DRM is modelled after the PC setup where your "graphics card" does
> > > everything - it has the GPU, display and connectors all integrated
> > > together.  This is not the case on embedded SoCs, which can be a
> > > collection of different IPs all integrated together.
> > 
> > actually it isn't even the case on desktop/laptop anymore, where you
> > can have one gpu with scanout and a second one without (or just with
> > display controller not hooked up to anything, etc, etc)
> > 
> > That is the point of dmabuf and the upcoming fence/reservation stuff.
> 
> Okay, but dmabuf really needs to be fixed, because as it stands this API
> is really quite broken wrt the DMA API.  dma_map_sg() is (a) not supposed
> to have its return value ignored - mappings can fail, and (b) the returned
> number indicates how many entries are valid for the _mapped_ version of
> the scatterlist.
> 
> Both these points are important if your DMA API implementation uses an
> IOMMU, which may coalesce the scatterlist array when creating mappings -
> much as described in Documentation/DMA-API.txt and
> Documentation/DMA-API-HOWTO.txt.
> 
> That is not all DRMs fault - (a) is attributable to DRM's prime
> implementation:
> 
>         sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> 
>         if (!IS_ERR_OR_NULL(sgt))
>                 dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
> 
> and quite why it does the dma_map_sg() beneath the struct_mutex is
> beyond me - if the array of pages isn't safe without the mutex being
> held, then it isn't safe after the dma_map_sg() operation has completed
> and the mutex has been released.
> 
> However, (b) is more a problem for dmabuf (which I just rather aptly
> mistyped as dmabug) because either dmabuf's .map_dma_buf method needs
> to return the value from dma_map_sg(), or it needs to stop requiring
> this of the .map_dma_buf method and have it done by the caller of
> this method so the caller can have access to that returned value.
> 
> Added Sumit Semwal to this email for the dmabuf issue.
> 
> Thankfully, this being correct isn't a requirement for me, but it's
> something which _should_ be fixed.

Okay, so Sumit Semwal has been a victim of TI's cuts, so don't expect
dma_buf to get sorted by the original author...

Now we come to this:

drivers/gpu/drm/exynos/exynos_drm_dmabuf.c: return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops,
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c-                         exynos_gem_obj->base.size, flags);
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c:  return dma_buf_export(obj, &omap_dmabuf_ops, obj->size, flags);
drivers/gpu/drm/drm_prime.c:        return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
drivers/gpu/drm/drm_prime.c-                              0600);
drivers/gpu/drm/i915/i915_gem_dmabuf.c:     return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags);

Of the three implementations which don't use the generic version, they all
pass 'flags' to dma_buf_export.  drm_prime.c doesn't, it passes a fixed
file mode.  What's the correct version, or is flags | 0600 actually the
right one (as flags only contains O_CLOEXEC)?