[Intel-gfx] [RFC 2/3] drm/i915: component master at i915 driver load
Ramalingam C
ramalingam.c at intel.com
Thu Jul 12 09:00:15 UTC 2018
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 at 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
>>
More information about the Intel-gfx
mailing list