diff mbox series

[03/26] drm/irq: Ditch DRIVER_IRQ_SHARED

Message ID 20190124165831.16427-4-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show
Series some cleanups, mostly around fbdev emulation | expand

Commit Message

Daniel Vetter Jan. 24, 2019, 4:58 p.m. UTC 
This is only used by drm_irq_install(), which is an optional helper.
And the right choice is to set it for all pci devices, and not for
everything else.

Any driver with special needs should just use request_irq() directly,
and there's plenty of drivers doing that already.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 drivers/gpu/drm/drm_irq.c               |  4 ++--
 drivers/gpu/drm/gma500/psb_drv.c        |  3 +--
 drivers/gpu/drm/i915/i915_drv.c         |  2 +-
 drivers/gpu/drm/mga/mga_drv.c           |  2 +-
 drivers/gpu/drm/qxl/qxl_drv.c           |  1 -
 drivers/gpu/drm/r128/r128_drv.c         |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.c     |  4 +---
 drivers/gpu/drm/via/via_drv.c           |  3 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     |  2 +-
 drivers/staging/vboxvideo/vbox_drv.c    |  3 +--
 include/drm/drm_drv.h                   | 24 +++++++-----------------
 12 files changed, 18 insertions(+), 34 deletions(-)

Comments

Emil Velikov Jan. 25, 2019, 2:46 p.m. UTC | #1 
On Thu, 24 Jan 2019 at 16:58, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> This is only used by drm_irq_install(), which is an optional helper.
> And the right choice is to set it for all pci devices, and not for
> everything else.
>
Can you please add some information (or reference) why it's the right choice?

Thanks
Emil
Daniel Vetter Jan. 25, 2019, 4:14 p.m. UTC | #2 
On Fri, Jan 25, 2019 at 02:46:55PM +0000, Emil Velikov wrote:
> On Thu, 24 Jan 2019 at 16:58, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > This is only used by drm_irq_install(), which is an optional helper.
> > And the right choice is to set it for all pci devices, and not for
> > everything else.
> >
> Can you please add some information (or reference) why it's the right choice?

pci devices can have a shared interrupt line. That's definitely the case
for legacy pci, and I guess carries over to pcie. msi/msi-x interrupts
will give you dedicated interrupts, but it doesn't really hurt to mark
them as shared.

I guess I could rephrase to state:

"This is only used by drm_irq_install(), which is an optional helper.
For legacy pci devices this is required (due to interrupt sharing without
msi/msi-x), and just making this the default exactly matches the behaviour
of all existing drivers using the drm_irq_install() helpers. In case that
ever becomes wrong drivers can roll their own irq handling, as many
drivers already do (for other reasons like needing a threaded interrupt
handler, or having an entire pile of different interrupt sources)."

That better?
-Daniel
Emil Velikov Jan. 28, 2019, 11:47 a.m. UTC | #3 
On 2019/01/25, Daniel Vetter wrote:
> On Fri, Jan 25, 2019 at 02:46:55PM +0000, Emil Velikov wrote:
> > On Thu, 24 Jan 2019 at 16:58, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > >
> > > This is only used by drm_irq_install(), which is an optional helper.
> > > And the right choice is to set it for all pci devices, and not for
> > > everything else.
> > >
> > Can you please add some information (or reference) why it's the right choice?
> 
> pci devices can have a shared interrupt line. That's definitely the case
> for legacy pci, and I guess carries over to pcie. msi/msi-x interrupts
> will give you dedicated interrupts, but it doesn't really hurt to mark
> them as shared.
> 
> I guess I could rephrase to state:
> 
> "This is only used by drm_irq_install(), which is an optional helper.
> For legacy pci devices this is required (due to interrupt sharing without
> msi/msi-x), and just making this the default exactly matches the behaviour
> of all existing drivers using the drm_irq_install() helpers. In case that
> ever becomes wrong drivers can roll their own irq handling, as many
> drivers already do (for other reasons like needing a threaded interrupt
> handler, or having an entire pile of different interrupt sources)."
> 
> That better?

It's amazing. Thank you very much.

-Emil
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 22502417c18c..a1bb3773087b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1189,7 +1189,7 @@  amdgpu_get_crtc_scanout_position(struct drm_device *dev, unsigned int pipe,
 static struct drm_driver kms_driver = {
 	.driver_features =
 	    DRIVER_USE_AGP | DRIVER_ATOMIC |
-	    DRIVER_IRQ_SHARED | DRIVER_GEM |
+	    DRIVER_GEM |
 	    DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
 	.load = amdgpu_driver_load_kms,
 	.open = amdgpu_driver_open_kms,
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c5babb3e4752..9bd8908d5fd8 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -120,8 +120,8 @@  int drm_irq_install(struct drm_device *dev, int irq)
 	if (dev->driver->irq_preinstall)
 		dev->driver->irq_preinstall(dev);
 
-	/* Install handler */
-	if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
+	/* PCI devices require shared interrupts. */
+	if (dev->pdev)
 		sh_flags = IRQF_SHARED;
 
 	ret = request_irq(irq, dev->driver->irq_handler,
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 7cf14aeb1c28..eefaf4daff2b 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -468,8 +468,7 @@  static const struct file_operations psb_gem_fops = {
 };
 
 static struct drm_driver driver = {
-	.driver_features = DRIVER_IRQ_SHARED | \
-			   DRIVER_MODESET | DRIVER_GEM,
+	.driver_features = DRIVER_MODESET | DRIVER_GEM,
 	.load = psb_driver_load,
 	.unload = psb_driver_unload,
 	.lastclose = drm_fb_helper_lastclose,
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 550cfb945942..a7aaa1ac4c99 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -3010,7 +3010,7 @@  static struct drm_driver driver = {
 	 * deal with them for Intel hardware.
 	 */
 	.driver_features =
-	    DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME |
+	    DRIVER_GEM | DRIVER_PRIME |
 	    DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ,
 	.release = i915_driver_release,
 	.open = i915_driver_open,
diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c
index 1aad27813c23..6e1d1054ad06 100644
--- a/drivers/gpu/drm/mga/mga_drv.c
+++ b/drivers/gpu/drm/mga/mga_drv.c
@@ -57,7 +57,7 @@  static const struct file_operations mga_driver_fops = {
 static struct drm_driver driver = {
 	.driver_features =
 	    DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_LEGACY |
-	    DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
+	    DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ,
 	.dev_priv_size = sizeof(drm_mga_buf_priv_t),
 	.load = mga_driver_load,
 	.unload = mga_driver_unload,
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 44daac129205..d856615bdb50 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -243,7 +243,6 @@  static struct pci_driver qxl_pci_driver = {
 
 static struct drm_driver qxl_driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
-			   DRIVER_IRQ_SHARED |
 			   DRIVER_ATOMIC,
 
 	.dumb_create = qxl_mode_dumb_create,
diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c
index 0d2b7e42b3a7..4b1a505ab353 100644
--- a/drivers/gpu/drm/r128/r128_drv.c
+++ b/drivers/gpu/drm/r128/r128_drv.c
@@ -57,7 +57,7 @@  static const struct file_operations r128_driver_fops = {
 static struct drm_driver driver = {
 	.driver_features =
 	    DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | DRIVER_LEGACY |
-	    DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
+	    DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ,
 	.dev_priv_size = sizeof(drm_r128_buf_priv_t),
 	.load = r128_driver_load,
 	.preclose = r128_driver_preclose,
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 450a9d473c30..2e96c886392b 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -534,9 +534,7 @@  radeon_get_crtc_scanout_position(struct drm_device *dev, unsigned int pipe,
 
 static struct drm_driver kms_driver = {
 	.driver_features =
-	    DRIVER_USE_AGP |
-	    DRIVER_IRQ_SHARED | DRIVER_GEM |
-	    DRIVER_PRIME | DRIVER_RENDER,
+	    DRIVER_USE_AGP | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER,
 	.load = radeon_driver_load_kms,
 	.open = radeon_driver_open_kms,
 	.postclose = radeon_driver_postclose_kms,
diff --git a/drivers/gpu/drm/via/via_drv.c b/drivers/gpu/drm/via/via_drv.c
index aaf766f7cca2..af6a12d3c058 100644
--- a/drivers/gpu/drm/via/via_drv.c
+++ b/drivers/gpu/drm/via/via_drv.c
@@ -70,8 +70,7 @@  static const struct file_operations via_driver_fops = {
 
 static struct drm_driver driver = {
 	.driver_features =
-	    DRIVER_USE_AGP | DRIVER_HAVE_IRQ | DRIVER_LEGACY |
-	    DRIVER_IRQ_SHARED,
+	    DRIVER_USE_AGP | DRIVER_HAVE_IRQ | DRIVER_LEGACY,
 	.load = via_driver_load,
 	.unload = via_driver_unload,
 	.open = via_driver_open,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index d159d0400013..4638f6791cda 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1582,7 +1582,7 @@  static const struct file_operations vmwgfx_driver_fops = {
 };
 
 static struct drm_driver driver = {
-	.driver_features = DRIVER_IRQ_SHARED |
+	.driver_features =
 	DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC,
 	.load = vmw_driver_load,
 	.unload = vmw_driver_unload,
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c
index 78cbcd68d4f3..b0d73d5fba5d 100644
--- a/drivers/staging/vboxvideo/vbox_drv.c
+++ b/drivers/staging/vboxvideo/vbox_drv.c
@@ -221,8 +221,7 @@  static void vbox_master_drop(struct drm_device *dev, struct drm_file *file_priv)
 
 static struct drm_driver driver = {
 	.driver_features =
-	    DRIVER_MODESET | DRIVER_GEM | DRIVER_IRQ_SHARED |
-	    DRIVER_PRIME | DRIVER_ATOMIC,
+	    DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_ATOMIC,
 	.dev_priv_size = 0,
 
 	.lastclose = drm_fb_helper_lastclose,
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index fbbcd2887ea8..60318480675f 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -94,42 +94,32 @@  enum drm_driver_feature {
 	 * code by calling request_irq() directly.
 	 */
 	DRIVER_HAVE_IRQ			= BIT(5),
-	/**
-	 * @DRIVER_IRQ_SHARED:
-	 *
-	 * Indicates to drm_irq_install() that a shared irq should be requested.
-	 *
-	 * FIXME: This should be an explicit argument for non-legacy drivers, or
-	 * at least the default for PCI devices (which would cover all current
-	 * users).
-	 */
-	DRIVER_IRQ_SHARED		= BIT(6),
 	/**
 	 * @DRIVER_GEM:
 	 *
 	 * Driver use the GEM memory manager. This should be set for all modern
 	 * drivers.
 	 */
-	DRIVER_GEM			= BIT(7),
+	DRIVER_GEM			= BIT(6),
 	/**
 	 * @DRIVER_MODESET:
 	 *
 	 * Driver supports mode setting interfaces (KMS).
 	 */
-	DRIVER_MODESET			= BIT(8),
+	DRIVER_MODESET			= BIT(7),
 	/**
 	 * @DRIVER_PRIME:
 	 *
 	 * Driver implements DRM PRIME buffer sharing.
 	 */
-	DRIVER_PRIME			= BIT(9),
+	DRIVER_PRIME			= BIT(8),
 	/**
 	 * @DRIVER_RENDER:
 	 *
 	 * Driver supports dedicated render nodes. See also the :ref:`section on
 	 * render nodes <drm_render_node>` for details.
 	 */
-	DRIVER_RENDER			= BIT(10),
+	DRIVER_RENDER			= BIT(9),
 	/**
 	 * @DRIVER_ATOMIC:
 	 *
@@ -139,21 +129,21 @@  enum drm_driver_feature {
 	 * multi-plane updates are not guaranteed to be tear-free) should not
 	 * set this flag.
 	 */
-	DRIVER_ATOMIC			= BIT(11),
+	DRIVER_ATOMIC			= BIT(10),
 	/**
 	 * @DRIVER_KMS_LEGACY_CONTEXT:
 	 *
 	 * Used only by nouveau for backwards compatibility with existing
 	 * userspace.  Do not use.
 	 */
-	DRIVER_KMS_LEGACY_CONTEXT	= BIT(12),
+	DRIVER_KMS_LEGACY_CONTEXT	= BIT(11),
 	/**
 	 * @DRIVER_SYNCOBJ:
 	 *
 	 * Driver supports &drm_syncobj for explicit synchronization of command
 	 * submission.
 	 */
-	DRIVER_SYNCOBJ                  = BIT(13),
+	DRIVER_SYNCOBJ                  = BIT(12),
 };
 
 /**