Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Jose Abreu]

HI Sylwester,


On 19-06-2017 23:10, Sylwester Nawrocki wrote:
> On 06/19/2017 11:33 AM, Jose Abreu wrote:
>> On 18-06-2017 19:04, Sylwester Nawrocki wrote:
>>> On 06/16/2017 06:38 PM, Jose Abreu wrote:
 This is an initial submission for the Synopsys Designware HDMI RX
 Controller Driver. This driver interacts with a phy driver so that
 a communication between them is created and a video pipeline is
 configured.

 The controller + phy pipeline can then be integrated into a fully
 featured system that can be able to receive video up to 4k@60Hz
 with deep color 48bit RGB, depending on the platform. Although,
 this initial version does not yet handle deep color modes.
 Signed-off-by: Jose Abreu 

 +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
 +{
 +  request_module(pdevinfo.name);
 +
 +  dw_dev->phy_pdev = platform_device_register_full();
 +  if (IS_ERR(dw_dev->phy_pdev)) {
 +  dev_err(dw_dev->dev, "failed to register phy device\n");
 +  return PTR_ERR(dw_dev->phy_pdev);
 +  }
 +
 +  if (!dw_dev->phy_pdev->dev.driver) {
 +  dev_err(dw_dev->dev, "failed to initialize phy driver\n");
 +  goto err;
 +  }
>>> I think this is not safe because there is nothing preventing unbinding
>>> or unloading the driver at this point.
>>>
 +  if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
>>> So dw_dev->phy_pdev->dev.driver may be already NULL here.
>> How can I make sure it wont be NULL? Because I've seen other
>> media drivers do this and I don't think they do any kind of
>> locking, but they do this mainly for I2C subdevs.
> You could do device_lock(dev)/device_unlock(dev) to avoid possible races. 
> And setting 'suppress_bind_attrs' field in the sub-device drivers would 
> disable sysfs unbind attributes, so sub-device driver wouldn't get unbound
> unexpectedly trough sysfs.

Hmm, ok. I changed this, now I'm using driver_find() +
driver_for_each_device(). Its working but I'm starting to think
about whether this should go into v4l2 core because I've seen
other drivers also do this.

>  
 +  dev_err(dw_dev->dev, "failed to get phy module\n");
 +  goto err;
 +  }
 +
 +  dw_dev->phy_sd = dev_get_drvdata(_dev->phy_pdev->dev);
 +  if (!dw_dev->phy_sd) {
 +  dev_err(dw_dev->dev, "failed to get phy subdev\n");
 +  goto err_put;
 +  }
 +
 +  if (v4l2_device_register_subdev(_dev->v4l2_dev, dw_dev->phy_sd)) {
 +  dev_err(dw_dev->dev, "failed to register phy subdev\n");
 +  goto err_put;
 +  }
>>> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
>>> registration.  And with recent change [1] you could handle this PHY subdev
>>> in a standard way.  That might be more complicated than it is now but should
>>> make any future platform integration easier.
>> So I will instantiate phy driver and then wait for phy driver to
>> register into v4l2 core?
> Yes, for instance the RX controller driver registers a notifier, instantiates
> the child PHY device and then waits until the PHY driver completes 
> initialization.
>
 +  module_put(dw_dev->phy_pdev->dev.driver->owner);
 +  return 0;
 +
 +err_put:
 +  module_put(dw_dev->phy_pdev->dev.driver->owner);
 +err:
 +  platform_device_unregister(dw_dev->phy_pdev);
 +  return -EINVAL;
 +}
 +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
 +{
 +  struct dw_hdmi_work_data *data;
 +  unsigned long flags;
 +
 +  data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);
>>> Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
>>> in the device's probe() callback, but in other places, including interrupt
>>> handler?  devm_* API is normally used when life time of a resource is more
>>> or less equal to life time of struct device or its matched driver.  Were
>>> there any specific reasons to not just use kzalloc()/kfree() ?
>> No specific reason, I just thought it would be safer because if I
>> cancel a work before it started then memory will remain
>> allocated. But I will change to kzalloc().
> OK, I overlooked such situation. Since you allow one job queued maybe
> just embed struct work_struct in struct dw_hdmi_dev and retrieve it with
> container_of() macro in the work handler and use additional field in
> struct dw_hdmi_dev protected with dw_dev->lock for passing the input 
> index?

Yes, seems ok. As I'm already locking before queuing work and
also checking if theres is a pending work I can just overwrite
configured_input earlier.

Best regards,
Jose Miguel Abreu


>
 +  if (!data)
 +  return -ENOMEM;
 +
 +  INIT_WORK(>work, dw_hdmi_work_handler);
 +  data->dw_dev = dw_dev;
 +  data->input = input;
 +
 +  spin_lock_irqsave(_dev->lock, flags);
 +  if 

Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Sylwester Nawrocki]

On 06/19/2017 11:33 AM, Jose Abreu wrote:
> On 18-06-2017 19:04, Sylwester Nawrocki wrote:
>> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>>> This is an initial submission for the Synopsys Designware HDMI RX
>>> Controller Driver. This driver interacts with a phy driver so that
>>> a communication between them is created and a video pipeline is
>>> configured.
>>>
>>> The controller + phy pipeline can then be integrated into a fully
>>> featured system that can be able to receive video up to 4k@60Hz
>>> with deep color 48bit RGB, depending on the platform. Although,
>>> this initial version does not yet handle deep color modes.
>>> Signed-off-by: Jose Abreu 
>>>
>>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>>> +{

>>> +   request_module(pdevinfo.name);
>>> +
>>> +   dw_dev->phy_pdev = platform_device_register_full();
>>> +   if (IS_ERR(dw_dev->phy_pdev)) {
>>> +   dev_err(dw_dev->dev, "failed to register phy device\n");
>>> +   return PTR_ERR(dw_dev->phy_pdev);
>>> +   }
>>> +
>>> +   if (!dw_dev->phy_pdev->dev.driver) {
>>> +   dev_err(dw_dev->dev, "failed to initialize phy driver\n");
>>> +   goto err;
>>> +   }
>> I think this is not safe because there is nothing preventing unbinding
>> or unloading the driver at this point.
>>
>>> +   if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
>> So dw_dev->phy_pdev->dev.driver may be already NULL here.
> 
> How can I make sure it wont be NULL? Because I've seen other
> media drivers do this and I don't think they do any kind of
> locking, but they do this mainly for I2C subdevs.

You could do device_lock(dev)/device_unlock(dev) to avoid possible races. 
And setting 'suppress_bind_attrs' field in the sub-device drivers would 
disable sysfs unbind attributes, so sub-device driver wouldn't get unbound
unexpectedly trough sysfs.
 
>>> +   dev_err(dw_dev->dev, "failed to get phy module\n");
>>> +   goto err;
>>> +   }
>>> +
>>> +   dw_dev->phy_sd = dev_get_drvdata(_dev->phy_pdev->dev);
>>> +   if (!dw_dev->phy_sd) {
>>> +   dev_err(dw_dev->dev, "failed to get phy subdev\n");
>>> +   goto err_put;
>>> +   }
>>> +
>>> +   if (v4l2_device_register_subdev(_dev->v4l2_dev, dw_dev->phy_sd)) {
>>> +   dev_err(dw_dev->dev, "failed to register phy subdev\n");
>>> +   goto err_put;
>>> +   }
>>
>> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
>> registration.  And with recent change [1] you could handle this PHY subdev
>> in a standard way.  That might be more complicated than it is now but should
>> make any future platform integration easier.

> So I will instantiate phy driver and then wait for phy driver to
> register into v4l2 core?

Yes, for instance the RX controller driver registers a notifier, instantiates
the child PHY device and then waits until the PHY driver completes 
initialization.

>>> +   module_put(dw_dev->phy_pdev->dev.driver->owner);
>>> +   return 0;
>>> +
>>> +err_put:
>>> +   module_put(dw_dev->phy_pdev->dev.driver->owner);
>>> +err:
>>> +   platform_device_unregister(dw_dev->phy_pdev);
>>> +   return -EINVAL;
>>> +}

>>> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>>> +{
>>> +   struct dw_hdmi_work_data *data;
>>> +   unsigned long flags;
>>> +
>>> +   data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);
>>
>> Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
>> in the device's probe() callback, but in other places, including interrupt
>> handler?  devm_* API is normally used when life time of a resource is more
>> or less equal to life time of struct device or its matched driver.  Were
>> there any specific reasons to not just use kzalloc()/kfree() ?
> 
> No specific reason, I just thought it would be safer because if I
> cancel a work before it started then memory will remain
> allocated. But I will change to kzalloc().

OK, I overlooked such situation. Since you allow one job queued maybe
just embed struct work_struct in struct dw_hdmi_dev and retrieve it with
container_of() macro in the work handler and use additional field in
struct dw_hdmi_dev protected with dw_dev->lock for passing the input 
index?

>>> +   if (!data)
>>> +   return -ENOMEM;
>>> +
>>> +   INIT_WORK(>work, dw_hdmi_work_handler);
>>> +   data->dw_dev = dw_dev;
>>> +   data->input = input;
>>> +
>>> +   spin_lock_irqsave(_dev->lock, flags);
>>> +   if (dw_dev->pending_config) {
>>> +   devm_kfree(dw_dev->dev, data);
>>> +   spin_unlock_irqrestore(_dev->lock, flags);
>>> +   return 0;
>>> +   }
>>> +
>>> +   queue_work(dw_dev->wq, >work);
>>> +   dw_dev->pending_config = true;
>>> +   spin_unlock_irqrestore(_dev->lock, flags);
>>> +   return 0;
>>> +}

>>> +struct dw_hdmi_rx_pdata {
>>> +   /* Controller configuration */

>>> +   struct dw_hdmi_hdcp14_key hdcp14_keys;
>>> +   /* 5V sense interface */
>>> +   bool (*dw_5v_status)(void __iomem 

Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Jose Abreu]

Hi Sylwester,


Thanks again for the feedback!


On 18-06-2017 19:04, Sylwester Nawrocki wrote:
> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>> This is an initial submission for the Synopsys Designware HDMI RX
>> Controller Driver. This driver interacts with a phy driver so that
>> a communication between them is created and a video pipeline is
>> configured.
>>
>> The controller + phy pipeline can then be integrated into a fully
>> featured system that can be able to receive video up to 4k@60Hz
>> with deep color 48bit RGB, depending on the platform. Although,
>> this initial version does not yet handle deep color modes.
>> Signed-off-by: Jose Abreu 
>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>> +{
>> +struct dw_phy_pdata *phy = _dev->phy_config;
>> +struct platform_device_info pdevinfo;
>> +
>> +memset(, 0, sizeof(pdevinfo));
>> +
>> +phy->funcs = _hdmi_phy_funcs;
>> +phy->funcs_arg = dw_dev;
>> +
>> +pdevinfo.parent = dw_dev->dev;
>> +pdevinfo.id = PLATFORM_DEVID_NONE;
>> +pdevinfo.name = dw_dev->phy_drv;
>> +pdevinfo.data = phy;
>> +pdevinfo.size_data = sizeof(*phy);
>> +pdevinfo.dma_mask = DMA_BIT_MASK(32);
>> +
>> +request_module(pdevinfo.name);
>> +
>> +dw_dev->phy_pdev = platform_device_register_full();
>> +if (IS_ERR(dw_dev->phy_pdev)) {
>> +dev_err(dw_dev->dev, "failed to register phy device\n");
>> +return PTR_ERR(dw_dev->phy_pdev);
>> +}
>> +
>> +if (!dw_dev->phy_pdev->dev.driver) {
>> +dev_err(dw_dev->dev, "failed to initialize phy driver\n");
>> +goto err;
>> +}
> I think this is not safe because there is nothing preventing unbinding 
> or unloading the driver at this point.
>
>> +if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
> So dw_dev->phy_pdev->dev.driver may be already NULL here.

How can I make sure it wont be NULL? Because I've seen other
media drivers do this and I don't think they do any kind of
locking, but they do this mainly for I2C subdevs.

>
>> +dev_err(dw_dev->dev, "failed to get phy module\n");
>> +goto err;
>> +}
>> +
>> +dw_dev->phy_sd = dev_get_drvdata(_dev->phy_pdev->dev);
>> +if (!dw_dev->phy_sd) {
>> +dev_err(dw_dev->dev, "failed to get phy subdev\n");
>> +goto err_put;
>> +}
>> +
>> +if (v4l2_device_register_subdev(_dev->v4l2_dev, dw_dev->phy_sd)) {
>> +dev_err(dw_dev->dev, "failed to register phy subdev\n");
>> +goto err_put;
>> +}
> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
> registration.  And with recent change [1] you could handle this PHY subdev
> in a standard way.  That might be more complicated than it is now but should 
> make any future platform integration easier.
>
> [1] 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.org_patch_41834=DwICaQ=DPL6_X_6JkXFx7AXWqB0tg=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw=uHTyp6WsEj6vN19Zc09HqSNUhCx62OI8u-tAgi-EVts=WjyPjIwN1uGvPoV7ZlcmzOgdptakzluHywuKRA8ZG8M=
>  

So I will instantiate phy driver and then wait for phy driver to
register into v4l2 core?

>
>> +module_put(dw_dev->phy_pdev->dev.driver->owner);
>> +return 0;
>> +
>> +err_put:
>> +module_put(dw_dev->phy_pdev->dev.driver->owner);
>> +err:
>> +platform_device_unregister(dw_dev->phy_pdev);
>> +return -EINVAL;
>> +}
>> +
>> +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
>> +{
>> +if (!IS_ERR(dw_dev->phy_pdev))
>> +platform_device_unregister(dw_dev->phy_pdev);
>> +}
>> +static int dw_hdmi_config_hdcp(struct dw_hdmi_dev *dw_dev)
>> +{
>> +for (i = 0; i < DW_HDMI_HDCP14_KEYS_SIZE; i += 2) {
>> +for (j = 0; j < key_write_tries; j++) {
>> +if (is_hdcp14_key_write_allowed(dw_dev))
>> +break;
>> +mdelay(10);
> usleep_range()? I've seen more (busy waiting) mdelay() calls in this
> patch series.

I will change.

>
>
>> +static int __dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>> +{
>> +unsigned long flags;
>> +int ret;
>> +
>> +ret = dw_hdmi_config(dw_dev, input);
>> +
>> +spin_lock_irqsave(_dev->lock, flags);
>> +dw_dev->pending_config = false;
>> +spin_unlock_irqrestore(_dev->lock, flags);
>> +
>> +return ret;
>> +}
>> +
>> +struct dw_hdmi_work_data {
>> +struct dw_hdmi_dev *dw_dev;
>> +struct work_struct work;
>> +u32 input;
>> +};
>> +
>> +static void dw_hdmi_work_handler(struct work_struct *work)
>> +{
>> +struct dw_hdmi_work_data *data = container_of(work,
>> +struct dw_hdmi_work_data, work);
>> +
>> +__dw_hdmi_power_on(data->dw_dev, data->input);
>> +devm_kfree(data->dw_dev->dev, data);
>> +}
>> +
>> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>> +{
>> +struct dw_hdmi_work_data *data;
>> +unsigned 

Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Sylwester Nawrocki]

On 06/16/2017 06:38 PM, Jose Abreu wrote:
> This is an initial submission for the Synopsys Designware HDMI RX
> Controller Driver. This driver interacts with a phy driver so that
> a communication between them is created and a video pipeline is
> configured.
> 
> The controller + phy pipeline can then be integrated into a fully
> featured system that can be able to receive video up to 4k@60Hz
> with deep color 48bit RGB, depending on the platform. Although,
> this initial version does not yet handle deep color modes.

> Signed-off-by: Jose Abreu 

> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
> +{
> + struct dw_phy_pdata *phy = _dev->phy_config;
> + struct platform_device_info pdevinfo;
> +
> + memset(, 0, sizeof(pdevinfo));
> +
> + phy->funcs = _hdmi_phy_funcs;
> + phy->funcs_arg = dw_dev;
> +
> + pdevinfo.parent = dw_dev->dev;
> + pdevinfo.id = PLATFORM_DEVID_NONE;
> + pdevinfo.name = dw_dev->phy_drv;
> + pdevinfo.data = phy;
> + pdevinfo.size_data = sizeof(*phy);
> + pdevinfo.dma_mask = DMA_BIT_MASK(32);
> +
> + request_module(pdevinfo.name);
> +
> + dw_dev->phy_pdev = platform_device_register_full();
> + if (IS_ERR(dw_dev->phy_pdev)) {
> + dev_err(dw_dev->dev, "failed to register phy device\n");
> + return PTR_ERR(dw_dev->phy_pdev);
> + }
> +
> + if (!dw_dev->phy_pdev->dev.driver) {
> + dev_err(dw_dev->dev, "failed to initialize phy driver\n");
> + goto err;
> + }

I think this is not safe because there is nothing preventing unbinding 
or unloading the driver at this point.

> + if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {

So dw_dev->phy_pdev->dev.driver may be already NULL here.

> + dev_err(dw_dev->dev, "failed to get phy module\n");
> + goto err;
> + }
> +
> + dw_dev->phy_sd = dev_get_drvdata(_dev->phy_pdev->dev);
> + if (!dw_dev->phy_sd) {
> + dev_err(dw_dev->dev, "failed to get phy subdev\n");
> + goto err_put;
> + }
> +
> + if (v4l2_device_register_subdev(_dev->v4l2_dev, dw_dev->phy_sd)) {
> + dev_err(dw_dev->dev, "failed to register phy subdev\n");
> + goto err_put;
> + }

I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
registration.  And with recent change [1] you could handle this PHY subdev
in a standard way.  That might be more complicated than it is now but should 
make any future platform integration easier.

[1] https://patchwork.linuxtv.org/patch/41834

> + module_put(dw_dev->phy_pdev->dev.driver->owner);
> + return 0;
> +
> +err_put:
> + module_put(dw_dev->phy_pdev->dev.driver->owner);
> +err:
> + platform_device_unregister(dw_dev->phy_pdev);
> + return -EINVAL;
> +}
> +
> +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
> +{
> + if (!IS_ERR(dw_dev->phy_pdev))
> + platform_device_unregister(dw_dev->phy_pdev);
> +}

> +static int dw_hdmi_config_hdcp(struct dw_hdmi_dev *dw_dev)
> +{

> + for (i = 0; i < DW_HDMI_HDCP14_KEYS_SIZE; i += 2) {
> + for (j = 0; j < key_write_tries; j++) {
> + if (is_hdcp14_key_write_allowed(dw_dev))
> + break;
> + mdelay(10);

usleep_range()? I've seen more (busy waiting) mdelay() calls in this
patch series.


> +static int __dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
> +{
> + unsigned long flags;
> + int ret;
> +
> + ret = dw_hdmi_config(dw_dev, input);
> +
> + spin_lock_irqsave(_dev->lock, flags);
> + dw_dev->pending_config = false;
> + spin_unlock_irqrestore(_dev->lock, flags);
> +
> + return ret;
> +}
> +
> +struct dw_hdmi_work_data {
> + struct dw_hdmi_dev *dw_dev;
> + struct work_struct work;
> + u32 input;
> +};
> +
> +static void dw_hdmi_work_handler(struct work_struct *work)
> +{
> + struct dw_hdmi_work_data *data = container_of(work,
> + struct dw_hdmi_work_data, work);
> +
> + __dw_hdmi_power_on(data->dw_dev, data->input);
> + devm_kfree(data->dw_dev->dev, data);
> +}
> +
> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
> +{
> + struct dw_hdmi_work_data *data;
> + unsigned long flags;
> +
> + data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);

Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
in the device's probe() callback, but in other places, including interrupt 
handler?  devm_* API is normally used when life time of a resource is more 
or less equal to life time of struct device or its matched driver.  Were 
there any specific reasons to not just use kzalloc()/kfree() ?

> + if (!data)
> + return -ENOMEM;
> +
> + INIT_WORK(>work, dw_hdmi_work_handler);
> + data->dw_dev = dw_dev;
> + data->input = input;
> +
> + spin_lock_irqsave(_dev->lock, flags);