| Message ID | 1411401956-29330-1-git-send-email-p.zabel@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | c5d28e29833c8bc80d96cb2f46c3cf06b43a8fa4 |
| Headers | show |
On 22 September 2014 18:05, Philipp Zabel <p.zabel@pengutronix.de> wrote: > From: Ulf Hansson <ulf.hansson@linaro.org> > > For several reasons it's good practice to leave devices in runtime PM > active state while those have been probed. > > In this cases we also want to prevent the device from going inactive, > until the firmware has been completely installed, especially when using > a PM domain. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Thanks for moving this to the next version, I have been a bit busy the last week. Changes looking good! Kind regards Uffe > > --- > Changes since v1: > - Deactivate PM domain on error > - Added a comment to runtime PM setup > --- > drivers/media/platform/coda/coda-common.c | 55 ++++++++++++------------------- > 1 file changed, 21 insertions(+), 34 deletions(-) > > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c > index 0997b5c..ced4760 100644 > --- a/drivers/media/platform/coda/coda-common.c > +++ b/drivers/media/platform/coda/coda-common.c > @@ -1688,7 +1688,7 @@ static void coda_fw_callback(const struct firmware *fw, void *context) > > if (!fw) { > v4l2_err(&dev->v4l2_dev, "firmware request failed\n"); > - return; > + goto put_pm; > } > > /* allocate auxiliary per-device code buffer for the BIT processor */ > @@ -1696,50 +1696,27 @@ static void coda_fw_callback(const struct firmware *fw, void *context) > dev->debugfs_root); > if (ret < 0) { > dev_err(&pdev->dev, "failed to allocate code buffer\n"); > - return; > + goto put_pm; > } > > /* Copy the whole firmware image to the code buffer */ > memcpy(dev->codebuf.vaddr, fw->data, fw->size); > release_firmware(fw); > > - if (pm_runtime_enabled(&pdev->dev) && pdev->dev.pm_domain) { > - /* > - * Enabling power temporarily will cause coda_hw_init to be > - * called via coda_runtime_resume by the pm domain. > - */ > - ret = pm_runtime_get_sync(&dev->plat_dev->dev); > - if (ret < 0) { > - v4l2_err(&dev->v4l2_dev, "failed to power on: %d\n", > - ret); > - return; > - } > - > - ret = coda_check_firmware(dev); > - if (ret < 0) > - return; > - > - pm_runtime_put_sync(&dev->plat_dev->dev); > - } else { > - /* > - * If runtime pm is disabled or pm_domain is not set, > - * initialize once manually. > - */ > - ret = coda_hw_init(dev); > - if (ret < 0) { > - v4l2_err(&dev->v4l2_dev, "HW initialization failed\n"); > - return; > - } > - > - ret = coda_check_firmware(dev); > - if (ret < 0) > - return; > + ret = coda_hw_init(dev); > + if (ret < 0) { > + v4l2_err(&dev->v4l2_dev, "HW initialization failed\n"); > + goto put_pm; > } > > + ret = coda_check_firmware(dev); > + if (ret < 0) > + goto put_pm; > + > dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev); > if (IS_ERR(dev->alloc_ctx)) { > v4l2_err(&dev->v4l2_dev, "Failed to alloc vb2 context\n"); > - return; > + goto put_pm; > } > > dev->m2m_dev = v4l2_m2m_init(&coda_m2m_ops); > @@ -1771,12 +1748,15 @@ static void coda_fw_callback(const struct firmware *fw, void *context) > v4l2_info(&dev->v4l2_dev, "codec registered as /dev/video[%d-%d]\n", > dev->vfd[0].num, dev->vfd[1].num); > > + pm_runtime_put_sync(&pdev->dev); > return; > > rel_m2m: > v4l2_m2m_release(dev->m2m_dev); > rel_ctx: > vb2_dma_contig_cleanup_ctx(dev->alloc_ctx); > +put_pm: > + pm_runtime_put_sync(&pdev->dev); > } > > static int coda_firmware_request(struct coda_dev *dev) > @@ -1998,6 +1978,13 @@ static int coda_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, dev); > > + /* > + * Start activated so we can directly call coda_hw_init in > + * coda_fw_callback regardless of whether CONFIG_PM_RUNTIME is > + * enabled or whether the device is associated with a PM domain. > + */ > + pm_runtime_get_noresume(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > > return coda_firmware_request(dev); > -- > 2.1.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 October 2014 13:57, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Hi Ulf, > > Am Montag, den 22.09.2014, 20:44 +0200 schrieb Ulf Hansson: >> On 22 September 2014 18:05, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > From: Ulf Hansson <ulf.hansson@linaro.org> >> > >> > For several reasons it's good practice to leave devices in runtime PM >> > active state while those have been probed. >> > >> > In this cases we also want to prevent the device from going inactive, >> > until the firmware has been completely installed, especially when using >> > a PM domain. >> > >> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >> >> Thanks for moving this to the next version, I have been a bit busy the >> last week. >> >> Changes looking good! > > If I load the coda module on v3.18-rc1 with the GPC power domain patch > applied (at this point the power domain is disabled), the domain's > poweron callback is never called. It does work tough if I switch back to > explicitly calling pm_runtime_get_sync: > > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c > index ac71e11..5421969 100644 > --- a/drivers/media/platform/coda/coda-common.c > +++ b/drivers/media/platform/coda/coda-common.c > @@ -2393,9 +2393,8 @@ static int coda_probe(struct platform_device *pdev) > * coda_fw_callback regardless of whether CONFIG_PM_RUNTIME is > * enabled or whether the device is associated with a PM domain. > */ > - pm_runtime_get_noresume(&pdev->dev); > - pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > + pm_runtime_get_sync(&pdev->dev); > > return coda_firmware_request(dev); > } > > At what point is the pm domain supposed to be enabled when I load the > module? Hi Philipp, The PM domain shall be powered on prior your driver starts probing. This is a common problem when using the generic PM domain. The workaround, which is causing other issues, is a pm_runtime_get_sync(). Now, could you please try to apply the below patchset, that should hopefully fix your issue: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot http://marc.info/?l=linux-pm&m=141320895122707&w=2 Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, Am Donnerstag, den 23.10.2014, 14:15 +0200 schrieb Ulf Hansson: > > At what point is the pm domain supposed to be enabled when I load the > > module? > > Hi Philipp, > > The PM domain shall be powered on prior your driver starts probing. > This is a common problem when using the generic PM domain. The > workaround, which is causing other issues, is a pm_runtime_get_sync(). > > Now, could you please try to apply the below patchset, that should > hopefully fix your issue: > > [PATCH v3 0/9] PM / Domains: Fix race conditions during boot > http://marc.info/?l=linux-pm&m=141320895122707&w=2 thank you, that series does fix the issue. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 0997b5c..ced4760 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -1688,7 +1688,7 @@ static void coda_fw_callback(const struct firmware *fw, void *context) if (!fw) { v4l2_err(&dev->v4l2_dev, "firmware request failed\n"); - return; + goto put_pm; } /* allocate auxiliary per-device code buffer for the BIT processor */ @@ -1696,50 +1696,27 @@ static void coda_fw_callback(const struct firmware *fw, void *context) dev->debugfs_root); if (ret < 0) { dev_err(&pdev->dev, "failed to allocate code buffer\n"); - return; + goto put_pm; } /* Copy the whole firmware image to the code buffer */ memcpy(dev->codebuf.vaddr, fw->data, fw->size); release_firmware(fw); - if (pm_runtime_enabled(&pdev->dev) && pdev->dev.pm_domain) { - /* - * Enabling power temporarily will cause coda_hw_init to be - * called via coda_runtime_resume by the pm domain. - */ - ret = pm_runtime_get_sync(&dev->plat_dev->dev); - if (ret < 0) { - v4l2_err(&dev->v4l2_dev, "failed to power on: %d\n", - ret); - return; - } - - ret = coda_check_firmware(dev); - if (ret < 0) - return; - - pm_runtime_put_sync(&dev->plat_dev->dev); - } else { - /* - * If runtime pm is disabled or pm_domain is not set, - * initialize once manually. - */ - ret = coda_hw_init(dev); - if (ret < 0) { - v4l2_err(&dev->v4l2_dev, "HW initialization failed\n"); - return; - } - - ret = coda_check_firmware(dev); - if (ret < 0) - return; + ret = coda_hw_init(dev); + if (ret < 0) { + v4l2_err(&dev->v4l2_dev, "HW initialization failed\n"); + goto put_pm; } + ret = coda_check_firmware(dev); + if (ret < 0) + goto put_pm; + dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev); if (IS_ERR(dev->alloc_ctx)) { v4l2_err(&dev->v4l2_dev, "Failed to alloc vb2 context\n"); - return; + goto put_pm; } dev->m2m_dev = v4l2_m2m_init(&coda_m2m_ops); @@ -1771,12 +1748,15 @@ static void coda_fw_callback(const struct firmware *fw, void *context) v4l2_info(&dev->v4l2_dev, "codec registered as /dev/video[%d-%d]\n", dev->vfd[0].num, dev->vfd[1].num); + pm_runtime_put_sync(&pdev->dev); return; rel_m2m: v4l2_m2m_release(dev->m2m_dev); rel_ctx: vb2_dma_contig_cleanup_ctx(dev->alloc_ctx); +put_pm: + pm_runtime_put_sync(&pdev->dev); } static int coda_firmware_request(struct coda_dev *dev) @@ -1998,6 +1978,13 @@ static int coda_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dev); + /* + * Start activated so we can directly call coda_hw_init in + * coda_fw_callback regardless of whether CONFIG_PM_RUNTIME is + * enabled or whether the device is associated with a PM domain. + */ + pm_runtime_get_noresume(&pdev->dev); + pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev); return coda_firmware_request(dev);