Re: [PATCH v7 4/9] media: venus: adding core part and helper functions

[Stanimir Varbanov]

Hi Hans,

Thanks for the comments!

On 03/24/2017 04:23 PM, Hans Verkuil wrote:

Some review comments below:

On 03/13/17 17:37, Stanimir Varbanov wrote:

 * core.c has implemented the platform dirver methods, file


dirver -> driver


operations and v4l2 registration.

 * helpers.c has implemented common helper functions for:
   - buffer management

   - vb2_ops and functions for format propagation,

   - functions for allocating and freeing buffers for
   internal usage. The buffer parameters describing internal
   buffers depends on current format, resolution and codec.

   - functions for calculation of current load of the
   hardware. Depending on the count of instances and
   resolutions it selects the best clock rate for the video
   core.

 * firmware loader

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/core.c | 386 
 drivers/media/platform/qcom/venus/core.h | 306 +
 drivers/media/platform/qcom/venus/firmware.c | 107 +
 drivers/media/platform/qcom/venus/firmware.h |  22 +
 drivers/media/platform/qcom/venus/helpers.c  | 632 +++
 drivers/media/platform/qcom/venus/helpers.h  |  41 ++
 6 files changed, 1494 insertions(+)
 create mode 100644 drivers/media/platform/qcom/venus/core.c
 create mode 100644 drivers/media/platform/qcom/venus/core.h
 create mode 100644 drivers/media/platform/qcom/venus/firmware.c
 create mode 100644 drivers/media/platform/qcom/venus/firmware.h
 create mode 100644 drivers/media/platform/qcom/venus/helpers.c
 create mode 100644 drivers/media/platform/qcom/venus/helpers.h

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
new file mode 100644
index ..557b6ec4cc48
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -0,0 +1,386 @@
+/*
+ * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "core.h"
+#include "vdec.h"
+#include "venc.h"
+#include "firmware.h"
+
+static const struct hfi_core_ops venus_core_ops;
+
+static void venus_sys_error_handler(struct work_struct *work)
+{
+   struct venus_core *core =
+   container_of(work, struct venus_core, work.work);
+   int ret;
+
+   dev_warn(core->dev, "system error occurred, starting recovery!\n");
+
+   pm_runtime_get_sync(core->dev);
+
+   hfi_core_deinit(core, true);
+
+   hfi_destroy(core);
+
+   mutex_lock(>lock);
+
+   venus_shutdown(>dev_fw);
+
+   pm_runtime_put_sync(core->dev);
+
+   ret = hfi_create(core, _core_ops);
+
+   pm_runtime_get_sync(core->dev);
+
+   ret = venus_boot(core->dev, >dev_fw);
+
+   ret = hfi_core_resume(core, true);


Why assign to ret if you're going to ignore it anyway? Either drop the 
assignment
or do something with it.


Since this is on the recovery path I'm not sure how to proceed on error 
path. Delay the workqueue and try again later? Whatever, I can rework 
this to print the errors instead of ignoring them.





+
+   enable_irq(core->irq);
+
+   mutex_unlock(>lock);
+
+   ret = hfi_core_init(core);
+   if (ret)
+   dev_err(core->dev, "hfi_core_init (%d)\n", ret);
+
+   pm_runtime_put_sync(core->dev);
+
+   core->sys_error = false;
+}
+





+/**
+ * struct venus_inst - holds per instance paramerters
+ *
+ * @list:  used for attach an instance to the core
+ * @lock:  instance lock
+ * @core:  a reference to the core struct
+ * @internalbufs:  a list of internal bufferes
+ * @registeredbufs:a list of registered capture bufferes
+ * @ctrl_handler:  v4l control handler
+ * @controls:  an union of decoder and encoder control parameters
+ * @fh: a holder of v4l file handle structure
+ * @width: current capture width
+ * @height:current capture height
+ * @out_width: current output width
+ * @out_height:current output height
+ * @colorspace:current color space
+ * @quantization:  current quantization
+ * @xfer_func: current xfer function
+ * @fps:   holds current FPS
+ * @timeperframe:  holds current time per frame structure
+ * @fmt_out:   a reference to output format structure
+ * @fmt_cap:   a reference to capture format structure
+ * @num_input_bufs:

Re: [PATCH v7 4/9] media: venus: adding core part and helper functions

[Hans Verkuil]

Some review comments below:

On 03/13/17 17:37, Stanimir Varbanov wrote:
>  * core.c has implemented the platform dirver methods, file

dirver -> driver

> operations and v4l2 registration.
> 
>  * helpers.c has implemented common helper functions for:
>- buffer management
> 
>- vb2_ops and functions for format propagation,
> 
>- functions for allocating and freeing buffers for
>internal usage. The buffer parameters describing internal
>buffers depends on current format, resolution and codec.
> 
>- functions for calculation of current load of the
>hardware. Depending on the count of instances and
>resolutions it selects the best clock rate for the video
>core.
> 
>  * firmware loader
> 
> Signed-off-by: Stanimir Varbanov 
> ---
>  drivers/media/platform/qcom/venus/core.c | 386 
>  drivers/media/platform/qcom/venus/core.h | 306 +
>  drivers/media/platform/qcom/venus/firmware.c | 107 +
>  drivers/media/platform/qcom/venus/firmware.h |  22 +
>  drivers/media/platform/qcom/venus/helpers.c  | 632 
> +++
>  drivers/media/platform/qcom/venus/helpers.h  |  41 ++
>  6 files changed, 1494 insertions(+)
>  create mode 100644 drivers/media/platform/qcom/venus/core.c
>  create mode 100644 drivers/media/platform/qcom/venus/core.h
>  create mode 100644 drivers/media/platform/qcom/venus/firmware.c
>  create mode 100644 drivers/media/platform/qcom/venus/firmware.h
>  create mode 100644 drivers/media/platform/qcom/venus/helpers.c
>  create mode 100644 drivers/media/platform/qcom/venus/helpers.h
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c 
> b/drivers/media/platform/qcom/venus/core.c
> new file mode 100644
> index ..557b6ec4cc48
> --- /dev/null
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -0,0 +1,386 @@
> +/*
> + * Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "core.h"
> +#include "vdec.h"
> +#include "venc.h"
> +#include "firmware.h"
> +
> +static const struct hfi_core_ops venus_core_ops;
> +
> +static void venus_sys_error_handler(struct work_struct *work)
> +{
> + struct venus_core *core =
> + container_of(work, struct venus_core, work.work);
> + int ret;
> +
> + dev_warn(core->dev, "system error occurred, starting recovery!\n");
> +
> + pm_runtime_get_sync(core->dev);
> +
> + hfi_core_deinit(core, true);
> +
> + hfi_destroy(core);
> +
> + mutex_lock(>lock);
> +
> + venus_shutdown(>dev_fw);
> +
> + pm_runtime_put_sync(core->dev);
> +
> + ret = hfi_create(core, _core_ops);
> +
> + pm_runtime_get_sync(core->dev);
> +
> + ret = venus_boot(core->dev, >dev_fw);
> +
> + ret = hfi_core_resume(core, true);

Why assign to ret if you're going to ignore it anyway? Either drop the 
assignment
or do something with it.

> +
> + enable_irq(core->irq);
> +
> + mutex_unlock(>lock);
> +
> + ret = hfi_core_init(core);
> + if (ret)
> + dev_err(core->dev, "hfi_core_init (%d)\n", ret);
> +
> + pm_runtime_put_sync(core->dev);
> +
> + core->sys_error = false;
> +}
> +
> +static void venus_event_notify(struct venus_core *core, u32 event)
> +{
> + struct venus_inst *inst;
> +
> + switch (event) {
> + case EVT_SYS_WATCHDOG_TIMEOUT:
> + case EVT_SYS_ERROR:
> + break;
> + default:
> + return;
> + }
> +
> + mutex_lock(>lock);
> + core->sys_error = true;
> + list_for_each_entry(inst, >instances, list)
> + inst->ops->event_notify(inst, EVT_SESSION_ERROR, NULL);
> + mutex_unlock(>lock);
> +
> + disable_irq_nosync(core->irq);
> +
> + /*
> +  * Sleep for 5 sec to ensure venus has completed any pending cache
> +  * operations. Without this sleep, we see device reset when firmware is
> +  * unloaded after a system error.
> +  */
> + schedule_delayed_work(>work, msecs_to_jiffies(100));
> +}
> +
> +static const struct hfi_core_ops venus_core_ops = {
> + .event_notify = venus_event_notify,
> +};
> +
> +static int venus_clks_get(struct venus_core *core)
> +{
> + const struct venus_resources *res = core->res;
> + struct device *dev = core->dev;
> +