[PATCH 00/18] drm_bus cleanups and other cruft removal

[PATCH 00/18] drm_bus cleanups and other cruft removal

[Daniel Vetter]

Hi all,

I've chatted a bit with Thierry about how we could allow drivers to not even
required a drm_bus any more. Which is relevant when e.g. due to the new
master/component no platform device is conveniently around.

So I've brushed off my old series to remove some drm_bus functions and other
cruft and rebased it onto latest drm-next.

It gets rid of everything but drm_bus->set_busid, but Thierry has a good plan to
make that one optional too - it's only really needed for backwards compat with
some old libdrm versions on pci drm drivers.

Comments and review highly welcome.

Presuming no one screams I plan to send a pull request with these patches to
Dave fairly early for 3.16 so that Thierry can base his tegra rework on top of
it.

Cheers, Daniel

Daniel Vetter (18):
  drm/omap: fix up pdev_remove
  drm/irq: simplify irq checks in drm_wait_vblank
  drm/pci: fold in irq_by_busid support
  drm/irq: drm_control is a legacy ioctl, so pci devices only
  drm/irq: remove cargo-culted locking from irq_install/unistall
  drm: remove drm_dev_to_irq from drivers
  drm: kill drm_bus->bus_type
  drm: Rip out totally bogus vga_switcheroo->can_switch locking
  drm: rename dev->count_lock to dev->buf_lock
  drm/irq: track the irq installed in drm_irq_install in dev->irq
  drm/irq: Look up the pci irq directly in the drm_control ioctl
  drm: pass the irq explicitly to drm_irq_install
  drm: remove bus->get_irq implementations
  drm: inline drm_pci_set_unique
  drm: rip out dev->devname
  drm: remove drm_bus->get_name
  drm: Remove dev->kdriver
  drm/<drivers>: don't set driver->dev_priv_size to 0

 Documentation/DocBook/drm.tmpl           |  10 +--
 drivers/gpu/drm/armada/armada_drv.c      |   2 +-
 drivers/gpu/drm/ast/ast_drv.c            |   1 -
 drivers/gpu/drm/drm_bufs.c               |  32 +++++-----
 drivers/gpu/drm/drm_info.c               |   6 +-
 drivers/gpu/drm/drm_ioctl.c              |  13 ++--
 drivers/gpu/drm/drm_irq.c                | 106 +++++++++++--------------------
 drivers/gpu/drm/drm_pci.c                |  94 +++++++++++++--------------
 drivers/gpu/drm/drm_platform.c           |  25 --------
 drivers/gpu/drm/drm_stub.c               |   7 +-
 drivers/gpu/drm/drm_usb.c                |  14 ----
 drivers/gpu/drm/gma500/psb_drv.c         |   2 +-
 drivers/gpu/drm/i915/i915_dma.c          |   9 ++-
 drivers/gpu/drm/i915/i915_drv.c          |   9 ++-
 drivers/gpu/drm/i915/i915_gem.c          |   7 +-
 drivers/gpu/drm/mga/mga_state.c          |   2 +-
 drivers/gpu/drm/msm/msm_drv.c            |   2 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c    |   7 +-
 drivers/gpu/drm/omapdrm/omap_drv.c       |   2 +
 drivers/gpu/drm/qxl/qxl_drv.c            |   1 -
 drivers/gpu/drm/qxl/qxl_irq.c            |   2 +-
 drivers/gpu/drm/r128/r128_state.c        |   2 +-
 drivers/gpu/drm/radeon/radeon_device.c   |   7 +-
 drivers/gpu/drm/radeon/radeon_drv.c      |   1 -
 drivers/gpu/drm/radeon/radeon_irq_kms.c  |   2 +-
 drivers/gpu/drm/radeon/radeon_state.c    |   2 +-
 drivers/gpu/drm/shmobile/shmob_drm_drv.c |   2 +-
 drivers/gpu/drm/tegra/bus.c              |  11 ----
 drivers/gpu/drm/tilcdc/tilcdc_drv.c      |   2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c      |   2 +-
 include/drm/drmP.h                       |  33 +++-------
 31 files changed, 159 insertions(+), 258 deletions(-)

-- 
1.8.5.2

[PATCH 01/18] drm/omap: fix up pdev_remove

[Daniel Vetter]

Dave accidentally merged the wrong version of the patch in

commit fd3c02531461924853db65f2664db361b53a70d3
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Dec 11 11:34:26 2013 +0100

    drm/omap: call drm_put_dev directly in ->remove

which did not include the fix from Rob's review: omapdrm is split into
the drm driver and the dmm driver (yeah, I've complained about that
almost-impossible-to-spot difference, too). We need to unregister the
dmm driver in the pdev_remove hook.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index bf39fcc49e0f..89557989737d 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -699,6 +699,8 @@ static int pdev_remove(struct platform_device *device)
 	omap_disconnect_dssdevs();
 	omap_crtc_pre_uninit();
 
+	platform_driver_unregister(&omap_dmm_driver);
+
 	drm_put_dev(platform_get_drvdata(device));
 	return 0;
 }
-- 
1.8.5.2

Re: [PATCH 01/18] drm/omap: fix up pdev_remove

[Rob Clark]

On Fri, Apr 11, 2014 at 5:35 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Dave accidentally merged the wrong version of the patch in
>
> commit fd3c02531461924853db65f2664db361b53a70d3
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Dec 11 11:34:26 2013 +0100
>
>     drm/omap: call drm_put_dev directly in ->remove
>
> which did not include the fix from Rob's review: omapdrm is split into
> the drm driver and the dmm driver (yeah, I've complained about that
> almost-impossible-to-spot difference, too). We need to unregister the
> dmm driver in the pdev_remove hook.
>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index bf39fcc49e0f..89557989737d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -699,6 +699,8 @@ static int pdev_remove(struct platform_device *device)
>         omap_disconnect_dssdevs();
>         omap_crtc_pre_uninit();
>
> +       platform_driver_unregister(&omap_dmm_driver);
> +
>         drm_put_dev(platform_get_drvdata(device));
>         return 0;
>  }
> --
> 1.8.5.2
>

Re: [PATCH 01/18] drm/omap: fix up pdev_remove

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1716 bytes --]

Hi,

On 12/04/14 00:35, Daniel Vetter wrote:
> Dave accidentally merged the wrong version of the patch in
> 
> commit fd3c02531461924853db65f2664db361b53a70d3
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Dec 11 11:34:26 2013 +0100
> 
>     drm/omap: call drm_put_dev directly in ->remove
> 
> which did not include the fix from Rob's review: omapdrm is split into
> the drm driver and the dmm driver (yeah, I've complained about that
> almost-impossible-to-spot difference, too). We need to unregister the
> dmm driver in the pdev_remove hook.
> 
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index bf39fcc49e0f..89557989737d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -699,6 +699,8 @@ static int pdev_remove(struct platform_device *device)
>  	omap_disconnect_dssdevs();
>  	omap_crtc_pre_uninit();
>  
> +	platform_driver_unregister(&omap_dmm_driver);
> +
>  	drm_put_dev(platform_get_drvdata(device));
>  	return 0;
>  }

Your patch does fix the "drm/omap: call drm_put_dev directly in
->remove" patch, but I think the unregistering is not right if done in
pdev_remove.

I sent this patch a few weeks ago:

http://www.spinics.net/lists/dri-devel/msg56464.html

It unregisters the dmm driver at the module exit function, which is the
proper place for it as the dmm driver is registered in the module init
function.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/18] drm/omap: fix up pdev_remove

[Daniel Vetter]

On Mon, Apr 14, 2014 at 09:26:14AM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 12/04/14 00:35, Daniel Vetter wrote:
> > Dave accidentally merged the wrong version of the patch in
> > 
> > commit fd3c02531461924853db65f2664db361b53a70d3
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Wed Dec 11 11:34:26 2013 +0100
> > 
> >     drm/omap: call drm_put_dev directly in ->remove
> > 
> > which did not include the fix from Rob's review: omapdrm is split into
> > the drm driver and the dmm driver (yeah, I've complained about that
> > almost-impossible-to-spot difference, too). We need to unregister the
> > dmm driver in the pdev_remove hook.
> > 
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/omapdrm/omap_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> > index bf39fcc49e0f..89557989737d 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > @@ -699,6 +699,8 @@ static int pdev_remove(struct platform_device *device)
> >  	omap_disconnect_dssdevs();
> >  	omap_crtc_pre_uninit();
> >  
> > +	platform_driver_unregister(&omap_dmm_driver);
> > +
> >  	drm_put_dev(platform_get_drvdata(device));
> >  	return 0;
> >  }
> 
> Your patch does fix the "drm/omap: call drm_put_dev directly in
> ->remove" patch, but I think the unregistering is not right if done in
> pdev_remove.
> 
> I sent this patch a few weeks ago:
> 
> http://www.spinics.net/lists/dri-devel/msg56464.html
> 
> It unregisters the dmm driver at the module exit function, which is the
> proper place for it as the dmm driver is registered in the module init
> function.

I don't mind how this is getting fixed, I just don't like when regression
caused by my patches are left lingering too long ;-)

Have you merged your patch for 3.14-rc already so that I can drop this one
here?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/18] drm/omap: fix up pdev_remove

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 379 bytes --]

On 16/04/14 11:15, Daniel Vetter wrote:

> I don't mind how this is getting fixed, I just don't like when regression
> caused by my patches are left lingering too long ;-)
> 
> Have you merged your patch for 3.14-rc already so that I can drop this one
> here?

Did you mean 3.15-rc? It's on my branch for 3.15 omapdrm fixes, which
I'm about to send today.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/18] drm/omap: fix up pdev_remove

[Daniel Vetter]

On Wed, Apr 16, 2014 at 10:27 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 16/04/14 11:15, Daniel Vetter wrote:
>
>> I don't mind how this is getting fixed, I just don't like when regression
>> caused by my patches are left lingering too long ;-)
>>
>> Have you merged your patch for 3.14-rc already so that I can drop this one
>> here?
>
> Did you mean 3.15-rc? It's on my branch for 3.15 omapdrm fixes, which
> I'm about to send today.

Yeah, 3.15-fixes, typoe'ed that ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 02/18] drm/irq: simplify irq checks in drm_wait_vblank

[Daniel Vetter]

Checking for both an irq number _and_ whether it's enabled is
redundant. Originally I've thought the drm_dev_to_irq call would break
drivers which do their own irq checking, but those shouldn't have
DRIVER_HAVE_IRQ set as Thierry Reding pointed out. But such drivers
already need to set dev->irq_enabled for other reasons, so we might as
well ditch that check, too.

v2: Also drop the HAVE_IRQ check.

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index bd6e65fde1b9..7ef98c01c3d5 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1160,9 +1160,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	int ret;
 	unsigned int flags, seq, crtc, high_crtc;
 
-	if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
-		if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled))
-			return -EINVAL;
+	if (!dev->irq_enabled)
+		return -EINVAL;
 
 	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
 		return -EINVAL;
-- 
1.8.5.2

Re: [PATCH 02/18] drm/irq: simplify irq checks in drm_wait_vblank

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 750 bytes --]

On Fri, Apr 11, 2014 at 11:35:59PM +0200, Daniel Vetter wrote:
> Checking for both an irq number _and_ whether it's enabled is
> redundant. Originally I've thought the drm_dev_to_irq call would break
> drivers which do their own irq checking, but those shouldn't have
> DRIVER_HAVE_IRQ set as Thierry Reding pointed out. But such drivers
> already need to set dev->irq_enabled for other reasons, so we might as
> well ditch that check, too.
> 
> v2: Also drop the HAVE_IRQ check.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/18] drm/pci: fold in irq_by_busid support

[Daniel Vetter]

This is a ums-only ioctl, and we've only ever supported ums (at least
in upstream) on pci devices. So no point in keeping that piece of
legacy logic abstracted within the drm bus driver.

To keep things work without CONFIG_PCI also add a dummy ioctl.

v2: Block the irq_by_busid ioctl for modeset drivers.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 27 ---------------------------
 drivers/gpu/drm/drm_pci.c | 40 ++++++++++++++++++++++++++++++++++++++--
 include/drm/drmP.h        |  1 -
 3 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 7ef98c01c3d5..c02b602325cb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -56,33 +56,6 @@
  */
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 1000000
 
-/**
- * Get interrupt from bus id.
- *
- * \param inode device inode.
- * \param file_priv DRM file private.
- * \param cmd command.
- * \param arg user argument, pointing to a drm_irq_busid structure.
- * \return zero on success or a negative number on failure.
- *
- * Finds the PCI device with the specified bus id and gets its IRQ number.
- * This IOCTL is deprecated, and will now return EINVAL for any busid not equal
- * to that of the device that this DRM instance attached to.
- */
-int drm_irq_by_busid(struct drm_device *dev, void *data,
-		     struct drm_file *file_priv)
-{
-	struct drm_irq_busid *p = data;
-
-	if (!dev->driver->bus->irq_by_busid)
-		return -EINVAL;
-
-	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
-		return -EINVAL;
-
-	return dev->driver->bus->irq_by_busid(dev, p);
-}
-
 /*
  * Clear vblank timestamp buffer for a crtc.
  */
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 9c696a5ad74d..e8040e57cbbb 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -247,7 +247,6 @@ err:
 	return ret;
 }
 
-
 static int drm_pci_irq_by_busid(struct drm_device *dev, struct drm_irq_busid *p)
 {
 	if ((p->busnum >> 8) != drm_get_pci_domain(dev) ||
@@ -262,6 +261,38 @@ static int drm_pci_irq_by_busid(struct drm_device *dev, struct drm_irq_busid *p)
 	return 0;
 }
 
+/**
+ * Get interrupt from bus id.
+ *
+ * \param inode device inode.
+ * \param file_priv DRM file private.
+ * \param cmd command.
+ * \param arg user argument, pointing to a drm_irq_busid structure.
+ * \return zero on success or a negative number on failure.
+ *
+ * Finds the PCI device with the specified bus id and gets its IRQ number.
+ * This IOCTL is deprecated, and will now return EINVAL for any busid not equal
+ * to that of the device that this DRM instance attached to.
+ */
+int drm_irq_by_busid(struct drm_device *dev, void *data,
+		     struct drm_file *file_priv)
+{
+	struct drm_irq_busid *p = data;
+
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	/* UMS was only ever support on pci devices. */
+	if (WARN_ON(!dev->pdev))
+		return -EINVAL;
+
+	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
+		return -EINVAL;
+
+	return drm_pci_irq_by_busid(dev, p);
+}
+
+
 static void drm_pci_agp_init(struct drm_device *dev)
 {
 	if (drm_core_check_feature(dev, DRIVER_USE_AGP)) {
@@ -292,7 +323,6 @@ static struct drm_bus drm_pci_bus = {
 	.get_name = drm_pci_get_name,
 	.set_busid = drm_pci_set_busid,
 	.set_unique = drm_pci_set_unique,
-	.irq_by_busid = drm_pci_irq_by_busid,
 };
 
 /**
@@ -453,6 +483,12 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
 }
 
 void drm_pci_agp_destroy(struct drm_device *dev) {}
+
+int drm_irq_by_busid(struct drm_device *dev, void *data,
+		     struct drm_file *file_priv)
+{
+	return -EINVAL;
+}
 #endif
 
 EXPORT_SYMBOL(drm_pci_init);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index a7c2a862b4f4..6d7ca98d0143 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -737,7 +737,6 @@ struct drm_bus {
 	int (*set_busid)(struct drm_device *dev, struct drm_master *master);
 	int (*set_unique)(struct drm_device *dev, struct drm_master *master,
 			  struct drm_unique *unique);
-	int (*irq_by_busid)(struct drm_device *dev, struct drm_irq_busid *p);
 };
 
 /**
-- 
1.8.5.2

Re: [PATCH 03/18] drm/pci: fold in irq_by_busid support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 671 bytes --]

On Fri, Apr 11, 2014 at 11:36:00PM +0200, Daniel Vetter wrote:
[...]
> +int drm_irq_by_busid(struct drm_device *dev, void *data,
> +		     struct drm_file *file_priv)
> +{
> +	struct drm_irq_busid *p = data;
> +
> +	if (drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
> +
> +	/* UMS was only ever support on pci devices. */

Nit: s/pci/PCI?

> +	if (WARN_ON(!dev->pdev))
> +		return -EINVAL;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> +		return -EINVAL;
> +
> +	return drm_pci_irq_by_busid(dev, p);
> +}
> +
> +

Nit: there's a gratuitous blank line introduced here.

Other than that:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/18] drm/irq: drm_control is a legacy ioctl, so pci devices only

[Daniel Vetter]

This just adds a correspdonding check, follow-up patches will exploit
this.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c02b602325cb..4b019646f556 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -386,22 +386,22 @@ int drm_control(struct drm_device *dev, void *data,
 	 * this used to be a separate function in drm_dma.h
 	 */
 
+	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
+		return 0;
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		return 0;
+	/* UMS was only ever support on pci devices. */
+	if (WARN_ON(!dev->pdev))
+		return -EINVAL;
 
 	switch (ctl->func) {
 	case DRM_INST_HANDLER:
-		if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
-			return 0;
-		if (drm_core_check_feature(dev, DRIVER_MODESET))
-			return 0;
 		if (dev->if_version < DRM_IF_VERSION(1, 2) &&
 		    ctl->irq != drm_dev_to_irq(dev))
 			return -EINVAL;
+
 		return drm_irq_install(dev);
 	case DRM_UNINST_HANDLER:
-		if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
-			return 0;
-		if (drm_core_check_feature(dev, DRIVER_MODESET))
-			return 0;
 		return drm_irq_uninstall(dev);
 	default:
 		return -EINVAL;
-- 
1.8.5.2

Re: [PATCH 04/18] drm/irq: drm_control is a legacy ioctl, so pci devices only

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1088 bytes --]

On Fri, Apr 11, 2014 at 11:36:01PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c02b602325cb..4b019646f556 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -386,22 +386,22 @@ int drm_control(struct drm_device *dev, void *data,
>  	 * this used to be a separate function in drm_dma.h
>  	 */
>  
> +	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> +		return 0;
> +	if (drm_core_check_feature(dev, DRIVER_MODESET))
> +		return 0;
> +	/* UMS was only ever support on pci devices. */
> +	if (WARN_ON(!dev->pdev))
> +		return -EINVAL;
>  
>  	switch (ctl->func) {
>  	case DRM_INST_HANDLER:
> -		if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> -			return 0;
> -		if (drm_core_check_feature(dev, DRIVER_MODESET))
> -			return 0;
>  		if (dev->if_version < DRM_IF_VERSION(1, 2) &&
>  		    ctl->irq != drm_dev_to_irq(dev))
>  			return -EINVAL;
> +

Nit: This blank line seems unnecessary, but either way:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/18] drm/irq: remove cargo-culted locking from irq_install/unistall

[Daniel Vetter]

The dev->struct_mutex locking in drm_irq.c only protects
dev->irq_enabled. Which isn't really much at all and only prevents
especially nasty ums userspace from concurrently installing the
interrupt handling a few times. Or at least trying.

There are tons of unlocked readers of dev->irqs_enabled in the vblank
wait code (and by extension also in the pageflip code since that uses
the same vblank timestamp engine).

Real modesetting drivers should ensure that nothing can go haywire
with a sane setup teardown sequence. So we only really need this for
the drm_control ioctl, everywhere else this will just paper over
nastiness.

Note that drm/i915 is a bit specially due to the gem+ums combination.
So there we also need to properly protect the entervt and leavevt
ioctls. But it's definitely saner to do everything in one go than to
drop the lock in-between.

Finally there's the gpu reset code in drm/i915. That one's just race
(concurrent userspace calls to for vblank waits of pageflips could
spuriously fail). So wrap it up in with a nice comment since fixing
this is more involved.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c       | 30 +++++++++++++-----------------
 drivers/gpu/drm/i915/i915_drv.c |  5 +++++
 drivers/gpu/drm/i915/i915_gem.c |  5 +++--
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 4b019646f556..1a6fc8fc0cd5 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -254,20 +254,13 @@ int drm_irq_install(struct drm_device *dev)
 	if (drm_dev_to_irq(dev) == 0)
 		return -EINVAL;
 
-	mutex_lock(&dev->struct_mutex);
-
 	/* Driver must have been initialized */
-	if (!dev->dev_private) {
-		mutex_unlock(&dev->struct_mutex);
+	if (!dev->dev_private)
 		return -EINVAL;
-	}
 
-	if (dev->irq_enabled) {
-		mutex_unlock(&dev->struct_mutex);
+	if (dev->irq_enabled)
 		return -EBUSY;
-	}
 	dev->irq_enabled = true;
-	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
 
@@ -288,9 +281,7 @@ int drm_irq_install(struct drm_device *dev)
 			  sh_flags, irqname, dev);
 
 	if (ret < 0) {
-		mutex_lock(&dev->struct_mutex);
 		dev->irq_enabled = false;
-		mutex_unlock(&dev->struct_mutex);
 		return ret;
 	}
 
@@ -302,9 +293,7 @@ int drm_irq_install(struct drm_device *dev)
 		ret = dev->driver->irq_postinstall(dev);
 
 	if (ret < 0) {
-		mutex_lock(&dev->struct_mutex);
 		dev->irq_enabled = false;
-		mutex_unlock(&dev->struct_mutex);
 		if (!drm_core_check_feature(dev, DRIVER_MODESET))
 			vga_client_register(dev->pdev, NULL, NULL, NULL);
 		free_irq(drm_dev_to_irq(dev), dev);
@@ -330,10 +319,8 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
 		return -EINVAL;
 
-	mutex_lock(&dev->struct_mutex);
 	irq_enabled = dev->irq_enabled;
 	dev->irq_enabled = false;
-	mutex_unlock(&dev->struct_mutex);
 
 	/*
 	 * Wake up any waiters so they don't hang.
@@ -381,6 +368,7 @@ int drm_control(struct drm_device *dev, void *data,
 		struct drm_file *file_priv)
 {
 	struct drm_control *ctl = data;
+	int ret = 0;
 
 	/* if we haven't irq we fallback for compatibility reasons -
 	 * this used to be a separate function in drm_dma.h
@@ -400,9 +388,17 @@ int drm_control(struct drm_device *dev, void *data,
 		    ctl->irq != drm_dev_to_irq(dev))
 			return -EINVAL;
 
-		return drm_irq_install(dev);
+		mutex_lock(&dev->struct_mutex);
+		ret = drm_irq_install(dev);
+		mutex_unlock(&dev->struct_mutex);
+
+		return ret;
 	case DRM_UNINST_HANDLER:
-		return drm_irq_uninstall(dev);
+		mutex_lock(&dev->struct_mutex);
+		ret = drm_irq_uninstall(dev);
+		mutex_unlock(&dev->struct_mutex);
+
+		return ret;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 82f4d1f47d3b..87ce105910f2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -746,6 +746,11 @@ int i915_reset(struct drm_device *dev)
 			return ret;
 		}
 
+		/*
+		 * FIXME: This is horribly race against concurrent pageflip and
+		 * vblank wait ioctls since they can observe dev->irqs_disabled
+		 * being false when they shouldn't be able to.
+		 */
 		drm_irq_uninstall(dev);
 		drm_irq_install(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6370a761d137..48abe3b4976b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4522,16 +4522,15 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 	}
 
 	BUG_ON(!list_empty(&dev_priv->gtt.base.active_list));
-	mutex_unlock(&dev->struct_mutex);
 
 	ret = drm_irq_install(dev);
 	if (ret)
 		goto cleanup_ringbuffer;
+	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
 
 cleanup_ringbuffer:
-	mutex_lock(&dev->struct_mutex);
 	i915_gem_cleanup_ringbuffer(dev);
 	dev_priv->ums.mm_suspended = 1;
 	mutex_unlock(&dev->struct_mutex);
@@ -4546,7 +4545,9 @@ i915_gem_leavevt_ioctl(struct drm_device *dev, void *data,
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return 0;
 
+	mutex_lock(&dev->struct_mutex);
 	drm_irq_uninstall(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	return i915_gem_suspend(dev);
 }
-- 
1.8.5.2

Re: [PATCH 05/18] drm/irq: remove cargo-culted locking from irq_install/unistall

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1584 bytes --]

On Fri, Apr 11, 2014 at 11:36:02PM +0200, Daniel Vetter wrote:
> The dev->struct_mutex locking in drm_irq.c only protects
> dev->irq_enabled. Which isn't really much at all and only prevents
> especially nasty ums userspace from concurrently installing the
> interrupt handling a few times. Or at least trying.
> 
> There are tons of unlocked readers of dev->irqs_enabled in the vblank
> wait code (and by extension also in the pageflip code since that uses
> the same vblank timestamp engine).
> 
> Real modesetting drivers should ensure that nothing can go haywire
> with a sane setup teardown sequence. So we only really need this for
> the drm_control ioctl, everywhere else this will just paper over
> nastiness.
> 
> Note that drm/i915 is a bit specially due to the gem+ums combination.
> So there we also need to properly protect the entervt and leavevt
> ioctls. But it's definitely saner to do everything in one go than to
> drop the lock in-between.
> 
> Finally there's the gpu reset code in drm/i915. That one's just race
> (concurrent userspace calls to for vblank waits of pageflips could
> spuriously fail). So wrap it up in with a nice comment since fixing
> this is more involved.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c       | 30 +++++++++++++-----------------
>  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
>  drivers/gpu/drm/i915/i915_gem.c |  5 +++--
>  3 files changed, 21 insertions(+), 19 deletions(-)

Looks good to me:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/18] drm/irq: remove cargo-culted locking from irq_install/unistall

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1790 bytes --]

On Thu, Apr 17, 2014 at 04:43:17PM +0200, Thierry Reding wrote:
> On Fri, Apr 11, 2014 at 11:36:02PM +0200, Daniel Vetter wrote:
> > The dev->struct_mutex locking in drm_irq.c only protects
> > dev->irq_enabled. Which isn't really much at all and only prevents
> > especially nasty ums userspace from concurrently installing the
> > interrupt handling a few times. Or at least trying.
> > 
> > There are tons of unlocked readers of dev->irqs_enabled in the vblank
> > wait code (and by extension also in the pageflip code since that uses
> > the same vblank timestamp engine).
> > 
> > Real modesetting drivers should ensure that nothing can go haywire
> > with a sane setup teardown sequence. So we only really need this for
> > the drm_control ioctl, everywhere else this will just paper over
> > nastiness.
> > 
> > Note that drm/i915 is a bit specially due to the gem+ums combination.
> > So there we also need to properly protect the entervt and leavevt
> > ioctls. But it's definitely saner to do everything in one go than to
> > drop the lock in-between.
> > 
> > Finally there's the gpu reset code in drm/i915. That one's just race
> > (concurrent userspace calls to for vblank waits of pageflips could
> > spuriously fail). So wrap it up in with a nice comment since fixing
> > this is more involved.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_irq.c       | 30 +++++++++++++-----------------
> >  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
> >  drivers/gpu/drm/i915/i915_gem.c |  5 +++--
> >  3 files changed, 21 insertions(+), 19 deletions(-)
> 
> Looks good to me:
> 
> Reviewed-by: Thierry Reding <treding@nvidia.com>

Oh, perhaps s/unistall/uninstall/ in the commit subject.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/18] drm: remove drm_dev_to_irq from drivers

[Daniel Vetter]

Only used in some legacy pci drivers, and dereferncing the pci irq is
actually shorter ...

Since this removes all users for drm_dev_to_irq from the tree except
in drm_irq.c, move the inline helper in there. It'll disappear soon,
too.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c             | 5 +++++
 drivers/gpu/drm/mga/mga_state.c       | 2 +-
 drivers/gpu/drm/r128/r128_state.c     | 2 +-
 drivers/gpu/drm/radeon/radeon_state.c | 2 +-
 include/drm/drmP.h                    | 5 -----
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 1a6fc8fc0cd5..330e85b19115 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -233,6 +233,11 @@ static void drm_irq_vgaarb_nokms(void *cookie, bool state)
 	}
 }
 
+static inline int drm_dev_to_irq(struct drm_device *dev)
+{
+	return dev->driver->bus->get_irq(dev);
+}
+
 /**
  * Install IRQ handler.
  *
diff --git a/drivers/gpu/drm/mga/mga_state.c b/drivers/gpu/drm/mga/mga_state.c
index 314685b7f41f..3cb58df5237e 100644
--- a/drivers/gpu/drm/mga/mga_state.c
+++ b/drivers/gpu/drm/mga/mga_state.c
@@ -1020,7 +1020,7 @@ static int mga_getparam(struct drm_device *dev, void *data, struct drm_file *fil
 
 	switch (param->param) {
 	case MGA_PARAM_IRQ_NR:
-		value = drm_dev_to_irq(dev);
+		value = dev->pdev->irq;
 		break;
 	case MGA_PARAM_CARD_TYPE:
 		value = dev_priv->chipset;
diff --git a/drivers/gpu/drm/r128/r128_state.c b/drivers/gpu/drm/r128/r128_state.c
index e806dacd452f..97064dd434c2 100644
--- a/drivers/gpu/drm/r128/r128_state.c
+++ b/drivers/gpu/drm/r128/r128_state.c
@@ -1594,7 +1594,7 @@ static int r128_getparam(struct drm_device *dev, void *data, struct drm_file *fi
 
 	switch (param->param) {
 	case R128_PARAM_IRQ_NR:
-		value = drm_dev_to_irq(dev);
+		value = dev->pdev->irq;
 		break;
 	default:
 		return -EINVAL;
diff --git a/drivers/gpu/drm/radeon/radeon_state.c b/drivers/gpu/drm/radeon/radeon_state.c
index 956ab7f14e16..b576549fc783 100644
--- a/drivers/gpu/drm/radeon/radeon_state.c
+++ b/drivers/gpu/drm/radeon/radeon_state.c
@@ -3054,7 +3054,7 @@ static int radeon_cp_getparam(struct drm_device *dev, void *data, struct drm_fil
 		if ((dev_priv->flags & RADEON_FAMILY_MASK) >= CHIP_R600)
 			value = 0;
 		else
-			value = drm_dev_to_irq(dev);
+			value = dev->pdev->irq;
 		break;
 	case RADEON_PARAM_GART_BASE:
 		value = dev_priv->gart_vm_start;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 6d7ca98d0143..41839ea0c1ee 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1185,11 +1185,6 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev,
 	return ((dev->driver->driver_features & feature) ? 1 : 0);
 }
 
-static inline int drm_dev_to_irq(struct drm_device *dev)
-{
-	return dev->driver->bus->get_irq(dev);
-}
-
 static inline void drm_device_set_unplugged(struct drm_device *dev)
 {
 	smp_wmb();
-- 
1.8.5.2

Re: [PATCH 06/18] drm: remove drm_dev_to_irq from drivers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 465 bytes --]

On Fri, Apr 11, 2014 at 11:36:03PM +0200, Daniel Vetter wrote:
> Only used in some legacy pci drivers, and dereferncing the pci irq is
> actually shorter ...
> 
> Since this removes all users for drm_dev_to_irq from the tree except
> in drm_irq.c, move the inline helper in there. It'll disappear soon,
> too.

Nit: s/dereferncing/dereferencing/ and s/pci/PCI/? Other than that, glad
to see it go away.

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/18] drm: kill drm_bus->bus_type

[Daniel Vetter]

Completely unused. Hooray, midlayer mistakes that didn't cause work to
undo!

v2: Rebase on top of the recent tegra changes which added a host1x drm
bus.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_pci.c      | 1 -
 drivers/gpu/drm/drm_platform.c | 1 -
 drivers/gpu/drm/drm_usb.c      | 1 -
 drivers/gpu/drm/tegra/bus.c    | 1 -
 include/drm/drmP.h             | 6 ------
 5 files changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index e8040e57cbbb..ad2fc48d0f0b 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -318,7 +318,6 @@ void drm_pci_agp_destroy(struct drm_device *dev)
 }
 
 static struct drm_bus drm_pci_bus = {
-	.bus_type = DRIVER_BUS_PCI,
 	.get_irq = drm_pci_get_irq,
 	.get_name = drm_pci_get_name,
 	.set_busid = drm_pci_set_busid,
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 319ff5385601..5cd8c695824f 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -123,7 +123,6 @@ err:
 }
 
 static struct drm_bus drm_platform_bus = {
-	.bus_type = DRIVER_BUS_PLATFORM,
 	.get_irq = drm_platform_get_irq,
 	.get_name = drm_platform_get_name,
 	.set_busid = drm_platform_set_busid,
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index c3406aad2944..ed60f887c805 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -53,7 +53,6 @@ static int drm_usb_set_busid(struct drm_device *dev,
 }
 
 static struct drm_bus drm_usb_bus = {
-	.bus_type = DRIVER_BUS_USB,
 	.get_irq = drm_usb_get_irq,
 	.get_name = drm_usb_get_name,
 	.set_busid = drm_usb_set_busid,
diff --git a/drivers/gpu/drm/tegra/bus.c b/drivers/gpu/drm/tegra/bus.c
index 71cef5c13dc8..6916fa51cfa4 100644
--- a/drivers/gpu/drm/tegra/bus.c
+++ b/drivers/gpu/drm/tegra/bus.c
@@ -37,7 +37,6 @@ static int drm_host1x_set_busid(struct drm_device *dev,
 }
 
 static struct drm_bus drm_host1x_bus = {
-	.bus_type = DRIVER_BUS_HOST1X,
 	.set_busid = drm_host1x_set_busid,
 };
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 41839ea0c1ee..9f1fb8d36b67 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -143,11 +143,6 @@ int drm_err(const char *func, const char *format, ...);
 #define DRIVER_PRIME       0x4000
 #define DRIVER_RENDER      0x8000
 
-#define DRIVER_BUS_PCI 0x1
-#define DRIVER_BUS_PLATFORM 0x2
-#define DRIVER_BUS_USB 0x3
-#define DRIVER_BUS_HOST1X 0x4
-
 /***********************************************************************/
 /** \name Begin the DRM... */
 /*@{*/
@@ -731,7 +726,6 @@ struct drm_master {
 #define DRM_SCANOUTPOS_ACCURATE     (1 << 2)
 
 struct drm_bus {
-	int bus_type;
 	int (*get_irq)(struct drm_device *dev);
 	const char *(*get_name)(struct drm_device *dev);
 	int (*set_busid)(struct drm_device *dev, struct drm_master *master);
-- 
1.8.5.2

Re: [PATCH 07/18] drm: kill drm_bus->bus_type

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 848 bytes --]

On Fri, Apr 11, 2014 at 11:36:04PM +0200, Daniel Vetter wrote:
> Completely unused. Hooray, midlayer mistakes that didn't cause work to
> undo!

\o/

> v2: Rebase on top of the recent tegra changes which added a host1x drm
> bus.

FWIW, I'm hoping to get rid of the host1x DRM bus soon. With this series
applied it takes only a small additional patch to get rid of the
requirement for .set_busid() and then I can simply to drm_dev_alloc()
followed by drm_dev_register() directly in the driver.

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_pci.c      | 1 -
>  drivers/gpu/drm/drm_platform.c | 1 -
>  drivers/gpu/drm/drm_usb.c      | 1 -
>  drivers/gpu/drm/tegra/bus.c    | 1 -
>  include/drm/drmP.h             | 6 ------
>  5 files changed, 10 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/18] drm: Rip out totally bogus vga_switcheroo->can_switch locking

[Daniel Vetter]

So I just wanted to add a new field to struct drm_device and
accidentally stumbled over something. According to comments
dev->open_count is protected by dev->count_lock, but that's totally
not the case. It's protected by drm_global_mutex.

Unfortunately the vga switcheroo callbacks took this comment at face
value. The problem is that we can't just take the drm_global_mutex
because:
- It would lead to a locking inversion with the driver load/unload
  paths.
- It wouldn't actually protect anything, for that we'd need to wrap
  the entire vga switcheroo code in the drm_global_mutex. And I'm not
  sure whether that would actually solve anything.

What we probably want is a try_to_grab_switcheroo reference kind of
thing which is used in the driver's ->open callback. Then we could
move all that ->can_switch madness into the vga switcheroo core where
it really belongs.

But since that would amount to real work take the easy way out and
just add a comment. It's definitely not going to make anything worse
since doing switcheroo state changes while restarting X just isn't
recommended. Even though the delayed switching code does exactly that.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c        | 7 +++++--
 drivers/gpu/drm/nouveau/nouveau_vga.c  | 7 +++++--
 drivers/gpu/drm/radeon/radeon_device.c | 7 +++++--
 include/drm/drmP.h                     | 2 +-
 4 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4dff829c2d89..dcb67c6564dd 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1286,9 +1286,12 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	bool can_switch;
 
-	spin_lock(&dev->count_lock);
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
 	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
 	return can_switch;
 }
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index fb84da3cb50d..7b10c5a9196b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -66,9 +66,12 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	bool can_switch;
 
-	spin_lock(&dev->count_lock);
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
 	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
 	return can_switch;
 }
 
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 835516d2d257..c9811d01a3c7 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1124,9 +1124,12 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	bool can_switch;
 
-	spin_lock(&dev->count_lock);
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
 	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
 	return can_switch;
 }
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9f1fb8d36b67..bd16a4c8ece4 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1076,7 +1076,7 @@ struct drm_device {
 
 	/** \name Usage Counters */
 	/*@{ */
-	int open_count;			/**< Outstanding files open */
+	int open_count;			/**< Outstanding files open, protected by drm_global_lock. */
 	int buf_use;			/**< Buffers in use -- cannot alloc */
 	atomic_t buf_alloc;		/**< Buffer allocation in progress */
 	/*@} */
-- 
1.8.5.2

Re: [PATCH 08/18] drm: Rip out totally bogus vga_switcheroo->can_switch locking

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1387 bytes --]

On Fri, Apr 11, 2014 at 11:36:05PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 4dff829c2d89..dcb67c6564dd 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1286,9 +1286,12 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  	bool can_switch;
>  
> -	spin_lock(&dev->count_lock);
> +	/*
> +	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
> +	 * locking inversion with the driver load path. And the access here is
> +	 * completely racy anyway. So don't bother with locking for now.
> +	 */
>  	can_switch = (dev->open_count == 0);
> -	spin_unlock(&dev->count_lock);
>  	return can_switch;
>  }

Couldn't this now be shortened to:

	return (dev->open_count == 0);

? Similarily for the other drivers.

> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 9f1fb8d36b67..bd16a4c8ece4 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1076,7 +1076,7 @@ struct drm_device {
>  
>  	/** \name Usage Counters */
>  	/*@{ */
> -	int open_count;			/**< Outstanding files open */
> +	int open_count;			/**< Outstanding files open, protected by drm_global_lock. */

s/drm_global_lock/drm_global_mutex/?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Rip out totally bogus vga_switcheroo->can_switch locking

[Daniel Vetter]

So I just wanted to add a new field to struct drm_device and
accidentally stumbled over something. According to comments
dev->open_count is protected by dev->count_lock, but that's totally
not the case. It's protected by drm_global_mutex.

Unfortunately the vga switcheroo callbacks took this comment at face
value. The problem is that we can't just take the drm_global_mutex
because:
- It would lead to a locking inversion with the driver load/unload
  paths.
- It wouldn't actually protect anything, for that we'd need to wrap
  the entire vga switcheroo code in the drm_global_mutex. And I'm not
  sure whether that would actually solve anything.

What we probably want is a try_to_grab_switcheroo reference kind of
thing which is used in the driver's ->open callback. Then we could
move all that ->can_switch madness into the vga switcheroo core where
it really belongs.

But since that would amount to real work take the easy way out and
just add a comment. It's definitely not going to make anything worse
since doing switcheroo state changes while restarting X just isn't
recommended. Even though the delayed switching code does exactly that.

v2:
- Simplify the ->can_switch implementations more (Thierry)
- Fix comment about the dev->open_count locking (Thierry)

Cc: Thierry Reding <treding@nvidia.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c        | 11 ++++++-----
 drivers/gpu/drm/nouveau/nouveau_vga.c  | 11 ++++++-----
 drivers/gpu/drm/radeon/radeon_device.c | 11 ++++++-----
 include/drm/drmP.h                     |  2 +-
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 96177eec0a0e..283ff06001bc 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1277,12 +1277,13 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_
 static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
-	bool can_switch;
 
-	spin_lock(&dev->count_lock);
-	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
-	return can_switch;
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
+	return dev->open_count == 0;
 }
 
 static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index fb84da3cb50d..4f4c3fec6916 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -64,12 +64,13 @@ static bool
 nouveau_switcheroo_can_switch(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
-	bool can_switch;
 
-	spin_lock(&dev->count_lock);
-	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
-	return can_switch;
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
+	return dev->open_count == 0;
 }
 
 static const struct vga_switcheroo_client_ops
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 511fe26198e4..9aa1afd1786e 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1125,12 +1125,13 @@ static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero
 static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
-	bool can_switch;
 
-	spin_lock(&dev->count_lock);
-	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
-	return can_switch;
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
+	return dev->open_count == 0;
 }
 
 static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9f1fb8d36b67..a20d882ca265 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1076,7 +1076,7 @@ struct drm_device {
 
 	/** \name Usage Counters */
 	/*@{ */
-	int open_count;			/**< Outstanding files open */
+	int open_count;			/**< Outstanding files open, protected by drm_global_mutex. */
 	int buf_use;			/**< Buffers in use -- cannot alloc */
 	atomic_t buf_alloc;		/**< Buffer allocation in progress */
 	/*@} */
-- 
1.9.2

Re: [PATCH] drm: Rip out totally bogus vga_switcheroo->can_switch locking

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1955 bytes --]

On Tue, Apr 22, 2014 at 10:45:19PM +0200, Daniel Vetter wrote:
> So I just wanted to add a new field to struct drm_device and
> accidentally stumbled over something. According to comments
> dev->open_count is protected by dev->count_lock, but that's totally
> not the case. It's protected by drm_global_mutex.
> 
> Unfortunately the vga switcheroo callbacks took this comment at face
> value. The problem is that we can't just take the drm_global_mutex
> because:
> - It would lead to a locking inversion with the driver load/unload
>   paths.
> - It wouldn't actually protect anything, for that we'd need to wrap
>   the entire vga switcheroo code in the drm_global_mutex. And I'm not
>   sure whether that would actually solve anything.
> 
> What we probably want is a try_to_grab_switcheroo reference kind of
> thing which is used in the driver's ->open callback. Then we could
> move all that ->can_switch madness into the vga switcheroo core where
> it really belongs.
> 
> But since that would amount to real work take the easy way out and
> just add a comment. It's definitely not going to make anything worse
> since doing switcheroo state changes while restarting X just isn't
> recommended. Even though the delayed switching code does exactly that.
> 
> v2:
> - Simplify the ->can_switch implementations more (Thierry)
> - Fix comment about the dev->open_count locking (Thierry)
> 
> Cc: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_dma.c        | 11 ++++++-----
>  drivers/gpu/drm/nouveau/nouveau_vga.c  | 11 ++++++-----
>  drivers/gpu/drm/radeon/radeon_device.c | 11 ++++++-----
>  include/drm/drmP.h                     |  2 +-
>  4 files changed, 19 insertions(+), 16 deletions(-)

Looks good:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/18] drm: rename dev->count_lock to dev->buf_lock

[Daniel Vetter]

Since really that's all it protects - legacy horror stories in
drm_bufs.c. Since I don't want to waste any more time on this I didn't
bother to actually look at what it protects in there, but it's at
least contained now.

v2: Move the spurious hunk to the right patch (Thierry).

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_bufs.c | 32 ++++++++++++++++----------------
 drivers/gpu/drm/drm_stub.c |  2 +-
 include/drm/drmP.h         |  2 +-
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index edec31fe3fed..ef7f0199a0f1 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -656,13 +656,13 @@ int drm_addbufs_agp(struct drm_device * dev, struct drm_buf_desc * request)
 		DRM_DEBUG("zone invalid\n");
 		return -EINVAL;
 	}
-	spin_lock(&dev->count_lock);
+	spin_lock(&dev->buf_lock);
 	if (dev->buf_use) {
-		spin_unlock(&dev->count_lock);
+		spin_unlock(&dev->buf_lock);
 		return -EBUSY;
 	}
 	atomic_inc(&dev->buf_alloc);
-	spin_unlock(&dev->count_lock);
+	spin_unlock(&dev->buf_lock);
 
 	mutex_lock(&dev->struct_mutex);
 	entry = &dma->bufs[order];
@@ -805,13 +805,13 @@ int drm_addbufs_pci(struct drm_device * dev, struct drm_buf_desc * request)
 	page_order = order - PAGE_SHIFT > 0 ? order - PAGE_SHIFT : 0;
 	total = PAGE_SIZE << page_order;
 
-	spin_lock(&dev->count_lock);
+	spin_lock(&dev->buf_lock);
 	if (dev->buf_use) {
-		spin_unlock(&dev->count_lock);
+		spin_unlock(&dev->buf_lock);
 		return -EBUSY;
 	}
 	atomic_inc(&dev->buf_alloc);
-	spin_unlock(&dev->count_lock);
+	spin_unlock(&dev->buf_lock);
 
 	mutex_lock(&dev->struct_mutex);
 	entry = &dma->bufs[order];
@@ -1015,13 +1015,13 @@ static int drm_addbufs_sg(struct drm_device * dev, struct drm_buf_desc * request
 	if (order < DRM_MIN_ORDER || order > DRM_MAX_ORDER)
 		return -EINVAL;
 
-	spin_lock(&dev->count_lock);
+	spin_lock(&dev->buf_lock);
 	if (dev->buf_use) {
-		spin_unlock(&dev->count_lock);
+		spin_unlock(&dev->buf_lock);
 		return -EBUSY;
 	}
 	atomic_inc(&dev->buf_alloc);
-	spin_unlock(&dev->count_lock);
+	spin_unlock(&dev->buf_lock);
 
 	mutex_lock(&dev->struct_mutex);
 	entry = &dma->bufs[order];
@@ -1175,7 +1175,7 @@ int drm_addbufs(struct drm_device *dev, void *data,
  * \param arg pointer to a drm_buf_info structure.
  * \return zero on success or a negative number on failure.
  *
- * Increments drm_device::buf_use while holding the drm_device::count_lock
+ * Increments drm_device::buf_use while holding the drm_device::buf_lock
  * lock, preventing of allocating more buffers after this call. Information
  * about each requested buffer is then copied into user space.
  */
@@ -1196,13 +1196,13 @@ int drm_infobufs(struct drm_device *dev, void *data,
 	if (!dma)
 		return -EINVAL;
 
-	spin_lock(&dev->count_lock);
+	spin_lock(&dev->buf_lock);
 	if (atomic_read(&dev->buf_alloc)) {
-		spin_unlock(&dev->count_lock);
+		spin_unlock(&dev->buf_lock);
 		return -EBUSY;
 	}
 	++dev->buf_use;		/* Can't allocate more after this call */
-	spin_unlock(&dev->count_lock);
+	spin_unlock(&dev->buf_lock);
 
 	for (i = 0, count = 0; i < DRM_MAX_ORDER + 1; i++) {
 		if (dma->bufs[i].buf_count)
@@ -1381,13 +1381,13 @@ int drm_mapbufs(struct drm_device *dev, void *data,
 	if (!dma)
 		return -EINVAL;
 
-	spin_lock(&dev->count_lock);
+	spin_lock(&dev->buf_lock);
 	if (atomic_read(&dev->buf_alloc)) {
-		spin_unlock(&dev->count_lock);
+		spin_unlock(&dev->buf_lock);
 		return -EBUSY;
 	}
 	dev->buf_use++;		/* Can't allocate more after this call */
-	spin_unlock(&dev->count_lock);
+	spin_unlock(&dev->buf_lock);
 
 	if (request->count >= dma->buf_count) {
 		if ((dev->agp && (dma->flags & _DRM_DMA_USE_AGP))
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 4c24c3ac1efa..5394b201c3d0 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -569,7 +569,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 	INIT_LIST_HEAD(&dev->maplist);
 	INIT_LIST_HEAD(&dev->vblank_event_list);
 
-	spin_lock_init(&dev->count_lock);
+	spin_lock_init(&dev->buf_lock);
 	spin_lock_init(&dev->event_lock);
 	mutex_init(&dev->struct_mutex);
 	mutex_init(&dev->ctxlist_mutex);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index bd16a4c8ece4..8b23a34a103e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1069,7 +1069,6 @@ struct drm_device {
 
 	/** \name Locks */
 	/*@{ */
-	spinlock_t count_lock;		/**< For inuse, drm_device::open_count, drm_device::buf_use */
 	struct mutex struct_mutex;	/**< For others */
 	struct mutex master_mutex;      /**< For drm_minor::master and drm_file::is_master */
 	/*@} */
@@ -1077,6 +1076,7 @@ struct drm_device {
 	/** \name Usage Counters */
 	/*@{ */
 	int open_count;			/**< Outstanding files open, protected by drm_global_lock. */
+	spinlock_t buf_lock;		/**< For drm_device::buf_use and a few other things. */
 	int buf_use;			/**< Buffers in use -- cannot alloc */
 	atomic_t buf_alloc;		/**< Buffer allocation in progress */
 	/*@} */
-- 
1.8.5.2

Re: [PATCH 09/18] drm: rename dev->count_lock to dev->buf_lock

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 747 bytes --]

On Fri, Apr 11, 2014 at 11:36:06PM +0200, Daniel Vetter wrote:
> Since really that's all it protects - legacy horror stories in
> drm_bufs.c. Since I don't want to waste any more time on this I didn't
> bother to actually look at what it protects in there, but it's at
> least contained now.
> 
> v2: Move the spurious hunk to the right patch (Thierry).
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_bufs.c | 32 ++++++++++++++++----------------
>  drivers/gpu/drm/drm_stub.c |  2 +-
>  include/drm/drmP.h         |  2 +-
>  3 files changed, 18 insertions(+), 18 deletions(-)

Looks good:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/18] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Daniel Vetter]

To get rid of the dev->bus->get_irq callback we need to pass in the
desired irq explicitly into drm_irq_install. To avoid having to do the
same for drm_irq_unistall just track it internally. That leaves
drivers with less room to botch things up.

v2: Add the hunk lost in an earlier patch to this one (Thierry).

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
 include/drm/drmP.h        |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 330e85b19115..1c3b6229363d 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
  */
 int drm_irq_install(struct drm_device *dev)
 {
-	int ret;
+	int ret, irq;
 	unsigned long sh_flags = 0;
 	char *irqname;
 
+	irq = drm_dev_to_irq(dev);
+
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
 		return -EINVAL;
 
-	if (drm_dev_to_irq(dev) == 0)
+	if (irq == 0)
 		return -EINVAL;
 
 	/* Driver must have been initialized */
@@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
 		return -EBUSY;
 	dev->irq_enabled = true;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", dev->irq);
 
 	/* Before installing handler */
 	if (dev->driver->irq_preinstall)
@@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
 	else
 		irqname = dev->driver->name;
 
-	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
+	ret = request_irq(dev->irq, dev->driver->irq_handler,
 			  sh_flags, irqname, dev);
 
 	if (ret < 0) {
@@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
 		dev->irq_enabled = false;
 		if (!drm_core_check_feature(dev, DRIVER_MODESET))
 			vga_client_register(dev->pdev, NULL, NULL, NULL);
-		free_irq(drm_dev_to_irq(dev), dev);
+		free_irq(dev->irq, dev);
+	} else {
+		dev->irq = irq;
 	}
 
 	return ret;
@@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (!irq_enabled)
 		return -EINVAL;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", dev->irq);
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		vga_client_register(dev->pdev, NULL, NULL, NULL);
@@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (dev->driver->irq_uninstall)
 		dev->driver->irq_uninstall(dev);
 
-	free_irq(drm_dev_to_irq(dev), dev);
+	free_irq(dev->irq, dev);
 
 	return 0;
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 8b23a34a103e..6f512cd97cd5 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1107,6 +1107,8 @@ struct drm_device {
 	/** \name Context support */
 	/*@{ */
 	bool irq_enabled;		/**< True if irq handler is enabled */
+	int irq;
+
 	__volatile__ long context_flag;	/**< Context swapping flag */
 	int last_context;		/**< Last current context */
 	/*@} */
-- 
1.8.5.2

Re: [PATCH 10/18] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Friday 11 April 2014 23:36:07 Daniel Vetter wrote:
> To get rid of the dev->bus->get_irq callback we need to pass in the
> desired irq explicitly into drm_irq_install. To avoid having to do the
> same for drm_irq_unistall just track it internally. That leaves
> drivers with less room to botch things up.
> 
> v2: Add the hunk lost in an earlier patch to this one (Thierry).
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
>  include/drm/drmP.h        |  2 ++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 330e85b19115..1c3b6229363d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device
> *dev) */
>  int drm_irq_install(struct drm_device *dev)
>  {
> -	int ret;
> +	int ret, irq;
>  	unsigned long sh_flags = 0;
>  	char *irqname;
> 
> +	irq = drm_dev_to_irq(dev);
> +
>  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
>  		return -EINVAL;
> 
> -	if (drm_dev_to_irq(dev) == 0)
> +	if (irq == 0)
>  		return -EINVAL;
> 
>  	/* Driver must have been initialized */
> @@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
>  		return -EBUSY;
>  	dev->irq_enabled = true;
> 
> -	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> +	DRM_DEBUG("irq=%d\n", dev->irq);

I might be missing something obvious, but you use dev->irq throughout the 
whole function while you only set it at the end.

>  	/* Before installing handler */
>  	if (dev->driver->irq_preinstall)
> @@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
>  	else
>  		irqname = dev->driver->name;
> 
> -	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
> +	ret = request_irq(dev->irq, dev->driver->irq_handler,
>  			  sh_flags, irqname, dev);
> 
>  	if (ret < 0) {
> @@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
>  		dev->irq_enabled = false;
>  		if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  			vga_client_register(dev->pdev, NULL, NULL, NULL);
> -		free_irq(drm_dev_to_irq(dev), dev);
> +		free_irq(dev->irq, dev);
> +	} else {
> +		dev->irq = irq;
>  	}
> 
>  	return ret;
> @@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
>  	if (!irq_enabled)
>  		return -EINVAL;
> 
> -	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> +	DRM_DEBUG("irq=%d\n", dev->irq);
> 
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		vga_client_register(dev->pdev, NULL, NULL, NULL);
> @@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
>  	if (dev->driver->irq_uninstall)
>  		dev->driver->irq_uninstall(dev);
> 
> -	free_irq(drm_dev_to_irq(dev), dev);
> +	free_irq(dev->irq, dev);
> 
>  	return 0;
>  }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 8b23a34a103e..6f512cd97cd5 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1107,6 +1107,8 @@ struct drm_device {
>  	/** \name Context support */
>  	/*@{ */
>  	bool irq_enabled;		/**< True if irq handler is enabled */
> +	int irq;
> +
>  	__volatile__ long context_flag;	/**< Context swapping flag */
>  	int last_context;		/**< Last current context */
>  	/*@} */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 10/18] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Laurent Pinchart]

And another comment...

On Friday 11 April 2014 23:36:07 Daniel Vetter wrote:
> To get rid of the dev->bus->get_irq callback we need to pass in the
> desired irq explicitly into drm_irq_install. To avoid having to do the
> same for drm_irq_unistall just track it internally. That leaves
> drivers with less room to botch things up.
> 
> v2: Add the hunk lost in an earlier patch to this one (Thierry).
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
>  include/drm/drmP.h        |  2 ++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 330e85b19115..1c3b6229363d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device
> *dev) */
>  int drm_irq_install(struct drm_device *dev)
>  {
> -	int ret;
> +	int ret, irq;
>  	unsigned long sh_flags = 0;
>  	char *irqname;
> 
> +	irq = drm_dev_to_irq(dev);
> +
>  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
>  		return -EINVAL;
> 
> -	if (drm_dev_to_irq(dev) == 0)
> +	if (irq == 0)

Isn't 0 a valid IRQ number ? Shouldn't you check for irq < 0 instead ? At 
least platform_get_irq() returns a negative error value on failure.

>  		return -EINVAL;
> 
>  	/* Driver must have been initialized */
> @@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
>  		return -EBUSY;
>  	dev->irq_enabled = true;
> 
> -	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> +	DRM_DEBUG("irq=%d\n", dev->irq);
> 
>  	/* Before installing handler */
>  	if (dev->driver->irq_preinstall)
> @@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
>  	else
>  		irqname = dev->driver->name;
> 
> -	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
> +	ret = request_irq(dev->irq, dev->driver->irq_handler,
>  			  sh_flags, irqname, dev);
> 
>  	if (ret < 0) {
> @@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
>  		dev->irq_enabled = false;
>  		if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  			vga_client_register(dev->pdev, NULL, NULL, NULL);
> -		free_irq(drm_dev_to_irq(dev), dev);
> +		free_irq(dev->irq, dev);
> +	} else {
> +		dev->irq = irq;
>  	}
> 
>  	return ret;
> @@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
>  	if (!irq_enabled)
>  		return -EINVAL;
> 
> -	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> +	DRM_DEBUG("irq=%d\n", dev->irq);
> 
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		vga_client_register(dev->pdev, NULL, NULL, NULL);
> @@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
>  	if (dev->driver->irq_uninstall)
>  		dev->driver->irq_uninstall(dev);
> 
> -	free_irq(drm_dev_to_irq(dev), dev);
> +	free_irq(dev->irq, dev);
> 
>  	return 0;
>  }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 8b23a34a103e..6f512cd97cd5 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1107,6 +1107,8 @@ struct drm_device {
>  	/** \name Context support */
>  	/*@{ */
>  	bool irq_enabled;		/**< True if irq handler is enabled */
> +	int irq;
> +
>  	__volatile__ long context_flag;	/**< Context swapping flag */
>  	int last_context;		/**< Last current context */
>  	/*@} */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 10/18] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Daniel Vetter]

On Thu, Apr 17, 2014 at 12:02:07AM +0200, Laurent Pinchart wrote:
> And another comment...
> 
> On Friday 11 April 2014 23:36:07 Daniel Vetter wrote:
> > To get rid of the dev->bus->get_irq callback we need to pass in the
> > desired irq explicitly into drm_irq_install. To avoid having to do the
> > same for drm_irq_unistall just track it internally. That leaves
> > drivers with less room to botch things up.
> > 
> > v2: Add the hunk lost in an earlier patch to this one (Thierry).
> > 
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
> >  include/drm/drmP.h        |  2 ++
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 330e85b19115..1c3b6229363d 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device
> > *dev) */
> >  int drm_irq_install(struct drm_device *dev)
> >  {
> > -	int ret;
> > +	int ret, irq;
> >  	unsigned long sh_flags = 0;
> >  	char *irqname;
> > 
> > +	irq = drm_dev_to_irq(dev);
> > +
> >  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> >  		return -EINVAL;
> > 
> > -	if (drm_dev_to_irq(dev) == 0)
> > +	if (irq == 0)
> 
> Isn't 0 a valid IRQ number ? Shouldn't you check for irq < 0 instead ? At 
> least platform_get_irq() returns a negative error value on failure.

tbh I'm not really clear on how this works. If we want to change this then
I think that should be a separate patch. Also it might be better to
extract this check into drm_control, which is the only function that
really needs it since it is called by userspace.

For all other places it is a simple driver bug and I'm not sure how much
we should care - request_irq should catch any abuse already.

In any case this should be a separate patch.
-Daniel

> 
> >  		return -EINVAL;
> > 
> >  	/* Driver must have been initialized */
> > @@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
> >  		return -EBUSY;
> >  	dev->irq_enabled = true;
> > 
> > -	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> > +	DRM_DEBUG("irq=%d\n", dev->irq);
> > 
> >  	/* Before installing handler */
> >  	if (dev->driver->irq_preinstall)
> > @@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
> >  	else
> >  		irqname = dev->driver->name;
> > 
> > -	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
> > +	ret = request_irq(dev->irq, dev->driver->irq_handler,
> >  			  sh_flags, irqname, dev);
> > 
> >  	if (ret < 0) {
> > @@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
> >  		dev->irq_enabled = false;
> >  		if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >  			vga_client_register(dev->pdev, NULL, NULL, NULL);
> > -		free_irq(drm_dev_to_irq(dev), dev);
> > +		free_irq(dev->irq, dev);
> > +	} else {
> > +		dev->irq = irq;
> >  	}
> > 
> >  	return ret;
> > @@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
> >  	if (!irq_enabled)
> >  		return -EINVAL;
> > 
> > -	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> > +	DRM_DEBUG("irq=%d\n", dev->irq);
> > 
> >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >  		vga_client_register(dev->pdev, NULL, NULL, NULL);
> > @@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
> >  	if (dev->driver->irq_uninstall)
> >  		dev->driver->irq_uninstall(dev);
> > 
> > -	free_irq(drm_dev_to_irq(dev), dev);
> > +	free_irq(dev->irq, dev);
> > 
> >  	return 0;
> >  }
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 8b23a34a103e..6f512cd97cd5 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1107,6 +1107,8 @@ struct drm_device {
> >  	/** \name Context support */
> >  	/*@{ */
> >  	bool irq_enabled;		/**< True if irq handler is enabled */
> > +	int irq;
> > +
> >  	__volatile__ long context_flag;	/**< Context swapping flag */
> >  	int last_context;		/**< Last current context */
> >  	/*@} */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/18] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2333 bytes --]

On Tue, Apr 22, 2014 at 11:46:45AM +0200, Daniel Vetter wrote:
> On Thu, Apr 17, 2014 at 12:02:07AM +0200, Laurent Pinchart wrote:
> > And another comment...
> > 
> > On Friday 11 April 2014 23:36:07 Daniel Vetter wrote:
> > > To get rid of the dev->bus->get_irq callback we need to pass in the
> > > desired irq explicitly into drm_irq_install. To avoid having to do the
> > > same for drm_irq_unistall just track it internally. That leaves
> > > drivers with less room to botch things up.
> > > 
> > > v2: Add the hunk lost in an earlier patch to this one (Thierry).
> > > 
> > > Cc: Thierry Reding <thierry.reding@gmail.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
> > >  include/drm/drmP.h        |  2 ++
> > >  2 files changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index 330e85b19115..1c3b6229363d 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device
> > > *dev) */
> > >  int drm_irq_install(struct drm_device *dev)
> > >  {
> > > -	int ret;
> > > +	int ret, irq;
> > >  	unsigned long sh_flags = 0;
> > >  	char *irqname;
> > > 
> > > +	irq = drm_dev_to_irq(dev);
> > > +
> > >  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> > >  		return -EINVAL;
> > > 
> > > -	if (drm_dev_to_irq(dev) == 0)
> > > +	if (irq == 0)
> > 
> > Isn't 0 a valid IRQ number ? Shouldn't you check for irq < 0 instead ? At 
> > least platform_get_irq() returns a negative error value on failure.
> 
> tbh I'm not really clear on how this works. If we want to change this then
> I think that should be a separate patch. Also it might be better to
> extract this check into drm_control, which is the only function that
> really needs it since it is called by userspace.
> 
> For all other places it is a simple driver bug and I'm not sure how much
> we should care - request_irq should catch any abuse already.
> 
> In any case this should be a separate patch.

I agree. This patch mostly mechanically replaces things and this bug did
already exist previously. So let's fix it separately (if at all).

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/18] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2721 bytes --]

On Fri, Apr 11, 2014 at 11:36:07PM +0200, Daniel Vetter wrote:
> To get rid of the dev->bus->get_irq callback we need to pass in the
> desired irq explicitly into drm_irq_install. To avoid having to do the
> same for drm_irq_unistall just track it internally. That leaves
> drivers with less room to botch things up.
> 
> v2: Add the hunk lost in an earlier patch to this one (Thierry).
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
>  include/drm/drmP.h        |  2 ++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 330e85b19115..1c3b6229363d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
>   */
>  int drm_irq_install(struct drm_device *dev)
>  {
> -	int ret;
> +	int ret, irq;
>  	unsigned long sh_flags = 0;
>  	char *irqname;
>  
> +	irq = drm_dev_to_irq(dev);
> +

This looks odd...

>  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
>  		return -EINVAL;
>  
> -	if (drm_dev_to_irq(dev) == 0)
> +	if (irq == 0)
>  		return -EINVAL;

Wouldn't it make more sense to assign irq only here? And while at it,
perhaps you meant to store irq directly within dev->irq, rather than in
a local variable? That would address Laurent's comment as well.

>  
>  	/* Driver must have been initialized */
> @@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
>  		return -EBUSY;
>  	dev->irq_enabled = true;
>  
> -	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> +	DRM_DEBUG("irq=%d\n", dev->irq);
>  
>  	/* Before installing handler */
>  	if (dev->driver->irq_preinstall)
> @@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
>  	else
>  		irqname = dev->driver->name;
>  
> -	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
> +	ret = request_irq(dev->irq, dev->driver->irq_handler,
>  			  sh_flags, irqname, dev);
>  
>  	if (ret < 0) {
> @@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
>  		dev->irq_enabled = false;
>  		if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  			vga_client_register(dev->pdev, NULL, NULL, NULL);
> -		free_irq(drm_dev_to_irq(dev), dev);
> +		free_irq(dev->irq, dev);
> +	} else {
> +		dev->irq = irq;
>  	}

But perhaps you really only want to assign it after it's been properly
initialized, so I guess the local variable is fine. But in that case you
probably need to make sure to not use dev->irq until after this
assignment.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Daniel Vetter]

To get rid of the dev->bus->get_irq callback we need to pass in the
desired irq explicitly into drm_irq_install. To avoid having to do the
same for drm_irq_unistall just track it internally. That leaves
drivers with less room to botch things up.

v2: Add the hunk lost in an earlier patch to this one (Thierry).

v3: Fix up the totally fumbled logic in drm_irq_install and use the
local variable consistently. Spotted by both Thierry and Laurent.
Shame on me for failing to properly test the rebase version of this
patch ...

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
 include/drm/drmP.h        |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 589e865832cd..7cf407bbfed5 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
  */
 int drm_irq_install(struct drm_device *dev)
 {
-	int ret;
+	int ret, irq;
 	unsigned long sh_flags = 0;
 	char *irqname;
 
+	irq = drm_dev_to_irq(dev);
+
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
 		return -EINVAL;
 
-	if (drm_dev_to_irq(dev) == 0)
+	if (irq == 0)
 		return -EINVAL;
 
 	/* Driver must have been initialized */
@@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
 		return -EBUSY;
 	dev->irq_enabled = true;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", irq);
 
 	/* Before installing handler */
 	if (dev->driver->irq_preinstall)
@@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
 	else
 		irqname = dev->driver->name;
 
-	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
+	ret = request_irq(irq, dev->driver->irq_handler,
 			  sh_flags, irqname, dev);
 
 	if (ret < 0) {
@@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
 		dev->irq_enabled = false;
 		if (!drm_core_check_feature(dev, DRIVER_MODESET))
 			vga_client_register(dev->pdev, NULL, NULL, NULL);
-		free_irq(drm_dev_to_irq(dev), dev);
+		free_irq(irq, dev);
+	} else {
+		dev->irq = irq;
 	}
 
 	return ret;
@@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (!irq_enabled)
 		return -EINVAL;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", dev->irq);
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		vga_client_register(dev->pdev, NULL, NULL, NULL);
@@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (dev->driver->irq_uninstall)
 		dev->driver->irq_uninstall(dev);
 
-	free_irq(drm_dev_to_irq(dev), dev);
+	free_irq(dev->irq, dev);
 
 	return 0;
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 85682d959c7e..8e8c392d6fa8 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1107,6 +1107,8 @@ struct drm_device {
 	/** \name Context support */
 	/*@{ */
 	bool irq_enabled;		/**< True if irq handler is enabled */
+	int irq;
+
 	__volatile__ long context_flag;	/**< Context swapping flag */
 	int last_context;		/**< Last current context */
 	/*@} */
-- 
1.9.2

Re: [PATCH] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1034 bytes --]

On Tue, Apr 22, 2014 at 10:44:20PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 589e865832cd..7cf407bbfed5 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
>   */
>  int drm_irq_install(struct drm_device *dev)
>  {
> -	int ret;
> +	int ret, irq;
>  	unsigned long sh_flags = 0;
>  	char *irqname;
>  
> +	irq = drm_dev_to_irq(dev);

I think the assignment could have happened either when the variable is
declared, or...

> +
>  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
>  		return -EINVAL;
>  
> -	if (drm_dev_to_irq(dev) == 0)
> +	if (irq == 0)

... right above this, since it is where it is first used (it may not be
necessary to query it before here at all if the driver doesn't set
DRIVER_HAVE_IRQ).

But I realize that that's pure bike-shedding, so either way:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Daniel Vetter]

On Wed, Apr 23, 2014 at 09:27:58AM +0200, Thierry Reding wrote:
> On Tue, Apr 22, 2014 at 10:44:20PM +0200, Daniel Vetter wrote:
> [...]
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 589e865832cd..7cf407bbfed5 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
> >   */
> >  int drm_irq_install(struct drm_device *dev)
> >  {
> > -	int ret;
> > +	int ret, irq;
> >  	unsigned long sh_flags = 0;
> >  	char *irqname;
> >  
> > +	irq = drm_dev_to_irq(dev);
> 
> I think the assignment could have happened either when the variable is
> declared, or...
> 
> > +
> >  	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> >  		return -EINVAL;
> >  
> > -	if (drm_dev_to_irq(dev) == 0)
> > +	if (irq == 0)
> 
> ... right above this, since it is where it is first used (it may not be
> necessary to query it before here at all if the driver doesn't set
> DRIVER_HAVE_IRQ).
> 
> But I realize that that's pure bike-shedding, so either way:

Follow-on patches will move this assignement into drivers and make int irq
an function parameter, so I think I'll leave this ;-)
> 
> Reviewed-by: Thierry Reding <treding@nvidia.com>

Thanks for the review, I'll send the pull request to Dave now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 11/18] drm/irq: Look up the pci irq directly in the drm_control ioctl

[Daniel Vetter]

It's only ever called for legacy drivers, which are all pci.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 1c3b6229363d..a83b037f5615 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -377,7 +377,7 @@ int drm_control(struct drm_device *dev, void *data,
 		struct drm_file *file_priv)
 {
 	struct drm_control *ctl = data;
-	int ret = 0;
+	int ret = 0, irq;
 
 	/* if we haven't irq we fallback for compatibility reasons -
 	 * this used to be a separate function in drm_dma.h
@@ -393,8 +393,10 @@ int drm_control(struct drm_device *dev, void *data,
 
 	switch (ctl->func) {
 	case DRM_INST_HANDLER:
+		irq = dev->pdev->irq;
+
 		if (dev->if_version < DRM_IF_VERSION(1, 2) &&
-		    ctl->irq != drm_dev_to_irq(dev))
+		    ctl->irq != irq)
 			return -EINVAL;
 
 		mutex_lock(&dev->struct_mutex);
-- 
1.8.5.2

[PATCH 12/18] drm: pass the irq explicitly to drm_irq_install

[Daniel Vetter]

Unfortunately this requires a drm-wide change, and I didn't see a sane
way around that. Luckily it's fairly simple, we just need to inline
the respective get_irq implementation from either drm_pci.c or
drm_platform.c.

With that we can now also remove drm_dev_to_irq from drm_irq.c.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl           | 10 +---------
 drivers/gpu/drm/armada/armada_drv.c      |  2 +-
 drivers/gpu/drm/drm_irq.c                | 13 +++----------
 drivers/gpu/drm/gma500/psb_drv.c         |  2 +-
 drivers/gpu/drm/i915/i915_dma.c          |  2 +-
 drivers/gpu/drm/i915/i915_drv.c          |  4 ++--
 drivers/gpu/drm/i915/i915_gem.c          |  2 +-
 drivers/gpu/drm/msm/msm_drv.c            |  2 +-
 drivers/gpu/drm/qxl/qxl_irq.c            |  2 +-
 drivers/gpu/drm/radeon/radeon_irq_kms.c  |  2 +-
 drivers/gpu/drm/shmobile/shmob_drm_drv.c |  2 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c      |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c      |  2 +-
 include/drm/drmP.h                       |  2 +-
 14 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 702c4474919c..039ac10d7584 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -342,21 +342,13 @@ char *date;</synopsis>
         <sect4>
           <title>Managed IRQ Registration</title>
           <para>
-            Both the <function>drm_irq_install</function> and
-	    <function>drm_irq_uninstall</function> functions get the device IRQ by
-	    calling <function>drm_dev_to_irq</function>. This inline function will
-	    call a bus-specific operation to retrieve the IRQ number. For platform
-	    devices, <function>platform_get_irq</function>(..., 0) is used to
-	    retrieve the IRQ number.
-          </para>
-          <para>
             <function>drm_irq_install</function> starts by calling the
             <methodname>irq_preinstall</methodname> driver operation. The operation
             is optional and must make sure that the interrupt will not get fired by
             clearing all pending interrupt flags or disabling the interrupt.
           </para>
           <para>
-            The IRQ will then be requested by a call to
+            The passed-in IRQ will then be requested by a call to
             <function>request_irq</function>. If the DRIVER_IRQ_SHARED driver
             feature flag is set, a shared (IRQF_SHARED) IRQ handler will be
             requested.
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 32982da82694..567cfbde0883 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -173,7 +173,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 	if (ret)
 		goto err_kms;
 
-	ret = drm_irq_install(dev);
+	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
 	if (ret)
 		goto err_kms;
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index a83b037f5615..1259e3db6d62 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -233,11 +233,6 @@ static void drm_irq_vgaarb_nokms(void *cookie, bool state)
 	}
 }
 
-static inline int drm_dev_to_irq(struct drm_device *dev)
-{
-	return dev->driver->bus->get_irq(dev);
-}
-
 /**
  * Install IRQ handler.
  *
@@ -247,14 +242,12 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
  * \c irq_preinstall() and \c irq_postinstall() functions
  * before and after the installation.
  */
-int drm_irq_install(struct drm_device *dev)
+int drm_irq_install(struct drm_device *dev, int irq)
 {
-	int ret, irq;
+	int ret;
 	unsigned long sh_flags = 0;
 	char *irqname;
 
-	irq = drm_dev_to_irq(dev);
-
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
 		return -EINVAL;
 
@@ -400,7 +393,7 @@ int drm_control(struct drm_device *dev, void *data,
 			return -EINVAL;
 
 		mutex_lock(&dev->struct_mutex);
-		ret = drm_irq_install(dev);
+		ret = drm_irq_install(dev, irq);
 		mutex_unlock(&dev->struct_mutex);
 
 		return ret;
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index b686e56646eb..0a3101a3db19 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -354,7 +354,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 	PSB_WVDC32(0xFFFFFFFF, PSB_INT_MASK_R);
 	spin_unlock_irqrestore(&dev_priv->irqmask_lock, irqflags);
 
-	drm_irq_install(dev);
+	drm_irq_install(dev, dev->pdev->irq);
 
 	dev->vblank_disable_allowed = true;
 	dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index dcb67c6564dd..53765aa8fc4a 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1336,7 +1336,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_power_domains_init_hw(dev_priv);
 
-	ret = drm_irq_install(dev);
+	ret = drm_irq_install(dev, dev->pdev->irq);
 	if (ret)
 		goto cleanup_gem_stolen;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 87ce105910f2..6124b491a19e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -574,7 +574,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		mutex_unlock(&dev->struct_mutex);
 
 		/* We need working interrupts for modeset enabling ... */
-		drm_irq_install(dev);
+		drm_irq_install(dev, dev->pdev->irq);
 
 		intel_modeset_init_hw(dev);
 
@@ -752,7 +752,7 @@ int i915_reset(struct drm_device *dev)
 		 * being false when they shouldn't be able to.
 		 */
 		drm_irq_uninstall(dev);
-		drm_irq_install(dev);
+		drm_irq_install(dev, dev->pdev->irq);
 
 		/* rps/rc6 re-init is necessary to restore state lost after the
 		 * reset and the re-install of drm irq. Skip for ironlake per
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 48abe3b4976b..74b872bf7fcc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4523,7 +4523,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 
 	BUG_ON(!list_empty(&dev_priv->gtt.base.active_list));
 
-	ret = drm_irq_install(dev);
+	ret = drm_irq_install(dev, dev->pdev->irq);
 	if (ret)
 		goto cleanup_ringbuffer;
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f9de156b9e65..50ec1bed5820 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -288,7 +288,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	pm_runtime_get_sync(dev->dev);
-	ret = drm_irq_install(dev);
+	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
 	pm_runtime_put_sync(dev->dev);
 	if (ret < 0) {
 		dev_err(dev->dev, "failed to install IRQ handler\n");
diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
index 28f84b4fce32..34d6a85e9023 100644
--- a/drivers/gpu/drm/qxl/qxl_irq.c
+++ b/drivers/gpu/drm/qxl/qxl_irq.c
@@ -87,7 +87,7 @@ int qxl_irq_init(struct qxl_device *qdev)
 	atomic_set(&qdev->irq_received_cursor, 0);
 	atomic_set(&qdev->irq_received_io_cmd, 0);
 	qdev->irq_received_error = 0;
-	ret = drm_irq_install(qdev->ddev);
+	ret = drm_irq_install(qdev->ddev, qdev->ddev->pdev->irq);
 	qdev->ram_header->int_mask = QXL_INTERRUPT_MASK;
 	if (unlikely(ret != 0)) {
 		DRM_ERROR("Failed installing irq: %d\n", ret);
diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
index 089c9ffb0aa9..16807afab362 100644
--- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
@@ -287,7 +287,7 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
 	INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
 
 	rdev->irq.installed = true;
-	r = drm_irq_install(rdev->ddev);
+	r = drm_irq_install(rdev->ddev, rdev->ddev->pdev->irq);
 	if (r) {
 		rdev->irq.installed = false;
 		flush_work(&rdev->hotplug_work);
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
index c839c9c89efb..82c84c7fd4f6 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
@@ -185,7 +185,7 @@ static int shmob_drm_load(struct drm_device *dev, unsigned long flags)
 		goto done;
 	}
 
-	ret = drm_irq_install(dev);
+	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to install IRQ handler\n");
 		goto done;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 171a8203892c..b20b69488dc9 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -268,7 +268,7 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	pm_runtime_get_sync(dev->dev);
-	ret = drm_irq_install(dev);
+	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
 	pm_runtime_put_sync(dev->dev);
 	if (ret < 0) {
 		dev_err(dev->dev, "failed to install IRQ handler\n");
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 4a223bbea3b3..6bdd15eea7e8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -806,7 +806,7 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 	}
 
 	if (dev_priv->capabilities & SVGA_CAP_IRQMASK) {
-		ret = drm_irq_install(dev);
+		ret = drm_irq_install(dev, dev->pdev->irq);
 		if (ret != 0) {
 			DRM_ERROR("Failed installing irq: %d\n", ret);
 			goto out_no_irq;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 6f512cd97cd5..da28f49d7950 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1353,7 +1353,7 @@ extern void drm_core_reclaim_buffers(struct drm_device *dev,
 				/* IRQ support (drm_irq.h) */
 extern int drm_control(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv);
-extern int drm_irq_install(struct drm_device *dev);
+extern int drm_irq_install(struct drm_device *dev, int irq);
 extern int drm_irq_uninstall(struct drm_device *dev);
 
 extern int drm_vblank_init(struct drm_device *dev, int num_crtcs);
-- 
1.8.5.2

Re: [PATCH 12/18] drm: pass the irq explicitly to drm_irq_install

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1325 bytes --]

On Fri, Apr 11, 2014 at 11:36:09PM +0200, Daniel Vetter wrote:
> Unfortunately this requires a drm-wide change, and I didn't see a sane
> way around that. Luckily it's fairly simple, we just need to inline
> the respective get_irq implementation from either drm_pci.c or
> drm_platform.c.
> 
> With that we can now also remove drm_dev_to_irq from drm_irq.c.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl           | 10 +---------
>  drivers/gpu/drm/armada/armada_drv.c      |  2 +-
>  drivers/gpu/drm/drm_irq.c                | 13 +++----------
>  drivers/gpu/drm/gma500/psb_drv.c         |  2 +-
>  drivers/gpu/drm/i915/i915_dma.c          |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c          |  4 ++--
>  drivers/gpu/drm/i915/i915_gem.c          |  2 +-
>  drivers/gpu/drm/msm/msm_drv.c            |  2 +-
>  drivers/gpu/drm/qxl/qxl_irq.c            |  2 +-
>  drivers/gpu/drm/radeon/radeon_irq_kms.c  |  2 +-
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c |  2 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c      |  2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c      |  2 +-
>  include/drm/drmP.h                       |  2 +-
>  14 files changed, 17 insertions(+), 32 deletions(-)

Looks good to me:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/18] drm: remove bus->get_irq implementations

[Daniel Vetter]

Now that they're all unused we can get rid of them, including the
dummy version in drm_usb.c.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_pci.c      | 6 ------
 drivers/gpu/drm/drm_platform.c | 6 ------
 drivers/gpu/drm/drm_usb.c      | 6 ------
 include/drm/drmP.h             | 1 -
 4 files changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index ad2fc48d0f0b..7d388d188c4c 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -137,11 +137,6 @@ static int drm_get_pci_domain(struct drm_device *dev)
 	return pci_domain_nr(dev->pdev->bus);
 }
 
-static int drm_pci_get_irq(struct drm_device *dev)
-{
-	return dev->pdev->irq;
-}
-
 static const char *drm_pci_get_name(struct drm_device *dev)
 {
 	struct pci_driver *pdriver = dev->driver->kdriver.pci;
@@ -318,7 +313,6 @@ void drm_pci_agp_destroy(struct drm_device *dev)
 }
 
 static struct drm_bus drm_pci_bus = {
-	.get_irq = drm_pci_get_irq,
 	.get_name = drm_pci_get_name,
 	.set_busid = drm_pci_set_busid,
 	.set_unique = drm_pci_set_unique,
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 5cd8c695824f..5b918212b1c3 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -68,11 +68,6 @@ err_free:
 	return ret;
 }
 
-static int drm_platform_get_irq(struct drm_device *dev)
-{
-	return platform_get_irq(dev->platformdev, 0);
-}
-
 static const char *drm_platform_get_name(struct drm_device *dev)
 {
 	return dev->platformdev->name;
@@ -123,7 +118,6 @@ err:
 }
 
 static struct drm_bus drm_platform_bus = {
-	.get_irq = drm_platform_get_irq,
 	.get_name = drm_platform_get_name,
 	.set_busid = drm_platform_set_busid,
 };
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index ed60f887c805..abdc265c9cc8 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -36,11 +36,6 @@ err_free:
 }
 EXPORT_SYMBOL(drm_get_usb_dev);
 
-static int drm_usb_get_irq(struct drm_device *dev)
-{
-	return 0;
-}
-
 static const char *drm_usb_get_name(struct drm_device *dev)
 {
 	return "USB";
@@ -53,7 +48,6 @@ static int drm_usb_set_busid(struct drm_device *dev,
 }
 
 static struct drm_bus drm_usb_bus = {
-	.get_irq = drm_usb_get_irq,
 	.get_name = drm_usb_get_name,
 	.set_busid = drm_usb_set_busid,
 };
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index da28f49d7950..f62891998388 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -726,7 +726,6 @@ struct drm_master {
 #define DRM_SCANOUTPOS_ACCURATE     (1 << 2)
 
 struct drm_bus {
-	int (*get_irq)(struct drm_device *dev);
 	const char *(*get_name)(struct drm_device *dev);
 	int (*set_busid)(struct drm_device *dev, struct drm_master *master);
 	int (*set_unique)(struct drm_device *dev, struct drm_master *master,
-- 
1.8.5.2

Re: [PATCH 13/18] drm: remove bus->get_irq implementations

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 509 bytes --]

On Fri, Apr 11, 2014 at 11:36:10PM +0200, Daniel Vetter wrote:
> Now that they're all unused we can get rid of them, including the
> dummy version in drm_usb.c.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_pci.c      | 6 ------
>  drivers/gpu/drm/drm_platform.c | 6 ------
>  drivers/gpu/drm/drm_usb.c      | 6 ------
>  include/drm/drmP.h             | 1 -
>  4 files changed, 19 deletions(-)

Nice!

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/18] drm: inline drm_pci_set_unique

[Daniel Vetter]

This is only used for drm versions 1.0, and kms drivers have never
been there. So we can appropriately restrict this to legacy and hence
pci devices and inline everything.

v2: Make the dummy function actually return something, caught by Wu
Fengguang's 0-day tester.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_ioctl.c | 10 +++++++---
 drivers/gpu/drm/drm_pci.c   | 14 ++++++++++----
 include/drm/drmP.h          |  5 +++--
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 93a42040bedb..5aea5b57ba4b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -93,7 +93,8 @@ drm_unset_busid(struct drm_device *dev,
  * Copies the bus id from userspace into drm_device::unique, and verifies that
  * it matches the device this DRM is attached to (EINVAL otherwise).  Deprecated
  * in interface version 1.1 and will return EBUSY when setversion has requested
- * version 1.1 or greater.
+ * version 1.1 or greater. Also note that KMS is all version 1.1 and later and
+ * UMS was only ever support on pci devices.
  */
 int drm_setunique(struct drm_device *dev, void *data,
 		  struct drm_file *file_priv)
@@ -108,10 +109,13 @@ int drm_setunique(struct drm_device *dev, void *data,
 	if (!u->unique_len || u->unique_len > 1024)
 		return -EINVAL;
 
-	if (!dev->driver->bus->set_unique)
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		return 0;
+
+	if (WARN_ON(!dev->pdev))
 		return -EINVAL;
 
-	ret = dev->driver->bus->set_unique(dev, master, u);
+	ret = drm_pci_set_unique(dev, master, u);
 	if (ret)
 		goto err;
 
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 7d388d188c4c..0fa42072262b 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -185,9 +185,9 @@ err:
 	return ret;
 }
 
-static int drm_pci_set_unique(struct drm_device *dev,
-			      struct drm_master *master,
-			      struct drm_unique *u)
+int drm_pci_set_unique(struct drm_device *dev,
+		       struct drm_master *master,
+		       struct drm_unique *u)
 {
 	int domain, bus, slot, func, ret;
 	const char *bus_name;
@@ -315,7 +315,6 @@ void drm_pci_agp_destroy(struct drm_device *dev)
 static struct drm_bus drm_pci_bus = {
 	.get_name = drm_pci_get_name,
 	.set_busid = drm_pci_set_busid,
-	.set_unique = drm_pci_set_unique,
 };
 
 /**
@@ -482,6 +481,13 @@ int drm_irq_by_busid(struct drm_device *dev, void *data,
 {
 	return -EINVAL;
 }
+
+int drm_pci_set_unique(struct drm_device *dev,
+		       struct drm_master *master,
+		       struct drm_unique *u)
+{
+	return -EINVAL;
+}
 #endif
 
 EXPORT_SYMBOL(drm_pci_init);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f62891998388..5333659d4d0e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -728,8 +728,6 @@ struct drm_master {
 struct drm_bus {
 	const char *(*get_name)(struct drm_device *dev);
 	int (*set_busid)(struct drm_device *dev, struct drm_master *master);
-	int (*set_unique)(struct drm_device *dev, struct drm_master *master,
-			  struct drm_unique *unique);
 };
 
 /**
@@ -1511,6 +1509,9 @@ extern drm_dma_handle_t *drm_pci_alloc(struct drm_device *dev, size_t size,
 				       size_t align);
 extern void __drm_pci_free(struct drm_device *dev, drm_dma_handle_t * dmah);
 extern void drm_pci_free(struct drm_device *dev, drm_dma_handle_t * dmah);
+extern int drm_pci_set_unique(struct drm_device *dev,
+			      struct drm_master *master,
+			      struct drm_unique *u);
 
 			       /* sysfs support (drm_sysfs.c) */
 struct drm_sysfs_class;
-- 
1.8.5.2

Re: [PATCH 14/18] drm: inline drm_pci_set_unique

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 820 bytes --]

On Fri, Apr 11, 2014 at 11:36:11PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 93a42040bedb..5aea5b57ba4b 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -93,7 +93,8 @@ drm_unset_busid(struct drm_device *dev,
>   * Copies the bus id from userspace into drm_device::unique, and verifies that
>   * it matches the device this DRM is attached to (EINVAL otherwise).  Deprecated
>   * in interface version 1.1 and will return EBUSY when setversion has requested
> - * version 1.1 or greater.
> + * version 1.1 or greater. Also note that KMS is all version 1.1 and later and
> + * UMS was only ever support on pci devices.

Nit: s/support on/supported on/, otherwise:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 15/18] drm: rip out dev->devname

[Daniel Vetter]

This was only ever used to pretty-print the irq driver name. And on
kms systems due to set_version bonghits we never set up the prettier
name, ever. Which make this a bit pointless.

Also, we can always dig out the driver-instance/irq relationship
through other means, so this isn't that useful. So just rip it out to
simplify the set_version/set_busid insanity a bit.

Also delete the temporary busname from drm_pci_set_busid, it's now
unused.

v2: Rebase on top of the new host1x drm_bus for tegra.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_ioctl.c    |  3 ---
 drivers/gpu/drm/drm_irq.c      |  8 +-------
 drivers/gpu/drm/drm_pci.c      | 25 -------------------------
 drivers/gpu/drm/drm_platform.c | 11 -----------
 drivers/gpu/drm/drm_stub.c     |  5 -----
 drivers/gpu/drm/tegra/bus.c    | 10 ----------
 include/drm/drmP.h             |  1 -
 7 files changed, 1 insertion(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 5aea5b57ba4b..2dd3a6d8382b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -72,9 +72,6 @@ static void
 drm_unset_busid(struct drm_device *dev,
 		struct drm_master *master)
 {
-	kfree(dev->devname);
-	dev->devname = NULL;
-
 	kfree(master->unique);
 	master->unique = NULL;
 	master->unique_len = 0;
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 1259e3db6d62..dd695f9c2b89 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -246,7 +246,6 @@ int drm_irq_install(struct drm_device *dev, int irq)
 {
 	int ret;
 	unsigned long sh_flags = 0;
-	char *irqname;
 
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
 		return -EINVAL;
@@ -272,13 +271,8 @@ int drm_irq_install(struct drm_device *dev, int irq)
 	if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
 		sh_flags = IRQF_SHARED;
 
-	if (dev->devname)
-		irqname = dev->devname;
-	else
-		irqname = dev->driver->name;
-
 	ret = request_irq(dev->irq, dev->driver->irq_handler,
-			  sh_flags, irqname, dev);
+			  sh_flags, dev->driver->name, dev);
 
 	if (ret < 0) {
 		dev->irq_enabled = false;
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 0fa42072262b..da86ef8d531a 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -146,7 +146,6 @@ static const char *drm_pci_get_name(struct drm_device *dev)
 static int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master)
 {
 	int len, ret;
-	struct pci_driver *pdriver = dev->driver->kdriver.pci;
 	master->unique_len = 40;
 	master->unique_size = master->unique_len;
 	master->unique = kmalloc(master->unique_size, GFP_KERNEL);
@@ -168,18 +167,6 @@ static int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master)
 	} else
 		master->unique_len = len;
 
-	dev->devname =
-		kmalloc(strlen(pdriver->name) +
-			master->unique_len + 2, GFP_KERNEL);
-
-	if (dev->devname == NULL) {
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	sprintf(dev->devname, "%s@%s", pdriver->name,
-		master->unique);
-
 	return 0;
 err:
 	return ret;
@@ -190,7 +177,6 @@ int drm_pci_set_unique(struct drm_device *dev,
 		       struct drm_unique *u)
 {
 	int domain, bus, slot, func, ret;
-	const char *bus_name;
 
 	master->unique_len = u->unique_len;
 	master->unique_size = u->unique_len + 1;
@@ -207,17 +193,6 @@ int drm_pci_set_unique(struct drm_device *dev,
 
 	master->unique[master->unique_len] = '\0';
 
-	bus_name = dev->driver->bus->get_name(dev);
-	dev->devname = kmalloc(strlen(bus_name) +
-			       strlen(master->unique) + 2, GFP_KERNEL);
-	if (!dev->devname) {
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	sprintf(dev->devname, "%s@%s", bus_name,
-		master->unique);
-
 	/* Return error if the busid submitted doesn't match the device's actual
 	 * busid.
 	 */
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index 5b918212b1c3..b97b11ea2dc4 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -101,17 +101,6 @@ static int drm_platform_set_busid(struct drm_device *dev, struct drm_master *mas
 		goto err;
 	}
 
-	dev->devname =
-		kmalloc(strlen(dev->platformdev->name) +
-			master->unique_len + 2, GFP_KERNEL);
-
-	if (dev->devname == NULL) {
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	sprintf(dev->devname, "%s@%s", dev->platformdev->name,
-		master->unique);
 	return 0;
 err:
 	return ret;
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 5394b201c3d0..3a8e832ad151 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -166,9 +166,6 @@ static void drm_master_destroy(struct kref *kref)
 		master->unique_len = 0;
 	}
 
-	kfree(dev->devname);
-	dev->devname = NULL;
-
 	list_for_each_entry_safe(pt, next, &master->magicfree, head) {
 		list_del(&pt->head);
 		drm_ht_remove_item(&master->magiclist, &pt->hash_item);
@@ -648,8 +645,6 @@ static void drm_dev_release(struct kref *ref)
 	drm_minor_free(dev, DRM_MINOR_RENDER);
 	drm_minor_free(dev, DRM_MINOR_CONTROL);
 
-	kfree(dev->devname);
-
 	mutex_destroy(&dev->master_mutex);
 	kfree(dev);
 }
diff --git a/drivers/gpu/drm/tegra/bus.c b/drivers/gpu/drm/tegra/bus.c
index 6916fa51cfa4..b3a66d65cb53 100644
--- a/drivers/gpu/drm/tegra/bus.c
+++ b/drivers/gpu/drm/tegra/bus.c
@@ -12,9 +12,7 @@ static int drm_host1x_set_busid(struct drm_device *dev,
 				struct drm_master *master)
 {
 	const char *device = dev_name(dev->dev);
-	const char *driver = dev->driver->name;
 	const char *bus = dev->dev->bus->name;
-	int length;
 
 	master->unique_len = strlen(bus) + 1 + strlen(device);
 	master->unique_size = master->unique_len;
@@ -25,14 +23,6 @@ static int drm_host1x_set_busid(struct drm_device *dev,
 
 	snprintf(master->unique, master->unique_len + 1, "%s:%s", bus, device);
 
-	length = strlen(driver) + 1 + master->unique_len;
-
-	dev->devname = kmalloc(length + 1, GFP_KERNEL);
-	if (!dev->devname)
-		return -ENOMEM;
-
-	snprintf(dev->devname, length + 1, "%s@%s", driver, master->unique);
-
 	return 0;
 }
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 5333659d4d0e..9d8c690732cf 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1048,7 +1048,6 @@ struct drm_vblank_crtc {
  */
 struct drm_device {
 	struct list_head legacy_dev_list;/**< list of devices per driver for stealth attach cleanup */
-	char *devname;			/**< For /proc/interrupts */
 	int if_version;			/**< Highest interface version set */
 
 	/** \name Lifetime Management */
-- 
1.8.5.2

Re: [PATCH 15/18] drm: rip out dev->devname

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1138 bytes --]

On Fri, Apr 11, 2014 at 11:36:12PM +0200, Daniel Vetter wrote:
> This was only ever used to pretty-print the irq driver name. And on
> kms systems due to set_version bonghits we never set up the prettier
> name, ever. Which make this a bit pointless.
> 
> Also, we can always dig out the driver-instance/irq relationship
> through other means, so this isn't that useful. So just rip it out to
> simplify the set_version/set_busid insanity a bit.
> 
> Also delete the temporary busname from drm_pci_set_busid, it's now
> unused.
> 
> v2: Rebase on top of the new host1x drm_bus for tegra.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_ioctl.c    |  3 ---
>  drivers/gpu/drm/drm_irq.c      |  8 +-------
>  drivers/gpu/drm/drm_pci.c      | 25 -------------------------
>  drivers/gpu/drm/drm_platform.c | 11 -----------
>  drivers/gpu/drm/drm_stub.c     |  5 -----
>  drivers/gpu/drm/tegra/bus.c    | 10 ----------
>  include/drm/drmP.h             |  1 -
>  7 files changed, 1 insertion(+), 62 deletions(-)

Excellent!

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 16/18] drm: remove drm_bus->get_name

[Daniel Vetter]

The only user is the info debugfs file, so we only need something
human readable. Now for both pci and platform devices we've used the
name of the underlying device driver, which matches the name of the
drm driver in all cases. So we can just use that instead.

The exception is usb, which used a generic "USB". Not to harmful with
just one usb driver, but better to use "udl", too.

With that converted we can rip out all the ->get_name implementations.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_info.c     | 6 ++----
 drivers/gpu/drm/drm_pci.c      | 7 -------
 drivers/gpu/drm/drm_platform.c | 6 ------
 drivers/gpu/drm/drm_usb.c      | 6 ------
 include/drm/drmP.h             | 1 -
 5 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 7473035dd28b..86feedd5e6f6 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -47,18 +47,16 @@ int drm_name_info(struct seq_file *m, void *data)
 	struct drm_minor *minor = node->minor;
 	struct drm_device *dev = minor->dev;
 	struct drm_master *master = minor->master;
-	const char *bus_name;
 	if (!master)
 		return 0;
 
-	bus_name = dev->driver->bus->get_name(dev);
 	if (master->unique) {
 		seq_printf(m, "%s %s %s\n",
-			   bus_name,
+			   dev->driver->name,
 			   dev_name(dev->dev), master->unique);
 	} else {
 		seq_printf(m, "%s %s\n",
-			   bus_name, dev_name(dev->dev));
+			   dev->driver->name, dev_name(dev->dev));
 	}
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index da86ef8d531a..262f1ca07a73 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -137,12 +137,6 @@ static int drm_get_pci_domain(struct drm_device *dev)
 	return pci_domain_nr(dev->pdev->bus);
 }
 
-static const char *drm_pci_get_name(struct drm_device *dev)
-{
-	struct pci_driver *pdriver = dev->driver->kdriver.pci;
-	return pdriver->name;
-}
-
 static int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master)
 {
 	int len, ret;
@@ -288,7 +282,6 @@ void drm_pci_agp_destroy(struct drm_device *dev)
 }
 
 static struct drm_bus drm_pci_bus = {
-	.get_name = drm_pci_get_name,
 	.set_busid = drm_pci_set_busid,
 };
 
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index b97b11ea2dc4..c7ec27bbe7c6 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -68,11 +68,6 @@ err_free:
 	return ret;
 }
 
-static const char *drm_platform_get_name(struct drm_device *dev)
-{
-	return dev->platformdev->name;
-}
-
 static int drm_platform_set_busid(struct drm_device *dev, struct drm_master *master)
 {
 	int len, ret, id;
@@ -107,7 +102,6 @@ err:
 }
 
 static struct drm_bus drm_platform_bus = {
-	.get_name = drm_platform_get_name,
 	.set_busid = drm_platform_set_busid,
 };
 
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index abdc265c9cc8..fae4aa4e1426 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -36,11 +36,6 @@ err_free:
 }
 EXPORT_SYMBOL(drm_get_usb_dev);
 
-static const char *drm_usb_get_name(struct drm_device *dev)
-{
-	return "USB";
-}
-
 static int drm_usb_set_busid(struct drm_device *dev,
 			       struct drm_master *master)
 {
@@ -48,7 +43,6 @@ static int drm_usb_set_busid(struct drm_device *dev,
 }
 
 static struct drm_bus drm_usb_bus = {
-	.get_name = drm_usb_get_name,
 	.set_busid = drm_usb_set_busid,
 };
     
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9d8c690732cf..2dd68ef22818 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -726,7 +726,6 @@ struct drm_master {
 #define DRM_SCANOUTPOS_ACCURATE     (1 << 2)
 
 struct drm_bus {
-	const char *(*get_name)(struct drm_device *dev);
 	int (*set_busid)(struct drm_device *dev, struct drm_master *master);
 };
 
-- 
1.8.5.2

Re: [PATCH 16/18] drm: remove drm_bus->get_name

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 971 bytes --]

On Fri, Apr 11, 2014 at 11:36:13PM +0200, Daniel Vetter wrote:
> The only user is the info debugfs file, so we only need something
> human readable. Now for both pci and platform devices we've used the
> name of the underlying device driver, which matches the name of the
> drm driver in all cases. So we can just use that instead.
> 
> The exception is usb, which used a generic "USB". Not to harmful with
> just one usb driver, but better to use "udl", too.
> 
> With that converted we can rip out all the ->get_name implementations.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_info.c     | 6 ++----
>  drivers/gpu/drm/drm_pci.c      | 7 -------
>  drivers/gpu/drm/drm_platform.c | 6 ------
>  drivers/gpu/drm/drm_usb.c      | 6 ------
>  include/drm/drmP.h             | 1 -
>  5 files changed, 2 insertions(+), 24 deletions(-)

Happy to see it gone:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 17/18] drm: Remove dev->kdriver

[Daniel Vetter]

With the last patch to ditch the ->get_name callbacks the last
user is now gone.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_pci.c      | 1 -
 drivers/gpu/drm/drm_platform.c | 1 -
 drivers/gpu/drm/drm_usb.c      | 1 -
 include/drm/drmP.h             | 5 -----
 4 files changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 262f1ca07a73..112203e7ae49 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -365,7 +365,6 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
 
 	DRM_DEBUG("\n");
 
-	driver->kdriver.pci = pdriver;
 	driver->bus = &drm_pci_bus;
 
 	if (driver->driver_features & DRIVER_MODESET)
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index c7ec27bbe7c6..234e0bc1ae51 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -121,7 +121,6 @@ int drm_platform_init(struct drm_driver *driver, struct platform_device *platfor
 {
 	DRM_DEBUG("\n");
 
-	driver->kdriver.platform_device = platform_device;
 	driver->bus = &drm_platform_bus;
 	return drm_get_platform_dev(platform_device, driver);
 }
diff --git a/drivers/gpu/drm/drm_usb.c b/drivers/gpu/drm/drm_usb.c
index fae4aa4e1426..c6c7c29ad46f 100644
--- a/drivers/gpu/drm/drm_usb.c
+++ b/drivers/gpu/drm/drm_usb.c
@@ -51,7 +51,6 @@ int drm_usb_init(struct drm_driver *driver, struct usb_driver *udriver)
 	int res;
 	DRM_DEBUG("\n");
 
-	driver->kdriver.usb = udriver;
 	driver->bus = &drm_usb_bus;
 
 	res = usb_register(udriver);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2dd68ef22818..c4272acaac8f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -963,11 +963,6 @@ struct drm_driver {
 	const struct drm_ioctl_desc *ioctls;
 	int num_ioctls;
 	const struct file_operations *fops;
-	union {
-		struct pci_driver *pci;
-		struct platform_device *platform_device;
-		struct usb_driver *usb;
-	} kdriver;
 	struct drm_bus *bus;
 
 	/* List of devices hanging off this driver with stealth attach. */
-- 
1.8.5.2

Re: [PATCH 17/18] drm: Remove dev->kdriver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 506 bytes --]

On Fri, Apr 11, 2014 at 11:36:14PM +0200, Daniel Vetter wrote:
> With the last patch to ditch the ->get_name callbacks the last
> user is now gone.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_pci.c      | 1 -
>  drivers/gpu/drm/drm_platform.c | 1 -
>  drivers/gpu/drm/drm_usb.c      | 1 -
>  include/drm/drmP.h             | 5 -----
>  4 files changed, 8 deletions(-)

More goodness... thank you.

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 18/18] drm/<drivers>: don't set driver->dev_priv_size to 0

[Daniel Vetter]

Especially not on modesetting drivers - this is used to size
the driver private structure for legacy drm buffers.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_drv.c       | 1 -
 drivers/gpu/drm/qxl/qxl_drv.c       | 1 -
 drivers/gpu/drm/radeon/radeon_drv.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 5137f15dba19..2ba39ac7d222 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -198,7 +198,6 @@ static const struct file_operations ast_fops = {
 
 static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM,
-	.dev_priv_size = 0,
 
 	.load = ast_driver_load,
 	.unload = ast_driver_unload,
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index fee8748bdca5..6e936634d65c 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -214,7 +214,6 @@ static struct pci_driver qxl_pci_driver = {
 static struct drm_driver qxl_driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET |
 			   DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
-	.dev_priv_size = 0,
 	.load = qxl_driver_load,
 	.unload = qxl_driver_unload,
 
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index d0eba48dd74e..855ad16101c6 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -535,7 +535,6 @@ static struct drm_driver kms_driver = {
 	    DRIVER_USE_AGP |
 	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
 	    DRIVER_PRIME | DRIVER_RENDER,
-	.dev_priv_size = 0,
 	.load = radeon_driver_load_kms,
 	.open = radeon_driver_open_kms,
 	.preclose = radeon_driver_preclose_kms,
-- 
1.8.5.2

Re: [PATCH 18/18] drm/<drivers>: don't set driver->dev_priv_size to 0

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 319 bytes --]

On Fri, Apr 11, 2014 at 11:36:15PM +0200, Daniel Vetter wrote:
> Especially not on modesetting drivers - this is used to size
> the driver private structure for legacy drm buffers.

Besides being pointless anyway, since that's what it will be initialized
by default...

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/18] drm_bus cleanups and other cruft removal

[Laurent Pinchart]

Hi Daniel,

On Friday 11 April 2014 23:35:57 Daniel Vetter wrote:
> Hi all,
> 
> I've chatted a bit with Thierry about how we could allow drivers to not even
> required a drm_bus any more. Which is relevant when e.g. due to the new
> master/component no platform device is conveniently around.
> 
> So I've brushed off my old series to remove some drm_bus functions and other
> cruft and rebased it onto latest drm-next.
> 
> It gets rid of everything but drm_bus->set_busid, but Thierry has a good
> plan to make that one optional too - it's only really needed for backwards
> compat with some old libdrm versions on pci drm drivers.
> 
> Comments and review highly welcome.
> 
> Presuming no one screams I plan to send a pull request with these patches to
> Dave fairly early for 3.16 so that Thierry can base his tegra rework on top
> of it.

Apart from a few comments on patch 10/16, there's nothing I could complain 
about. Great work, as usual :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/18] drm_bus cleanups and other cruft removal

[David Herrmann]

Hi

On Fri, Apr 11, 2014 at 11:35 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
>
> I've chatted a bit with Thierry about how we could allow drivers to not even
> required a drm_bus any more. Which is relevant when e.g. due to the new
> master/component no platform device is conveniently around.
>
> So I've brushed off my old series to remove some drm_bus functions and other
> cruft and rebased it onto latest drm-next.
>
> It gets rid of everything but drm_bus->set_busid, but Thierry has a good plan to
> make that one optional too - it's only really needed for backwards compat with
> some old libdrm versions on pci drm drivers.
>
> Comments and review highly welcome.
>
> Presuming no one screams I plan to send a pull request with these patches to
> Dave fairly early for 3.16 so that Thierry can base his tegra rework on top of
> it.

Apart from the irq patches, this looks all good. Thierry caught all
the typos, so please go ahead and send a pull-request. No objections
from me.

Thanks
David

[PATCH 37/50] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Daniel Vetter]

To get rid of the dev->bus->get_irq callback we need to pass in the
desired irq explicitly into drm_irq_install. To avoid having to do the
same for drm_irq_unistall just track it internally. That leaves
drivers with less room to botch things up.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 38c0bab3296e..1348e041c2af 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
  */
 int drm_irq_install(struct drm_device *dev)
 {
-	int ret;
+	int ret, irq;
 	unsigned long sh_flags = 0;
 	char *irqname;
 
+	irq = drm_dev_to_irq(dev);
+
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
 		return -EINVAL;
 
-	if (drm_dev_to_irq(dev) == 0)
+	if (irq == 0)
 		return -EINVAL;
 
 	/* Driver must have been initialized */
@@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
 		return -EBUSY;
 	dev->irq_enabled = true;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", dev->irq);
 
 	/* Before installing handler */
 	if (dev->driver->irq_preinstall)
@@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
 	else
 		irqname = dev->driver->name;
 
-	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
+	ret = request_irq(dev->irq, dev->driver->irq_handler,
 			  sh_flags, irqname, dev);
 
 	if (ret < 0) {
@@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
 		dev->irq_enabled = false;
 		if (!drm_core_check_feature(dev, DRIVER_MODESET))
 			vga_client_register(dev->pdev, NULL, NULL, NULL);
-		free_irq(drm_dev_to_irq(dev), dev);
+		free_irq(dev->irq, dev);
+	} else {
+		dev->irq = irq;
 	}
 
 	return ret;
@@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (!irq_enabled)
 		return -EINVAL;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", dev->irq);
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		vga_client_register(dev->pdev, NULL, NULL, NULL);
@@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (dev->driver->irq_uninstall)
 		dev->driver->irq_uninstall(dev);
 
-	free_irq(drm_dev_to_irq(dev), dev);
+	free_irq(dev->irq, dev);
 
 	return 0;
 }
-- 
1.8.4.3

[PATCH] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Daniel Vetter]

To get rid of the dev->bus->get_irq callback we need to pass in the
desired irq explicitly into drm_irq_install. To avoid having to do the
same for drm_irq_unistall just track it internally. That leaves
drivers with less room to botch things up.

v2: Add the hunk lost in an earlier patch to this one (Thierry).

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
 include/drm/drmP.h        |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 64fd3fcf958a..0d1aef32ef10 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device *dev)
  */
 int drm_irq_install(struct drm_device *dev)
 {
-	int ret;
+	int ret, irq;
 	unsigned long sh_flags = 0;
 	char *irqname;
 
+	irq = drm_dev_to_irq(dev);
+
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
 		return -EINVAL;
 
-	if (drm_dev_to_irq(dev) == 0)
+	if (irq == 0)
 		return -EINVAL;
 
 	/* Driver must have been initialized */
@@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
 		return -EBUSY;
 	dev->irq_enabled = true;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", dev->irq);
 
 	/* Before installing handler */
 	if (dev->driver->irq_preinstall)
@@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
 	else
 		irqname = dev->driver->name;
 
-	ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
+	ret = request_irq(dev->irq, dev->driver->irq_handler,
 			  sh_flags, irqname, dev);
 
 	if (ret < 0) {
@@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
 		dev->irq_enabled = false;
 		if (!drm_core_check_feature(dev, DRIVER_MODESET))
 			vga_client_register(dev->pdev, NULL, NULL, NULL);
-		free_irq(drm_dev_to_irq(dev), dev);
+		free_irq(dev->irq, dev);
+	} else {
+		dev->irq = irq;
 	}
 
 	return ret;
@@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (!irq_enabled)
 		return -EINVAL;
 
-	DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
+	DRM_DEBUG("irq=%d\n", dev->irq);
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		vga_client_register(dev->pdev, NULL, NULL, NULL);
@@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (dev->driver->irq_uninstall)
 		dev->driver->irq_uninstall(dev);
 
-	free_irq(drm_dev_to_irq(dev), dev);
+	free_irq(dev->irq, dev);
 
 	return 0;
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 386745913d2a..0219ceae5270 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1118,6 +1118,8 @@ struct drm_device {
 	/** \name Context support */
 	/*@{ */
 	bool irq_enabled;		/**< True if irq handler is enabled */
+	int irq;
+
 	__volatile__ long context_flag;	/**< Context swapping flag */
 	int last_context;		/**< Last current context */
 	/*@} */
-- 
1.8.4.3

Re: [PATCH] drm/irq: track the irq installed in drm_irq_install in dev->irq

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 709 bytes --]

On Mon, Dec 16, 2013 at 11:29:13AM +0100, Daniel Vetter wrote:
> To get rid of the dev->bus->get_irq callback we need to pass in the
> desired irq explicitly into drm_irq_install. To avoid having to do the
> same for drm_irq_unistall just track it internally. That leaves
> drivers with less room to botch things up.
> 
> v2: Add the hunk lost in an earlier patch to this one (Thierry).
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
>  include/drm/drmP.h        |  2 ++
>  2 files changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel