Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 1/3] media: vimc: Support multiple media bus codes for
 each pixelformat
To:     =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= 
        <nfraprado@protonmail.com>
Cc:     linux-media@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        linux-kernel@vger.kernel.org, lkcamp@lists.libreplanetbr.org
References: <20200326214730.2449707-1-nfraprado@protonmail.com>
 <20200326214730.2449707-2-nfraprado@protonmail.com>
 <e41520f6-1d2e-9d36-3693-1e6d950ecf2f@collabora.com>
 <20200420203646.6hhbsb57ybwh6u76@ArchWay.local>
From:   Helen Koike <helen.koike@collabora.com>
Message-ID: <41e25337-1f23-8650-37fb-745321ab2e9e@collabora.com>
Date:   Mon, 20 Apr 2020 17:50:30 -0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200420203646.6hhbsb57ybwh6u76@ArchWay.local>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Nícolas,

On 4/20/20 5:36 PM, Nícolas F. R. A. Prado wrote:
> 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@protonmail.com>
>>> ---
>>>
>>> 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--;
> 

Looks better, I prefer reducing indentation.

>>
>>> +				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.

Make sense, thank you for your explanation.

Regards,
Helen

> 
>>
>>>
>>>  	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@collabora.com>
>>
>> Regards,
>> Helen
> 
> Thank you,
> Nícolas
>