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

[Guennadi Liakhovetski]

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 at 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. 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;

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