Re: [PATCH v2 1/3] media: vimc: Support multiple media bus codes for each pixelformat

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

 



Hi Helen,

thanks for the review.

Some comments below.

On Mon, Mar 30, 2020 at 04:36:45PM -0300, Helen Koike wrote:
> 
> Hi Nícolas,
> 
> thank you for the patch.
> 
> The series looks good in general, just minor comments below.
> 
> On 3/26/20 6:47 PM, Nícolas F. R. A. Prado wrote:
> > Change vimc_pix_map_list to allow multiple media bus codes to map to the
> > same pixelformat, making it possible to add media bus codes for which
> > there are no pixelformat.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> > - Fix vimc_mbus_code_by_index not checking code array bounds
> > - Fix array formatting
> > - Rename variables
> > - Change code array size
> > - Add comment about vimc_mbus_code_by_index return value
> >
> >  drivers/media/platform/vimc/vimc-common.c | 70 ++++++++++++++---------
> >  drivers/media/platform/vimc/vimc-common.h | 11 +++-
> >  drivers/media/platform/vimc/vimc-scaler.c | 10 +++-
> >  drivers/media/platform/vimc/vimc-sensor.c |  6 +-
> >  4 files changed, 65 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> > index c95c17c048f2..119846f3eaa5 100644
> > --- a/drivers/media/platform/vimc/vimc-common.c
> > +++ b/drivers/media/platform/vimc/vimc-common.c
> > @@ -19,19 +19,19 @@ static const struct vimc_pix_map vimc_pix_map_list[] = {
> >
> >  	/* RGB formats */
> >  	{
> > -		.code = MEDIA_BUS_FMT_BGR888_1X24,
> > +		.code = { MEDIA_BUS_FMT_BGR888_1X24 },
> >  		.pixelformat = V4L2_PIX_FMT_BGR24,
> >  		.bpp = 3,
> >  		.bayer = false,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_RGB888_1X24,
> > +		.code = { MEDIA_BUS_FMT_RGB888_1X24 },
> >  		.pixelformat = V4L2_PIX_FMT_RGB24,
> >  		.bpp = 3,
> >  		.bayer = false,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_ARGB8888_1X32,
> > +		.code = { MEDIA_BUS_FMT_ARGB8888_1X32 },
> >  		.pixelformat = V4L2_PIX_FMT_ARGB32,
> >  		.bpp = 4,
> >  		.bayer = false,
> > @@ -39,49 +39,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = {
> >
> >  	/* Bayer formats */
> >  	{
> > -		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SBGGR8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SBGGR8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SGBRG8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SGBRG8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SGRBG8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SGRBG8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SRGGB8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SRGGB8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > +		.code = { MEDIA_BUS_FMT_SBGGR10_1X10 },
> >  		.pixelformat = V4L2_PIX_FMT_SBGGR10,
> >  		.bpp = 2,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > +		.code = { MEDIA_BUS_FMT_SGBRG10_1X10 },
> >  		.pixelformat = V4L2_PIX_FMT_SGBRG10,
> >  		.bpp = 2,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > +		.code = { MEDIA_BUS_FMT_SGRBG10_1X10 },
> >  		.pixelformat = V4L2_PIX_FMT_SGRBG10,
> >  		.bpp = 2,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > +		.code = { MEDIA_BUS_FMT_SRGGB10_1X10 },
> >  		.pixelformat = V4L2_PIX_FMT_SRGGB10,
> >  		.bpp = 2,
> >  		.bayer = true,
> > @@ -89,25 +89,25 @@ static const struct vimc_pix_map vimc_pix_map_list[] = {
> >
> >  	/* 10bit raw bayer a-law compressed to 8 bits */
> >  	{
> > -		.code = MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SBGGR10ALAW8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SGBRG10ALAW8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SGRBG10ALAW8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SRGGB10ALAW8,
> >  		.bpp = 1,
> >  		.bayer = true,
> > @@ -115,49 +115,49 @@ static const struct vimc_pix_map vimc_pix_map_list[] = {
> >
> >  	/* 10bit raw bayer DPCM compressed to 8 bits */
> >  	{
> > -		.code = MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SBGGR10DPCM8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SGBRG10DPCM8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SGRBG10DPCM8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8,
> > +		.code = { MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8 },
> >  		.pixelformat = V4L2_PIX_FMT_SRGGB10DPCM8,
> >  		.bpp = 1,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > +		.code = { MEDIA_BUS_FMT_SBGGR12_1X12 },
> >  		.pixelformat = V4L2_PIX_FMT_SBGGR12,
> >  		.bpp = 2,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > +		.code = { MEDIA_BUS_FMT_SGBRG12_1X12 },
> >  		.pixelformat = V4L2_PIX_FMT_SGBRG12,
> >  		.bpp = 2,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > +		.code = { MEDIA_BUS_FMT_SGRBG12_1X12 },
> >  		.pixelformat = V4L2_PIX_FMT_SGRBG12,
> >  		.bpp = 2,
> >  		.bayer = true,
> >  	},
> >  	{
> > -		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > +		.code = { MEDIA_BUS_FMT_SRGGB12_1X12 },
> >  		.pixelformat = V4L2_PIX_FMT_SRGGB12,
> >  		.bpp = 2,
> >  		.bayer = true,
> > @@ -182,13 +182,31 @@ const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i)
> >  	return &vimc_pix_map_list[i];
> >  }
> >
> > +const u32 vimc_mbus_code_by_index(unsigned int index)
> > +{
> > +	unsigned int i, j;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) {
> > +		for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) {
> > +			if (vimc_pix_map_list[i].code[j]) {
> 
> Can this be false?

Actually it can, but after you asked I realized this code could be way clearer.

When vimc_pix_map_list[i].code[j] is 0, it means that this is an unused value of
the array, so we should skip to the next pixelformat.
I think writing it this way instead would be better, what do you think?

        for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) {
                for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) {
                        if (!vimc_pix_map_list[i].code[j])
                                break;
 
                        if (!index)
                                return vimc_pix_map_list[i].code[j];
                        index--;

> 
> > +				if (!index)
> > +					return vimc_pix_map_list[i].code[j];
> > +				index--;
> > +			}
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> >  const struct vimc_pix_map *vimc_pix_map_by_code(u32 code)
> >  {
> > -	unsigned int i;
> > +	unsigned int i, j;
> >
> >  	for (i = 0; i < ARRAY_SIZE(vimc_pix_map_list); i++) {
> > -		if (vimc_pix_map_list[i].code == code)
> > -			return &vimc_pix_map_list[i];
> > +		for (j = 0; j < ARRAY_SIZE(vimc_pix_map_list[i].code); j++) {
> > +			if (vimc_pix_map_list[i].code[j] == code)
> > +				return &vimc_pix_map_list[i];
> > +		}
> >  	}
> >  	return NULL;
> >  }
> > diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> > index 616d5a6b0754..585441694c86 100644
> > --- a/drivers/media/platform/vimc/vimc-common.h
> > +++ b/drivers/media/platform/vimc/vimc-common.h
> > @@ -69,7 +69,7 @@ do {									\
> >   * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp)
> >   */
> >  struct vimc_pix_map {
> > -	unsigned int code;
> > +	unsigned int code[1];
> >  	unsigned int bpp;
> >  	u32 pixelformat;
> >  	bool bayer;
> > @@ -172,6 +172,15 @@ void vimc_sen_release(struct vimc_ent_device *ved);
> >   */
> >  const struct vimc_pix_map *vimc_pix_map_by_index(unsigned int i);
> >
> > +/**
> > + * vimc_mbus_code_by_index - get mbus code by its index
> > + *
> > + * @index:		index of the mbus code in vimc_pix_map_list
> > + *
> > + * Returns 0 if no mbus code is found for the given index.
> > + */
> > +const u32 vimc_mbus_code_by_index(unsigned int index);
> > +
> >  /**
> >   * vimc_pix_map_by_code - get vimc_pix_map struct by media bus code
> >   *
> > diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> > index 7521439747c5..6bac1fa65a6f 100644
> > --- a/drivers/media/platform/vimc/vimc-scaler.c
> > +++ b/drivers/media/platform/vimc/vimc-scaler.c
> > @@ -110,13 +110,19 @@ static int vimc_sca_enum_mbus_code(struct v4l2_subdev *sd,
> >  				   struct v4l2_subdev_pad_config *cfg,
> >  				   struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > -	const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index);
> > +	const u32 mbus_code = vimc_mbus_code_by_index(code->index);
> > +	const struct vimc_pix_map *vpix;
> > +
> > +	if (!mbus_code)
> > +		return -EINVAL;
> > +
> > +	vpix = vimc_pix_map_by_code(mbus_code);
> >
> >  	/* We don't support bayer format */
> >  	if (!vpix || vpix->bayer)
> >  		return -EINVAL;
> >
> > -	code->code = vpix->code;
> > +	code->code = mbus_code;
> 
> no need to change this.

This change is actually needed, because after this patch, the code property of
vimc_pix_map_list is an array, so there isn't a 1 to 1 relation between mbus
code and pixmap format anymore.
Since we already got the mbus code by index through
vimc_mbus_code_by_index(code->index), we just use it.

> 
> >
> >  	return 0;
> >  }
> > diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> > index 92daee58209e..b8bd430809c1 100644
> > --- a/drivers/media/platform/vimc/vimc-sensor.c
> > +++ b/drivers/media/platform/vimc/vimc-sensor.c
> > @@ -52,12 +52,12 @@ static int vimc_sen_enum_mbus_code(struct v4l2_subdev *sd,
> >  				   struct v4l2_subdev_pad_config *cfg,
> >  				   struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > -	const struct vimc_pix_map *vpix = vimc_pix_map_by_index(code->index);
> > +	const u32 mbus_code = vimc_mbus_code_by_index(code->index);
> >
> > -	if (!vpix)
> > +	if (!mbus_code)
> >  		return -EINVAL;
> >
> > -	code->code = vpix->code;
> > +	code->code = mbus_code;
> >
> >  	return 0;
> >  }
> >
> 
> With these changes
> 
> Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
> 
> Regards,
> Helen

Thank you,
Nícolas





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux