[Intel-gfx] [PATCH 04/17] drm/i915: Use drm_i915_private directly from drv_get_drvdata()

[Andi Shyti]

Hi Chris,

>  static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
>  {
> -	struct drm_device *dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *i915 = pdev_to_i915(pdev);
>  	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
>  
> +	if (!i915) {
> +		dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
> +		return;
> +	}
> +
>  	if (state == VGA_SWITCHEROO_ON) {
>  		pr_info("switched on\n");
> -		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
>  		/* i915 resume handler doesn't set to D0 */
>  		pci_set_power_state(pdev, PCI_D0);
> -		i915_resume_switcheroo(dev);
> -		dev->switch_power_state = DRM_SWITCH_POWER_ON;
> +		i915_resume_switcheroo(i915);
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_ON;
>  	} else {
>  		pr_info("switched off\n");
> -		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> -		i915_suspend_switcheroo(dev, pmm);
> -		dev->switch_power_state = DRM_SWITCH_POWER_OFF;
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_CHANGING;
> +		i915_suspend_switcheroo(i915, pmm);
> +		i915->drm.switch_power_state = DRM_SWITCH_POWER_OFF;

doesn't have anything to do with this patch, but don't we care about
the resume and suspend failures?

>  static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
> @@ -1841,7 +1847,8 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	i915->drm.pdev = pdev;
>  	i915->drm.dev_private = i915;
> -	pci_set_drvdata(pdev, &i915->drm);
> +	BUILD_BUG_ON(offsetof(typeof(*i915), drm));
> +	pci_set_drvdata(pdev, i915);

This looks a bit too fragile to me and it's not documented
anywhere that need to have "drm" in a specific position.

At the end I wonder, why do we need "drm" to be there? Unless I
missed it, I haven't seen anywhere any double reference to
"i916"/"drm".

The rest of the patch looks quite straight forward.

Andi