On Thu, Dec 13, 2018 at 09:31:07AM +0530, Ramalingam C wrote:
> A generic component master is added to hold the i915 registration
> until 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 (i915
> registration) will be triggered with no wait.
>
> Similarly when a associated component is removed for some reasons,
> I915 will be unregistered through component master unbind.
>
> v2:
> i915_driver_unregister is added to the unbind of master.
> v3:
> In master_unbind i915_unregister->drm_atomic_helper_shutdown->
> component_unbind_all [Daniel]
>
> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 86 +++++++++++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> include/drm/i915_component.h | 11 ++++++
> 3 files changed, 92 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b310a897a4ad..b8a204072e60 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"
> @@ -1577,8 +1579,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
> @@ -1609,7 +1609,6 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> intel_power_domains_disable(dev_priv);
>
> intel_fbdev_unregister(dev_priv);
> - intel_audio_deinit(dev_priv);
>
> /*
> * After flushing the fbdev (incl. a late async config which will
> @@ -1694,6 +1693,48 @@ static void i915_driver_destroy(struct drm_i915_private *i915)
> pci_set_drvdata(pdev, NULL);
> }
>
> +static void i915_driver_load_tail(struct drm_i915_private *dev_priv)
> +{
> + i915_driver_register(dev_priv);
> +
> + DRM_INFO("load_tail: I915 driver registered\n");
> +}
> +
> +static void i915_driver_unload_head(struct drm_i915_private *dev_priv)
> +{
> + i915_driver_unregister(dev_priv);
> +
> + DRM_INFO("unload_head: I915 driver unregistered\n");
> +}
> +
> +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);
> +
> + i915_driver_unload_head(dev_priv);
> + drm_atomic_helper_shutdown(&dev_priv->drm);
> + component_unbind_all(dev, dev_priv->comp_master);
> +}
> +
> +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
> @@ -1720,9 +1761,22 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
> if (!i915_modparams.nuclear_pageflip && match_info->gen < 5)
> dev_priv->drm.driver_features &= ~DRIVER_ATOMIC;
>
> + 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;
> + }
> +
> ret = pci_enable_device(pdev);
> if (ret)
> - goto out_fini;
> + goto out_comp_master_clean;
>
> ret = i915_driver_init_early(dev_priv);
> if (ret < 0)
> @@ -1742,14 +1796,27 @@ 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);
> + 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;
> + }
> +
I think a FIXME here would be nice, all we need to do is have a
component_add_locked functions to handle the recursion. Logically there's
not issue really. Same with intel_audio_deinit() below.
Otoh, the component we expose to snd-hda isn't really related to anything
uapi related, it's just a very low-level interface for power domains and a
few other things. Nothing bad happens if we don't register/unregister that
together with all the uapi/kms/fbdev interfaces.
Either way: Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> + intel_audio_init(dev_priv);
>
> enable_rpm_wakeref_asserts(dev_priv);
>
> i915_welcome_messages(dev_priv);
>
> + DRM_INFO("I915 registration waits for req component(s). 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:
> @@ -1759,6 +1826,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);
> i915_driver_destroy(dev_priv);
> @@ -1772,13 +1841,14 @@ void i915_driver_unload(struct drm_device *dev)
>
> disable_rpm_wakeref_asserts(dev_priv);
>
> - i915_driver_unregister(dev_priv);
> + component_master_del(dev_priv->drm.dev, &i915_component_master_ops);
> + kfree(dev_priv->comp_master);
> +
> + intel_audio_deinit(dev_priv);
>
> if (i915_gem_suspend(dev_priv))
> DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>
> - drm_atomic_helper_shutdown(dev);
> -
> intel_gvt_cleanup(dev_priv);
>
> intel_modeset_cleanup(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e70707e79386..25dc3d7a1e3b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2002,6 +2002,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 fca22d463e1b..6f94ddd3f2c2 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -46,4 +46,15 @@ struct i915_audio_component {
> int aud_sample_rate[MAX_PORTS];
> };
>
> +/**
> + * struct i915_component_master - Used for communication between i915
> + * and any other drivers for the services
> + * of different feature.
> + */
> +struct i915_component_master {
> + /*
> + * Add here the interface details between I915 and interested modules.
> + */
> +};
> +
> #endif /* _I915_COMPONENT_H_ */
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
