Re: [PATCH 13/57] media: atomisp: Remove deferred firmware loading support

Andy Shevchenko
2023-01-23T14:38:01Z

On Mon, Jan 23, 2023 at 01:51:21PM +0100, Hans de Goede wrote:
> Make atomisp behave like any othet drivers and have it load the firmware

other

> at probe time (as it was already doing by default).
>
> The deferred firmware loading support needlessly complicates the
> v4l2_file_operations.open callback (atomisp_open()), getting in
> the way of allowing multiple opens like a normal v4l2 device would.

So it actually removes that variable and accompanying code w.o.
changing behaviour (as default).

Reviewed-by: Andy Shevchenko <andy@kernel.org>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> .../staging/media/atomisp/pci/atomisp_fops.c | 25 -----------
> .../staging/media/atomisp/pci/atomisp_fops.h | 2 -
> .../staging/media/atomisp/pci/atomisp_v4l2.c | 42 +++++++------------
> 3 files changed, 14 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
> index 682239ea042f..036ad339b344 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
> @@ -757,25 +757,6 @@ static int atomisp_open(struct file *file)
> mutex_lock(&isp->mutex);
>
> asd->subdev.devnode = vdev;
> - /* Deferred firmware loading case. */
> - if (isp->css_env.isp_css_fw.bytes == 0) {
> - dev_err(isp->dev, "Deferred firmware load.\n");
> - isp->firmware = atomisp_load_firmware(isp);
> - if (!isp->firmware) {
> - dev_err(isp->dev, "Failed to load ISP firmware.\n");
> - ret = -ENOENT;
> - goto error;
> - }
> - ret = atomisp_css_load_firmware(isp);
> - if (ret) {
> - dev_err(isp->dev, "Failed to init css.\n");
> - goto error;
> - }
> - /* No need to keep FW in memory anymore. */
> - release_firmware(isp->firmware);
> - isp->firmware = NULL;
> - isp->css_env.isp_css_fw.data = NULL;
> - }
>
> if (!isp->input_cnt) {
> dev_err(isp->dev, "no camera attached\n");
> @@ -901,12 +882,6 @@ static int atomisp_release(struct file *file)
>
> atomisp_destroy_pipes_stream_force(asd);
>
> - if (defer_fw_load) {
> - ia_css_unload_firmware();
> - isp->css_env.isp_css_fw.data = NULL;
> - isp->css_env.isp_css_fw.bytes = 0;
> - }
> -
> ret = v4l2_subdev_call(isp->flash, core, s_power, 0);
> if (ret < 0 && ret != -ENODEV && ret != -ENOIOCTLCMD)
> dev_warn(isp->dev, "Failed to power-off flash\n");
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.h b/drivers/staging/media/atomisp/pci/atomisp_fops.h
> index 10e43126b693..2efc5245e571 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.h
> @@ -33,6 +33,4 @@ int atomisp_qbuffers_to_css(struct atomisp_sub_device *asd);
>
> extern const struct v4l2_file_operations atomisp_fops;
>
> -extern bool defer_fw_load;
> -
> #endif /* __ATOMISP_FOPS_H__ */
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index aa05c69a5c6b..2a949d3dc5d1 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -58,12 +58,6 @@ static uint skip_fwload;
> module_param(skip_fwload, uint, 0644);
> MODULE_PARM_DESC(skip_fwload, "Skip atomisp firmware load");
>
> -/* memory optimization: deferred firmware loading */
> -bool defer_fw_load;
> -module_param(defer_fw_load, bool, 0644);
> -MODULE_PARM_DESC(defer_fw_load,
> - "Defer FW loading until device is opened (default:disable)");
> -
> /* cross componnet debug message flag */
> int dbg_level;
> module_param(dbg_level, int, 0644);
> @@ -1514,21 +1508,17 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
> isp->max_isr_latency = ATOMISP_MAX_ISR_LATENCY;
>
> /* Load isp firmware from user space */
> - if (!defer_fw_load) {
> - isp->firmware = atomisp_load_firmware(isp);
> - if (!isp->firmware) {
> - err = -ENOENT;
> - dev_dbg(&pdev->dev, "Firmware load failed\n");
> - goto load_fw_fail;
> - }
> + isp->firmware = atomisp_load_firmware(isp);
> + if (!isp->firmware) {
> + err = -ENOENT;
> + dev_dbg(&pdev->dev, "Firmware load failed\n");
> + goto load_fw_fail;
> + }
>
> - err = sh_css_check_firmware_version(isp->dev, isp->firmware->data);
> - if (err) {
> - dev_dbg(&pdev->dev, "Firmware version check failed\n");
> - goto fw_validation_fail;
> - }
> - } else {
> - dev_info(&pdev->dev, "Firmware load will be deferred\n");
> + err = sh_css_check_firmware_version(isp->dev, isp->firmware->data);
> + if (err) {
> + dev_dbg(&pdev->dev, "Firmware version check failed\n");
> + goto fw_validation_fail;
> }
>
> pci_set_master(pdev);
> @@ -1628,14 +1618,10 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
> }
>
> /* Load firmware into ISP memory */
> - if (!defer_fw_load) {
> - err = atomisp_css_load_firmware(isp);
> - if (err) {
> - dev_err(&pdev->dev, "Failed to init css.\n");
> - goto css_init_fail;
> - }
> - } else {
> - dev_dbg(&pdev->dev, "Skip css init.\n");
> + err = atomisp_css_load_firmware(isp);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to init css.\n");
> + goto css_init_fail;
> }
> /* Clear FW image from memory */
> release_firmware(isp->firmware);
> --
> 2.39.0
>

--
With Best Regards,
Andy Shevchenko