| Message ID | 1531318288-23380-3-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 11, 2018 at 07:41:27PM +0530, Ramalingam C wrote: > A generic component master is added to hold the i915 registration > untill all required kernel modules are up and active. > > This is achieved through following steps: > - moving the i915 driver registration to the component master's > bind call > - all required kernel modules will add one component each to > component_match of I915 component master. > > If no component is added to the I915 component master, due to CONFIG > selection or HW limitation, component master's bind call will be > triggered with no wait. > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 87 +++++++++++++++++++++++++++++++++++------ > drivers/gpu/drm/i915/i915_drv.h | 3 ++ > include/drm/i915_component.h | 16 ++++++++ > 3 files changed, 94 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 0db3c83cce29..598ab3afc131 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -39,12 +39,14 @@ > #include <linux/vgaarb.h> > #include <linux/vga_switcheroo.h> > #include <linux/vt.h> > +#include <linux/component.h> > #include <acpi/video.h> > > #include <drm/drmP.h> > #include <drm/drm_crtc_helper.h> > #include <drm/drm_atomic_helper.h> > #include <drm/i915_drm.h> > +#include <drm/i915_component.h> > > #include "i915_drv.h" > #include "i915_trace.h" > @@ -1247,8 +1249,6 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > if (IS_GEN5(dev_priv)) > intel_gpu_ips_init(dev_priv); > > - intel_audio_init(dev_priv); > - > /* > * Some ports require correctly set-up hpd registers for detection to > * work properly (leading to ghost connected connector status), e.g. VGA > @@ -1310,6 +1310,47 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv) > DRM_INFO("DRM_I915_DEBUG_GEM enabled\n"); > } > > +static void i915_driver_load_tail(struct drm_i915_private *dev_priv) > +{ > + i915_driver_register(dev_priv); > + > + intel_runtime_pm_enable(dev_priv); > + > + intel_init_ipc(dev_priv); > + > + intel_runtime_pm_put(dev_priv); > + > + i915_welcome_messages(dev_priv); > +} > + > +static int i915_component_master_bind(struct device *dev) > +{ > + struct drm_i915_private *dev_priv = kdev_to_i915(dev); > + int ret; > + > + ret = component_bind_all(dev, dev_priv->comp_master); > + if (ret < 0) > + return ret; > + > + i915_driver_load_tail(dev_priv); > + > + return 0; > +} > + > +static void i915_component_master_unbind(struct device *dev) > +{ > + struct drm_i915_private *dev_priv = kdev_to_i915(dev); > + > + component_unbind_all(dev, dev_priv->comp_master); > + > + /* TO-DO: Whether I915 should be unloaded on any component unbind!? */ I think we also need a FIXME here because the audio side will blow up if we do this? If that's not the case then yes I think we should do the i915_dev_unregister call from here, for symmetry. That way we should be able to unload mei, and it should force the i915 driver out too. This also depends upon how the entire mei reset story pans out ofc. Aside from this tiny nit I think the load side here looks good now. -Daniel > +} > + > +static const struct component_master_ops i915_component_master_ops = { > + .bind = i915_component_master_bind, > + .unbind = i915_component_master_unbind, > +}; > + > /** > * i915_driver_load - setup chip and create an initial config > * @pdev: PCI device > @@ -1341,12 +1382,25 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) > goto out_free; > } > > + dev_priv->comp_master = kzalloc(sizeof(*dev_priv->comp_master), > + GFP_KERNEL); > + if (!dev_priv->comp_master) { > + ret = -ENOMEM; > + goto out_fini; > + } > + > + component_match_alloc(dev_priv->drm.dev, &dev_priv->master_match); > + if (!dev_priv->master_match) { > + ret = -ENOMEM; > + goto out_comp_master_clean; > + } > + > dev_priv->drm.pdev = pdev; > dev_priv->drm.dev_private = dev_priv; > > ret = pci_enable_device(pdev); > if (ret) > - goto out_fini; > + goto out_comp_master_clean; > > pci_set_drvdata(pdev, &dev_priv->drm); > /* > @@ -1389,18 +1443,21 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) > if (ret < 0) > goto out_cleanup_hw; > > - i915_driver_register(dev_priv); > - > - intel_runtime_pm_enable(dev_priv); > - > - intel_init_ipc(dev_priv); > - > - intel_runtime_pm_put(dev_priv); > - > - i915_welcome_messages(dev_priv); > + ret = component_master_add_with_match(dev_priv->drm.dev, > + &i915_component_master_ops, > + dev_priv->master_match); > + if (ret < 0) { > + DRM_DEV_ERROR(&pdev->dev, "Master comp add failed %d\n", > + ret); > + goto out_cleanup_modeset; > + } > + intel_audio_init(dev_priv); I think a FIXME comment explaining what's going on here would be good: /* * FIXME: component.c doesn't (yet) support recursion, and snd-hda * doesn't handle delayed component registration correctly. For * now just uncoditionally register the audio component here, * outside of the master->bind callback. */ Hopefully we can untangle this later on, but for now I think this approach is good. > + DRM_DEV_INFO(&pdev->dev, "I915 waits for Components (If any).\n"); > > return 0; > > +out_cleanup_modeset: > + intel_modeset_cleanup(&dev_priv->drm); > out_cleanup_hw: > i915_driver_cleanup_hw(dev_priv); > out_cleanup_mmio: > @@ -1410,6 +1467,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) > i915_driver_cleanup_early(dev_priv); > out_pci_disable: > pci_disable_device(pdev); > +out_comp_master_clean: > + kfree(dev_priv->comp_master); > out_fini: > i915_load_error(dev_priv, "Device initialization failed (%d)\n", ret); > drm_dev_fini(&dev_priv->drm); > @@ -1428,6 +1487,10 @@ void i915_driver_unload(struct drm_device *dev) > if (i915_gem_suspend(dev_priv)) > DRM_ERROR("failed to idle hardware; continuing to unload!\n"); > > + component_master_del(dev_priv->drm.dev, > + &i915_component_master_ops); > + kfree(dev_priv->comp_master); > + > intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); > > drm_atomic_helper_shutdown(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 09ab12458244..620f41e13dbe 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2119,6 +2119,9 @@ struct drm_i915_private { > > struct i915_pmu pmu; > > + struct i915_component_master *comp_master; > + struct component_match *master_match; > + > /* > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch > * will be rejected. Instead look for a better place. > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h > index 346b1f5cb180..52313bc227b2 100644 > --- a/include/drm/i915_component.h > +++ b/include/drm/i915_component.h > @@ -121,4 +121,20 @@ struct i915_audio_component { > const struct i915_audio_component_audio_ops *audio_ops; > }; > > +/** > + * struct i915_component_master - Used for communication between i915 > + * and any other drivers for the services of different feature. > + */ > +struct i915_component_master { > + /** > + * @i915_kdev: Kdev of I915. Used from the client component for > + * removing the reference to mei_cldev. > + */ > + struct device *i915_kdev; > + > + /* > + * Add here the interface details between I915 and interested modules. > + */ > +}; > + > #endif /* _I915_COMPONENT_H_ */ > -- > 2.7.4 >
On Thursday 12 July 2018 01:56 PM, Daniel Vetter wrote: > On Wed, Jul 11, 2018 at 07:41:27PM +0530, Ramalingam C wrote: >> A generic component master is added to hold the i915 registration >> untill all required kernel modules are up and active. >> >> This is achieved through following steps: >> - moving the i915 driver registration to the component master's >> bind call >> - all required kernel modules will add one component each to >> component_match of I915 component master. >> >> If no component is added to the I915 component master, due to CONFIG >> selection or HW limitation, component master's bind call will be >> triggered with no wait. >> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 87 +++++++++++++++++++++++++++++++++++------ >> drivers/gpu/drm/i915/i915_drv.h | 3 ++ >> include/drm/i915_component.h | 16 ++++++++ >> 3 files changed, 94 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 0db3c83cce29..598ab3afc131 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -39,12 +39,14 @@ >> #include <linux/vgaarb.h> >> #include <linux/vga_switcheroo.h> >> #include <linux/vt.h> >> +#include <linux/component.h> >> #include <acpi/video.h> >> >> #include <drm/drmP.h> >> #include <drm/drm_crtc_helper.h> >> #include <drm/drm_atomic_helper.h> >> #include <drm/i915_drm.h> >> +#include <drm/i915_component.h> >> >> #include "i915_drv.h" >> #include "i915_trace.h" >> @@ -1247,8 +1249,6 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) >> if (IS_GEN5(dev_priv)) >> intel_gpu_ips_init(dev_priv); >> >> - intel_audio_init(dev_priv); >> - >> /* >> * Some ports require correctly set-up hpd registers for detection to >> * work properly (leading to ghost connected connector status), e.g. VGA >> @@ -1310,6 +1310,47 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv) >> DRM_INFO("DRM_I915_DEBUG_GEM enabled\n"); >> } >> >> +static void i915_driver_load_tail(struct drm_i915_private *dev_priv) >> +{ >> + i915_driver_register(dev_priv); >> + >> + intel_runtime_pm_enable(dev_priv); >> + >> + intel_init_ipc(dev_priv); >> + >> + intel_runtime_pm_put(dev_priv); >> + >> + i915_welcome_messages(dev_priv); >> +} >> + >> +static int i915_component_master_bind(struct device *dev) >> +{ >> + struct drm_i915_private *dev_priv = kdev_to_i915(dev); >> + int ret; >> + >> + ret = component_bind_all(dev, dev_priv->comp_master); >> + if (ret < 0) >> + return ret; >> + >> + i915_driver_load_tail(dev_priv); >> + >> + return 0; >> +} >> + >> +static void i915_component_master_unbind(struct device *dev) >> +{ >> + struct drm_i915_private *dev_priv = kdev_to_i915(dev); >> + >> + component_unbind_all(dev, dev_priv->comp_master); >> + >> + /* TO-DO: Whether I915 should be unloaded on any component unbind!? */ > I think we also need a FIXME here because the audio side will blow up if > we do this? With audio_init is moved out of bind i dont see the audio failure message. May be we should get the hdac tested with these changes along with INTEL_MEI_HDCP as y/n/m. > > If that's not the case then yes I think we should do the > i915_dev_unregister call from here, for symmetry. That way we should be > able to unload mei, and it should force the i915 driver out too. > > This also depends upon how the entire mei reset story pans out ofc. I will start a thread straight away to understand the freq of ME reset and duration of it. > > Aside from this tiny nit I think the load side here looks good now. > -Daniel > > >> +} >> + >> +static const struct component_master_ops i915_component_master_ops = { >> + .bind = i915_component_master_bind, >> + .unbind = i915_component_master_unbind, >> +}; >> + >> /** >> * i915_driver_load - setup chip and create an initial config >> * @pdev: PCI device >> @@ -1341,12 +1382,25 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) >> goto out_free; >> } >> >> + dev_priv->comp_master = kzalloc(sizeof(*dev_priv->comp_master), >> + GFP_KERNEL); >> + if (!dev_priv->comp_master) { >> + ret = -ENOMEM; >> + goto out_fini; >> + } >> + >> + component_match_alloc(dev_priv->drm.dev, &dev_priv->master_match); >> + if (!dev_priv->master_match) { >> + ret = -ENOMEM; >> + goto out_comp_master_clean; >> + } >> + >> dev_priv->drm.pdev = pdev; >> dev_priv->drm.dev_private = dev_priv; >> >> ret = pci_enable_device(pdev); >> if (ret) >> - goto out_fini; >> + goto out_comp_master_clean; >> >> pci_set_drvdata(pdev, &dev_priv->drm); >> /* >> @@ -1389,18 +1443,21 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) >> if (ret < 0) >> goto out_cleanup_hw; >> >> - i915_driver_register(dev_priv); >> - >> - intel_runtime_pm_enable(dev_priv); >> - >> - intel_init_ipc(dev_priv); >> - >> - intel_runtime_pm_put(dev_priv); >> - >> - i915_welcome_messages(dev_priv); >> + ret = component_master_add_with_match(dev_priv->drm.dev, >> + &i915_component_master_ops, >> + dev_priv->master_match); >> + if (ret < 0) { >> + DRM_DEV_ERROR(&pdev->dev, "Master comp add failed %d\n", >> + ret); >> + goto out_cleanup_modeset; >> + } >> + intel_audio_init(dev_priv); > I think a FIXME comment explaining what's going on here would be good: > > /* > * FIXME: component.c doesn't (yet) support recursion, and snd-hda > * doesn't handle delayed component registration correctly. For > * now just uncoditionally register the audio component here, > * outside of the master->bind callback. > */ > > Hopefully we can untangle this later on, but for now I think this approach > is good. >> + DRM_DEV_INFO(&pdev->dev, "I915 waits for Components (If any).\n"); >> >> return 0; >> >> +out_cleanup_modeset: >> + intel_modeset_cleanup(&dev_priv->drm); >> out_cleanup_hw: >> i915_driver_cleanup_hw(dev_priv); >> out_cleanup_mmio: >> @@ -1410,6 +1467,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) >> i915_driver_cleanup_early(dev_priv); >> out_pci_disable: >> pci_disable_device(pdev); >> +out_comp_master_clean: >> + kfree(dev_priv->comp_master); >> out_fini: >> i915_load_error(dev_priv, "Device initialization failed (%d)\n", ret); >> drm_dev_fini(&dev_priv->drm); >> @@ -1428,6 +1487,10 @@ void i915_driver_unload(struct drm_device *dev) >> if (i915_gem_suspend(dev_priv)) >> DRM_ERROR("failed to idle hardware; continuing to unload!\n"); >> >> + component_master_del(dev_priv->drm.dev, >> + &i915_component_master_ops); >> + kfree(dev_priv->comp_master); >> + >> intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); >> >> drm_atomic_helper_shutdown(dev); >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 09ab12458244..620f41e13dbe 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2119,6 +2119,9 @@ struct drm_i915_private { >> >> struct i915_pmu pmu; >> >> + struct i915_component_master *comp_master; >> + struct component_match *master_match; >> + >> /* >> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch >> * will be rejected. Instead look for a better place. >> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h >> index 346b1f5cb180..52313bc227b2 100644 >> --- a/include/drm/i915_component.h >> +++ b/include/drm/i915_component.h >> @@ -121,4 +121,20 @@ struct i915_audio_component { >> const struct i915_audio_component_audio_ops *audio_ops; >> }; >> >> +/** >> + * struct i915_component_master - Used for communication between i915 >> + * and any other drivers for the services of different feature. >> + */ >> +struct i915_component_master { >> + /** >> + * @i915_kdev: Kdev of I915. Used from the client component for >> + * removing the reference to mei_cldev. >> + */ >> + struct device *i915_kdev; >> + >> + /* >> + * Add here the interface details between I915 and interested modules. >> + */ >> +}; >> + >> #endif /* _I915_COMPONENT_H_ */ >> -- >> 2.7.4 >>
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 0db3c83cce29..598ab3afc131 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -39,12 +39,14 @@ #include <linux/vgaarb.h> #include <linux/vga_switcheroo.h> #include <linux/vt.h> +#include <linux/component.h> #include <acpi/video.h> #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> #include <drm/i915_drm.h> +#include <drm/i915_component.h> #include "i915_drv.h" #include "i915_trace.h" @@ -1247,8 +1249,6 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) if (IS_GEN5(dev_priv)) intel_gpu_ips_init(dev_priv); - intel_audio_init(dev_priv); - /* * Some ports require correctly set-up hpd registers for detection to * work properly (leading to ghost connected connector status), e.g. VGA @@ -1310,6 +1310,47 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv) DRM_INFO("DRM_I915_DEBUG_GEM enabled\n"); } +static void i915_driver_load_tail(struct drm_i915_private *dev_priv) +{ + i915_driver_register(dev_priv); + + intel_runtime_pm_enable(dev_priv); + + intel_init_ipc(dev_priv); + + intel_runtime_pm_put(dev_priv); + + i915_welcome_messages(dev_priv); +} + +static int i915_component_master_bind(struct device *dev) +{ + struct drm_i915_private *dev_priv = kdev_to_i915(dev); + int ret; + + ret = component_bind_all(dev, dev_priv->comp_master); + if (ret < 0) + return ret; + + i915_driver_load_tail(dev_priv); + + return 0; +} + +static void i915_component_master_unbind(struct device *dev) +{ + struct drm_i915_private *dev_priv = kdev_to_i915(dev); + + component_unbind_all(dev, dev_priv->comp_master); + + /* TO-DO: Whether I915 should be unloaded on any component unbind!? */ +} + +static const struct component_master_ops i915_component_master_ops = { + .bind = i915_component_master_bind, + .unbind = i915_component_master_unbind, +}; + /** * i915_driver_load - setup chip and create an initial config * @pdev: PCI device @@ -1341,12 +1382,25 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) goto out_free; } + dev_priv->comp_master = kzalloc(sizeof(*dev_priv->comp_master), + GFP_KERNEL); + if (!dev_priv->comp_master) { + ret = -ENOMEM; + goto out_fini; + } + + component_match_alloc(dev_priv->drm.dev, &dev_priv->master_match); + if (!dev_priv->master_match) { + ret = -ENOMEM; + goto out_comp_master_clean; + } + dev_priv->drm.pdev = pdev; dev_priv->drm.dev_private = dev_priv; ret = pci_enable_device(pdev); if (ret) - goto out_fini; + goto out_comp_master_clean; pci_set_drvdata(pdev, &dev_priv->drm); /* @@ -1389,18 +1443,21 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret < 0) goto out_cleanup_hw; - i915_driver_register(dev_priv); - - intel_runtime_pm_enable(dev_priv); - - intel_init_ipc(dev_priv); - - intel_runtime_pm_put(dev_priv); - - i915_welcome_messages(dev_priv); + ret = component_master_add_with_match(dev_priv->drm.dev, + &i915_component_master_ops, + dev_priv->master_match); + if (ret < 0) { + DRM_DEV_ERROR(&pdev->dev, "Master comp add failed %d\n", + ret); + goto out_cleanup_modeset; + } + intel_audio_init(dev_priv); + DRM_DEV_INFO(&pdev->dev, "I915 waits for Components (If any).\n"); return 0; +out_cleanup_modeset: + intel_modeset_cleanup(&dev_priv->drm); out_cleanup_hw: i915_driver_cleanup_hw(dev_priv); out_cleanup_mmio: @@ -1410,6 +1467,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) i915_driver_cleanup_early(dev_priv); out_pci_disable: pci_disable_device(pdev); +out_comp_master_clean: + kfree(dev_priv->comp_master); out_fini: i915_load_error(dev_priv, "Device initialization failed (%d)\n", ret); drm_dev_fini(&dev_priv->drm); @@ -1428,6 +1487,10 @@ void i915_driver_unload(struct drm_device *dev) if (i915_gem_suspend(dev_priv)) DRM_ERROR("failed to idle hardware; continuing to unload!\n"); + component_master_del(dev_priv->drm.dev, + &i915_component_master_ops); + kfree(dev_priv->comp_master); + intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); drm_atomic_helper_shutdown(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 09ab12458244..620f41e13dbe 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2119,6 +2119,9 @@ struct drm_i915_private { struct i915_pmu pmu; + struct i915_component_master *comp_master; + struct component_match *master_match; + /* * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch * will be rejected. Instead look for a better place. diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 346b1f5cb180..52313bc227b2 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -121,4 +121,20 @@ struct i915_audio_component { const struct i915_audio_component_audio_ops *audio_ops; }; +/** + * struct i915_component_master - Used for communication between i915 + * and any other drivers for the services of different feature. + */ +struct i915_component_master { + /** + * @i915_kdev: Kdev of I915. Used from the client component for + * removing the reference to mei_cldev. + */ + struct device *i915_kdev; + + /* + * Add here the interface details between I915 and interested modules. + */ +}; + #endif /* _I915_COMPONENT_H_ */
A generic component master is added to hold the i915 registration untill all required kernel modules are up and active. This is achieved through following steps: - moving the i915 driver registration to the component master's bind call - all required kernel modules will add one component each to component_match of I915 component master. If no component is added to the I915 component master, due to CONFIG selection or HW limitation, component master's bind call will be triggered with no wait. Signed-off-by: Ramalingam C <ramalingam.c@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 87 +++++++++++++++++++++++++++++++++++------ drivers/gpu/drm/i915/i915_drv.h | 3 ++ include/drm/i915_component.h | 16 ++++++++ 3 files changed, 94 insertions(+), 12 deletions(-)