Re: [PATCH] drm: Allow platform devices to register as DRM devices

[Jordan Crouse]

Dave Airlie wrote:
> On Tue, Mar 2, 2010 at 2:00 AM, Jordan Crouse <jcrouse@xxxxxxxxxxxxxx> wrote:
> > Allow platform devices without PCI resources to be DRM devices.
> >
> > Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
>
> This patch has a bunch of whitespace damage at least in my inbox and
> also in patchwork

Yes, PBCAK.

> Please also be careful with the places you add copyrights, you moved a
> bunch of code
> from drm_drv.c to drm_pci.c and added a copyright for Code Aurora in
> drm_pci.c? You can
> only assert copyright if you actually wrote the code, otherwise the patch
> contains your authorship details and who contributed the code. I'm
> guessing you might have
> copyright over one file in this that being drm_platform.c, the rest
> I'm less sure about.
>
> I also wonder if we could/should separate the arm drm support out from
> this patch (i.e.
> the __arm__ changes).

That might not be a bad idea, I'll do that.

> Some other misc comments below:
>
> >  #
> >  menuconfig DRM
> >        tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI
> > support)"
> > -       depends on (AGP || AGP=n) && PCI && !EMULATED_CMPXCHG && MMU
>
> > From what I can see the AGP || AGP==n is still required, you just want
> to drop the PCI
> dependancy??

I guess technically we could also drop the AGP requirement, but since it worked
on my box with AGP=n it seemed to me like a NOP.

> > +       depends on !EMULATED_CMPXCHG && MMU
> >
> > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> > index 8417cc4..4177f60 100644
> > --- a/drivers/gpu/drm/drm_bufs.c
> > +++ b/drivers/gpu/drm/drm_bufs.c
> > @@ -11,6 +11,7 @@
> >  *
> >  * Copyright 1999, 2000 Precision Insight, Inc., Cedar Park, Texas.
> >  * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
> > + * Copyright (c) 2009, Code Aurora Forum.
> >  * All Rights Reserved.
>
> There have been much bigger changes to these files without copyright
> additions, I'm not sure how big something needs to be but I'm not 100% sure
> this comes close.
>
> >  resource_size_t drm_get_resource_start(struct drm_device *dev, unsigned int
> > resource)
> >  {
> > +       if (drm_core_check_feature(dev, DRIVER_USE_PLATFORM_DEVICE)) {
> > +               struct resource *r;
> > +               r = platform_get_resource(dev->platformdev, IORESOURCE_MEM,
> > +                                            resource);
> > +
> > +               return r ? r->start : 0;
> > +       }
> > +
> > +#ifdef CONFIG_PCI
> >        return pci_resource_start(dev->pdev, resource);
> > +#endif
> > +
> > +       return 0;
> >  }
> >  EXPORT_SYMBOL(drm_get_resource_start);
> >
> >  resource_size_t drm_get_resource_len(struct drm_device *dev, unsigned int
> > resource)
> >  {
> > +       if (drm_core_check_feature(dev, DRIVER_USE_PLATFORM_DEVICE)) {
> > +               struct resource *r;
> > +               r = platform_get_resource(dev->platformdev, IORESOURCE_MEM,
> > +                       resource);
> > +
> > +               return r ? (r->end - r->start) : 0;
> > +       }
> > +
> > +#ifdef CONFIG_PCI
> >        return pci_resource_len(dev->pdev, resource);
> > +#endif
> > +
> > +       return 0;
> >  }
> >
> >  EXPORT_SYMBOL(drm_get_resource_len);
> > @@ -188,7 +213,7 @@ static int drm_addmap_core(struct drm_device * dev,
> > resource_size_t offset,
> >        switch (map->type) {
> >        case _DRM_REGISTERS:
> >        case _DRM_FRAME_BUFFER:
> > -#if !defined(__sparc__) && !defined(__alpha__) && !defined(__ia64__) &&
> > !defined(__powerpc64__) && !defined(__x86_64__)
> > +#if !defined(__sparc__) && !defined(__alpha__) && !defined(__ia64__) &&
> > !defined(__powerpc64__) && !defined(__x86_64__) && !defined(__arm__)
> >                if (map->offset + (map->size-1) < map->offset ||
> >                    map->offset < virt_to_phys(high_memory)) {
> >                        kfree(map);
>
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 766c468..b5171ed 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -24,6 +24,7 @@
> >  *
> >  * Copyright 1999, 2000 Precision Insight, Inc., Cedar Park, Texas.
> >  * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California.
> > + * Copyright (c) 2009, Code Aurora Forum.
> >  * All Rights Reserved.
>
> again similiar, but even less code since you mostly remove code.
>
> >  *
> >  * Permission is hereby granted, free of charge, to any person obtaining a
> > @@ -242,47 +243,20 @@ int drm_lastclose(struct drm_device * dev)
> >  *
> >  * Initializes an array of drm_device structures, and attempts to
> >  * initialize all available devices, using consecutive minors, registering
> > the
> > - * stubs and initializing the AGP device.
> > + * stubs and initializing the device.
> >  *
> >  * Expands the \c DRIVER_PREINIT and \c DRIVER_POST_INIT macros before and
> >  * after the initialization for driver customization.
> >  */
> >  int drm_init(struct drm_driver *driver)
> >  {
> > -       struct pci_dev *pdev = NULL;
> > -       const struct pci_device_id *pid;
> > -       int i;
> > -
> >        DRM_DEBUG("\n");
> > -
> >        INIT_LIST_HEAD(&driver->device_list);
> >
> > -       if (driver->driver_features & DRIVER_MODESET)
> > -               return pci_register_driver(&driver->pci_driver);
> > -
> > -       /* If not using KMS, fall back to stealth mode manual scanning. */
> > -       for (i = 0; driver->pci_driver.id_table[i].vendor != 0; i++) {
> > -               pid = &driver->pci_driver.id_table[i];
> > -
> > -               /* Loop around setting up a DRM device for each PCI device
> > -                * matching our ID and device class.  If we had the internal
> > -                * function that pci_get_subsys and pci_get_class used, we'd
> > -                * be able to just pass pid in instead of doing a two-stage
> > -                * thing.
> > -                */
> > -               pdev = NULL;
> > -               while ((pdev =
> > -                       pci_get_subsys(pid->vendor, pid->device,
> > pid->subvendor,
> > -                                      pid->subdevice, pdev)) != NULL) {
> > -                       if ((pdev->class & pid->class_mask) != pid->class)
> > -                               continue;
> > -
> > -                       /* stealth mode requires a manual probe */
> > -                       pci_dev_get(pdev);
> > -                       drm_get_dev(pdev, pid, driver);
> > -               }
> > -       }
> > -       return 0;
> > +       if (driver->driver_features & DRIVER_USE_PLATFORM_DEVICE)
> > +               return drm_platform_init(driver);
> > +       else
> > +               return drm_pci_init(driver);
> >  }
> >
> >  EXPORT_SYMBOL(drm_init);
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index ab6c973..48a14a0 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1220,6 +1220,9 @@ struct edid *drm_get_edid(struct drm_connector
> > *connector,
> >        int ret;
> >        struct edid *edid;
> >
> > +       if (drm_core_check_feature(connector->dev,
> > DRIVER_USE_PLATFORM_DEVICE))
> > +               return NULL;
> > +
>
>
> This makes no sense, having the ability to probe EDID or not is most
> definitely not a platform vs PCI problem.

Yeah, that was my poor man's "Don't probe EDID" hack.  I'm not sure if there
is a smart way to implicitly check to see if EDID should be probed, but I'm
worried about abusing the features flag too badly, though if a DRIVER_USE_EDID
is needed then we shouldn't be shy about using it.

> >  {
> >        struct drm_irq_busid *p = data;
> >
> > +       if (drm_core_check_feature(dev, DRIVER_USE_PLATFORM_DEVICE))
> > +               return -EINVAL;
>
> Not 100% sure about this, but if you only intend on KMS and don't need to
> inform userspace of irq support this should be okay.

Again, a "don't do this" hack.  I'll look at this more carefully and see if there
is a good way to evaluate this based on the hooks that the platform has defined.

> > diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> > index 4ac900f..22aaaae 100644
> > --- a/drivers/gpu/drm/drm_vm.c
> > +++ b/drivers/gpu/drm/drm_vm.c
> > @@ -60,7 +60,7 @@ static pgprot_t drm_io_prot(uint32_t map_type, struct
> > vm_area_struct *vma)
> >                tmp = pgprot_writecombine(tmp);
> >        else
> >                tmp = pgprot_noncached(tmp);
> > -#elif defined(__sparc__)
> > +#elif defined(__sparc__) || defined(__arm__)
> >        tmp = pgprot_noncached(tmp);
> >  #endif
> >        return tmp;
> > @@ -600,6 +600,7 @@ int drm_mmap_locked(struct file *filp, struct
> > vm_area_struct *vma)
> >        }
> >
> >        switch (map->type) {
> > +#if !defined(__arm__)
> >        case _DRM_AGP:
> >                if (drm_core_has_AGP(dev) && dev->agp->cant_use_aperture) {
> >                        /*
> > @@ -614,20 +615,31 @@ int drm_mmap_locked(struct file *filp, struct
> > vm_area_struct *vma)
> >                        break;
> >                }
> >                /* fall through to _DRM_FRAME_BUFFER... */
> > +#endif
> >        case _DRM_FRAME_BUFFER:
> >        case _DRM_REGISTERS:
> >                offset = dev->driver->get_reg_ofs(dev);
> >                vma->vm_flags |= VM_IO; /* not in core dump */
> >                vma->vm_page_prot = drm_io_prot(map->type, vma);
> > +#if !defined(__arm__)
> >                if (io_remap_pfn_range(vma, vma->vm_start,
> >                                       (map->offset + offset) >> PAGE_SHIFT,
> >                                       vma->vm_end - vma->vm_start,
> >                                       vma->vm_page_prot))
> >                        return -EAGAIN;
> > +#else
> > +               if (remap_pfn_range(vma, vma->vm_start,
> > +                                       (map->offset + offset) >>
> > PAGE_SHIFT,
> > +                                       vma->vm_end - vma->vm_start,
> > +                                       vma->vm_page_prot))
> > +                       return -EAGAIN;
> > +#endif
> > +
> >                DRM_DEBUG("   Type = %d; start = 0x%lx, end = 0x%lx,"
> >                          " offset = 0x%llx\n",
> >                          map->type,
> >                          vma->vm_start, vma->vm_end, (unsigned long
> > long)(map->offset + offset));
> > +
> >                vma->vm_ops = &drm_vm_ops;
> >                break;
> >        case _DRM_CONSISTENT:
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index cf4cb3e..29d26c2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -392,7 +392,7 @@ int i965_reset(struct drm_device *dev, u8 flags)
> >  static int __devinit
> >  i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  {
> > -       return drm_get_dev(pdev, ent, &driver);
> > +       return drm_get_pci_dev(pdev, ent, &driver);
> >  }
> >
> >  static void
> > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
> > b/drivers/gpu/drm/radeon/radeon_drv.c
> > index 8ba3de7..6586f6a 100644
> > --- a/drivers/gpu/drm/radeon/radeon_drv.c
> > +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> > @@ -223,7 +223,7 @@ static struct drm_driver kms_driver;
> >  static int __devinit
> >  radeon_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  {
> > -       return drm_get_dev(pdev, ent, &kms_driver);
> > +       return drm_get_pci_dev(pdev, ent, &kms_driver);
> >  }
>
> This also doesn't address noueau/vmwgfx entry points.

Yep, thats my bad.  I'll refresh and push better patches without whitespace stupidity.
Thanks for your comments.

Jordan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/