Re: [PATCH v2 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Kieran Bingham]

On 14/07/17 00:31, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Thursday 13 Jul 2017 18:49:19 Kieran Bingham wrote:
>> On 26/06/17 19:12, Laurent Pinchart wrote:
>>> New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
>>> as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
>>> them in the VSP device info table.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> 
>>
>> Code in the patch looks OK - but I can't see where the difference between
>> the horizontal widths are supported between VSPD H3/VC
>>
>> I see this in the datasheet: (32.1.1.6 in this particular part)
>>
>> Direct connection to display module
>> — Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car M3-W/ R-Car
>> M3-N]
>> — Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car
>> D3/R-Car E3]
>>
>> Do we have this information encoded anywhere? or are they just talking about
>> maximum performance capability there?
> 
> No, we don't. It's a limit that we should have. I think we should fix that in 
> a separate patch, as the 4096 pixels limit isn't implemented either.

I'm not so worried about these limits (unless they cause the hardware to hang),
but I think they should be encoded somewhere, and I would certainly count that
as a separate patch.

Of course (excluding pipelines using BRU/BRS) the partition algorithm could
provide the capability to support image processing beyond limitations of the
pipeline maximum size...

But this can't cater for throughput limitations of bandwidth so there will be a
limit to how big an image we really want to support ...

Non realtime processing of large megapixel images might be relevant though - but
still not a use case to worry about here.

>> Also some features that are implied as supported aren't mentioned - but
>> that's not a blocker to adding in the initial devices at all.
>>
>> Therefore:
>>
>> Reviewed-by: Kieran Bingham 
>>
>>> ---
>>>
>>>  drivers/media/platform/vsp1/vsp1_drv.c  | 24 
>>>  drivers/media/platform/vsp1/vsp1_regs.h | 15 +--
>>>  2 files changed, 37 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
>>> b/drivers/media/platform/vsp1/vsp1_drv.c index 6a9aeb71aedf..c4f2ac61f7d2
>>> 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_drv.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>>> @@ -710,6 +710,14 @@ static const struct vsp1_device_info
>>> vsp1_device_infos[] = {> 
>>> .num_bru_inputs = 5,
>>> .uapi = true,
>>> }, {
>>> +   .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
>>> +   .model = "VSP2-BS",
>>> +   .gen = 3,
>>> +   .features = VSP1_HAS_BRS,
>>
>> 32.1.1.5 implies:
>> | VSP1_HAS_WPF_VFLIP
>>
>> But Figure 32.5 implies that it doesn't ...
> 
> The figures only tell whether the full combination of rotation and H/V flip 
> is 
> available. I think you're right, I'll add VSP1_HAS_WPF_VFLIP.
> 
>> Figure 32.5 also implies that | VSP1_HAS_CLU is there too on both RPF0, and
>> RPF1
> 
> Note that CLUT != CLU. I know it's confusing :-)

Oh ! :-S ... /me goes back to the datasheet ...

> 
>>> +   .rpf_count = 2,
>>> +   .wpf_count = 1,
>>> +   .uapi = true,
>>> +   }, {
>>> .version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
>>> .model = "VSP2-D",
>>> .gen = 3,
>>> @@ -717,6 +725,22 @@ static const struct vsp1_device_info
>>> vsp1_device_infos[] = {> 
>>> .rpf_count = 5,
>>> .wpf_count = 2,
>>> .num_bru_inputs = 5,
>>> +   }, {
>>> +   .version = VI6_IP_VERSION_MODEL_VSPD_V3,
>>> +   .model = "VSP2-D",
>>> +   .gen = 3,
>>> +   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
>>> +   .rpf_count = 5,
>>> +   .wpf_count = 1,
>>> +   .num_bru_inputs = 5,
>>> +   }, {
>>> +   .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
>>> +   .model = "VSP2-DL",
>>> +   .gen = 3,
>>> +   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
>>
>> Hrm. 32.1.1.7 says:
>> — Vertical flipping in case of output to memory.
>> So thats some sort of a conditional : | VSP1_HAS_WPF_VFLIP
>>
>> So looking at this and the settings of the existing models, I guess it looks
>> like we don't support flip if we have an LIF output (as that would then be
>> unsupported)
> 
> On Gen3 vertical flipping seems to always be supported, unlike on Gen2 where 
> VSPD is specifically documented as not supporting vertical flipping. We could 
> add the WFLIP on all VSP2-D* instances. This would create a corresponding 
> control, which wouldn't do much harm as the VSPD instances on Gen3 are not 
> exposed to userspace, but that would waste a bit of memory for no good 
> purpose 
> (beside correctness I suppose). I wonder if that's worth it, what do you 
> think 
> ? If 

Re: [PATCH v2 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Laurent Pinchart]

Hi Kieran,

On Thursday 13 Jul 2017 18:49:19 Kieran Bingham wrote:
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
> > as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
> > them in the VSP device info table.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Code in the patch looks OK - but I can't see where the difference between
> the horizontal widths are supported between VSPD H3/VC
> 
> I see this in the datasheet: (32.1.1.6 in this particular part)
> 
> Direct connection to display module
> — Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car M3-W/ R-Car
> M3-N]
> — Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car
> D3/R-Car E3]
> 
> Do we have this information encoded anywhere? or are they just talking about
> maximum performance capability there?

No, we don't. It's a limit that we should have. I think we should fix that in 
a separate patch, as the 4096 pixels limit isn't implemented either.

> Also some features that are implied as supported aren't mentioned - but
> that's not a blocker to adding in the initial devices at all.
> 
> Therefore:
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_drv.c  | 24 
> >  drivers/media/platform/vsp1/vsp1_regs.h | 15 +--
> >  2 files changed, 37 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > b/drivers/media/platform/vsp1/vsp1_drv.c index 6a9aeb71aedf..c4f2ac61f7d2
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > @@ -710,6 +710,14 @@ static const struct vsp1_device_info
> > vsp1_device_infos[] = {> 
> > .num_bru_inputs = 5,
> > .uapi = true,
> > }, {
> > +   .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
> > +   .model = "VSP2-BS",
> > +   .gen = 3,
> > +   .features = VSP1_HAS_BRS,
> 
> 32.1.1.5 implies:
> | VSP1_HAS_WPF_VFLIP
> 
> But Figure 32.5 implies that it doesn't ...

The figures only tell whether the full combination of rotation and H/V flip is 
available. I think you're right, I'll add VSP1_HAS_WPF_VFLIP.

> Figure 32.5 also implies that | VSP1_HAS_CLU is there too on both RPF0, and
> RPF1

Note that CLUT != CLU. I know it's confusing :-)

> > +   .rpf_count = 2,
> > +   .wpf_count = 1,
> > +   .uapi = true,
> > +   }, {
> > .version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
> > .model = "VSP2-D",
> > .gen = 3,
> > @@ -717,6 +725,22 @@ static const struct vsp1_device_info
> > vsp1_device_infos[] = {> 
> > .rpf_count = 5,
> > .wpf_count = 2,
> > .num_bru_inputs = 5,
> > +   }, {
> > +   .version = VI6_IP_VERSION_MODEL_VSPD_V3,
> > +   .model = "VSP2-D",
> > +   .gen = 3,
> > +   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
> > +   .rpf_count = 5,
> > +   .wpf_count = 1,
> > +   .num_bru_inputs = 5,
> > +   }, {
> > +   .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
> > +   .model = "VSP2-DL",
> > +   .gen = 3,
> > +   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
> 
> Hrm. 32.1.1.7 says:
> — Vertical flipping in case of output to memory.
> So thats some sort of a conditional : | VSP1_HAS_WPF_VFLIP
> 
> So looking at this and the settings of the existing models, I guess it looks
> like we don't support flip if we have an LIF output (as that would then be
> unsupported)

On Gen3 vertical flipping seems to always be supported, unlike on Gen2 where 
VSPD is specifically documented as not supporting vertical flipping. We could 
add the WFLIP on all VSP2-D* instances. This would create a corresponding 
control, which wouldn't do much harm as the VSPD instances on Gen3 are not 
exposed to userspace, but that would waste a bit of memory for no good purpose 
(beside correctness I suppose). I wonder if that's worth it, what do you think 
? If so, VSP2-D should be fixed too, so I'd prefer doing that in a separate 
patch.

> > +   .rpf_count = 5,
> > +   .wpf_count = 2,
> > +   .num_bru_inputs = 5,
> > },
> >  };
> > 

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Kieran Bingham]

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:
> New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
> as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
> them in the VSP device info table.
> 
> Signed-off-by: Laurent Pinchart 

Code in the patch looks OK - but I can't see where the difference between the
horizontal widths are supported between VSPD H3/VC

I see this in the datasheet: (32.1.1.6 in this particular part)

Direct connection to display module
— Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car M3-W/ R-Car 
M3-N]
— Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car
D3/R-Car E3]

Do we have this information encoded anywhere? or are they just talking about
maximum performance capability there?

Also some features that are implied as supported aren't mentioned - but that's
not a blocker to adding in the initial devices at all.

Therefore:

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_drv.c  | 24 
>  drivers/media/platform/vsp1/vsp1_regs.h | 15 +--
>  2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 6a9aeb71aedf..c4f2ac61f7d2 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -710,6 +710,14 @@ static const struct vsp1_device_info vsp1_device_infos[] 
> = {
>   .num_bru_inputs = 5,
>   .uapi = true,
>   }, {
> + .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
> + .model = "VSP2-BS",
> + .gen = 3,
> + .features = VSP1_HAS_BRS,

32.1.1.5 implies:

| VSP1_HAS_WPF_VFLIP

But Figure 32.5 implies that it doesn't ...

Figure 32.5 also implies that | VSP1_HAS_CLU is there too on both RPF0, and RPF1

> + .rpf_count = 2,
> + .wpf_count = 1,
> + .uapi = true,
> + }, {
>   .version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
>   .model = "VSP2-D",
>   .gen = 3,
> @@ -717,6 +725,22 @@ static const struct vsp1_device_info vsp1_device_infos[] 
> = {
>   .rpf_count = 5,
>   .wpf_count = 2,
>   .num_bru_inputs = 5,
> + }, {
> + .version = VI6_IP_VERSION_MODEL_VSPD_V3,
> + .model = "VSP2-D",
> + .gen = 3,
> + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
> + .rpf_count = 5,
> + .wpf_count = 1,
> + .num_bru_inputs = 5,
> + }, {
> + .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
> + .model = "VSP2-DL",
> + .gen = 3,
> + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,

Hrm. 32.1.1.7 says:
— Vertical flipping in case of output to memory.
So thats some sort of a conditional : | VSP1_HAS_WPF_VFLIP

So looking at this and the settings of the existing models, I guess it looks
like we don't support flip if we have an LIF output (as that would then be
unsupported)

> + .rpf_count = 5,
> + .wpf_count = 2,
> + .num_bru_inputs = 5,
>   },
>  };
>  
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h 
> b/drivers/media/platform/vsp1/vsp1_regs.h
> index 744217e020b9..ab439a60a100 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -699,9 +699,20 @@
>  #define VI6_IP_VERSION_MODEL_VSPBD_GEN3  (0x15 << 8)
>  #define VI6_IP_VERSION_MODEL_VSPBC_GEN3  (0x16 << 8)
>  #define VI6_IP_VERSION_MODEL_VSPD_GEN3   (0x17 << 8)
> +#define VI6_IP_VERSION_MODEL_VSPD_V3 (0x18 << 8)
> +#define VI6_IP_VERSION_MODEL_VSPDL_GEN3  (0x19 << 8)
> +#define VI6_IP_VERSION_MODEL_VSPBS_GEN3  (0x1a << 8)
>  #define VI6_IP_VERSION_SOC_MASK  (0xff << 0)
> -#define VI6_IP_VERSION_SOC_H (0x01 << 0)
> -#define VI6_IP_VERSION_SOC_M (0x02 << 0)
> +#define VI6_IP_VERSION_SOC_H2(0x01 << 0)
> +#define VI6_IP_VERSION_SOC_V2H   (0x01 << 0)
> +#define VI6_IP_VERSION_SOC_V3M   (0x01 << 0)
> +#define VI6_IP_VERSION_SOC_M2(0x02 << 0)
> +#define VI6_IP_VERSION_SOC_M3W   (0x02 << 0)
> +#define VI6_IP_VERSION_SOC_V3H   (0x02 << 0)
> +#define VI6_IP_VERSION_SOC_H3(0x03 << 0)
> +#define VI6_IP_VERSION_SOC_D3(0x04 << 0)
> +#define VI6_IP_VERSION_SOC_M3N   (0x04 << 0)
> +#define VI6_IP_VERSION_SOC_E3(0x04 << 0)
>  
>  /* 
> -
>   * RPF CLUT Registers
> 

[PATCH v2 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Laurent Pinchart]

New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
them in the VSP device info table.

Signed-off-by: Laurent Pinchart 
---
 drivers/media/platform/vsp1/vsp1_drv.c  | 24 
 drivers/media/platform/vsp1/vsp1_regs.h | 15 +--
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
b/drivers/media/platform/vsp1/vsp1_drv.c
index 6a9aeb71aedf..c4f2ac61f7d2 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -710,6 +710,14 @@ static const struct vsp1_device_info vsp1_device_infos[] = 
{
.num_bru_inputs = 5,
.uapi = true,
}, {
+   .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
+   .model = "VSP2-BS",
+   .gen = 3,
+   .features = VSP1_HAS_BRS,
+   .rpf_count = 2,
+   .wpf_count = 1,
+   .uapi = true,
+   }, {
.version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
.model = "VSP2-D",
.gen = 3,
@@ -717,6 +725,22 @@ static const struct vsp1_device_info vsp1_device_infos[] = 
{
.rpf_count = 5,
.wpf_count = 2,
.num_bru_inputs = 5,
+   }, {
+   .version = VI6_IP_VERSION_MODEL_VSPD_V3,
+   .model = "VSP2-D",
+   .gen = 3,
+   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
+   .rpf_count = 5,
+   .wpf_count = 1,
+   .num_bru_inputs = 5,
+   }, {
+   .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
+   .model = "VSP2-DL",
+   .gen = 3,
+   .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
+   .rpf_count = 5,
+   .wpf_count = 2,
+   .num_bru_inputs = 5,
},
 };
 
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h 
b/drivers/media/platform/vsp1/vsp1_regs.h
index 744217e020b9..ab439a60a100 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -699,9 +699,20 @@
 #define VI6_IP_VERSION_MODEL_VSPBD_GEN3(0x15 << 8)
 #define VI6_IP_VERSION_MODEL_VSPBC_GEN3(0x16 << 8)
 #define VI6_IP_VERSION_MODEL_VSPD_GEN3 (0x17 << 8)
+#define VI6_IP_VERSION_MODEL_VSPD_V3   (0x18 << 8)
+#define VI6_IP_VERSION_MODEL_VSPDL_GEN3(0x19 << 8)
+#define VI6_IP_VERSION_MODEL_VSPBS_GEN3(0x1a << 8)
 #define VI6_IP_VERSION_SOC_MASK(0xff << 0)
-#define VI6_IP_VERSION_SOC_H   (0x01 << 0)
-#define VI6_IP_VERSION_SOC_M   (0x02 << 0)
+#define VI6_IP_VERSION_SOC_H2  (0x01 << 0)
+#define VI6_IP_VERSION_SOC_V2H (0x01 << 0)
+#define VI6_IP_VERSION_SOC_V3M (0x01 << 0)
+#define VI6_IP_VERSION_SOC_M2  (0x02 << 0)
+#define VI6_IP_VERSION_SOC_M3W (0x02 << 0)
+#define VI6_IP_VERSION_SOC_V3H (0x02 << 0)
+#define VI6_IP_VERSION_SOC_H3  (0x03 << 0)
+#define VI6_IP_VERSION_SOC_D3  (0x04 << 0)
+#define VI6_IP_VERSION_SOC_M3N (0x04 << 0)
+#define VI6_IP_VERSION_SOC_E3  (0x04 << 0)
 
 /* 
-
  * RPF CLUT Registers
-- 
Regards,

Laurent Pinchart