subject:"\[PATCH v2 02\/29\] venus\: hfi\: preparation to support venus 4xx"

Re: [PATCH v2 02/29] venus: hfi: preparation to support venus 4xx

2018-05-21 Thread Stanimir Varbanov

Hi Tomasz,

Thanks for the comments!

On 05/18/2018 12:44 PM, Tomasz Figa wrote:
> Hi Stanimir,
> 
> On Tue, May 15, 2018 at 5:14 PM Stanimir Varbanov <
> stanimir.varba...@linaro.org> wrote:
> 
>> This covers the differences between 1xx,3xx and 4xx.
> 
>> Signed-off-by: Stanimir Varbanov 
>> ---
>>drivers/media/platform/qcom/venus/core.h |  4 ++
>>drivers/media/platform/qcom/venus/helpers.c  | 37 +++
>>drivers/media/platform/qcom/venus/hfi_helper.h   | 84
> ++--
>>drivers/media/platform/qcom/venus/hfi_venus_io.h | 24 +++
>>drivers/media/platform/qcom/venus/vdec.c |  5 +-
>>drivers/media/platform/qcom/venus/venc.c |  5 +-
>>6 files changed, 137 insertions(+), 22 deletions(-)
> 
> Please see my comments inline.
> 
> [snip]
> 
>> @@ -257,12 +273,11 @@ static int load_scale_clocks(struct venus_core
> *core)
> 
>>set_freq:
> 
>> -   if (core->res->hfi_version == HFI_VERSION_3XX) {
>> -   ret = clk_set_rate(clk, freq);
>> +   ret = clk_set_rate(clk, freq);
>> +
>> +   if (IS_V3(core) || IS_V4(core)) {
>>   ret |= clk_set_rate(core->core0_clk, freq);
>>   ret |= clk_set_rate(core->core1_clk, freq);
>> -   } else {
>> -   ret = clk_set_rate(clk, freq);
>>   }
> 
> nit: The clock API defines NULL clock as a special no-op value and so
> clk_set_rate(NULL, ...) would return 0 instantly. Maybe it would just make
> sense to have core0_clk and core1_clk set to NULL for V1 and remove the
> condition here?

OK, we could avoid the condition but I'll add a comment that those
clocks exist from v3 onwards.

> 
>>   if (ret) {
>> diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h
> b/drivers/media/platform/qcom/venus/hfi_helper.h
>> index 55d8eb21403a..1bc5aab1ce6b 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
>> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
>> @@ -121,6 +121,7 @@
>>#define HFI_EXTRADATA_METADATA_FILLER  0x7fe2
> 
>>#define HFI_INDEX_EXTRADATA_INPUT_CROP 0x070e
>> +#define HFI_INDEX_EXTRADATA_OUTPUT_CROP0x070f
> 
> nit: Would it make sense to suffix this _4XX, so that reader could know
> that it was introduced in this version? Similarly for other newly added
> definitions.

I personally don't like the version suffix on defines. Also seems this
is not used in the code, so I'll drop it for now.

> 
>>#define HFI_INDEX_EXTRADATA_DIGITAL_ZOOM   0x0710
>>#define HFI_INDEX_EXTRADATA_ASPECT_RATIO   0x7f13
> 
>> @@ -376,13 +377,18 @@
>>#define HFI_BUFFER_OUTPUT2 0x3
>>#define HFI_BUFFER_INTERNAL_PERSIST0x4
>>#define HFI_BUFFER_INTERNAL_PERSIST_1  0x5
>> -#define HFI_BUFFER_INTERNAL_SCRATCH0x101
>> -#define HFI_BUFFER_EXTRADATA_INPUT 0x102
>> -#define HFI_BUFFER_EXTRADATA_OUTPUT0x103
>> -#define HFI_BUFFER_EXTRADATA_OUTPUT2   0x104
>> -#define HFI_BUFFER_INTERNAL_SCRATCH_1  0x105
>> -#define HFI_BUFFER_INTERNAL_SCRATCH_2  0x106
>> -
>> +#define HFI_BUFFER_INTERNAL_SCRATCH(ver)   \
>> +   (((ver) == HFI_VERSION_4XX) ? 0x6 : 0x101)
>> +#define HFI_BUFFER_INTERNAL_SCRATCH_1(ver) \
>> +   (((ver) == HFI_VERSION_4XX) ? 0x7 : 0x105)
>> +#define HFI_BUFFER_INTERNAL_SCRATCH_2(ver) \
>> +   (((ver) == HFI_VERSION_4XX) ? 0x8 : 0x106)
>> +#define HFI_BUFFER_EXTRADATA_INPUT(ver)\
>> +   (((ver) == HFI_VERSION_4XX) ? 0xc : 0x102)
>> +#define HFI_BUFFER_EXTRADATA_OUTPUT(ver)   \
>> +   (((ver) == HFI_VERSION_4XX) ? 0xa : 0x103)
>> +#define HFI_BUFFER_EXTRADATA_OUTPUT2(ver)  \
>> +   (((ver) == HFI_VERSION_4XX) ? 0xb : 0x104)
> 
> nit: Does it make sense to add an argument, rather than simply defining
> separate HFI_BUFFER_INTERNAL_SCRATCH_1XX and
> HFI_BUFFER_INTERNAL_SCRATCH_4XX? In my subjective opinion, the argument

I'd like to keep the name of the define version agnostic.

> just makes it harder to read, as it's not clear how it is used inside the
> macro from reading just the call to it. Also it would get messy when adding
> further variants in future.
> 
> [snip]
> 
>> +/* HFI 4XX reorder the fields, use these macros */
>> +#define HFI_BUFREQ_HOLD_COUNT(bufreq, ver) \
>> +   ((ver) == HFI_VERSION_4XX ? 0 : (bufreq)->hold_count)
>> +#define HFI_BUFREQ_COUNT_MIN(bufreq, ver)  \
>> +   ((ver) == HFI_VERSION_4XX ? (bufreq)->hold_count :
> (bufreq)->count_min)
>> +#define HFI_BUFREQ_COUNT_MIN_HOST(bufreq, ver) \
>> +   ((ver) == HFI_VERSION_4XX ? (bufreq)->count_min : 0)
>> +
> 
> Hmm, this is a bit messy. The macro is supposed to return count_min, but it
> returns hold_count. Shouldn't we define a separate

yep, that was the purpose of the

Re: [PATCH v2 02/29] venus: hfi: preparation to support venus 4xx

2018-05-18 Thread Tomasz Figa

Hi Stanimir,

On Tue, May 15, 2018 at 5:14 PM Stanimir Varbanov <
stanimir.varba...@linaro.org> wrote:

> This covers the differences between 1xx,3xx and 4xx.

> Signed-off-by: Stanimir Varbanov 
> ---
>drivers/media/platform/qcom/venus/core.h |  4 ++
>drivers/media/platform/qcom/venus/helpers.c  | 37 +++
>drivers/media/platform/qcom/venus/hfi_helper.h   | 84
++--
>drivers/media/platform/qcom/venus/hfi_venus_io.h | 24 +++
>drivers/media/platform/qcom/venus/vdec.c |  5 +-
>drivers/media/platform/qcom/venus/venc.c |  5 +-
>6 files changed, 137 insertions(+), 22 deletions(-)

Please see my comments inline.

[snip]

> @@ -257,12 +273,11 @@ static int load_scale_clocks(struct venus_core
*core)

>set_freq:

> -   if (core->res->hfi_version == HFI_VERSION_3XX) {
> -   ret = clk_set_rate(clk, freq);
> +   ret = clk_set_rate(clk, freq);
> +
> +   if (IS_V3(core) || IS_V4(core)) {
>   ret |= clk_set_rate(core->core0_clk, freq);
>   ret |= clk_set_rate(core->core1_clk, freq);
> -   } else {
> -   ret = clk_set_rate(clk, freq);
>   }

nit: The clock API defines NULL clock as a special no-op value and so
clk_set_rate(NULL, ...) would return 0 instantly. Maybe it would just make
sense to have core0_clk and core1_clk set to NULL for V1 and remove the
condition here?

>   if (ret) {
> diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h
b/drivers/media/platform/qcom/venus/hfi_helper.h
> index 55d8eb21403a..1bc5aab1ce6b 100644
> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
> @@ -121,6 +121,7 @@
>#define HFI_EXTRADATA_METADATA_FILLER  0x7fe2

>#define HFI_INDEX_EXTRADATA_INPUT_CROP 0x070e
> +#define HFI_INDEX_EXTRADATA_OUTPUT_CROP0x070f

nit: Would it make sense to suffix this _4XX, so that reader could know
that it was introduced in this version? Similarly for other newly added
definitions.

>#define HFI_INDEX_EXTRADATA_DIGITAL_ZOOM   0x0710
>#define HFI_INDEX_EXTRADATA_ASPECT_RATIO   0x7f13

> @@ -376,13 +377,18 @@
>#define HFI_BUFFER_OUTPUT2 0x3
>#define HFI_BUFFER_INTERNAL_PERSIST0x4
>#define HFI_BUFFER_INTERNAL_PERSIST_1  0x5
> -#define HFI_BUFFER_INTERNAL_SCRATCH0x101
> -#define HFI_BUFFER_EXTRADATA_INPUT 0x102
> -#define HFI_BUFFER_EXTRADATA_OUTPUT0x103
> -#define HFI_BUFFER_EXTRADATA_OUTPUT2   0x104
> -#define HFI_BUFFER_INTERNAL_SCRATCH_1  0x105
> -#define HFI_BUFFER_INTERNAL_SCRATCH_2  0x106
> -
> +#define HFI_BUFFER_INTERNAL_SCRATCH(ver)   \
> +   (((ver) == HFI_VERSION_4XX) ? 0x6 : 0x101)
> +#define HFI_BUFFER_INTERNAL_SCRATCH_1(ver) \
> +   (((ver) == HFI_VERSION_4XX) ? 0x7 : 0x105)
> +#define HFI_BUFFER_INTERNAL_SCRATCH_2(ver) \
> +   (((ver) == HFI_VERSION_4XX) ? 0x8 : 0x106)
> +#define HFI_BUFFER_EXTRADATA_INPUT(ver)\
> +   (((ver) == HFI_VERSION_4XX) ? 0xc : 0x102)
> +#define HFI_BUFFER_EXTRADATA_OUTPUT(ver)   \
> +   (((ver) == HFI_VERSION_4XX) ? 0xa : 0x103)
> +#define HFI_BUFFER_EXTRADATA_OUTPUT2(ver)  \
> +   (((ver) == HFI_VERSION_4XX) ? 0xb : 0x104)

nit: Does it make sense to add an argument, rather than simply defining
separate HFI_BUFFER_INTERNAL_SCRATCH_1XX and
HFI_BUFFER_INTERNAL_SCRATCH_4XX? In my subjective opinion, the argument
just makes it harder to read, as it's not clear how it is used inside the
macro from reading just the call to it. Also it would get messy when adding
further variants in future.

[snip]

> +/* HFI 4XX reorder the fields, use these macros */
> +#define HFI_BUFREQ_HOLD_COUNT(bufreq, ver) \
> +   ((ver) == HFI_VERSION_4XX ? 0 : (bufreq)->hold_count)
> +#define HFI_BUFREQ_COUNT_MIN(bufreq, ver)  \
> +   ((ver) == HFI_VERSION_4XX ? (bufreq)->hold_count :
(bufreq)->count_min)
> +#define HFI_BUFREQ_COUNT_MIN_HOST(bufreq, ver) \
> +   ((ver) == HFI_VERSION_4XX ? (bufreq)->count_min : 0)
> +

Hmm, this is a bit messy. The macro is supposed to return count_min, but it
returns hold_count. Shouldn't we define a separate
hfi_buffer_requirements_4xx struct for 4XX?

Even though this seems to simplify the code eventually, I think it might be
quite confusing for anyone working with the driver in the future.

Also HFI_BUFREQ_HOLD_COUNT and HFI_BUFREQ_COUNT_MIN_HOST don't seem to be
used anywhere.

[snip]

> +/* vcodec noc error log registers */
> +#define VCODEC_CORE0_VIDEO_NOC_BASE_OFFS   0x4000
> +#define VCODEC_CORE1_VIDEO_NOC_BASE_OFFS   0xc000
> +#define VCODEC_COREX_VIDEO_NOC_ERR_SWID_LOW_OFFS   0x500
> +#define

[PATCH v2 02/29] venus: hfi: preparation to support venus 4xx

2018-05-15 Thread Stanimir Varbanov

This covers the differences between 1xx,3xx and 4xx.

Signed-off-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/core.h |  4 ++
 drivers/media/platform/qcom/venus/helpers.c  | 37 +++
 drivers/media/platform/qcom/venus/hfi_helper.h   | 84 ++--
 drivers/media/platform/qcom/venus/hfi_venus_io.h | 24 +++
 drivers/media/platform/qcom/venus/vdec.c |  5 +-
 drivers/media/platform/qcom/venus/venc.c |  5 +-
 6 files changed, 137 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h 
b/drivers/media/platform/qcom/venus/core.h
index 0360d295f4c8..8d3e150800c9 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -305,6 +305,10 @@ struct venus_inst {
struct hfi_buffer_requirements bufreq[HFI_BUFFER_TYPE_MAX];
 };
 
+#define IS_V1(core)((core)->res->hfi_version == HFI_VERSION_1XX)
+#define IS_V3(core)((core)->res->hfi_version == HFI_VERSION_3XX)
+#define IS_V4(core)((core)->res->hfi_version == HFI_VERSION_4XX)
+
 #define ctrl_to_inst(ctrl) \
container_of((ctrl)->handler, struct venus_inst, ctrl_handler)
 
diff --git a/drivers/media/platform/qcom/venus/helpers.c 
b/drivers/media/platform/qcom/venus/helpers.c
index 0ce9559a2924..d9065cc8a7d3 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -166,21 +166,37 @@ static int intbufs_unset_buffers(struct venus_inst *inst)
return ret;
 }
 
-static const unsigned int intbuf_types[] = {
-   HFI_BUFFER_INTERNAL_SCRATCH,
-   HFI_BUFFER_INTERNAL_SCRATCH_1,
-   HFI_BUFFER_INTERNAL_SCRATCH_2,
+static const unsigned int intbuf_types_1xx[] = {
+   HFI_BUFFER_INTERNAL_SCRATCH(HFI_VERSION_1XX),
+   HFI_BUFFER_INTERNAL_SCRATCH_1(HFI_VERSION_1XX),
+   HFI_BUFFER_INTERNAL_SCRATCH_2(HFI_VERSION_1XX),
+   HFI_BUFFER_INTERNAL_PERSIST,
+   HFI_BUFFER_INTERNAL_PERSIST_1,
+};
+
+static const unsigned int intbuf_types_4xx[] = {
+   HFI_BUFFER_INTERNAL_SCRATCH(HFI_VERSION_4XX),
+   HFI_BUFFER_INTERNAL_SCRATCH_1(HFI_VERSION_4XX),
+   HFI_BUFFER_INTERNAL_SCRATCH_2(HFI_VERSION_4XX),
HFI_BUFFER_INTERNAL_PERSIST,
HFI_BUFFER_INTERNAL_PERSIST_1,
 };
 
 static int intbufs_alloc(struct venus_inst *inst)
 {
-   unsigned int i;
+   size_t arr_sz;
+   size_t i;
int ret;
 
-   for (i = 0; i < ARRAY_SIZE(intbuf_types); i++) {
-   ret = intbufs_set_buffer(inst, intbuf_types[i]);
+   if (IS_V4(inst->core))
+   arr_sz = ARRAY_SIZE(intbuf_types_4xx);
+   else
+   arr_sz = ARRAY_SIZE(intbuf_types_1xx);
+
+   for (i = 0; i < arr_sz; i++) {
+   ret = intbufs_set_buffer(inst,
+   IS_V4(inst->core) ? intbuf_types_4xx[i] :
+   intbuf_types_1xx[i]);
if (ret)
goto error;
}
@@ -257,12 +273,11 @@ static int load_scale_clocks(struct venus_core *core)
 
 set_freq:
 
-   if (core->res->hfi_version == HFI_VERSION_3XX) {
-   ret = clk_set_rate(clk, freq);
+   ret = clk_set_rate(clk, freq);
+
+   if (IS_V3(core) || IS_V4(core)) {
ret |= clk_set_rate(core->core0_clk, freq);
ret |= clk_set_rate(core->core1_clk, freq);
-   } else {
-   ret = clk_set_rate(clk, freq);
}
 
if (ret) {
diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h 
b/drivers/media/platform/qcom/venus/hfi_helper.h
index 55d8eb21403a..1bc5aab1ce6b 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -121,6 +121,7 @@
 #define HFI_EXTRADATA_METADATA_FILLER  0x7fe2
 
 #define HFI_INDEX_EXTRADATA_INPUT_CROP 0x070e
+#define HFI_INDEX_EXTRADATA_OUTPUT_CROP0x070f
 #define HFI_INDEX_EXTRADATA_DIGITAL_ZOOM   0x0710
 #define HFI_INDEX_EXTRADATA_ASPECT_RATIO   0x7f13
 
@@ -376,13 +377,18 @@
 #define HFI_BUFFER_OUTPUT2 0x3
 #define HFI_BUFFER_INTERNAL_PERSIST0x4
 #define HFI_BUFFER_INTERNAL_PERSIST_1  0x5
-#define HFI_BUFFER_INTERNAL_SCRATCH0x101
-#define HFI_BUFFER_EXTRADATA_INPUT 0x102
-#define HFI_BUFFER_EXTRADATA_OUTPUT0x103
-#define HFI_BUFFER_EXTRADATA_OUTPUT2   0x104
-#define HFI_BUFFER_INTERNAL_SCRATCH_1  0x105
-#define HFI_BUFFER_INTERNAL_SCRATCH_2  0x106
-
+#define HFI_BUFFER_INTERNAL_SCRATCH(ver)   \
+   (((ver) == HFI_VERSION_4XX) ? 0x6 : 0x101)
+#define HFI_BUFFER_INTERNAL_SCRATCH_1(ver) \
+   (((ver) == HFI_VERSION_4XX) ? 0x7 : 0x105)
+#define HFI_BUFFER_INTERNAL_SCRATCH_2(ver) \
+   (((ver) == HFI_VERSION_4XX) ? 0x8 :

Re: [PATCH v2 02/29] venus: hfi: preparation to support venus 4xx

Re: [PATCH v2 02/29] venus: hfi: preparation to support venus 4xx

[PATCH v2 02/29] venus: hfi: preparation to support venus 4xx

3 matches

Site Navigation

Mail list logo

Footer information