Re: [PATCHv3 2/4] drm/tilcdc: Stop using struct drm_driver load() callback

[Jyri Sarha <jsarha@ti.com>]

On 11/03/16 20:15, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Wednesday 02 Nov 2016 17:57:50 Jyri Sarha wrote:
>> Stop using struct drm_driver load() and unload() callbacks. The
>> callbacks should not be used anymore. Instead of using load the
>> drm_device is allocated with drm_dev_alloc() and registered with
>> drm_dev_register() only after the driver is completely initialized.
>> The deinitialization is done directly either in component unbind
>> callback or in platform driver demove callback.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 124 ++++++++++++++++++--------------
>>  1 file changed, 70 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 5cf534c..11f3262 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> @@ -194,18 +194,22 @@ static int cpufreq_transition(struct notifier_block
>> *nb, * DRM operations:
>>   */
>>
>> -static int tilcdc_unload(struct drm_device *dev)
>> +static void tilcdc_fini(struct drm_device *dev)
>>  {
>>  	struct tilcdc_drm_private *priv = dev->dev_private;
>>
>> -	tilcdc_remove_external_encoders(dev);
>> +	drm_modeset_lock_crtc(priv->crtc, NULL);
>> +	tilcdc_crtc_disable(priv->crtc);
>> +	drm_modeset_unlock_crtc(priv->crtc);
>> +
>> +	drm_dev_unregister(dev);
>>
>> -	drm_fbdev_cma_fini(priv->fbdev);
>>  	drm_kms_helper_poll_fini(dev);
>> +	drm_fbdev_cma_fini(priv->fbdev);
> 
> Is there a need to reorder these ?

I am not sure actually. When the things did not work properly in the
beginning I just ordered unloading to happen strictly in the reverse
order compared to loading. I did not go back to check what changes were
actually needed when I got things working.

> 
>> +	drm_irq_uninstall(dev);
>>  	drm_mode_config_cleanup(dev);
>> -	drm_vblank_cleanup(dev);
>>
>> -	drm_irq_uninstall(dev);
>> +	tilcdc_remove_external_encoders(dev);
>>
>>  #ifdef CONFIG_CPU_FREQ
>>  	cpufreq_unregister_notifier(&priv->freq_transition,
>> @@ -225,28 +229,34 @@ static int tilcdc_unload(struct drm_device *dev)
>>
>>  	pm_runtime_disable(dev->dev);
>>
>> -	return 0;
>> +	drm_dev_unref(dev);
>>  }
>>
>> -static int tilcdc_load(struct drm_device *dev, unsigned long flags)
>> +static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
>>  {
>> -	struct platform_device *pdev = dev->platformdev;
>> -	struct device_node *node = pdev->dev.of_node;
>> +	struct drm_device *ddev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct device_node *node = dev->of_node;
>>  	struct tilcdc_drm_private *priv;
>>  	struct resource *res;
>>  	u32 bpp = 0;
>>  	int ret;
>>
>> -	priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>  	if (!priv) {
>> -		dev_err(dev->dev, "failed to allocate private data\n");
>> +		dev_err(dev, "failed to allocate private data\n");
>>  		return -ENOMEM;
>>  	}
>>
>> -	dev->dev_private = priv;
>> +	ddev = drm_dev_alloc(ddrv, dev);
> 
> As a follow-up patch you might want to move tilcdc_driver above this function 
> and use it directly to remove the ddrv argument.
> 

Ok, thanks.

> Apart from that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +	if (IS_ERR(ddev))
>> +		return PTR_ERR(ddev);
>> +
>> +	ddev->platformdev = pdev;
>> +	ddev->dev_private = priv;
> 
> [snip]
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel