| Message ID | 20170626181226.29575-9-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 <laurent.pinchart+renesas@ideasonboard.com> 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 <kieran.bingham+renesas@ideasonboard.com> > --- > 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 >
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 > > <laurent.pinchart+renesas@ideasonboard.com> > > 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 <kieran.bingham+renesas@ideasonboard.com> > > > --- > > > > 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]
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 >>> <laurent.pinchart+renesas@ideasonboard.com> >> >> 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 <kieran.bingham+renesas@ideasonboard.com> >> >>> --- >>> >>> 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 so, VSP2-D should be fixed too, so I'd prefer doing that in a separate > patch. I think it's only worth specifying the capability if it can be used. If we have no way to set the control of a DU pipeline, then there's no point adding the feature, and if the entity does not expose UAPI, then also there is no point adding it. We could set the feature flag but gate on has_uapi() or such ... This is likely a small corner case so it can wait until we have a feature request or something related. -- Kieran > >>> + .rpf_count = 5, >>> + .wpf_count = 2, >>> + .num_bru_inputs = 5, >>> }, >>> }; >>> > > [snip] >
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
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 <laurent.pinchart+renesas@ideasonboard.com> --- drivers/media/platform/vsp1/vsp1_drv.c | 24 ++++++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 15 +++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-)