On Mon, 30 Nov 2009, Hans Verkuil wrote:
>
>
> On Fri, 27 Nov 2009, Guennadi Liakhovetski wrote:
>
> > On Fri, 27 Nov 2009, Hans Verkuil wrote:
> >
> > > Hi Guennadi,
> > >
> > > > Convert soc-camera core and all soc-camera drivers to the new mediabus
> > > > API. This also takes soc-camera client drivers one step closer to also
> > > > be
> > > > usable with generic v4l2-subdev host drivers.
> > >
> > > Just a quick question:
> > >
> > > > @@ -323,28 +309,39 @@ static int mt9m001_s_fmt(struct v4l2_subdev *sd,
> > > > struct v4l2_format *f)
> > > > /* No support for scaling so far, just crop. TODO: use skipping */
> > > > ret = mt9m001_s_crop(sd, &a);
> > > > if (!ret) {
> > > > - pix->width = mt9m001->rect.width;
> > > > - pix->height = mt9m001->rect.height;
> > > > - mt9m001->fourcc = pix->pixelformat;
> > > > + mf->width = mt9m001->rect.width;
> > > > + mf->height = mt9m001->rect.height;
> > > > + mt9m001->fmt = soc_mbus_find_datafmt(mf->code,
> > > > + mt9m001->fmts, mt9m001->num_fmts);
> > > > + mf->colorspace = mt9m001->fmt->colorspace;
> > > > }
> > > >
> > > > return ret;
> > > > }
> > > >
> > > > -static int mt9m001_try_fmt(struct v4l2_subdev *sd, struct v4l2_format
> > > > *f)
> > > > +static int mt9m001_try_fmt(struct v4l2_subdev *sd,
> > > > + struct v4l2_mbus_framefmt *mf)
> > > > {
> > > > struct i2c_client *client = sd->priv;
> > > > struct mt9m001 *mt9m001 = to_mt9m001(client);
> > > > - struct v4l2_pix_format *pix = &f->fmt.pix;
> > > > + const struct soc_mbus_datafmt *fmt;
> > > >
> > > > - v4l_bound_align_image(&pix->width, MT9M001_MIN_WIDTH,
> > > > + v4l_bound_align_image(&mf->width, MT9M001_MIN_WIDTH,
> > > > MT9M001_MAX_WIDTH, 1,
> > > > - &pix->height, MT9M001_MIN_HEIGHT + mt9m001->y_skip_top,
> > > > + &mf->height, MT9M001_MIN_HEIGHT + mt9m001->y_skip_top,
> > > > MT9M001_MAX_HEIGHT + mt9m001->y_skip_top, 0, 0);
> > > >
> > > > - if (pix->pixelformat == V4L2_PIX_FMT_SBGGR8 ||
> > > > - pix->pixelformat == V4L2_PIX_FMT_SBGGR16)
> > > > - pix->height = ALIGN(pix->height - 1, 2);
> > > > + if (mt9m001->fmts == mt9m001_colour_fmts)
> > > > + mf->height = ALIGN(mf->height - 1, 2);
> > > > +
> > > > + fmt = soc_mbus_find_datafmt(mf->code, mt9m001->fmts,
> > > > + mt9m001->num_fmts);
> > > > + if (!fmt) {
> > > > + fmt = mt9m001->fmt;
> > > > + mf->code = fmt->code;
> > > > + }
> > > > +
> > > > + mf->colorspace = fmt->colorspace;
> > > >
> > > > return 0;
> > > > }
> > >
> > > Why do the sensor drivers use soc_mbus_find_datafmt? They only seem to be
> > > interested in the colorspace field, but I don't see the reason for that.
> > > Most if not all sensors have a fixed colorspace depending on the
> > > pixelcode, so they can just ignore the colorspace that the caller
> > > requested and replace it with their own.
> >
> > Right, that's exactly what's done here. mt9m001 and mt9v022 drivers
> > support different formats, depending on the exact detected or specified by
> > the user model. That's why they have to search for the requested format in
> > supported list. and then - yes, they just put the found format into user
> > request:
> >
> > > > + mf->colorspace = fmt->colorspace;
> >
> > > I didn't have time for a full review, so I might have missed something.
>
> I looked at this more closely and I realized that I did indeed miss that
> soc_mbus_find_datafmt just searched in the pixelcode -> colorspace mapping
> array.
>
> I also realized that there is no need for that data structure and function
> to be soc-camera specific. I believe I said otherwise in an earlier review.
> My apologies for that, all I can say is that I had very little time to do
> the reviews...
No, you did not say otherwise about _these_ struct and function - they
only appeared in v2 of the mediabus API, after you'd suggested to move
colorspace into struct v4l2_mbus_framefmt.
> That said, there is no need for both the soc_mbus_datafmt struct and the
> soc_mbus_find_datafmt function. These can easily be replaced by something
> like this as a local function in each subdev:
>
> static enum v4l2_colorspace mt9m111_g_colorspace(enum v4l2_mbus_pixelcode
> code)
> {
> switch (code) {
> case V4L2_MBUS_FMT_YUYV:
> case V4L2_MBUS_FMT_YVYU:
> case V4L2_MBUS_FMT_UYVY:
> case V4L2_MBUS_FMT_VYUY:
> return V4L2_COLORSPACE_JPEG;
>
> case V4L2_MBUS_FMT_RGB555:
> case V4L2_MBUS_FMT_RGB565:
> case V4L2_MBUS_FMT_SBGGR8:
> case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> return V4L2_COLORSPACE_SRGB;
>
> default:
> return 0;
> }
> }
>
> So if mt9m111_g_colorspace() returns 0, then the format wasn't found.
> (Note that the compiler might give a warning for the return 0, so that might
> need some editing)
>
> Much simpler and much easier to understand.
Drivers are not forced to use that small and trivial function - everyone
is welcome to reinvent the wheel:-) In many cases it is indeed an
overkill, like mt9t031, which only supports one format. However, in some
other drivers this is not that trivial. First, as I said, mt9m001 and
mt9v022 generate that array dynamically, depending on the chip version and
platform configuration. So, you anyway would have to iterate over the
array. In other drivers, like ov772x and the recently submitted mt9t112
this function is also used to retrieve register configuration for a
specific pixel code. So, I still think that function is useful, and being
kept under soc-camera mediabus extensions, and being inline, it shouldn't
cause too many problems.
Thanks
Guennadi
---
Guennadi Liakhovetski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
