Re: [PATCH v3] [media] coda: add Freescale firmware compatibility location
Hi Philipp, (Adding firmware loader maintainers to Cc). On Thu, Jan 19, 2017 at 10:44:54AM +0100, Philipp Zabel wrote: > On Wed, 2017-01-18 at 21:33 +0200, Baruch Siach wrote: > > > - if (dev->firmware == 1) { > > > + if (dev->firmware > 0) { > > > > Why would vpu/vpu_fw_*.bin and v4l-coda960-*.bin be considered fallback > > firmware? > > That was meant in the sense of a firmware loaded from fallback location. > > See the comment below, I needed a string to tell the user that the > preceding firmware not found error messages can be safely ignored. If > you have an idea for better wording, feel free submit a change. > > > > /* > > > > > >* Since we can't suppress warnings for failed asynchronous > > >* firmware requests, report that the fallback firmware was > > >* found. > > >*/ > > > dev_info(>dev, "Using fallback firmware %s\n", > > >dev->devtype->firmware[dev->firmware]); > > > } For the context, we are talking about these boot time messages: [2.152822] coda 204.vpu: Direct firmware load for vpu_fw_imx6q.bin failed with error -2 [2.162669] coda 204.vpu: Using fallback firmware vpu/vpu_fw_imx6q.bin Looks bad. There must be a better way to load a firmware file from a few optional locations. Actually, this use of the 'cont' parameter of request_firmware_nowait() to retry firmware load does not look like the intended use for the callback. How about extending the firmware loader to accept multiple possible firmware locations? baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}ooO--U--Ooo{= - bar...@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] [media] coda: add Freescale firmware compatibility location
Hi Baruch, On Wed, 2017-01-18 at 21:33 +0200, Baruch Siach wrote: [...] > > To increase the number of firmware paths, coda_fw_callback has to be > > modified, too. Otherwise it will just ignore firmware[2]: > > Thanks for catching that. But shouldn't we make the firmware files list a > NULL > terminated array instead of spreading the array size knowledge all over the > code? Maybe, although the array is not really variable length. Another possibility would be to save that tiny amount of wasted space and add a #define MAX_FIRMWARE_PATHS 3 > I have one more question below. > > > static void coda_fw_callback(const struct firmware *fw, void *context) > > { > > struct coda_dev *dev = context; > > struct platform_device *pdev = dev->plat_dev; > > int i, ret; > > > > - if (!fw && dev->firmware == 1) { > > + if (!fw && dev->firmware == 2) { > > v4l2_err(>v4l2_dev, "firmware request failed\n"); > > goto put_pm; > > } > > if (!fw) { > > - dev->firmware = 1; > > + dev->firmware++; > > coda_firmware_request(dev); > > return; > > } > > - if (dev->firmware == 1) { > > + if (dev->firmware > 0) { > > Why would vpu/vpu_fw_*.bin and v4l-coda960-*.bin be considered fallback > firmware? That was meant in the sense of a firmware loaded from fallback location. See the comment below, I needed a string to tell the user that the preceding firmware not found error messages can be safely ignored. If you have an idea for better wording, feel free submit a change. > > /* > > > > * Since we can't suppress warnings for failed asynchronous > > * firmware requests, report that the fallback firmware was > > * found. > > */ > > dev_info(>dev, "Using fallback firmware %s\n", > > dev->devtype->firmware[dev->firmware]); > > } > > Thanks, > baruch regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] [media] coda: add Freescale firmware compatibility location
Hi Philipp, On Wed, Jan 18, 2017 at 12:30:29PM +0100, Philipp Zabel wrote: > On Sun, 2017-01-15 at 12:33 +0200, Baruch Siach wrote: > > The Freescale provided imx-vpu looks for firmware files under > > /lib/firmware/vpu > > by default. Make coda look there for firmware files to ease the update path. > > > > Cc: Fabio Estevam> > Signed-off-by: Baruch Siach > > --- > > v3: adjust the number of firmware locations in coda_devtype (kbuild test > > robot) > > > > v2: add compatibility path; don't change existing path (Fabio) > > --- > > drivers/media/platform/coda/coda-common.c | 4 > > drivers/media/platform/coda/coda.h| 2 +- > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/coda/coda-common.c > > b/drivers/media/platform/coda/coda-common.c > > index 9e6bdafa16f5..ce0d00f3f3ba 100644 > > --- a/drivers/media/platform/coda/coda-common.c > > +++ b/drivers/media/platform/coda/coda-common.c > > @@ -2079,6 +2079,7 @@ static const struct coda_devtype coda_devdata[] = { > > [CODA_IMX27] = { > > .firmware = { > > "vpu_fw_imx27_TO2.bin", > > + "vpu/vpu_fw_imx27_TO2.bin", > > "v4l-codadx6-imx27.bin" > > }, > > .product = CODA_DX6, > > @@ -2092,6 +2093,7 @@ static const struct coda_devtype coda_devdata[] = { > > [CODA_IMX53] = { > > .firmware = { > > "vpu_fw_imx53.bin", > > + "vpu/vpu_fw_imx53.bin", > > "v4l-coda7541-imx53.bin" > > }, > > .product = CODA_7541, > > @@ -2106,6 +2108,7 @@ static const struct coda_devtype coda_devdata[] = { > > [CODA_IMX6Q] = { > > .firmware = { > > "vpu_fw_imx6q.bin", > > + "vpu/vpu_fw_imx6q.bin", > > "v4l-coda960-imx6q.bin" > > }, > > .product = CODA_960, > > @@ -2120,6 +2123,7 @@ static const struct coda_devtype coda_devdata[] = { > > [CODA_IMX6DL] = { > > .firmware = { > > "vpu_fw_imx6d.bin", > > + "vpu/vpu_fw_imx6d.bin", > > "v4l-coda960-imx6dl.bin" > > }, > > .product = CODA_960, > > diff --git a/drivers/media/platform/coda/coda.h > > b/drivers/media/platform/coda/coda.h > > index 53f96661683c..8490bcb1fde2 100644 > > --- a/drivers/media/platform/coda/coda.h > > +++ b/drivers/media/platform/coda/coda.h > > @@ -50,7 +50,7 @@ enum coda_product { > > struct coda_video_device; > > > > struct coda_devtype { > > - char*firmware[2]; > > + char*firmware[3]; > > enum coda_product product; > > const struct coda_codec *codecs; > > unsigned intnum_codecs; > > To increase the number of firmware paths, coda_fw_callback has to be > modified, too. Otherwise it will just ignore firmware[2]: Thanks for catching that. But shouldn't we make the firmware files list a NULL terminated array instead of spreading the array size knowledge all over the code? I have one more question below. > static void coda_fw_callback(const struct firmware *fw, void *context) > { > struct coda_dev *dev = context; > struct platform_device *pdev = dev->plat_dev; > int i, ret; > > - if (!fw && dev->firmware == 1) { > + if (!fw && dev->firmware == 2) { > v4l2_err(>v4l2_dev, "firmware request failed\n"); > goto put_pm; > } > if (!fw) { > - dev->firmware = 1; > + dev->firmware++; > coda_firmware_request(dev); > return; > } > - if (dev->firmware == 1) { > + if (dev->firmware > 0) { Why would vpu/vpu_fw_*.bin and v4l-coda960-*.bin be considered fallback firmware? > /* > >* Since we can't suppress warnings for failed asynchronous >* firmware requests, report that the fallback firmware was >* found. >*/ > dev_info(>dev, "Using fallback firmware %s\n", >dev->devtype->firmware[dev->firmware]); > } Thanks, baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}ooO--U--Ooo{= - bar...@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] [media] coda: add Freescale firmware compatibility location
Hi Baruch, On Sun, 2017-01-15 at 12:33 +0200, Baruch Siach wrote: > The Freescale provided imx-vpu looks for firmware files under > /lib/firmware/vpu > by default. Make coda look there for firmware files to ease the update path. > > Cc: Fabio Estevam> Signed-off-by: Baruch Siach > --- > v3: adjust the number of firmware locations in coda_devtype (kbuild test > robot) > > v2: add compatibility path; don't change existing path (Fabio) > --- > drivers/media/platform/coda/coda-common.c | 4 > drivers/media/platform/coda/coda.h| 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/coda/coda-common.c > b/drivers/media/platform/coda/coda-common.c > index 9e6bdafa16f5..ce0d00f3f3ba 100644 > --- a/drivers/media/platform/coda/coda-common.c > +++ b/drivers/media/platform/coda/coda-common.c > @@ -2079,6 +2079,7 @@ static const struct coda_devtype coda_devdata[] = { > [CODA_IMX27] = { > .firmware = { > "vpu_fw_imx27_TO2.bin", > + "vpu/vpu_fw_imx27_TO2.bin", > "v4l-codadx6-imx27.bin" > }, > .product = CODA_DX6, > @@ -2092,6 +2093,7 @@ static const struct coda_devtype coda_devdata[] = { > [CODA_IMX53] = { > .firmware = { > "vpu_fw_imx53.bin", > + "vpu/vpu_fw_imx53.bin", > "v4l-coda7541-imx53.bin" > }, > .product = CODA_7541, > @@ -2106,6 +2108,7 @@ static const struct coda_devtype coda_devdata[] = { > [CODA_IMX6Q] = { > .firmware = { > "vpu_fw_imx6q.bin", > + "vpu/vpu_fw_imx6q.bin", > "v4l-coda960-imx6q.bin" > }, > .product = CODA_960, > @@ -2120,6 +2123,7 @@ static const struct coda_devtype coda_devdata[] = { > [CODA_IMX6DL] = { > .firmware = { > "vpu_fw_imx6d.bin", > + "vpu/vpu_fw_imx6d.bin", > "v4l-coda960-imx6dl.bin" > }, > .product = CODA_960, > diff --git a/drivers/media/platform/coda/coda.h > b/drivers/media/platform/coda/coda.h > index 53f96661683c..8490bcb1fde2 100644 > --- a/drivers/media/platform/coda/coda.h > +++ b/drivers/media/platform/coda/coda.h > @@ -50,7 +50,7 @@ enum coda_product { > struct coda_video_device; > > struct coda_devtype { > - char*firmware[2]; > + char*firmware[3]; > enum coda_product product; > const struct coda_codec *codecs; > unsigned intnum_codecs; To increase the number of firmware paths, coda_fw_callback has to be modified, too. Otherwise it will just ignore firmware[2]: static void coda_fw_callback(const struct firmware *fw, void *context) { struct coda_dev *dev = context; struct platform_device *pdev = dev->plat_dev; int i, ret; - if (!fw && dev->firmware == 1) { + if (!fw && dev->firmware == 2) { v4l2_err(>v4l2_dev, "firmware request failed\n"); goto put_pm; } if (!fw) { - dev->firmware = 1; + dev->firmware++; coda_firmware_request(dev); return; } - if (dev->firmware == 1) { + if (dev->firmware > 0) { /* * Since we can't suppress warnings for failed asynchronous * firmware requests, report that the fallback firmware was * found. */ dev_info(>dev, "Using fallback firmware %s\n", dev->devtype->firmware[dev->firmware]); } regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] [media] coda: add Freescale firmware compatibility location
On Sun, Jan 15, 2017 at 8:33 AM, Baruch Siachwrote: > The Freescale provided imx-vpu looks for firmware files under > /lib/firmware/vpu > by default. Make coda look there for firmware files to ease the update path. > > Cc: Fabio Estevam > Signed-off-by: Baruch Siach Reviewed-by: Fabio Estevam -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] [media] coda: add Freescale firmware compatibility location
The Freescale provided imx-vpu looks for firmware files under /lib/firmware/vpu by default. Make coda look there for firmware files to ease the update path. Cc: Fabio EstevamSigned-off-by: Baruch Siach --- v3: adjust the number of firmware locations in coda_devtype (kbuild test robot) v2: add compatibility path; don't change existing path (Fabio) --- drivers/media/platform/coda/coda-common.c | 4 drivers/media/platform/coda/coda.h| 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 9e6bdafa16f5..ce0d00f3f3ba 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -2079,6 +2079,7 @@ static const struct coda_devtype coda_devdata[] = { [CODA_IMX27] = { .firmware = { "vpu_fw_imx27_TO2.bin", + "vpu/vpu_fw_imx27_TO2.bin", "v4l-codadx6-imx27.bin" }, .product = CODA_DX6, @@ -2092,6 +2093,7 @@ static const struct coda_devtype coda_devdata[] = { [CODA_IMX53] = { .firmware = { "vpu_fw_imx53.bin", + "vpu/vpu_fw_imx53.bin", "v4l-coda7541-imx53.bin" }, .product = CODA_7541, @@ -2106,6 +2108,7 @@ static const struct coda_devtype coda_devdata[] = { [CODA_IMX6Q] = { .firmware = { "vpu_fw_imx6q.bin", + "vpu/vpu_fw_imx6q.bin", "v4l-coda960-imx6q.bin" }, .product = CODA_960, @@ -2120,6 +2123,7 @@ static const struct coda_devtype coda_devdata[] = { [CODA_IMX6DL] = { .firmware = { "vpu_fw_imx6d.bin", + "vpu/vpu_fw_imx6d.bin", "v4l-coda960-imx6dl.bin" }, .product = CODA_960, diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h index 53f96661683c..8490bcb1fde2 100644 --- a/drivers/media/platform/coda/coda.h +++ b/drivers/media/platform/coda/coda.h @@ -50,7 +50,7 @@ enum coda_product { struct coda_video_device; struct coda_devtype { - char*firmware[2]; + char*firmware[3]; enum coda_product product; const struct coda_codec *codecs; unsigned intnum_codecs; -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html