Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Robert Jarzmik <robert.jarzmik@free.fr>]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> @@ -901,15 +1042,33 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  static int pxa_camera_set_fmt(struct soc_camera_device *icd,
>  			      __u32 pixfmt, struct v4l2_rect *rect)
snip
> -	/*
> -	 * TODO: find a suitable supported by the SoC output format, check
> -	 * whether the sensor supports one of acceptable input formats.
> -	 */
>  	if (pixfmt) {
> -		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> +		struct camera_data *cam_data = icd->host_priv;
> +		int i;
> +
> +		/* First check camera native formats */
> +		for (i = 0; i < cam_data->camera_formats_num; i++)
> +			if (cam_data->camera_formats[i]->fourcc == pixfmt) {
> +				cam_fmt = cam_data->camera_formats[i];
> +				break;
> +			}
> +
> +		/* Next, if failed, check synthesized formats */
> +		if (!cam_fmt)
> +			for (i = 0; i < cam_data->extra_formats_num; i++)
> +				if (cam_data->extra_formats[i].fourcc ==
> +				    pixfmt) {
> +					cam_fmt = cam_data->extra_formats + i;
> +					/* TODO: synthesize... */
> +					dev_err(&icd->dev,
> +						"Cannot provide format 0x%x\n",
> +						pixfmt);
> +					return -EOPNOTSUPP;
> +				}
> +
>  		if (!cam_fmt)
>  			return -EINVAL;
>  	}

> @@ -924,18 +1083,31 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
snip
> -	/*
> -	 * TODO: find a suitable supported by the SoC output format, check
> -	 * whether the sensor supports one of acceptable input formats.
> -	 */
> -	cam_fmt = soc_camera_format_by_fourcc(icd, pix->pixelformat);
> +	/* First check camera native formats */
> +	for (i = 0; i < cam_data->camera_formats_num; i++)
> +		if (cam_data->camera_formats[i]->fourcc == pixfmt) {
> +			cam_fmt = cam_data->camera_formats[i];
> +			break;
> +		}
> +
> +	/* Next, if failed, check synthesized formats */
> +	if (!cam_fmt)
> +		for (i = 0; i < cam_data->extra_formats_num; i++)
> +			if (cam_data->extra_formats[i].fourcc == pixfmt) {
> +				cam_fmt = cam_data->extra_formats + i;
> +				break;
> +			}
> +
>  	if (!cam_fmt)
>  		return -EINVAL;
Isn't that the second time you're looking for a format the same way, with only a
printk making a difference ? Shouldn't that be grouped in a function
(pxa_camera_find_format(icd, pixfmt) ?) ...


More globally, all camera hosts will implement the creation of this formats
table. Why did you choose to put that in pxa_camera, and not in soc_camera to
make available to all host drivers ?
I had thought you would design something like :

 - soc_camera provides a format like :

struct soc_camera_format_translate {
       u32 host_pixfmt;
       u32 sensor_pixfmt;
};

 - camera host provide a static table like this :
struct soc_camera_format_translate pxa_pixfmt_translations[] = {
       { V4L2_PIX_FMT_YUYV, V4L2_PIX_FMT_YUYV },
       { V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_UYVY },
       ...
       { V4L2_PIX_FMT_YUV422P, V4L2_PIX_FMT_UYVY},
};

 - soc_camera provides functions like :
  - soc_camera_compute_formats(struct soc_camera_format_translate trans,
                               struct soc_camera_data_format sensor_formats)
    => that creates the formats table

 - camera host either :
  - call the generic soc_camera_compute_formats()
  - or make the computation themself if it is way too specific.

Is there a reason you chose to fully export the formats computation to hosts ?

Otherwise, the other 4 patches look fine to me, I'll make some tests tomorrow.

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list