Hi Laurent,
Thanks for the feedback.
> Subject: Re: [PATCH v13 4/5] media: renesas: vsp1: Add
> VSP1_HAS_NON_ZERO_LBA feature bit
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Thu, Aug 25, 2022 at 02:21:43PM +0100, Biju Das wrote:
> > As per HW manual V3M and RZ/G2L SoCs has nonzero LIF buffer
> > attributes. So, introduce a feature bit for handling the same.
> >
> > This patch also adds separate device info structure for V3M and V3H
> > SoCs, as both these SoCs share the same model ID, but V3H does not
> > have VSP1_HAS_NON_ZERO_LBA feature bit.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v12->v13:
> > * No change
> > v11->v12:
> > * No change
> > v10->v11:
> > * No change
> > v9->v10:
> > * No change
> > v8->v9:
> > * Used generic check for matching SoCs with LBA feature.
> > v8:
> > * New patch
> > ---
> > drivers/media/platform/renesas/vsp1/vsp1.h | 1 +
> > drivers/media/platform/renesas/vsp1/vsp1_drv.c | 18
> > +++++++++++++++++- drivers/media/platform/renesas/vsp1/vsp1_lif.c |
> > 3 +--
> > 3 files changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h
> > b/drivers/media/platform/renesas/vsp1/vsp1.h
> > index ff4435705abb..2f6f0c6ae555 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1.h
> > @@ -55,6 +55,7 @@ struct vsp1_uif;
> > #define VSP1_HAS_HGT BIT(8)
> > #define VSP1_HAS_BRS BIT(9)
> > #define VSP1_HAS_EXT_DL BIT(10)
> > +#define VSP1_HAS_NON_ZERO_LBA BIT(11)
> >
> > struct vsp1_device_info {
> > u32 version;
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > index 223dd5f557ba..c0d814d9044e 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > @@ -787,6 +787,7 @@ static const struct vsp1_device_info
> vsp1_device_infos[] = {
> > }, {
> > .version = VI6_IP_VERSION_MODEL_VSPD_V3,
> > .model = "VSP2-D",
> > + .soc = VI6_IP_VERSION_SOC_V3H,
> > .gen = 3,
> > .features = VSP1_HAS_BRS | VSP1_HAS_BRU,
> > .lif_count = 1,
> > @@ -794,6 +795,17 @@ static const struct vsp1_device_info
> vsp1_device_infos[] = {
> > .uif_count = 1,
> > .wpf_count = 1,
> > .num_bru_inputs = 5,
> > + }, {
> > + .version = VI6_IP_VERSION_MODEL_VSPD_V3,
> > + .model = "VSP2-D",
> > + .soc = VI6_IP_VERSION_SOC_V3M,
> > + .gen = 3,
> > + .features = VSP1_HAS_BRS | VSP1_HAS_BRU |
> VSP1_HAS_NON_ZERO_LBA,
> > + .lif_count = 1,
> > + .rpf_count = 5,
> > + .uif_count = 1,
> > + .wpf_count = 1,
> > + .num_bru_inputs = 5,
> > }, {
> > .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
> > .model = "VSP2-DL",
> > @@ -837,8 +849,12 @@ static const struct vsp1_device_info
> *vsp1_lookup_info(struct vsp1_device *vsp1)
> > for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > info = &vsp1_device_infos[i];
> >
> > - if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info-
> >version)
> > + if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info-
> >version) {
> > + if (info->soc &&
> > + ((vsp1->version & VI6_IP_VERSION_SOC_MASK) !=
> info->soc))
> > + continue;
> > return info;
> > + }
>
> How about making this more readable ?
>
> u32 model;
> u32 soc;
>
> ...
>
> model = vsp1->version & VI6_IP_VERSION_MODEL_MASK;
> soc = vsp1->version & VI6_IP_VERSION_SOC_MASK;
>
> for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> info = &vsp1_device_infos[i];
>
> if (model == info->version && (!info->soc || soc == info-
> >soc))
> return info;
> }
OK, it is much better logic. Will fix this in next version.
Cheers,
Biju
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> > }
> >
> > dev_err(vsp1->dev, "unsupported IP version 0x%08x\n",
> > vsp1->version); diff --git
> > a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > index 6a6857ac9327..9adb892edcdc 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > @@ -135,8 +135,7 @@ static void lif_configure_stream(struct
> vsp1_entity *entity,
> > * may appear on the output). The value required by the manual is
> not
> > * explained but is likely a buffer size or threshold.
> > */
> > - if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > - (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> > + if (vsp1_feature(entity->vsp1, VSP1_HAS_NON_ZERO_LBA))
> > vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> > VI6_LIF_LBA_LBA0 |
> > (1536 << VI6_LIF_LBA_LBA1_SHIFT));
>
> --
> Regards,
>
> Laurent Pinchart
