All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] media: i2c: max9286: Use remote endpoint image format
@ 2020-08-17 14:35 Jacopo Mondi
  2020-08-17 14:35 ` [PATCH 1/4] media: i2c: max9286: Initialize try formats Jacopo Mondi
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-17 14:35 UTC (permalink / raw)
  To: linux-renesas-soc, linux-media, Mauro Carvalho Chehab
  Cc: Jacopo Mondi, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, sakari.ailus, hverkuil, hyunk,
	manivannan.sadhasivam

In order to prepare to support multiple camera modules, which deliver image
streams in different formats, modify the max9286 driver in order to remote
the hardcoded formats it currently exposes.

The image formats, both for ACTIVE and TRY formats, are retrieved from
the remote subdevices instead of being hard-coded.

This series also dis-allow setting any format on the max9286 sink pads, as
the format only depends on the remote end and the MAX9286 chip cannot
perform any transformation of the image stream it de-serialize from GMSL
bus.

Thanks
   j

Jacopo Mondi (4):
  media: i2c: max9286: Initialize try formats
  media: i2c: max9286: Get format from remote ends
  media: i2c: max9286: Do not allow changing format
  media: i2c: max9286: Remove cached formats

 drivers/media/i2c/max9286.c | 124 +++++++++++++++---------------------
 1 file changed, 51 insertions(+), 73 deletions(-)


base-commit: f45882cfb152f5d3a421fd58f177f227e44843b9
--
2.27.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] media: i2c: max9286: Initialize try formats
  2020-08-17 14:35 [PATCH 0/4] media: i2c: max9286: Use remote endpoint image format Jacopo Mondi
@ 2020-08-17 14:35 ` Jacopo Mondi
  2020-08-18  7:09   ` Sakari Ailus
  2020-08-17 14:35 ` [PATCH 2/4] media: i2c: max9286: Get format from remote ends Jacopo Mondi
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-17 14:35 UTC (permalink / raw)
  To: linux-renesas-soc, linux-media, Mauro Carvalho Chehab
  Cc: Jacopo Mondi, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, sakari.ailus, hverkuil, hyunk,
	manivannan.sadhasivam

Initialize try formats at device node open time by querying the
format from the remote subdevices instead of hard-coding it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 47f280518fdb..7c292f2e2704 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -794,12 +794,29 @@ static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)

 static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
 {
+	struct max9286_priv *priv = sd_to_max9286(subdev);
+	struct device *dev = &priv->client->dev;
 	struct v4l2_mbus_framefmt *format;
-	unsigned int i;
+	struct max9286_source *source;
+
+	for_each_source(priv, source) {
+		struct v4l2_subdev_pad_config remote_config = {};
+		unsigned int i = to_index(priv, source);
+		struct v4l2_subdev_format remote_fmt = {
+			.which = V4L2_SUBDEV_FORMAT_TRY,
+			.pad = 0,
+		};
+		int ret;

-	for (i = 0; i < MAX9286_N_SINKS; i++) {
 		format = v4l2_subdev_get_try_format(subdev, fh->pad, i);
-		max9286_init_format(format);
+		ret = v4l2_subdev_call(source->sd, pad, get_fmt, &remote_config,
+				       &remote_fmt);
+		if (ret) {
+			dev_err(dev, "Unable get format on source %u\n", i);
+			return ret;
+		}
+
+		*format = remote_fmt.format;
 	}

 	return 0;
--
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/4] media: i2c: max9286: Get format from remote ends
  2020-08-17 14:35 [PATCH 0/4] media: i2c: max9286: Use remote endpoint image format Jacopo Mondi
  2020-08-17 14:35 ` [PATCH 1/4] media: i2c: max9286: Initialize try formats Jacopo Mondi
@ 2020-08-17 14:35 ` Jacopo Mondi
  2020-08-19 12:46   ` Laurent Pinchart
  2020-08-17 14:35 ` [PATCH 3/4] media: i2c: max9286: Do not allow changing format Jacopo Mondi
  2020-08-17 14:35 ` [PATCH 4/4] media: i2c: max9286: Remove cached formats Jacopo Mondi
  3 siblings, 1 reply; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-17 14:35 UTC (permalink / raw)
  To: linux-renesas-soc, linux-media, Mauro Carvalho Chehab
  Cc: Jacopo Mondi, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, sakari.ailus, hverkuil, hyunk,
	manivannan.sadhasivam

The MAX9286 chip does not allow any modification to the image stream
format it de-serializes from the GMSL bus to its MIPI CSI-2 output
interface.

For this reason, when the format is queried from on any of the MAX9286
pads, get the remote subdevice format and return it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 7c292f2e2704..e6a70dbd27df 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -742,8 +742,10 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_format *format)
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
+	struct v4l2_subdev_format remote_fmt = {};
+	struct device *dev = &priv->client->dev;
 	unsigned int pad = format->pad;
+	int ret;
 
 	/*
 	 * Multiplexed Stream Support: Support link validation by returning the
@@ -754,12 +756,26 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 	if (pad == MAX9286_SRC_PAD)
 		pad = __ffs(priv->bound_sources);
 
-	cfg_fmt = max9286_get_pad_format(priv, cfg, pad, format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+		mutex_lock(&priv->mutex);
+		format->format = *v4l2_subdev_get_try_format(&priv->sd,
+							     cfg, pad);
+		mutex_unlock(&priv->mutex);
+
+		return 0;
+	}
+
+	remote_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	remote_fmt.pad = 0;
+	ret = v4l2_subdev_call(priv->sources[pad].sd, pad, get_fmt, NULL,
+			       &remote_fmt);
+	if (ret) {
+		dev_err(dev, "Unable get format on source %d\n", pad);
+		return ret;
+	}
 
 	mutex_lock(&priv->mutex);
-	format->format = *cfg_fmt;
+	format->format = remote_fmt.format;
 	mutex_unlock(&priv->mutex);
 
 	return 0;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/4] media: i2c: max9286: Do not allow changing format
  2020-08-17 14:35 [PATCH 0/4] media: i2c: max9286: Use remote endpoint image format Jacopo Mondi
  2020-08-17 14:35 ` [PATCH 1/4] media: i2c: max9286: Initialize try formats Jacopo Mondi
  2020-08-17 14:35 ` [PATCH 2/4] media: i2c: max9286: Get format from remote ends Jacopo Mondi
@ 2020-08-17 14:35 ` Jacopo Mondi
  2020-08-17 14:35 ` [PATCH 4/4] media: i2c: max9286: Remove cached formats Jacopo Mondi
  3 siblings, 0 replies; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-17 14:35 UTC (permalink / raw)
  To: linux-renesas-soc, linux-media, Mauro Carvalho Chehab
  Cc: Jacopo Mondi, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, sakari.ailus, hverkuil, hyunk,
	manivannan.sadhasivam

As the MAX9286 chip does not allow changing the format of the video
stream, always return the format retrieived from the remote subdevices
when an attempt to change it is made.

The -max9286_get_pad_format() format is now unsued, so remove it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 58 +++++++------------------------------
 1 file changed, 10 insertions(+), 48 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index e6a70dbd27df..a4e23396c4b6 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -689,54 +689,6 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-max9286_get_pad_format(struct max9286_priv *priv,
-		       struct v4l2_subdev_pad_config *cfg,
-		       unsigned int pad, u32 which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_get_try_format(&priv->sd, cfg, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &priv->fmt[pad];
-	default:
-		return NULL;
-	}
-}
-
-static int max9286_set_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_pad_config *cfg,
-			   struct v4l2_subdev_format *format)
-{
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
-
-	if (format->pad == MAX9286_SRC_PAD)
-		return -EINVAL;
-
-	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
-	switch (format->format.code) {
-	case MEDIA_BUS_FMT_UYVY8_1X16:
-	case MEDIA_BUS_FMT_VYUY8_1X16:
-	case MEDIA_BUS_FMT_YUYV8_1X16:
-	case MEDIA_BUS_FMT_YVYU8_1X16:
-		break;
-	default:
-		format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
-		break;
-	}
-
-	cfg_fmt = max9286_get_pad_format(priv, cfg, format->pad, format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
-
-	mutex_lock(&priv->mutex);
-	*cfg_fmt = format->format;
-	mutex_unlock(&priv->mutex);
-
-	return 0;
-}
-
 static int max9286_get_fmt(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_pad_config *cfg,
 			   struct v4l2_subdev_format *format)
@@ -781,6 +733,16 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int max9286_set_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   struct v4l2_subdev_format *format)
+{
+	if (format->pad == MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	return max9286_get_fmt(sd, cfg, format);
+}
+
 static const struct v4l2_subdev_video_ops max9286_video_ops = {
 	.s_stream	= max9286_s_stream,
 };
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/4] media: i2c: max9286: Remove cached formats
  2020-08-17 14:35 [PATCH 0/4] media: i2c: max9286: Use remote endpoint image format Jacopo Mondi
                   ` (2 preceding siblings ...)
  2020-08-17 14:35 ` [PATCH 3/4] media: i2c: max9286: Do not allow changing format Jacopo Mondi
@ 2020-08-17 14:35 ` Jacopo Mondi
  2020-08-17 22:15   ` Hyun Kwon
  3 siblings, 1 reply; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-17 14:35 UTC (permalink / raw)
  To: linux-renesas-soc, linux-media, Mauro Carvalho Chehab
  Cc: Jacopo Mondi, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, sakari.ailus, hverkuil, hyunk,
	manivannan.sadhasivam

Now that the image stream formats are retrieved from the remote
sources there's no need to cache them in the driver structure.

Remove the cached mbus frame formats and their initialization.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index a4e23396c4b6..97dfee767bbf 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -160,8 +160,6 @@ struct max9286_priv {
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
 
-	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
-
 	/* Protects controls and fmt structures */
 	struct mutex mutex;
 
@@ -758,18 +756,6 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
 	.pad		= &max9286_pad_ops,
 };
 
-static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
-{
-	fmt->width		= 1280;
-	fmt->height		= 800;
-	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
-	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
-	fmt->field		= V4L2_FIELD_NONE;
-	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
-	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
-	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
-}
-
 static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
 {
 	struct max9286_priv *priv = sd_to_max9286(subdev);
@@ -834,9 +820,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 
 	/* Configure V4L2 for the MAX9286 itself */
 
-	for (i = 0; i < MAX9286_N_SINKS; i++)
-		max9286_init_format(&priv->fmt[i]);
-
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
 	priv->sd.internal_ops = &max9286_subdev_internal_ops;
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/4] media: i2c: max9286: Remove cached formats
  2020-08-17 14:35 ` [PATCH 4/4] media: i2c: max9286: Remove cached formats Jacopo Mondi
@ 2020-08-17 22:15   ` Hyun Kwon
  2020-08-18 12:05     ` Jacopo Mondi
  0 siblings, 1 reply; 11+ messages in thread
From: Hyun Kwon @ 2020-08-17 22:15 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: linux-renesas-soc, linux-media, Mauro Carvalho Chehab,
	Kieran Bingham, Laurent Pinchart, Niklas Söderlund,
	sakari.ailus, hverkuil, manivannan.sadhasivam

Hi Jacopo,

Thanks for sharing!

On Mon, Aug 17, 2020 at 07:35:40AM -0700, Jacopo Mondi wrote:
> Now that the image stream formats are retrieved from the remote
> sources there's no need to cache them in the driver structure.
> 
> Remove the cached mbus frame formats and their initialization.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index a4e23396c4b6..97dfee767bbf 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -160,8 +160,6 @@ struct max9286_priv {
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
>  
> -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> -
>  	/* Protects controls and fmt structures */
>  	struct mutex mutex;
>  
> @@ -758,18 +756,6 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
>  	.pad		= &max9286_pad_ops,
>  };
>  
> -static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
> -{
> -	fmt->width		= 1280;
> -	fmt->height		= 800;
> -	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;

There's one more hardcoded place left. This had some implication on output
format, MAX9286_DATATYPE_YUV422_8BIT, which is programmed at 0x12.
Now, this patch set can potentially result in a mismatch between bus format
and acutal programmed format.

Thanks,
-hyun

> -	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
> -	fmt->field		= V4L2_FIELD_NONE;
> -	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
> -	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
> -	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
> -}
> -
>  static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(subdev);
> @@ -834,9 +820,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  
>  	/* Configure V4L2 for the MAX9286 itself */
>  
> -	for (i = 0; i < MAX9286_N_SINKS; i++)
> -		max9286_init_format(&priv->fmt[i]);
> -
>  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> -- 
> 2.27.0
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] media: i2c: max9286: Initialize try formats
  2020-08-17 14:35 ` [PATCH 1/4] media: i2c: max9286: Initialize try formats Jacopo Mondi
@ 2020-08-18  7:09   ` Sakari Ailus
  2020-08-18 12:03     ` Jacopo Mondi
  0 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2020-08-18  7:09 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: linux-renesas-soc, linux-media, Mauro Carvalho Chehab,
	Kieran Bingham, Laurent Pinchart, Niklas Söderlund,
	hverkuil, hyunk, manivannan.sadhasivam

Hi Jacopo,

On Mon, Aug 17, 2020 at 04:35:37PM +0200, Jacopo Mondi wrote:
> Initialize try formats at device node open time by querying the
> format from the remote subdevices instead of hard-coding it.

The try formats are expected to be device defaults and not dependent on
configuration.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] media: i2c: max9286: Initialize try formats
  2020-08-18  7:09   ` Sakari Ailus
@ 2020-08-18 12:03     ` Jacopo Mondi
  0 siblings, 0 replies; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-18 12:03 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Jacopo Mondi, linux-renesas-soc, linux-media,
	Mauro Carvalho Chehab, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, hverkuil, hyunk, manivannan.sadhasivam

Hi Sakari,

On Tue, Aug 18, 2020 at 10:09:10AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Aug 17, 2020 at 04:35:37PM +0200, Jacopo Mondi wrote:
> > Initialize try formats at device node open time by querying the
> > format from the remote subdevices instead of hard-coding it.
>
> The try formats are expected to be device defaults and not dependent on
> configuration.
>

The deserializer defaul depends on what is serialized on the other
end. I think getting it from the remote end makes sense in this case.

Thanks
   j

> --
> Sakari Ailus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/4] media: i2c: max9286: Remove cached formats
  2020-08-17 22:15   ` Hyun Kwon
@ 2020-08-18 12:05     ` Jacopo Mondi
  0 siblings, 0 replies; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-18 12:05 UTC (permalink / raw)
  To: Hyun Kwon
  Cc: Jacopo Mondi, linux-renesas-soc, linux-media,
	Mauro Carvalho Chehab, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund, sakari.ailus, hverkuil,
	manivannan.sadhasivam

Hi Hyun,

On Mon, Aug 17, 2020 at 03:15:05PM -0700, Hyun Kwon wrote:
> Hi Jacopo,
>
> Thanks for sharing!
>
> On Mon, Aug 17, 2020 at 07:35:40AM -0700, Jacopo Mondi wrote:
> > Now that the image stream formats are retrieved from the remote
> > sources there's no need to cache them in the driver structure.
> >
> > Remove the cached mbus frame formats and their initialization.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 17 -----------------
> >  1 file changed, 17 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index a4e23396c4b6..97dfee767bbf 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -160,8 +160,6 @@ struct max9286_priv {
> >  	struct v4l2_ctrl_handler ctrls;
> >  	struct v4l2_ctrl *pixelrate;
> >
> > -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > -
> >  	/* Protects controls and fmt structures */
> >  	struct mutex mutex;
> >
> > @@ -758,18 +756,6 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
> >  	.pad		= &max9286_pad_ops,
> >  };
> >
> > -static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
> > -{
> > -	fmt->width		= 1280;
> > -	fmt->height		= 800;
> > -	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
>
> There's one more hardcoded place left. This had some implication on output
> format, MAX9286_DATATYPE_YUV422_8BIT, which is programmed at 0x12.
> Now, this patch set can potentially result in a mismatch between bus format
> and acutal programmed format.

Yup, very correct, I should adjust the DT based on the deserialized
format!

Thanks for noticing!

>
> Thanks,
> -hyun
>
> > -	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
> > -	fmt->field		= V4L2_FIELD_NONE;
> > -	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
> > -	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
> > -	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
> > -}
> > -
> >  static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> >  {
> >  	struct max9286_priv *priv = sd_to_max9286(subdev);
> > @@ -834,9 +820,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >
> >  	/* Configure V4L2 for the MAX9286 itself */
> >
> > -	for (i = 0; i < MAX9286_N_SINKS; i++)
> > -		max9286_init_format(&priv->fmt[i]);
> > -
> >  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> >  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
> >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > --
> > 2.27.0
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] media: i2c: max9286: Get format from remote ends
  2020-08-17 14:35 ` [PATCH 2/4] media: i2c: max9286: Get format from remote ends Jacopo Mondi
@ 2020-08-19 12:46   ` Laurent Pinchart
  2020-08-24  7:48     ` Jacopo Mondi
  0 siblings, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2020-08-19 12:46 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: linux-renesas-soc, linux-media, Mauro Carvalho Chehab,
	Kieran Bingham, Niklas Söderlund, sakari.ailus, hverkuil,
	hyunk, manivannan.sadhasivam

Hi Jacopo,

Thank you for the patch.

On Mon, Aug 17, 2020 at 04:35:38PM +0200, Jacopo Mondi wrote:
> The MAX9286 chip does not allow any modification to the image stream
> format it de-serializes from the GMSL bus to its MIPI CSI-2 output
> interface.
> 
> For this reason, when the format is queried from on any of the MAX9286
> pads, get the remote subdevice format and return it.

That's not how MC-based drivers are supposed to work though. Userspace
has to propagate formats between subdevs, it must not be done internally
in the kernel.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7c292f2e2704..e6a70dbd27df 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -742,8 +742,10 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  			   struct v4l2_subdev_format *format)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> +	struct v4l2_subdev_format remote_fmt = {};
> +	struct device *dev = &priv->client->dev;
>  	unsigned int pad = format->pad;
> +	int ret;
>  
>  	/*
>  	 * Multiplexed Stream Support: Support link validation by returning the
> @@ -754,12 +756,26 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  	if (pad == MAX9286_SRC_PAD)
>  		pad = __ffs(priv->bound_sources);
>  
> -	cfg_fmt = max9286_get_pad_format(priv, cfg, pad, format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		mutex_lock(&priv->mutex);
> +		format->format = *v4l2_subdev_get_try_format(&priv->sd,
> +							     cfg, pad);
> +		mutex_unlock(&priv->mutex);
> +
> +		return 0;
> +	}
> +
> +	remote_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	remote_fmt.pad = 0;
> +	ret = v4l2_subdev_call(priv->sources[pad].sd, pad, get_fmt, NULL,
> +			       &remote_fmt);
> +	if (ret) {
> +		dev_err(dev, "Unable get format on source %d\n", pad);
> +		return ret;
> +	}
>  
>  	mutex_lock(&priv->mutex);
> -	format->format = *cfg_fmt;
> +	format->format = remote_fmt.format;
>  	mutex_unlock(&priv->mutex);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] media: i2c: max9286: Get format from remote ends
  2020-08-19 12:46   ` Laurent Pinchart
@ 2020-08-24  7:48     ` Jacopo Mondi
  0 siblings, 0 replies; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-24  7:48 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, linux-renesas-soc, linux-media,
	Mauro Carvalho Chehab, Kieran Bingham, Niklas Söderlund,
	sakari.ailus, hverkuil, hyunk, manivannan.sadhasivam

Hi Laurent,

On Wed, Aug 19, 2020 at 03:46:46PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Aug 17, 2020 at 04:35:38PM +0200, Jacopo Mondi wrote:
> > The MAX9286 chip does not allow any modification to the image stream
> > format it de-serializes from the GMSL bus to its MIPI CSI-2 output
> > interface.
> >
> > For this reason, when the format is queried from on any of the MAX9286
> > pads, get the remote subdevice format and return it.
>
> That's not how MC-based drivers are supposed to work though. Userspace
> has to propagate formats between subdevs, it must not be done internally
> in the kernel.
>

I see your point, but in this case it's really not necessary to me.

The max9286 has not notion of image formats by itself, it just mirrors
what has been serialized on the GMSL bus and that's it.

As usual the line where things have to come from userspace and thing
that can be inferred by the driver is thin but if both you and Sakari
think this is not necessary, I'll drop.

Thanks
  j


> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 7c292f2e2704..e6a70dbd27df 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -742,8 +742,10 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> >  			   struct v4l2_subdev_format *format)
> >  {
> >  	struct max9286_priv *priv = sd_to_max9286(sd);
> > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > +	struct v4l2_subdev_format remote_fmt = {};
> > +	struct device *dev = &priv->client->dev;
> >  	unsigned int pad = format->pad;
> > +	int ret;
> >
> >  	/*
> >  	 * Multiplexed Stream Support: Support link validation by returning the
> > @@ -754,12 +756,26 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> >  	if (pad == MAX9286_SRC_PAD)
> >  		pad = __ffs(priv->bound_sources);
> >
> > -	cfg_fmt = max9286_get_pad_format(priv, cfg, pad, format->which);
> > -	if (!cfg_fmt)
> > -		return -EINVAL;
> > +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > +		mutex_lock(&priv->mutex);
> > +		format->format = *v4l2_subdev_get_try_format(&priv->sd,
> > +							     cfg, pad);
> > +		mutex_unlock(&priv->mutex);
> > +
> > +		return 0;
> > +	}
> > +
> > +	remote_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	remote_fmt.pad = 0;
> > +	ret = v4l2_subdev_call(priv->sources[pad].sd, pad, get_fmt, NULL,
> > +			       &remote_fmt);
> > +	if (ret) {
> > +		dev_err(dev, "Unable get format on source %d\n", pad);
> > +		return ret;
> > +	}
> >
> >  	mutex_lock(&priv->mutex);
> > -	format->format = *cfg_fmt;
> > +	format->format = remote_fmt.format;
> >  	mutex_unlock(&priv->mutex);
> >
> >  	return 0;
>
> --
> Regards,
>
> Laurent Pinchart

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-08-24  7:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-17 14:35 [PATCH 0/4] media: i2c: max9286: Use remote endpoint image format Jacopo Mondi
2020-08-17 14:35 ` [PATCH 1/4] media: i2c: max9286: Initialize try formats Jacopo Mondi
2020-08-18  7:09   ` Sakari Ailus
2020-08-18 12:03     ` Jacopo Mondi
2020-08-17 14:35 ` [PATCH 2/4] media: i2c: max9286: Get format from remote ends Jacopo Mondi
2020-08-19 12:46   ` Laurent Pinchart
2020-08-24  7:48     ` Jacopo Mondi
2020-08-17 14:35 ` [PATCH 3/4] media: i2c: max9286: Do not allow changing format Jacopo Mondi
2020-08-17 14:35 ` [PATCH 4/4] media: i2c: max9286: Remove cached formats Jacopo Mondi
2020-08-17 22:15   ` Hyun Kwon
2020-08-18 12:05     ` Jacopo Mondi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.