drm/i915: Fix module initialisation, v2.

Commit Message

Maarten Lankhorst Aug. 27, 2015, 1:15 p.m. UTC

Set DRIVER_MODESET and DRIVER_ATOMIC by default. The driver is fully atomic.
Remove the legacy suspend/resume, to fix a warning introduced by:

"drm: WARN_ON if a modeset driver uses legacy suspend/resume helpers"

and removing the .get_vblank_timestamp reset to NULL. It's a noop without UMS.

Signed-off-by: Maarten Lankhorst <[e-mail]>
---

Comments

Shuang He Aug. 29, 2015, 9:51 p.m. UTC | #1

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: [e-mail])
Task id: 7275
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              253/253              252/253
SNB                                  248/248              248/248
IVB                                  281/281              281/281
BYT                                  234/234              234/234
HSW                                  317/317              317/317
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@gem_reloc_vs_gpu@forked-interruptible-faulting-reloc-thrashing      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'

Matt Roper Aug. 31, 2015, 10:13 p.m. UTC | #2

On Thu, Aug 27, 2015 at 03:15:15PM +0200, Maarten Lankhorst wrote:
> Set DRIVER_MODESET and DRIVER_ATOMIC by default. The driver is fully atomic.
> Remove the legacy suspend/resume, to fix a warning introduced by:
> 
> "drm: WARN_ON if a modeset driver uses legacy suspend/resume helpers"
> 
> and removing the .get_vblank_timestamp reset to NULL. It's a noop without UMS.
> 
> Signed-off-by: Maarten Lankhorst <[e-mail]>

Reviewed-by: Matt Roper <[e-mail]>

> ---
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 097d4ba0421c..f0eaa6f8826b 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -364,12 +364,12 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_
>  		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
>  		/* i915 resume handler doesn't set to D0 */
>  		pci_set_power_state(dev->pdev, PCI_D0);
> -		i915_resume_legacy(dev);
> +		i915_resume_switcheroo(dev);
>  		dev->switch_power_state = DRM_SWITCH_POWER_ON;
>  	} else {
>  		pr_err("switched off\n");
>  		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> -		i915_suspend_legacy(dev, pmm);
> +		i915_suspend_switcheroo(dev, pmm);
>  		dev->switch_power_state = DRM_SWITCH_POWER_OFF;
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ce3bd0c713b9..4646fe1a0499 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -679,7 +679,7 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
>  	return 0;
>  }
>  
> -int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
> +int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
>  {
>  	int error;
>  
> @@ -812,7 +812,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  	return ret;
>  }
>  
> -int i915_resume_legacy(struct drm_device *dev)
> +int i915_resume_switcheroo(struct drm_device *dev)
>  {
>  	int ret;
>  
> @@ -1649,7 +1649,7 @@ static struct drm_driver driver = {
>  	 */
>  	.driver_features =
>  	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME |
> -	    DRIVER_RENDER,
> +	    DRIVER_RENDER | DRIVER_MODESET,
>  	.load = i915_driver_load,
>  	.unload = i915_driver_unload,
>  	.open = i915_driver_open,
> @@ -1658,10 +1658,6 @@ static struct drm_driver driver = {
>  	.postclose = i915_driver_postclose,
>  	.set_busid = drm_pci_set_busid,
>  
> -	/* Used in place of i915_pm_ops for non-DRIVER_MODESET */
> -	.suspend = i915_suspend_legacy,
> -	.resume = i915_resume_legacy,
> -
>  #if defined(CONFIG_DEBUG_FS)
>  	.debugfs_init = i915_debugfs_init,
>  	.debugfs_cleanup = i915_debugfs_cleanup,
> @@ -1704,7 +1700,6 @@ static int __init i915_init(void)
>  	 * either the i915.modeset prarameter or by the
>  	 * vga_text_mode_force boot option.
>  	 */
> -	driver.driver_features |= DRIVER_MODESET;
>  
>  	if (i915.modeset == 0)
>  		driver.driver_features &= ~DRIVER_MODESET;
> @@ -1715,17 +1710,11 @@ static int __init i915_init(void)
>  #endif
>  
>  	if (!(driver.driver_features & DRIVER_MODESET)) {
> -		driver.get_vblank_timestamp = NULL;
>  		/* Silently fail loading to not upset userspace. */
>  		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
>  		return 0;
>  	}
>  
> -	/*
> -	 * FIXME: Note that we're lying to the DRM core here so that we can get access
> -	 * to the atomic ioctl and the atomic properties.  Only plane operations on
> -	 * a single CRTC will actually work.
> -	 */
>  	if (i915.nuclear_pageflip)
>  		driver.driver_features |= DRIVER_ATOMIC;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8c938451a05e..5c2541ddf212 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2607,8 +2607,8 @@ struct drm_i915_cmd_table {
>  extern const struct drm_ioctl_desc i915_ioctls[];
>  extern int i915_max_ioctl;
>  
> -extern int i915_suspend_legacy(struct drm_device *dev, pm_message_t state);
> -extern int i915_resume_legacy(struct drm_device *dev);
> +extern int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state);
> +extern int i915_resume_switcheroo(struct drm_device *dev);
>  
>  /* i915_params.c */
>  struct i915_params {
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 1, 2015, 10:12 a.m. UTC | #3

On Mon, Aug 31, 2015 at 03:13:41PM -0700, Matt Roper wrote:
> On Thu, Aug 27, 2015 at 03:15:15PM +0200, Maarten Lankhorst wrote:
> > Set DRIVER_MODESET and DRIVER_ATOMIC by default. The driver is fully atomic.
> > Remove the legacy suspend/resume, to fix a warning introduced by:
> > 
> > "drm: WARN_ON if a modeset driver uses legacy suspend/resume helpers"
> > 
> > and removing the .get_vblank_timestamp reset to NULL. It's a noop without UMS.
> > 
> > Signed-off-by: Maarten Lankhorst <[e-mail]>
> 
> Reviewed-by: Matt Roper <[e-mail]>

Review should include the commit message and this commit message doesn't
match v2 at all ... Please rectify (or at least tell me what I should put
there).

Thanks, Daniel

> 
> > ---
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 097d4ba0421c..f0eaa6f8826b 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -364,12 +364,12 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_
> >  		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> >  		/* i915 resume handler doesn't set to D0 */
> >  		pci_set_power_state(dev->pdev, PCI_D0);
> > -		i915_resume_legacy(dev);
> > +		i915_resume_switcheroo(dev);
> >  		dev->switch_power_state = DRM_SWITCH_POWER_ON;
> >  	} else {
> >  		pr_err("switched off\n");
> >  		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> > -		i915_suspend_legacy(dev, pmm);
> > +		i915_suspend_switcheroo(dev, pmm);
> >  		dev->switch_power_state = DRM_SWITCH_POWER_OFF;
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index ce3bd0c713b9..4646fe1a0499 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -679,7 +679,7 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
> >  	return 0;
> >  }
> >  
> > -int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
> > +int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
> >  {
> >  	int error;
> >  
> > @@ -812,7 +812,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
> >  	return ret;
> >  }
> >  
> > -int i915_resume_legacy(struct drm_device *dev)
> > +int i915_resume_switcheroo(struct drm_device *dev)
> >  {
> >  	int ret;
> >  
> > @@ -1649,7 +1649,7 @@ static struct drm_driver driver = {
> >  	 */
> >  	.driver_features =
> >  	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME |
> > -	    DRIVER_RENDER,
> > +	    DRIVER_RENDER | DRIVER_MODESET,
> >  	.load = i915_driver_load,
> >  	.unload = i915_driver_unload,
> >  	.open = i915_driver_open,
> > @@ -1658,10 +1658,6 @@ static struct drm_driver driver = {
> >  	.postclose = i915_driver_postclose,
> >  	.set_busid = drm_pci_set_busid,
> >  
> > -	/* Used in place of i915_pm_ops for non-DRIVER_MODESET */
> > -	.suspend = i915_suspend_legacy,
> > -	.resume = i915_resume_legacy,
> > -
> >  #if defined(CONFIG_DEBUG_FS)
> >  	.debugfs_init = i915_debugfs_init,
> >  	.debugfs_cleanup = i915_debugfs_cleanup,
> > @@ -1704,7 +1700,6 @@ static int __init i915_init(void)
> >  	 * either the i915.modeset prarameter or by the
> >  	 * vga_text_mode_force boot option.
> >  	 */
> > -	driver.driver_features |= DRIVER_MODESET;
> >  
> >  	if (i915.modeset == 0)
> >  		driver.driver_features &= ~DRIVER_MODESET;
> > @@ -1715,17 +1710,11 @@ static int __init i915_init(void)
> >  #endif
> >  
> >  	if (!(driver.driver_features & DRIVER_MODESET)) {
> > -		driver.get_vblank_timestamp = NULL;
> >  		/* Silently fail loading to not upset userspace. */
> >  		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> >  		return 0;
> >  	}
> >  
> > -	/*
> > -	 * FIXME: Note that we're lying to the DRM core here so that we can get access
> > -	 * to the atomic ioctl and the atomic properties.  Only plane operations on
> > -	 * a single CRTC will actually work.
> > -	 */
> >  	if (i915.nuclear_pageflip)
> >  		driver.driver_features |= DRIVER_ATOMIC;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8c938451a05e..5c2541ddf212 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2607,8 +2607,8 @@ struct drm_i915_cmd_table {
> >  extern const struct drm_ioctl_desc i915_ioctls[];
> >  extern int i915_max_ioctl;
> >  
> > -extern int i915_suspend_legacy(struct drm_device *dev, pm_message_t state);
> > -extern int i915_resume_legacy(struct drm_device *dev);
> > +extern int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state);
> > +extern int i915_resume_switcheroo(struct drm_device *dev);
> >  
> >  /* i915_params.c */
> >  struct i915_params {
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]

Maarten Lankhorst Sept. 1, 2015, 11:51 a.m. UTC | #4

Op 01-09-15 om 12:12 schreef Daniel Vetter:
> On Mon, Aug 31, 2015 at 03:13:41PM -0700, Matt Roper wrote:
>> On Thu, Aug 27, 2015 at 03:15:15PM +0200, Maarten Lankhorst wrote:
>>> Set DRIVER_MODESET and DRIVER_ATOMIC by default. The driver is fully atomic.
>>> Remove the legacy suspend/resume, to fix a warning introduced by:
>>>
>>> "drm: WARN_ON if a modeset driver uses legacy suspend/resume helpers"
>>>
>>> and removing the .get_vblank_timestamp reset to NULL. It's a noop without UMS.
>>>
>>> Signed-off-by: Maarten Lankhorst <[e-mail]>
>> Reviewed-by: Matt Roper <[e-mail]>
> Review should include the commit message and this commit message doesn't
> match v2 at all ... Please rectify (or at least tell me what I should put
> there).
>
Oops..

The driver doesn't support UMS any more, so set DRIVER_MODESET by default,
remove the legacy s/r callbacks, and rename the s/r functions to make it more clear
they're only in use by switcheroo now.

Also remove an obsolete comment about atomic. Normal updates are supported only
async updates aren't yet.

Daniel Vetter Sept. 2, 2015, 7:53 a.m. UTC | #5

On Tue, Sep 01, 2015 at 01:51:38PM +0200, Maarten Lankhorst wrote:
> Op 01-09-15 om 12:12 schreef Daniel Vetter:
> > On Mon, Aug 31, 2015 at 03:13:41PM -0700, Matt Roper wrote:
> >> On Thu, Aug 27, 2015 at 03:15:15PM +0200, Maarten Lankhorst wrote:
> >>> Set DRIVER_MODESET and DRIVER_ATOMIC by default. The driver is fully atomic.
> >>> Remove the legacy suspend/resume, to fix a warning introduced by:
> >>>
> >>> "drm: WARN_ON if a modeset driver uses legacy suspend/resume helpers"
> >>>
> >>> and removing the .get_vblank_timestamp reset to NULL. It's a noop without UMS.
> >>>
> >>> Signed-off-by: Maarten Lankhorst <[e-mail]>
> >> Reviewed-by: Matt Roper <[e-mail]>
> > Review should include the commit message and this commit message doesn't
> > match v2 at all ... Please rectify (or at least tell me what I should put
> > there).
> >
> Oops..
> 
> The driver doesn't support UMS any more, so set DRIVER_MODESET by default,
> remove the legacy s/r callbacks, and rename the s/r functions to make it more clear
> they're only in use by switcheroo now.
> 
> Also remove an obsolete comment about atomic. Normal updates are supported only
> async updates aren't yet.

Queued for -next, thanks for the patch.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 097d4ba0421c..f0eaa6f8826b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -364,12 +364,12 @@  static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_
 		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
 		/* i915 resume handler doesn't set to D0 */
 		pci_set_power_state(dev->pdev, PCI_D0);
-		i915_resume_legacy(dev);
+		i915_resume_switcheroo(dev);
 		dev->switch_power_state = DRM_SWITCH_POWER_ON;
 	} else {
 		pr_err("switched off\n");
 		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
-		i915_suspend_legacy(dev, pmm);
+		i915_suspend_switcheroo(dev, pmm);
 		dev->switch_power_state = DRM_SWITCH_POWER_OFF;
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ce3bd0c713b9..4646fe1a0499 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -679,7 +679,7 @@  static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 	return 0;
 }
 
-int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
+int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
 {
 	int error;
 
@@ -812,7 +812,7 @@  static int i915_drm_resume_early(struct drm_device *dev)
 	return ret;
 }
 
-int i915_resume_legacy(struct drm_device *dev)
+int i915_resume_switcheroo(struct drm_device *dev)
 {
 	int ret;
 
@@ -1649,7 +1649,7 @@  static struct drm_driver driver = {
 	 */
 	.driver_features =
 	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME |
-	    DRIVER_RENDER,
+	    DRIVER_RENDER | DRIVER_MODESET,
 	.load = i915_driver_load,
 	.unload = i915_driver_unload,
 	.open = i915_driver_open,
@@ -1658,10 +1658,6 @@  static struct drm_driver driver = {
 	.postclose = i915_driver_postclose,
 	.set_busid = drm_pci_set_busid,
 
-	/* Used in place of i915_pm_ops for non-DRIVER_MODESET */
-	.suspend = i915_suspend_legacy,
-	.resume = i915_resume_legacy,
-
 #if defined(CONFIG_DEBUG_FS)
 	.debugfs_init = i915_debugfs_init,
 	.debugfs_cleanup = i915_debugfs_cleanup,
@@ -1704,7 +1700,6 @@  static int __init i915_init(void)
 	 * either the i915.modeset prarameter or by the
 	 * vga_text_mode_force boot option.
 	 */
-	driver.driver_features |= DRIVER_MODESET;
 
 	if (i915.modeset == 0)
 		driver.driver_features &= ~DRIVER_MODESET;
@@ -1715,17 +1710,11 @@  static int __init i915_init(void)
 #endif
 
 	if (!(driver.driver_features & DRIVER_MODESET)) {
-		driver.get_vblank_timestamp = NULL;
 		/* Silently fail loading to not upset userspace. */
 		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
 		return 0;
 	}
 
-	/*
-	 * FIXME: Note that we're lying to the DRM core here so that we can get access
-	 * to the atomic ioctl and the atomic properties.  Only plane operations on
-	 * a single CRTC will actually work.
-	 */
 	if (i915.nuclear_pageflip)
 		driver.driver_features |= DRIVER_ATOMIC;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8c938451a05e..5c2541ddf212 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2607,8 +2607,8 @@  struct drm_i915_cmd_table {
 extern const struct drm_ioctl_desc i915_ioctls[];
 extern int i915_max_ioctl;
 
-extern int i915_suspend_legacy(struct drm_device *dev, pm_message_t state);
-extern int i915_resume_legacy(struct drm_device *dev);
+extern int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state);
+extern int i915_resume_switcheroo(struct drm_device *dev);
 
 /* i915_params.c */
 struct i915_params {