[RFC,v2,1/2] media: platform: transfer format translations to soc_mediabus

  Transfer the formats translations to soc_mediabus. Even is soc_camera
was to be deprecated, soc_mediabus will survive, and should describe all
that happens on the bus connecting the image processing unit of the SoC
and the sensor.

The translation engine provides an easy way to compute the formats
available in the v4l2 device, given any sensors format capabilities
bound with known image processing transformations.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/platform/soc_camera/soc_camera.c   |  7 +--
 drivers/media/platform/soc_camera/soc_mediabus.c | 65 ++++++++++++++++++++++++
 include/media/drv-intf/soc_mediabus.h            | 22 ++++++++
 include/media/soc_camera.h                       | 15 ------
 4 files changed, 88 insertions(+), 21 deletions(-)
  

Hi Robert,

Not sure I understand, what should the purpose of this patch be? Why do 
you want to move some function(s) from one file to another? And you aren't 
even calling the new soc_mbus_build_fmts_xlate() function, and you aren't 
replacing the currently used analogous soc_camera_init_user_formats() 
function. Or was this patch not-to-be-reviewed?

Thanks
Guennadi

On Sat, 2 Apr 2016, Robert Jarzmik wrote:

> Transfer the formats translations to soc_mediabus. Even is soc_camera
> was to be deprecated, soc_mediabus will survive, and should describe all
> that happens on the bus connecting the image processing unit of the SoC
> and the sensor.
> 
> The translation engine provides an easy way to compute the formats
> available in the v4l2 device, given any sensors format capabilities
> bound with known image processing transformations.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/platform/soc_camera/soc_camera.c   |  7 +--
>  drivers/media/platform/soc_camera/soc_mediabus.c | 65 ++++++++++++++++++++++++
>  include/media/drv-intf/soc_mediabus.h            | 22 ++++++++
>  include/media/soc_camera.h                       | 15 ------
>  4 files changed, 88 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index 46c7186f7867..039524a20056 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -204,12 +204,7 @@ static void soc_camera_clock_stop(struct soc_camera_host *ici)
>  const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
>  	struct soc_camera_device *icd, unsigned int fourcc)
>  {
> -	unsigned int i;
> -
> -	for (i = 0; i < icd->num_user_formats; i++)
> -		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
> -			return icd->user_formats + i;
> -	return NULL;
> +	return soc_mbus_xlate_by_fourcc(icd->user_formats, fourcc);
>  }
>  EXPORT_SYMBOL(soc_camera_xlate_by_fourcc);
>  
> diff --git a/drivers/media/platform/soc_camera/soc_mediabus.c b/drivers/media/platform/soc_camera/soc_mediabus.c
> index e3e665e1c503..95c13055f50f 100644
> --- a/drivers/media/platform/soc_camera/soc_mediabus.c
> +++ b/drivers/media/platform/soc_camera/soc_mediabus.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/slab.h>
>  
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-mediabus.h>
> @@ -512,6 +513,70 @@ unsigned int soc_mbus_config_compatible(const struct v4l2_mbus_config *cfg,
>  }
>  EXPORT_SYMBOL(soc_mbus_config_compatible);
>  
> +struct soc_camera_format_xlate *soc_mbus_build_fmts_xlate(
> +	struct v4l2_device *v4l2_dev, struct v4l2_subdev *subdev,
> +	int (*get_formats)(struct v4l2_device *, unsigned int,
> +			   struct soc_camera_format_xlate *xlate))
> +{
> +	unsigned int i, fmts = 0, raw_fmts = 0;
> +	int ret;
> +	struct v4l2_subdev_mbus_code_enum code = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	struct soc_camera_format_xlate *user_formats;
> +
> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> +		raw_fmts++;
> +		code.index++;
> +	}
> +
> +	/*
> +	 * First pass - only count formats this host-sensor
> +	 * configuration can provide
> +	 */
> +	for (i = 0; i < raw_fmts; i++) {
> +		ret = get_formats(v4l2_dev, i, NULL);
> +		if (ret < 0)
> +			return ERR_PTR(ret);
> +		fmts += ret;
> +	}
> +
> +	if (!fmts)
> +		return ERR_PTR(-ENXIO);
> +
> +	user_formats = kcalloc(fmts + 1, sizeof(*user_formats), GFP_KERNEL);
> +	if (!user_formats)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Second pass - actually fill data formats */
> +	fmts = 0;
> +	for (i = 0; i < raw_fmts; i++) {
> +		ret = get_formats(v4l2_dev, i, user_formats + fmts);
> +		if (ret < 0)
> +			goto egfmt;
> +		fmts += ret;
> +	}
> +	user_formats[fmts].code = 0;
> +
> +	return user_formats;
> +egfmt:
> +	kfree(user_formats);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(soc_mbus_build_fmts_xlate);
> +
> +const struct soc_camera_format_xlate *soc_mbus_xlate_by_fourcc(
> +	struct soc_camera_format_xlate *user_formats, unsigned int fourcc)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; user_formats[i].code; i++)
> +		if (user_formats[i].host_fmt->fourcc == fourcc)
> +			return user_formats + i;
> +	return NULL;
> +}
> +EXPORT_SYMBOL(soc_mbus_xlate_by_fourcc);
> +
>  static int __init soc_mbus_init(void)
>  {
>  	return 0;
> diff --git a/include/media/drv-intf/soc_mediabus.h b/include/media/drv-intf/soc_mediabus.h
> index 2ff773785fb6..08af52f6338c 100644
> --- a/include/media/drv-intf/soc_mediabus.h
> +++ b/include/media/drv-intf/soc_mediabus.h
> @@ -95,6 +95,21 @@ struct soc_mbus_lookup {
>  	struct soc_mbus_pixelfmt	fmt;
>  };
>  
> +/**
> + * struct soc_camera_format_xlate - match between host and sensor formats
> + * @code: code of a sensor provided format
> + * @host_fmt: host format after host translation from code
> + *
> + * Host and sensor translation structure. Used in table of host and sensor
> + * formats matchings in soc_camera_device. A host can override the generic list
> + * generation by implementing get_formats(), and use it for format checks and
> + * format setup.
> + */
> +struct soc_camera_format_xlate {
> +	u32 code;
> +	const struct soc_mbus_pixelfmt *host_fmt;
> +};
> +
>  const struct soc_mbus_pixelfmt *soc_mbus_find_fmtdesc(
>  	u32 code,
>  	const struct soc_mbus_lookup *lookup,
> @@ -108,5 +123,12 @@ int soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf,
>  			unsigned int *numerator, unsigned int *denominator);
>  unsigned int soc_mbus_config_compatible(const struct v4l2_mbus_config *cfg,
>  					unsigned int flags);
> +struct soc_camera_format_xlate *soc_mbus_build_fmts_xlate(
> +	struct v4l2_device *v4l2_dev, struct v4l2_subdev *subdev,
> +	int (*get_formats)(struct v4l2_device *, unsigned int,
> +			   struct soc_camera_format_xlate *xlate));
> +const struct soc_camera_format_xlate *soc_mbus_xlate_by_fourcc(
> +	struct soc_camera_format_xlate *user_formats, unsigned int fourcc);
> +
>  
>  #endif
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 97aa13314bfd..db6ea91d5cb0 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -285,21 +285,6 @@ void soc_camera_host_unregister(struct soc_camera_host *ici);
>  const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
>  	struct soc_camera_device *icd, unsigned int fourcc);
>  
> -/**
> - * struct soc_camera_format_xlate - match between host and sensor formats
> - * @code: code of a sensor provided format
> - * @host_fmt: host format after host translation from code
> - *
> - * Host and sensor translation structure. Used in table of host and sensor
> - * formats matchings in soc_camera_device. A host can override the generic list
> - * generation by implementing get_formats(), and use it for format checks and
> - * format setup.
> - */
> -struct soc_camera_format_xlate {
> -	u32 code;
> -	const struct soc_mbus_pixelfmt *host_fmt;
> -};
> -
>  #define SOCAM_SENSE_PCLK_CHANGED	(1 << 0)
>  
>  /**
> -- 
> 2.1.4
> 
--
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
  

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

Hi Guennadi,
> Not sure I understand, what should the purpose of this patch be?
See in [1].

> Why do you want to move some function(s) from one file to another?  And you
> aren't even calling the new soc_mbus_build_fmts_xlate() function
I'm calling it in pxa_camera_build_formats() in patch 2/2.

> and you aren't replacing the currently used analogous
> soc_camera_init_user_formats() function.
I'm doing that in patch 2/2.

> Or was this patch not-to-be-reviewed?
Actually these 2 patches are designed to be discussion openers :)
For me, their purpose is to expose the transition of pxa_camera out of
soc_camera and see if the chosen path is good, or if there exists a better one.

In other words, these patches show that :
 - in a first stage, soc_mediabus should be kept [1]
   => at least for formats translation (soc_mbus_build_fmts_xlate())
   => and for used formats by sensors
   => this is why patch 1/1 exists

 - the conversion almost doesn't touch the pxa_camera_() core functions (IP
   manipulation), which is good, and only touch the upper layer

 - that soc_mediabus adherence removal will be another task

 - the amount of code which is shifted from soc_camera to pxa_camera

 - the functionalities that are lost through conversion which should be readded
   later
   => cropping is one
   => pixel clock sensing is another one

All in all, before submitting patch for real, ie. not in RFC mode, I wanted to
be sure the proposed conversion is sound, and compare to other drivers
conversion to see if we were going in the same direction.

As to whether this patch should be reviewed or not, I'd say that I was just
expecting to have an "that might be the way to go" or "NAK, wrong patch, let's
do something else instead".

Cheers.
  

On 04/02/2016 04:26 PM, Robert Jarzmik wrote:
> Transfer the formats translations to soc_mediabus. Even is soc_camera
> was to be deprecated, soc_mediabus will survive, and should describe all
> that happens on the bus connecting the image processing unit of the SoC
> and the sensor.
> 
> The translation engine provides an easy way to compute the formats
> available in the v4l2 device, given any sensors format capabilities
> bound with known image processing transformations.

I prefer that you just make a copy of this for use in the pxa driver.

We might make this (or a variant) available for all drivers in the future,
but for now just split off the pxa driver without introducing new media
includes.

Regards,

	Hans

> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/platform/soc_camera/soc_camera.c   |  7 +--
>  drivers/media/platform/soc_camera/soc_mediabus.c | 65 ++++++++++++++++++++++++
>  include/media/drv-intf/soc_mediabus.h            | 22 ++++++++
>  include/media/soc_camera.h                       | 15 ------
>  4 files changed, 88 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index 46c7186f7867..039524a20056 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -204,12 +204,7 @@ static void soc_camera_clock_stop(struct soc_camera_host *ici)
>  const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
>  	struct soc_camera_device *icd, unsigned int fourcc)
>  {
> -	unsigned int i;
> -
> -	for (i = 0; i < icd->num_user_formats; i++)
> -		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
> -			return icd->user_formats + i;
> -	return NULL;
> +	return soc_mbus_xlate_by_fourcc(icd->user_formats, fourcc);
>  }
>  EXPORT_SYMBOL(soc_camera_xlate_by_fourcc);
>  
> diff --git a/drivers/media/platform/soc_camera/soc_mediabus.c b/drivers/media/platform/soc_camera/soc_mediabus.c
> index e3e665e1c503..95c13055f50f 100644
> --- a/drivers/media/platform/soc_camera/soc_mediabus.c
> +++ b/drivers/media/platform/soc_camera/soc_mediabus.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/slab.h>
>  
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-mediabus.h>
> @@ -512,6 +513,70 @@ unsigned int soc_mbus_config_compatible(const struct v4l2_mbus_config *cfg,
>  }
>  EXPORT_SYMBOL(soc_mbus_config_compatible);
>  
> +struct soc_camera_format_xlate *soc_mbus_build_fmts_xlate(
> +	struct v4l2_device *v4l2_dev, struct v4l2_subdev *subdev,
> +	int (*get_formats)(struct v4l2_device *, unsigned int,
> +			   struct soc_camera_format_xlate *xlate))
> +{
> +	unsigned int i, fmts = 0, raw_fmts = 0;
> +	int ret;
> +	struct v4l2_subdev_mbus_code_enum code = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	struct soc_camera_format_xlate *user_formats;
> +
> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> +		raw_fmts++;
> +		code.index++;
> +	}
> +
> +	/*
> +	 * First pass - only count formats this host-sensor
> +	 * configuration can provide
> +	 */
> +	for (i = 0; i < raw_fmts; i++) {
> +		ret = get_formats(v4l2_dev, i, NULL);
> +		if (ret < 0)
> +			return ERR_PTR(ret);
> +		fmts += ret;
> +	}
> +
> +	if (!fmts)
> +		return ERR_PTR(-ENXIO);
> +
> +	user_formats = kcalloc(fmts + 1, sizeof(*user_formats), GFP_KERNEL);
> +	if (!user_formats)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Second pass - actually fill data formats */
> +	fmts = 0;
> +	for (i = 0; i < raw_fmts; i++) {
> +		ret = get_formats(v4l2_dev, i, user_formats + fmts);
> +		if (ret < 0)
> +			goto egfmt;
> +		fmts += ret;
> +	}
> +	user_formats[fmts].code = 0;
> +
> +	return user_formats;
> +egfmt:
> +	kfree(user_formats);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(soc_mbus_build_fmts_xlate);
> +
> +const struct soc_camera_format_xlate *soc_mbus_xlate_by_fourcc(
> +	struct soc_camera_format_xlate *user_formats, unsigned int fourcc)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; user_formats[i].code; i++)
> +		if (user_formats[i].host_fmt->fourcc == fourcc)
> +			return user_formats + i;
> +	return NULL;
> +}
> +EXPORT_SYMBOL(soc_mbus_xlate_by_fourcc);
> +
>  static int __init soc_mbus_init(void)
>  {
>  	return 0;
> diff --git a/include/media/drv-intf/soc_mediabus.h b/include/media/drv-intf/soc_mediabus.h
> index 2ff773785fb6..08af52f6338c 100644
> --- a/include/media/drv-intf/soc_mediabus.h
> +++ b/include/media/drv-intf/soc_mediabus.h
> @@ -95,6 +95,21 @@ struct soc_mbus_lookup {
>  	struct soc_mbus_pixelfmt	fmt;
>  };
>  
> +/**
> + * struct soc_camera_format_xlate - match between host and sensor formats
> + * @code: code of a sensor provided format
> + * @host_fmt: host format after host translation from code
> + *
> + * Host and sensor translation structure. Used in table of host and sensor
> + * formats matchings in soc_camera_device. A host can override the generic list
> + * generation by implementing get_formats(), and use it for format checks and
> + * format setup.
> + */
> +struct soc_camera_format_xlate {
> +	u32 code;
> +	const struct soc_mbus_pixelfmt *host_fmt;
> +};
> +
>  const struct soc_mbus_pixelfmt *soc_mbus_find_fmtdesc(
>  	u32 code,
>  	const struct soc_mbus_lookup *lookup,
> @@ -108,5 +123,12 @@ int soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf,
>  			unsigned int *numerator, unsigned int *denominator);
>  unsigned int soc_mbus_config_compatible(const struct v4l2_mbus_config *cfg,
>  					unsigned int flags);
> +struct soc_camera_format_xlate *soc_mbus_build_fmts_xlate(
> +	struct v4l2_device *v4l2_dev, struct v4l2_subdev *subdev,
> +	int (*get_formats)(struct v4l2_device *, unsigned int,
> +			   struct soc_camera_format_xlate *xlate));
> +const struct soc_camera_format_xlate *soc_mbus_xlate_by_fourcc(
> +	struct soc_camera_format_xlate *user_formats, unsigned int fourcc);
> +
>  
>  #endif
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 97aa13314bfd..db6ea91d5cb0 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -285,21 +285,6 @@ void soc_camera_host_unregister(struct soc_camera_host *ici);
>  const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
>  	struct soc_camera_device *icd, unsigned int fourcc);
>  
> -/**
> - * struct soc_camera_format_xlate - match between host and sensor formats
> - * @code: code of a sensor provided format
> - * @host_fmt: host format after host translation from code
> - *
> - * Host and sensor translation structure. Used in table of host and sensor
> - * formats matchings in soc_camera_device. A host can override the generic list
> - * generation by implementing get_formats(), and use it for format checks and
> - * format setup.
> - */
> -struct soc_camera_format_xlate {
> -	u32 code;
> -	const struct soc_mbus_pixelfmt *host_fmt;
> -};
> -
>  #define SOCAM_SENSE_PCLK_CHANGED	(1 << 0)
>  
>  /**
> 
--
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
  

Hans Verkuil <hverkuil@xs4all.nl> writes:

> On 04/02/2016 04:26 PM, Robert Jarzmik wrote:
>> Transfer the formats translations to soc_mediabus. Even is soc_camera
>> was to be deprecated, soc_mediabus will survive, and should describe all
>> that happens on the bus connecting the image processing unit of the SoC
>> and the sensor.
>> 
>> The translation engine provides an easy way to compute the formats
>> available in the v4l2 device, given any sensors format capabilities
>> bound with known image processing transformations.
>
> I prefer that you just make a copy of this for use in the pxa driver.
>
> We might make this (or a variant) available for all drivers in the future,
> but for now just split off the pxa driver without introducing new media
> includes.
Okay, as you wish.

Cheers.

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