Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Josh Wu <josh.wu@atmel.com>]

Dear Guennadi,

On 10/19/2015 9:48 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Wed, 14 Oct 2015, Josh Wu wrote:
>
>> Hi, Dear Guennadi
>>
>> Thanks for the review.
>>
>> On 10/5/2015 12:43 AM, Guennadi Liakhovetski wrote:
>>> Hi Josh,
>>>
>>> On Tue, 22 Sep 2015, Josh Wu wrote:
>>>
>>>> we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
>>>> sensor output and Atmel ISI output format.
>>>>
>>>> Current there are two cases Atmel ISI supported:
>>>>     1. Atmel ISI outputs YUYV format.
>>>>        This case we need to setup YCC_SWAP according to sensor output
>>>>        format.
>>>>     2. Atmel ISI output a pass-through formats, which means no swap.
>>>>        Just setup YCC_SWAP as default with no swap.
>>>>
>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>>> ---
>>>>
>>>>    drivers/media/platform/soc_camera/atmel-isi.c | 43
>>>> ++++++++++++++++++++-------
>>>>    1 file changed, 33 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> index 45e304a..df64294 100644
>>>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>>>>    	return readl(isi->regs + reg);
>>>>    }
>>>>    +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
>>>> +		const struct soc_camera_format_xlate *xlate)
>>>> +{
>>> This function will be called only for the four media codes from the
>>> switch-case statement below, namely for
>>>
>>> MEDIA_BUS_FMT_VYUY8_2X8
>>> MEDIA_BUS_FMT_UYVY8_2X8
>>> MEDIA_BUS_FMT_YVYU8_2X8
>>> MEDIA_BUS_FMT_YUYV8_2X8
>>>
>>>> +	/* By default, no swap for the codec path of Atmel ISI. So codec
>>>> +	* output is same as sensor's output.
>>>> +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
>>>> +	* And if sensor's output is UYVY, then codec outputs UYVY.
>>>> +	*/
>>>> +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
>>> Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for
>>> MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't
>>> think this initialisation is really justified.
>> This default initial value is for all host_fmt_fourcc case. Not just for
>> V4L2_PIX_FMT_YUYV this case.
> Right, yes, missed this. How about this then:
>
> 	if (xlate->host_fmt->fourcc != V4L2_PIX_FMT_YUYV)
> 		return ISI_CFG2_YCC_SWAP_DEFAULT;
>
> 	switch (xlate->code) {
> 	case MEDIA_BUS_FMT_VYUY8_2X8:
> 		return ISI_CFG2_YCC_SWAP_MODE_3;
> 	case MEDIA_BUS_FMT_UYVY8_2X8:
> 		return ISI_CFG2_YCC_SWAP_MODE_2;
> 	case MEDIA_BUS_FMT_YVYU8_2X8:
> 		return ISI_CFG2_YCC_SWAP_MODE_1;
> 	}
>
> 	return ISI_CFG2_YCC_SWAP_DEFAULT;
>
> Or even exactly as you have it in your patch only remove the cfg2_yuv_swap
> variable and do "return" directly after each "case MEDIA_..." and one more
> with SWAP_DEFAULT in the end.

Right, that's exactly what I want to do. That looks very neat. Thanks 
for your advice.
I'll fix it in v2.

>
>>>> +
>>>> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
>>>> +		/* all convert to YUYV */
>>>> +		switch (xlate->code) {
>>>> +		case MEDIA_BUS_FMT_VYUY8_2X8:
>>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
>>>> +			break;
>>>> +		case MEDIA_BUS_FMT_UYVY8_2X8:
>>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
>>>> +			break;
>>>> +		case MEDIA_BUS_FMT_YVYU8_2X8:
>>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return cfg2_yuv_swap;
>>>> +}
>>>> +
>>>>    static void configure_geometry(struct atmel_isi *isi, u32 width,
>>>> -			u32 height, u32 code)
>>>> +		u32 height, const struct soc_camera_format_xlate *xlate)
>>>>    {
>>>>    	u32 cfg2;
>>>>      	/* According to sensor's output format to set cfg2 */
>>>> -	switch (code) {
>>>> +	switch (xlate->code) {
>>>>    	default:
>>>>    	/* Grey */
>>>>    	case MEDIA_BUS_FMT_Y8_1X8:
>>>> @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi
>>>> *isi, u32 width,
>>>>    		break;
>>>>    	/* YUV */
>>>>    	case MEDIA_BUS_FMT_VYUY8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
>>>> -		break;
>>>>    	case MEDIA_BUS_FMT_UYVY8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
>>>> -		break;
>>>>    	case MEDIA_BUS_FMT_YVYU8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
>>>> -		break;
>>>>    	case MEDIA_BUS_FMT_YUYV8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
>>>> +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
>>>> +				setup_cfg2_yuv_swap(isi, xlate);
>>>>    		break;
>>>>    	/* RGB, TODO */
>>>>    	}
>>> I would move this switch-case completely inside setup_cfg2_yuv_swap().
>>> Just do
>>>
>>> 	cfg2 = setup_cfg2_yuv_swap(isi, xlate);
>>>
>>> and handle the
>>>
>>>    	case MEDIA_BUS_FMT_Y8_1X8:
>>>
>>> in the function too. These two switch-case statements really look
>>> redundant.
>> Technically, you can do that. But for my point of view, the
>> setup_cfg2_yuv_swap() only need to setup the yuv swap register field.
>>
>> And other cfg2 field need to be configured as well, especially in the case
>> sensor output a RGB data. That should be implement soon.
> Understand. I don't like redundant code and this certainly looks redundant
> to me. I'd be glad if you could remove this redundancy, but if you feel
> strongly about it, feel free to keep as is.

Good, thanks.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>>>> @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq,
>>>> unsigned int count)
>>>>    	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>>>      	configure_geometry(isi, icd->user_width, icd->user_height,
>>>> -				icd->current_fmt->code);
>>>> +				icd->current_fmt);
>>>>      	spin_lock_irq(&isi->lock);
>>>>    	/* Clear any pending interrupt */
>>>> -- 
>>>> 1.9.1
>>>>
>>> Thanks
>>> Guennadi
>> Best Regards,
>> Josh Wu
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html