Re: [PATCH 5/5] media: atmel-isi: support RGB565 output when sensor output YUV formats

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

Dear Guennadi,

On 10/19/2015 10:03 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Wed, 14 Oct 2015, Josh Wu wrote:
>
>> Dear Guennadi,
>>
>> Thanks for the review.
>>
>> On 10/5/2015 1:02 AM, Guennadi Liakhovetski wrote:
>>> On Tue, 22 Sep 2015, Josh Wu wrote:
>>>
>>>> This patch enable Atmel ISI preview path to convert the YUV to RGB format.
>>>>
>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>>> ---
>>>>
>>>>    drivers/media/platform/soc_camera/atmel-isi.c | 38
>>>> ++++++++++++++++++++-------
>>>>    1 file changed, 29 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> index e87d354..e33a16a 100644
>>>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> @@ -201,13 +201,20 @@ static bool is_supported(struct soc_camera_device
>>>> *icd,
>>>>    	case V4L2_PIX_FMT_UYVY:
>>>>    	case V4L2_PIX_FMT_YVYU:
>>>>    	case V4L2_PIX_FMT_VYUY:
>>>> +	/* RGB */
>>>> +	case V4L2_PIX_FMT_RGB565:
>>>>    		return true;
>>>> -	/* RGB, TODO */
>>>>    	default:
>>>>    		return false;
>>>>    	}
>>>>    }
>>>>    +static bool is_output_rgb(const struct soc_mbus_pixelfmt *host_fmt)
>>>> +{
>>>> +	return host_fmt->fourcc == V4L2_PIX_FMT_RGB565 ||
>>>> +			host_fmt->fourcc == V4L2_PIX_FMT_RGB32;
>>>> +}
>>>> +
>>> Why not just pass fourcc to this function? Or maybe just embed it in
>>> start_streaming - it won't clutter it a lot.
>> I think pass fourcc to the function is good.
>> Since configure_geometry() is hardware related, and the enable_preview_path is
>> also hardware related, so I prefer initialize enable_preview_path in
>> configure_geometry().
> But you don't, you do it in start_streaming() below.

Right, then I'll move it to configure_geometry() in v2..

> But actually my
> comment was not about _where_ to do it, but whether this calculation is
> worth a separate function. I would just perform this calculation directly
> where you're calling it:
>
> static ... start_streaming(...)
> {
> 	...
> 	u32 fourcc = icd->current_fmt->host_fmt->fourcc;
>
> 	isi->enable_preview_path = fourcc == V4L2_PIX_FMT_RGB565 ||
> 				fourcc == V4L2_PIX_FMT_RGB32;

I thought this "function" might be called in other place, but actually 
no one call it.
So yes, I think there is no need to have such separated function. I'll 
fix it in v2. Thanks.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>>>>    static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>>>>    {
>>>>    	if (isi->active) {
>>>> @@ -467,6 +474,8 @@ static int start_streaming(struct vb2_queue *vq,
>>>> unsigned int count)
>>>>    	struct atmel_isi *isi = ici->priv;
>>>>    	int ret;
>>>>    +	isi->enable_preview_path = is_output_rgb(icd->current_fmt->host_fmt);
>>>> +
>>>>    	pm_runtime_get_sync(ici->v4l2_dev.dev);
>>>>      	/* Reset ISI */
>>>> @@ -688,6 +697,14 @@ static const struct soc_mbus_pixelfmt
>>>> isi_camera_formats[] = {
>>>>    		.order			= SOC_MBUS_ORDER_LE,
>>>>    		.layout			= SOC_MBUS_LAYOUT_PACKED,
>>>>    	},
>>>> +	{
>>>> +		.fourcc			= V4L2_PIX_FMT_RGB565,
>>>> +		.name			= "RGB565",
>>>> +		.bits_per_sample	= 8,
>>>> +		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
>>>> +		.order			= SOC_MBUS_ORDER_LE,
>>>> +		.layout			= SOC_MBUS_LAYOUT_PACKED,
>>>> +	},
>>>>    };
>>>>      /* This will be corrected as we get more formats */
>>>> @@ -744,7 +761,7 @@ static int isi_camera_get_formats(struct
>>>> soc_camera_device *icd,
>>>>    				  struct soc_camera_format_xlate *xlate)
>>>>    {
>>>>    	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>>>> -	int formats = 0, ret;
>>>> +	int formats = 0, ret, i, n;
>>>>    	/* sensor format */
>>>>    	struct v4l2_subdev_mbus_code_enum code = {
>>>>    		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>> @@ -778,13 +795,16 @@ static int isi_camera_get_formats(struct
>>>> soc_camera_device *icd,
>>>>    	case MEDIA_BUS_FMT_VYUY8_2X8:
>>>>    	case MEDIA_BUS_FMT_YUYV8_2X8:
>>>>    	case MEDIA_BUS_FMT_YVYU8_2X8:
>>>> -		formats++;
>>>> -		if (xlate) {
>>>> -			xlate->host_fmt	= &isi_camera_formats[0];
>>>> -			xlate->code	= code.code;
>>>> -			xlate++;
>>>> -			dev_dbg(icd->parent, "Providing format %s using code
>>>> %d\n",
>>>> -				isi_camera_formats[0].name, code.code);
>>>> +		n = ARRAY_SIZE(isi_camera_formats);
>>>> +		formats += n;
>>>> +		for (i = 0; i < n; i++) {
>>>> +			if (xlate) {
>>> I'd put if outside of the loop, or just do
>>>
>>> +		for (i = 0; xlate && i < n; i++) {
>> yes, that simpler one. I'll take it. Thanks.
>>
>> Best Regards,
>> Josh Wu
>>>
>>>> +				xlate->host_fmt	= &isi_camera_formats[i];
>>>> +				xlate->code	= code.code;
>>>> +				dev_dbg(icd->parent, "Providing format %s
>>>> using code %d\n",
>>>> +					isi_camera_formats[0].name,
>>>> code.code);
>>>> +				xlate++;
>>>> +			}
>>>>    		}
>>>>    		break;
>>>>    	default:
>>>> -- 
>>>> 1.9.1
>>>>