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 06:05:12PM +0100, Russell King - ARM Linux wrote:
> 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...

And now the next muckup in dma-buf/drm_prime - though it's arguably the
fault of IS_ERR_OR_NULL existing in the first place:

drm_gem_prime_import()
{
        sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
        if (IS_ERR_OR_NULL(sgt)) {
                ret = PTR_ERR(sgt);
                goto fail_detach;
        }
fail_detach:
        dma_buf_detach(dma_buf, attach);
        return ERR_PTR(ret);
}

What happens if sgt is NULL here?  What value does ret get?  What does
drm_gem_prime_import() return?  How is that interpreted by
drm_gem_prime_fd_to_handle() ?

I can probably oops a kernel running DRM through this if I can manipulate
a dma_buf_map_attachment() to return NULL (there probably is a driver
which is capable of doing so.)

APIs which use IS_ERR_OR_NULL() are problems waiting to happen; we've
already reached some concensus a while back to get rid of this helper,
if only it wasn't soo prolific (like its errors are.)