| Message ID | 1464012490-30961-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 23 May 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Current intel_opregion_init is called during the driver registration > phase and intel_opregion_fini from the unregistration phase. Rename the > functions show that this is clear from their names. The phases tell us > what we expect the existing hw state to be, e.g. whether interrupts are > still enabled etc. Okay, for the naming per se, Reviewed-by: Jani Nikula <jani.nikula@intel.com> While not a problem in this patch, the whole init/cleanup of opregion is annoyingly asymmetric. You need to call both setup and init to make it work, but fini cleans up for both of them. So repeated init/fini pairs will fail. The setup also does some initialization that is only needed once (like INIT_WORK) so fini is not a complete counter-operation of setup+init either. BR, Jani. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.h | 4 ++-- > drivers/gpu/drm/i915/intel_opregion.c | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 363bd5884a56..07edaed9d5a2 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1376,7 +1376,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > > if (INTEL_INFO(dev_priv)->num_pipes) { > /* Must be done after probing outputs */ > - intel_opregion_init(dev_priv); > + intel_opregion_register(dev_priv); > acpi_video_register(); > } > > @@ -1395,7 +1395,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > i915_audio_component_cleanup(dev_priv); > intel_gpu_ips_teardown(); > acpi_video_unregister(); > - intel_opregion_fini(dev_priv); > + intel_opregion_unregister(dev_priv); > i915_teardown_sysfs(dev_priv->dev); > i915_gem_shrinker_cleanup(dev_priv); > } > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 7627bbec8e37..943d7b222fd4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -631,7 +631,7 @@ static int i915_drm_suspend(struct drm_device *dev) > intel_opregion_notify_adapter(dev_priv, opregion_target_state); > > intel_uncore_forcewake_reset(dev_priv, false); > - intel_opregion_fini(dev_priv); > + intel_opregion_unregister(dev_priv); > > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); > > @@ -794,7 +794,7 @@ static int i915_drm_resume(struct drm_device *dev) > /* Config may have changed between suspend and resume */ > drm_helper_hpd_irq_event(dev); > > - intel_opregion_init(dev_priv); > + intel_opregion_register(dev_priv); > > intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index caf7e45ae663..17fe272e9ef8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3609,8 +3609,8 @@ bool intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv, > /* intel_opregion.c */ > #ifdef CONFIG_ACPI > extern int intel_opregion_setup(struct drm_i915_private *dev_priv); > -extern void intel_opregion_init(struct drm_i915_private *dev_priv); > -extern void intel_opregion_fini(struct drm_i915_private *dev_priv); > +extern void intel_opregion_register(struct drm_i915_private *dev_priv); > +extern void intel_opregion_unregister(struct drm_i915_private *dev_priv); > extern void intel_opregion_asle_intr(struct drm_i915_private *dev_priv); > extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, > bool enable); > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > index f9cdec866e49..f6d8a21d2c49 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -778,7 +778,7 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv) > } while (++i < 8 && disp_id != 0); > } > > -void intel_opregion_init(struct drm_i915_private *dev_priv) > +void intel_opregion_register(struct drm_i915_private *dev_priv) > { > struct intel_opregion *opregion = &dev_priv->opregion; > > @@ -805,7 +805,7 @@ void intel_opregion_init(struct drm_i915_private *dev_priv) > } > } > > -void intel_opregion_fini(struct drm_i915_private *dev_priv) > +void intel_opregion_unregister(struct drm_i915_private *dev_priv) > { > struct intel_opregion *opregion = &dev_priv->opregion;
On Mon, May 23, 2016 at 05:42:49PM +0300, Jani Nikula wrote: > On Mon, 23 May 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Current intel_opregion_init is called during the driver registration > > phase and intel_opregion_fini from the unregistration phase. Rename the > > functions show that this is clear from their names. The phases tell us > > what we expect the existing hw state to be, e.g. whether interrupts are > > still enabled etc. > > Okay, for the naming per se, > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > While not a problem in this patch, the whole init/cleanup of opregion is > annoyingly asymmetric. You need to call both setup and init to make it > work, but fini cleans up for both of them. So repeated init/fini pairs > will fail. The setup also does some initialization that is only needed > once (like INIT_WORK) so fini is not a complete counter-operation of > setup+init either. Yeah, that's was kind of my starting goal to try and make it fit into the overarching init phases better. Looking at suspend/resume to try and understand why it didn't just unregister/register (or why it needed to do the acpi unregister at all) raised too many questions. -Chris
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 363bd5884a56..07edaed9d5a2 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1376,7 +1376,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) if (INTEL_INFO(dev_priv)->num_pipes) { /* Must be done after probing outputs */ - intel_opregion_init(dev_priv); + intel_opregion_register(dev_priv); acpi_video_register(); } @@ -1395,7 +1395,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) i915_audio_component_cleanup(dev_priv); intel_gpu_ips_teardown(); acpi_video_unregister(); - intel_opregion_fini(dev_priv); + intel_opregion_unregister(dev_priv); i915_teardown_sysfs(dev_priv->dev); i915_gem_shrinker_cleanup(dev_priv); } diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 7627bbec8e37..943d7b222fd4 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -631,7 +631,7 @@ static int i915_drm_suspend(struct drm_device *dev) intel_opregion_notify_adapter(dev_priv, opregion_target_state); intel_uncore_forcewake_reset(dev_priv, false); - intel_opregion_fini(dev_priv); + intel_opregion_unregister(dev_priv); intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); @@ -794,7 +794,7 @@ static int i915_drm_resume(struct drm_device *dev) /* Config may have changed between suspend and resume */ drm_helper_hpd_irq_event(dev); - intel_opregion_init(dev_priv); + intel_opregion_register(dev_priv); intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index caf7e45ae663..17fe272e9ef8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3609,8 +3609,8 @@ bool intel_bios_is_port_hpd_inverted(struct drm_i915_private *dev_priv, /* intel_opregion.c */ #ifdef CONFIG_ACPI extern int intel_opregion_setup(struct drm_i915_private *dev_priv); -extern void intel_opregion_init(struct drm_i915_private *dev_priv); -extern void intel_opregion_fini(struct drm_i915_private *dev_priv); +extern void intel_opregion_register(struct drm_i915_private *dev_priv); +extern void intel_opregion_unregister(struct drm_i915_private *dev_priv); extern void intel_opregion_asle_intr(struct drm_i915_private *dev_priv); extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable); diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index f9cdec866e49..f6d8a21d2c49 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -778,7 +778,7 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv) } while (++i < 8 && disp_id != 0); } -void intel_opregion_init(struct drm_i915_private *dev_priv) +void intel_opregion_register(struct drm_i915_private *dev_priv) { struct intel_opregion *opregion = &dev_priv->opregion; @@ -805,7 +805,7 @@ void intel_opregion_init(struct drm_i915_private *dev_priv) } } -void intel_opregion_fini(struct drm_i915_private *dev_priv) +void intel_opregion_unregister(struct drm_i915_private *dev_priv) { struct intel_opregion *opregion = &dev_priv->opregion;
Current intel_opregion_init is called during the driver registration phase and intel_opregion_fini from the unregistration phase. Rename the functions show that this is clear from their names. The phases tell us what we expect the existing hw state to be, e.g. whether interrupts are still enabled etc. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jani Nikula <jani.nikula@linux.intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 4 ++-- drivers/gpu/drm/i915/i915_drv.c | 4 ++-- drivers/gpu/drm/i915/i915_drv.h | 4 ++-- drivers/gpu/drm/i915/intel_opregion.c | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-)