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

* Re: [PATCH] drm: Allow platform devices to register as DRM devices
       [not found] ` <4B8BE489.7020402@codeaurora.org>
@ 2010-03-15  0:56   ` Dave Airlie
  2010-03-15 18:04     ` Jordan Crouse
  2010-03-16  2:41     ` Paul Mundt
  2010-03-15 19:27   ` Ville Syrjälä
                     ` (5 subsequent siblings)
  6 siblings, 2 replies; 17+ messages in thread
From: Dave Airlie @ 2010-03-15  0:56 UTC (permalink / raw)
  To: Jordan Crouse; +Cc: dri-devel, linux-kernel, linux-arm-msm

On Tue, Mar 2, 2010 at 2:00 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote:
>
> Allow platform devices without PCI resources to be DRM devices.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>

This patch has a bunch of whitespace damage at least in my inbox and
also in patchwork

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).

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??

> +       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.

>  {
>        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.

> 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.

Dave.

^ permalink raw reply	[flat|nested] 17+ messages in thread