| Message ID | 1418056929-7977-2-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 08 Dec 2014, Imre Deak <imre.deak@intel.com> wrote: > This will be needed by later patches, so factor it out. > > No functional change. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 15 ++++++--------- > drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++ > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 71be3c9..32c2e33 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev) > > static int i915_pm_suspend(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_i915_private *dev_priv = dev_to_i915_priv(dev); > + struct drm_device *drm_dev = dev_priv->dev; Seems like a funny back and forth pointer dance, but let's see what the next patches bring us... ;) > > - if (!drm_dev || !drm_dev->dev_private) { > + if (!drm_dev || !dev_priv) { > dev_err(dev, "DRM not initialized, aborting suspend.\n"); > return -ENODEV; > } > @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev) > > static int i915_pm_suspend_late(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > /* > * We have a suspedn ordering issue with the snd-hda driver also > @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev) > > static int i915_pm_resume_early(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > return 0; > @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev) > > static int i915_pm_resume(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > return 0; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 61a88fa..3de7a55 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc) > return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1; > } > > +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev) Bikeshed, I think dev_to_i915() is sufficient and in line with to_i915(). BR, Jani. > +{ > + struct pci_dev *pdev = to_pci_dev(dev); > + struct drm_device *drm_dev = pci_get_drvdata(pdev); > + > + return drm_dev->dev_private; > +} > + > /* intel_fifo_underrun.c */ > bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv, > enum pipe pipe, bool enable); > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2014-12-08 at 20:36 +0200, Jani Nikula wrote: > On Mon, 08 Dec 2014, Imre Deak <imre.deak@intel.com> wrote: > > This will be needed by later patches, so factor it out. > > > > No functional change. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 15 ++++++--------- > > drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++ > > 2 files changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 71be3c9..32c2e33 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev) > > > > static int i915_pm_suspend(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_i915_private *dev_priv = dev_to_i915_priv(dev); > > + struct drm_device *drm_dev = dev_priv->dev; > > Seems like a funny back and forth pointer dance, but let's see what the > next patches bring us... ;) Hm, right. This hunk doesn't simplify anything and doesn't even make sense with the below !drm_dev check. Btw if and why drm_dev can be NULL here would be worth investigating more. I'll remove this change for now. > > > > > - if (!drm_dev || !drm_dev->dev_private) { > > + if (!drm_dev || !dev_priv) { > > dev_err(dev, "DRM not initialized, aborting suspend.\n"); > > return -ENODEV; > > } > > @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev) > > > > static int i915_pm_suspend_late(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > > > /* > > * We have a suspedn ordering issue with the snd-hda driver also > > @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev) > > > > static int i915_pm_resume_early(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > > return 0; > > @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev) > > > > static int i915_pm_resume(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > > return 0; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 61a88fa..3de7a55 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc) > > return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1; > > } > > > > +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev) > > Bikeshed, I think dev_to_i915() is sufficient and in line with > to_i915(). Ok. > > BR, > Jani. > > > +{ > > + struct pci_dev *pdev = to_pci_dev(dev); > > + struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + > > + return drm_dev->dev_private; > > +} > > + > > /* intel_fifo_underrun.c */ > > bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv, > > enum pipe pipe, bool enable); > > -- > > 1.8.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Dec 08, 2014 at 06:42:05PM +0200, Imre Deak wrote: > This will be needed by later patches, so factor it out. > > No functional change. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 15 ++++++--------- > drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++ > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 71be3c9..32c2e33 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev) > > static int i915_pm_suspend(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_i915_private *dev_priv = dev_to_i915_priv(dev); > + struct drm_device *drm_dev = dev_priv->dev; > > - if (!drm_dev || !drm_dev->dev_private) { > + if (!drm_dev || !dev_priv) { btw all checks for drm_dev and dev_private are remnants from the old ums days and can all be killed. With fire, please. -Daniel
On Mon, Dec 08, 2014 at 06:42:05PM +0200, Imre Deak wrote: > This will be needed by later patches, so factor it out. The similiar function (for drm_device) is known as to_i915(), so just call this dev_to_i915(). > > No functional change. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 15 ++++++--------- > drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++ > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 71be3c9..32c2e33 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev) > > static int i915_pm_suspend(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_i915_private *dev_priv = dev_to_i915_priv(dev); > + struct drm_device *drm_dev = dev_priv->dev; > > - if (!drm_dev || !drm_dev->dev_private) { > + if (!drm_dev || !dev_priv) { This test is quite nonsensical since you've dereferenced both pointers getting to this point. If it is required, we need a new patch. > dev_err(dev, "DRM not initialized, aborting suspend.\n"); > return -ENODEV; > } > @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev) > > static int i915_pm_suspend_late(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > /* > * We have a suspedn ordering issue with the snd-hda driver also > @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev) > > static int i915_pm_resume_early(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > return 0; > @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev) > > static int i915_pm_resume(struct device *dev) > { > - struct pci_dev *pdev = to_pci_dev(dev); > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > return 0; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 61a88fa..3de7a55 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc) > return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1; > } > > +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev) > +{ > + struct pci_dev *pdev = to_pci_dev(dev); > + struct drm_device *drm_dev = pci_get_drvdata(pdev); > + > + return drm_dev->dev_private; I think you meant static inline struct drm_i915_private *dev_to_i915(struct device *dev) { return to_i915(pci_get_drvdata(to_pci_dev(dev))); } -Chris
On Mon, 2014-12-08 at 20:40 +0000, Chris Wilson wrote: > On Mon, Dec 08, 2014 at 06:42:05PM +0200, Imre Deak wrote: > > This will be needed by later patches, so factor it out. > > The similiar function (for drm_device) is known as to_i915(), so just > call this dev_to_i915(). > > > > No functional change. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 15 ++++++--------- > > drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++ > > 2 files changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 71be3c9..32c2e33 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev) > > > > static int i915_pm_suspend(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_i915_private *dev_priv = dev_to_i915_priv(dev); > > + struct drm_device *drm_dev = dev_priv->dev; > > > > - if (!drm_dev || !drm_dev->dev_private) { > > + if (!drm_dev || !dev_priv) { > > This test is quite nonsensical since you've dereferenced both pointers > getting to this point. If it is required, we need a new patch. Yep, I noticed this after Jani's comment. For now I would just leave this part unchanged. > > > dev_err(dev, "DRM not initialized, aborting suspend.\n"); > > return -ENODEV; > > } > > @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev) > > > > static int i915_pm_suspend_late(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > > > /* > > * We have a suspedn ordering issue with the snd-hda driver also > > @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev) > > > > static int i915_pm_resume_early(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > > return 0; > > @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev) > > > > static int i915_pm_resume(struct device *dev) > > { > > - struct pci_dev *pdev = to_pci_dev(dev); > > - struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; > > > > if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) > > return 0; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 61a88fa..3de7a55 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc) > > return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1; > > } > > > > +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev) > > +{ > > + struct pci_dev *pdev = to_pci_dev(dev); > > + struct drm_device *drm_dev = pci_get_drvdata(pdev); > > + > > + return drm_dev->dev_private; > > I think you meant > > static inline struct drm_i915_private *dev_to_i915(struct device *dev) > { > return to_i915(pci_get_drvdata(to_pci_dev(dev))); > } Yes, looks simpler. > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 71be3c9..32c2e33 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -923,10 +923,10 @@ i915_pci_remove(struct pci_dev *pdev) static int i915_pm_suspend(struct device *dev) { - struct pci_dev *pdev = to_pci_dev(dev); - struct drm_device *drm_dev = pci_get_drvdata(pdev); + struct drm_i915_private *dev_priv = dev_to_i915_priv(dev); + struct drm_device *drm_dev = dev_priv->dev; - if (!drm_dev || !drm_dev->dev_private) { + if (!drm_dev || !dev_priv) { dev_err(dev, "DRM not initialized, aborting suspend.\n"); return -ENODEV; } @@ -939,8 +939,7 @@ static int i915_pm_suspend(struct device *dev) static int i915_pm_suspend_late(struct device *dev) { - struct pci_dev *pdev = to_pci_dev(dev); - struct drm_device *drm_dev = pci_get_drvdata(pdev); + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; /* * We have a suspedn ordering issue with the snd-hda driver also @@ -959,8 +958,7 @@ static int i915_pm_suspend_late(struct device *dev) static int i915_pm_resume_early(struct device *dev) { - struct pci_dev *pdev = to_pci_dev(dev); - struct drm_device *drm_dev = pci_get_drvdata(pdev); + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) return 0; @@ -970,8 +968,7 @@ static int i915_pm_resume_early(struct device *dev) static int i915_pm_resume(struct device *dev) { - struct pci_dev *pdev = to_pci_dev(dev); - struct drm_device *drm_dev = pci_get_drvdata(pdev); + struct drm_device *drm_dev = dev_to_i915_priv(dev)->dev; if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF) return 0; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 61a88fa..3de7a55 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -775,6 +775,14 @@ static inline unsigned int intel_num_planes(struct intel_crtc *crtc) return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1; } +static inline struct drm_i915_private *dev_to_i915_priv(struct device *dev) +{ + struct pci_dev *pdev = to_pci_dev(dev); + struct drm_device *drm_dev = pci_get_drvdata(pdev); + + return drm_dev->dev_private; +} + /* intel_fifo_underrun.c */ bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv, enum pipe pipe, bool enable);
This will be needed by later patches, so factor it out. No functional change. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 15 ++++++--------- drivers/gpu/drm/i915/intel_drv.h | 8 ++++++++ 2 files changed, 14 insertions(+), 9 deletions(-)