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

Andi Shyti
2019-08-05T17:05:36Z

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