Re: [PATCH v3 1/3] drm: add SimpleDRM driver

[Daniel Vetter]

On Wed, Aug 17, 2016 at 12:19:02PM +0200, Noralf Trønnes wrote:
> 
> Den 17.08.2016 11:30, skrev Daniel Vetter:
> > On Tue, Aug 16, 2016 at 9:38 PM, Noralf Trønnes <noralf@xxxxxxxxxxx> wrote:
> > > > That's still a lot for what amounts to reimplementing mmap on shmem, but
> > > > badly. What I mean with redirecting is pointing the entire ->mmap
> > > > operation to the mmap implementation for the underlying mmap. Roughly:
> > > > 
> > > >          /* normal gem mmap checks first */
> > > > 
> > > >          /* redirect to shmem mmap */
> > > >          vma->vm_file = obj->filp;
> > > >          vma->vm_pgoff = 0;
> > > > 
> > > >          return obj->filp->f_op->mmap(obj->filp, vma);
> > > > 
> > > > Much less code ;-)
> > > 
> > > obj->filp is NULL in my case.
> > > 
> > > And looking at the docs, that's expected since I have driver specific
> > > backing?
> > > 
> > > /**
> > >   * @filp:
> > >   *
> > >   * SHMEM file node used as backing storage for swappable buffer objects.
> > >   * GEM also supports driver private objects with driver-specific backing
> > >   * storage (contiguous CMA memory, special reserved blocks). In this
> > >   * case @filp is NULL.
> > >   */
> > Hm, I totally misread the driver code. I assumed that we'd just
> > allocate normal shmem gem objects, and then copy them on-demand onto
> > the frontbuffer (in the dirty or plane update callbacks). Essentially
> > treat the firmware fb area as a manual upload display, except that we
> > don't use i2c or spi to do the upload, but normal mmio writes. I think
> > that would greatly simplify the driver, and more important: It would
> > also work like any other kms driver. Currently sdrm is violiting the
> > spec a bit by aliasing all dumb buffers to the same underlying backing
> > storage, and that's a bit evil.
> > 
> > The other bit I noticed (and why I was confused): The prime import
> > code reinvents a lot of wheels, and it digs into the backing storage
> > directly. Instead it should just call dma_buf_vmap/dma_buf_vunmap and
> > let the exporter figure out how it works.
> 
> Is there a driver I can look at that does it the way we want?
> I'm afraid that this gem framework is still over my head.
> Copying code or patterns is easy, starting from scratch demands
> understanding :-)

udl_gem.c is pretty much what I had in mind, including the handling of
dma-bufs and all that. Not sure how much point there is in sharing code,
at least if you can't test udl.

What udl doesn't do is directly redirect the mmap to the shmem node (and
reject the mmap for dma-buf imported buffers, since that's not a good idea
really).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch