V4L: soc-camera: regression fix: calculate .sizeimage in soc_camera.c
Commit Message
Hi Guennadi,
On Mon, Apr 11, 2011 at 8:23 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
>
> On Mon, 11 Apr 2011, Aguirre, Sergio wrote:
>
> > Hi Guennadi,
> >
> > On Mon, Apr 11, 2011 at 3:58 AM, Guennadi Liakhovetski <
> > g.liakhovetski@gmx.de> wrote:
> >
> > > A recent patch has given individual soc-camera host drivers a possibility
> > > to calculate .sizeimage and .bytesperline pixel format fields internally,
> > > however, some drivers relied on the core calculating these values for
> > > them, following a default algorithm. This patch restores the default
> > > calculation for such drivers.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > > diff --git a/drivers/media/video/soc_camera.c
> > > b/drivers/media/video/soc_camera.c
> > > index 4628448..0918c48 100644
> > > --- a/drivers/media/video/soc_camera.c
> > > +++ b/drivers/media/video/soc_camera.c
> > > @@ -376,6 +376,9 @@ static int soc_camera_set_fmt(struct soc_camera_device
> > > *icd,
> > > dev_dbg(&icd->dev, "S_FMT(%c%c%c%c, %ux%u)\n",
> > > pixfmtstr(pix->pixelformat), pix->width, pix->height);
> > >
> > > + pix->bytesperline = 0;
> > > + pix->sizeimage = 0;
> > > +
> > > /* We always call try_fmt() before set_fmt() or set_crop() */
> > > ret = ici->ops->try_fmt(icd, f);
> > > if (ret < 0)
> > > @@ -391,6 +394,17 @@ static int soc_camera_set_fmt(struct soc_camera_device
> > > *icd,
> > > return -EINVAL;
> > > }
> > >
> > > + if (!pix->sizeimage) {
> > > + if (!pix->bytesperline) {
> > > + ret = soc_mbus_bytes_per_line(pix->width,
> > > +
> > > icd->current_fmt->host_fmt);
> > > + if (ret > 0)
> > > + pix->bytesperline = ret;
> > > + }
> > > + if (pix->bytesperline)
> > > + pix->sizeimage = pix->bytesperline * pix->height;
> > > + }
> > > +
> > >
> >
> > Shouldn't all this be better done in try_fmt?
>
> Not _better_. We might choose to additionally do it for try_fmt too. But
>
> 1. we didn't do it before, applications don't seem to care.
> 2. you cannot store / reuse those .sizeimage & .bytesperline values - this
> is just a "try"
> 3. it would be a bit difficult to realise - we need a struct
> soc_mbus_pixelfmt to call soc_mbus_bytes_per_line(), which we don't have,
> so, we'd need to obtain it using soc_camera_xlate_by_fourcc().
>
> This all would work, but in any case it would be an enhancement, but not a
> regression fix.
Ok. And how about the attached patch? Would that work?
Regards,
Sergio
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
Comments
On Mon, 11 Apr 2011, Aguirre, Sergio wrote:
> Hi Guennadi,
>
> On Mon, Apr 11, 2011 at 8:23 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> >
> > On Mon, 11 Apr 2011, Aguirre, Sergio wrote:
> >
> > > Hi Guennadi,
> > >
> > > On Mon, Apr 11, 2011 at 3:58 AM, Guennadi Liakhovetski <
> > > g.liakhovetski@gmx.de> wrote:
> > >
> > > > A recent patch has given individual soc-camera host drivers a possibility
> > > > to calculate .sizeimage and .bytesperline pixel format fields internally,
> > > > however, some drivers relied on the core calculating these values for
> > > > them, following a default algorithm. This patch restores the default
> > > > calculation for such drivers.
> > > >
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > ---
> > > > diff --git a/drivers/media/video/soc_camera.c
> > > > b/drivers/media/video/soc_camera.c
> > > > index 4628448..0918c48 100644
> > > > --- a/drivers/media/video/soc_camera.c
> > > > +++ b/drivers/media/video/soc_camera.c
> > > > @@ -376,6 +376,9 @@ static int soc_camera_set_fmt(struct soc_camera_device
> > > > *icd,
> > > > dev_dbg(&icd->dev, "S_FMT(%c%c%c%c, %ux%u)\n",
> > > > pixfmtstr(pix->pixelformat), pix->width, pix->height);
> > > >
> > > > + pix->bytesperline = 0;
> > > > + pix->sizeimage = 0;
> > > > +
> > > > /* We always call try_fmt() before set_fmt() or set_crop() */
> > > > ret = ici->ops->try_fmt(icd, f);
> > > > if (ret < 0)
> > > > @@ -391,6 +394,17 @@ static int soc_camera_set_fmt(struct soc_camera_device
> > > > *icd,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + if (!pix->sizeimage) {
> > > > + if (!pix->bytesperline) {
> > > > + ret = soc_mbus_bytes_per_line(pix->width,
> > > > +
> > > > icd->current_fmt->host_fmt);
> > > > + if (ret > 0)
> > > > + pix->bytesperline = ret;
> > > > + }
> > > > + if (pix->bytesperline)
> > > > + pix->sizeimage = pix->bytesperline * pix->height;
> > > > + }
> > > > +
> > > >
> > >
> > > Shouldn't all this be better done in try_fmt?
> >
> > Not _better_. We might choose to additionally do it for try_fmt too. But
> >
> > 1. we didn't do it before, applications don't seem to care.
> > 2. you cannot store / reuse those .sizeimage & .bytesperline values - this
> > is just a "try"
> > 3. it would be a bit difficult to realise - we need a struct
> > soc_mbus_pixelfmt to call soc_mbus_bytes_per_line(), which we don't have,
> > so, we'd need to obtain it using soc_camera_xlate_by_fourcc().
> >
> > This all would work, but in any case it would be an enhancement, but not a
> > regression fix.
>
> Ok. And how about the attached patch? Would that work?
Please, post patches inline.
Yes, I think, ot would work too, only the call to
soc_camera_xlate_by_fourcc() in the S_FMT case is superfluous, after
ici->ops->set_fmt() we already have it in icd->current_fmt->host_fmt.
Otherwise - yes, we could do it this way too. Janusz, could you test,
please?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
On Mon, Apr 11, 2011 at 11:58 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Mon, 11 Apr 2011, Aguirre, Sergio wrote:
>
>> Hi Guennadi,
>>
>> On Mon, Apr 11, 2011 at 8:23 AM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> >
>> > On Mon, 11 Apr 2011, Aguirre, Sergio wrote:
>> >
>> > > Hi Guennadi,
>> > >
>> > > On Mon, Apr 11, 2011 at 3:58 AM, Guennadi Liakhovetski <
>> > > g.liakhovetski@gmx.de> wrote:
>> > >
>> > > > A recent patch has given individual soc-camera host drivers a possibility
>> > > > to calculate .sizeimage and .bytesperline pixel format fields internally,
>> > > > however, some drivers relied on the core calculating these values for
>> > > > them, following a default algorithm. This patch restores the default
>> > > > calculation for such drivers.
>> > > >
>> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > > > ---
>> > > > diff --git a/drivers/media/video/soc_camera.c
>> > > > b/drivers/media/video/soc_camera.c
>> > > > index 4628448..0918c48 100644
>> > > > --- a/drivers/media/video/soc_camera.c
>> > > > +++ b/drivers/media/video/soc_camera.c
>> > > > @@ -376,6 +376,9 @@ static int soc_camera_set_fmt(struct soc_camera_device
>> > > > *icd,
>> > > > dev_dbg(&icd->dev, "S_FMT(%c%c%c%c, %ux%u)\n",
>> > > > pixfmtstr(pix->pixelformat), pix->width, pix->height);
>> > > >
>> > > > + pix->bytesperline = 0;
>> > > > + pix->sizeimage = 0;
>> > > > +
>> > > > /* We always call try_fmt() before set_fmt() or set_crop() */
>> > > > ret = ici->ops->try_fmt(icd, f);
>> > > > if (ret < 0)
>> > > > @@ -391,6 +394,17 @@ static int soc_camera_set_fmt(struct soc_camera_device
>> > > > *icd,
>> > > > return -EINVAL;
>> > > > }
>> > > >
>> > > > + if (!pix->sizeimage) {
>> > > > + if (!pix->bytesperline) {
>> > > > + ret = soc_mbus_bytes_per_line(pix->width,
>> > > > +
>> > > > icd->current_fmt->host_fmt);
>> > > > + if (ret > 0)
>> > > > + pix->bytesperline = ret;
>> > > > + }
>> > > > + if (pix->bytesperline)
>> > > > + pix->sizeimage = pix->bytesperline * pix->height;
>> > > > + }
>> > > > +
>> > > >
>> > >
>> > > Shouldn't all this be better done in try_fmt?
>> >
>> > Not _better_. We might choose to additionally do it for try_fmt too. But
>> >
>> > 1. we didn't do it before, applications don't seem to care.
>> > 2. you cannot store / reuse those .sizeimage & .bytesperline values - this
>> > is just a "try"
>> > 3. it would be a bit difficult to realise - we need a struct
>> > soc_mbus_pixelfmt to call soc_mbus_bytes_per_line(), which we don't have,
>> > so, we'd need to obtain it using soc_camera_xlate_by_fourcc().
>> >
>> > This all would work, but in any case it would be an enhancement, but not a
>> > regression fix.
>>
>> Ok. And how about the attached patch? Would that work?
>
> Please, post patches inline.
Ok, sorry.
>
> Yes, I think, ot would work too, only the call to
> soc_camera_xlate_by_fourcc() in the S_FMT case is superfluous, after
> ici->ops->set_fmt() we already have it in icd->current_fmt->host_fmt.
Well, the difference is that, after my patch, this is called after
ici->ops->try_fmt, so
icd->current_fmt->host_fmt doesn't match the intended format to try, as it's not
stored as a current_fmt.
Unless i'm getting confused with something...
Regards,
Sergio
> Otherwise - yes, we could do it this way too. Janusz, could you test,
> please?
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
--
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
Dnia poniedzia?ek 11 kwiecie? 2011 o 18:58:51 Guennadi Liakhovetski napisa?(a):
> On Mon, 11 Apr 2011, Aguirre, Sergio wrote:
> >
> > Ok. And how about the attached patch? Would that work?
>
> Yes, I think, ot would work too, only the call to
> soc_camera_xlate_by_fourcc() in the S_FMT case is superfluous, after
> ici->ops->set_fmt() we already have it in icd->current_fmt->host_fmt.
> Otherwise - yes, we could do it this way too. Janusz, could you test,
> please?
Looks like not based on the current mainline (-rc2) tree:
CHECK drivers/media/video/soc_camera.c
drivers/media/video/soc_camera.c:146:9: error: undefined identifier 'pixfmtstr'
CC drivers/media/video/soc_camera.o
drivers/media/video/soc_camera.c: In function 'soc_camera_try_fmt':
drivers/media/video/soc_camera.c:146: error: implicit declaration of function 'pixfmtstr'
drivers/media/video/soc_camera.c:146: warning: too few arguments for format
drivers/media/video/soc_camera.c: In function 'soc_camera_try_fmt_vid_cap':
drivers/media/video/soc_camera.c:180: warning: unused variable 'ici'
Thanks,
Janusz
--
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
From 5442074c7760429c06f62917503c1d8a2f79cd21 Mon Sep 17 00:00:00 2001
From: Sergio Aguirre <saaguirre@ti.com>
Date: Mon, 11 Apr 2011 11:14:33 -0500
Subject: [PATCH] V4L: soc-camera: regression fix: calculate .sizeimage in soc_camera.c
A recent patch has given individual soc-camera host drivers a possibility
to calculate .sizeimage and .bytesperline pixel format fields internally,
however, some drivers relied on the core calculating these values for
them, following a default algorithm. This patch restores the default
calculation for such drivers.
Based on initial patch by Guennadi Liakhovetski, found here:
http://www.spinics.net/lists/linux-media/msg31282.html
Except that this covers try_fmt aswell.
Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
drivers/media/video/soc_camera.c | 41 ++++++++++++++++++++++++++++++++++++-
1 files changed, 39 insertions(+), 2 deletions(-)
@@ -136,6 +136,43 @@ unsigned long soc_camera_apply_sensor_flags(struct soc_camera_link *icl,
}
EXPORT_SYMBOL(soc_camera_apply_sensor_flags);
+static int soc_camera_try_fmt(struct soc_camera_device *icd,
+ struct v4l2_format *f)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct v4l2_pix_format *pix = &f->fmt.pix;
+ int ret;
+
+ dev_dbg(&icd->dev, "TRY_FMT(%c%c%c%c, %ux%u)\n",
+ pixfmtstr(pix->pixelformat), pix->width, pix->height);
+
+ pix->bytesperline = 0;
+ pix->sizeimage = 0;
+
+ ret = ici->ops->try_fmt(icd, f);
+ if (ret < 0)
+ return ret;
+
+ if (!pix->sizeimage) {
+ if (!pix->bytesperline) {
+ const struct soc_camera_format_xlate *xlate;
+
+ xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
+ if (!xlate)
+ return -EINVAL;
+
+ ret = soc_mbus_bytes_per_line(pix->width,
+ xlate->host_fmt);
+ if (ret > 0)
+ pix->bytesperline = ret;
+ }
+ if (pix->bytesperline)
+ pix->sizeimage = pix->bytesperline * pix->height;
+ }
+
+ return 0;
+}
+
static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)
{
@@ -149,7 +186,7 @@ static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
return -EINVAL;
/* limit format to hardware capabilities */
- return ici->ops->try_fmt(icd, f);
+ return soc_camera_try_fmt(icd, f);
}
static int soc_camera_enum_input(struct file *file, void *priv,
@@ -377,7 +414,7 @@ static int soc_camera_set_fmt(struct soc_camera_device *icd,
pixfmtstr(pix->pixelformat), pix->width, pix->height);
/* We always call try_fmt() before set_fmt() or set_crop() */
- ret = ici->ops->try_fmt(icd, f);
+ ret = soc_camera_try_fmt(icd, f);
if (ret < 0)
return ret;
--
1.7.0.4