| Message ID | 20200817143540.247340-3-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: i2c: max9286: Use remote endpoint image format | expand |
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;
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
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;
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(-)