RE: [PATCH v13 4/5] media: renesas: vsp1: Add VSP1_HAS_NON_ZERO_LBA feature bit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux