Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
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(&pdevinfo); + 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(&dw_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(&dw_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(&data->work, dw_hdmi_work_handler); + data->dw_dev = dw_dev; + data->input = input; + + spin_lock_irqsave(&dw_dev->lock, flags); + if
Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
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(&pdevinfo); >>> + 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(&dw_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(&dw_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(&data->work, dw_hdmi_work_handler); >>> + data->dw_dev = dw_dev; >>> + data->input = input; >>> + >>> + spin_lock_irqsave(&dw_dev->lock, flags); >>> + if (dw_dev->pending_config) { >>> + devm_kfree(dw_dev->dev, data); >>> + spin_unlock_irqrestore(&dw_dev->lock, flags); >>> + return 0; >>> + } >>> + >>> + queue_work(dw_dev->wq, &data->work); >>> + dw_dev->pending_config = true; >>> + spin_unlock_irqrestore(&dw_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)(
Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
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 = &dw_dev->phy_config; >> +struct platform_device_info pdevinfo; >> + >> +memset(&pdevinfo, 0, sizeof(pdevinfo)); >> + >> +phy->funcs = &dw_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(&pdevinfo); >> +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(&dw_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(&dw_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&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=uHTyp6WsEj6vN19Zc09HqSNUhCx62OI8u-tAgi-EVts&s=WjyPjIwN1uGvPoV7ZlcmzOgdptakzluHywuKRA8ZG8M&e= > 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(&dw_dev->lock, flags); >> +dw_dev->pending_config = false; >> +spin_unlock_irqrestore(&dw_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 *d
Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
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 = &dw_dev->phy_config; > + struct platform_device_info pdevinfo; > + > + memset(&pdevinfo, 0, sizeof(pdevinfo)); > + > + phy->funcs = &dw_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(&pdevinfo); > + 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(&dw_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(&dw_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(&dw_dev->lock, flags); > + dw_dev->pending_config = false; > + spin_unlock_irqrestore(&dw_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(&data->work, dw_hdmi_work_handler); > + data->dw_dev = dw_dev; > + data->input = input; > + > + spin_lock_irqsave(&