[PATCH 2/5] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Thu, Oct 10, 2013 at 05:25:15PM -0400, Rob Clark wrote:
> On Sun, Oct 6, 2013 at 6:08 PM, Russell King
> <rmk+kernel at arm.linux.org.uk> wrote:
> > +       /*
> > +        * If we have an overlay plane associated with this CRTC, disable
> > +        * it before the modeset to avoid its coordinates being outside
> > +        * the new mode parameters.  DRM doesn't provide help with this.
> > +        */
> 
> Getting a bit off topic, but yeah.. I'd be in favor of "clarifying"
> things by having crtc helpers disable planes on setcrtc (rather than
> only doing this on fbdev/lastclose restore).  I
> don't know of any userspace that this would cause problems for.  But
> not really sure if it would be considered an interface break or just
> "fixing a bug".. since we have different behavior on different
> drivers, I'd lean towards the latter.

The reasoning here is if the overlay plane is larger than the mode being
set, we will end up having the hardware programmed in an inconsistent
state - the overlay plane position/width/height can be outside of the
active region, which is invalid for the hardware.  So, it's safer to
disable the plane and wait for the next plane to be sent and have that
validated against the new mode.

It's not like you would be able to see the plane immediately, because
most monitors blank while they retrain to the new mode settings.

> > +static int armada_gem_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > +{
> > +       struct armada_gem_object *obj = drm_to_armada_gem(vma->vm_private_data);
> > +       unsigned long addr = (unsigned long)vmf->virtual_address;
> > +       unsigned long pfn = obj->phys_addr >> PAGE_SHIFT;
> > +       int ret;
> > +
> > +       pfn += (addr - vma->vm_start) >> PAGE_SHIFT;
> > +       ret = vm_insert_pfn(vma, addr, pfn);
> > +
> > +       switch (ret) {
> > +       case -EIO:
> > +       case -EAGAIN:
> > +               set_need_resched();
> 
> probably this thread is applicable here too?
> 
> https://lkml.org/lkml/2013/9/12/417
> 
> (although.. we have at least a few  slightly differet variants on the
> same errno -> VM_FAULT_x switch in different drivers.. maybe we should
> do something better)

Hmm.  It seems today's vm_insert_pfn() has the following error return
values:

-EFAULT - virtual address outside vma
-EINVAL - track_pfn_insert() failure
-ENOMEM - failed to get locked pte
-EBUSY - entry already exists in pte

So it seems my handling -EIO, -EAGAIN, -ERESTARTSYS and -EINTR are all
redundant and can be removed.  I'm not sure if it makes sense for this
to be generic - it looks like it's only Intel, gma500 and me who use
this, and Intel handles more error codes (due to it calling other
functions.)

I'll respin dropping those four unnecessary error codes, and post the
delta for this.

> > +/* Prime support */
> > +struct sg_table *
> > +armada_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
> > +       enum dma_data_direction dir)
> > +{
> > +       struct drm_gem_object *obj = attach->dmabuf->priv;
> > +       struct armada_gem_object *dobj = drm_to_armada_gem(obj);
> > +       struct scatterlist *sg;
> > +       struct sg_table *sgt;
> > +       int i, num;
> > +
> > +       sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
> > +       if (!sgt)
> > +               return NULL;
> > +
> > +       if (dobj->obj.filp) {
> > +               struct address_space *mapping;
> > +               gfp_t gfp;
> > +               int count;
> > +
> > +               count = dobj->obj.size / PAGE_SIZE;
> > +               if (sg_alloc_table(sgt, count, GFP_KERNEL))
> > +                       goto free_sgt;
> > +
> > +               mapping = file_inode(dobj->obj.filp)->i_mapping;
> > +               gfp = mapping_gfp_mask(mapping);
> > +
> > +               for_each_sg(sgt->sgl, sg, count, i) {
> > +                       struct page *page;
> > +
> > +                       page = shmem_read_mapping_page_gfp(mapping, i, gfp);
> 
> you could almost use drm_gem_get_pages().. although I guess you
> otherwise have no need for the page[] array?

Correct.  The page[] array would just be an additional unnecessary
allocation, and therefore would be an additional unnecessary point of
failure.

> > +#define DRM_ARMADA_GEM_CREATE          0x00
> > +#define DRM_ARMADA_GEM_MMAP            0x02
> > +#define DRM_ARMADA_GEM_PWRITE          0x03
> > +
> > +#define ARMADA_IOCTL(dir, name, str) \
> > +       DRM_##dir(DRM_COMMAND_BASE + DRM_ARMADA_##name, struct drm_armada_##str)
> > +
> > +struct drm_armada_gem_create {
> 
> Any reason not to throw in a 'uint32_t flags' field?  At least then
> you could indicate what sort of backing memory you want, and do things
> like allocate linear scanout buffer w/ your gem_create rather than
> having to use dumb_create.  Maybe not something you need now, but
> seems like eventually you'll come up with some reason or another to
> want to pass some flags into gem_create.

I thought one of the points of the dumb_create stuff was so that there
is a standard API for dumb scanout buffers across all DRM drivers.  It
has that advantage, and as I understand it, a generic KMS driver for X
is on the cards.

Thanks for the review.