[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