[PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Janusz Krzysztofik]

Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline and 
sizeimage memebers of v4l2_pix_format structure have no longer been 
calculated inside soc_camera_g_fmt_vid_cap(), but rather passed via 
soc_camera_device structure from a host driver callback invoked by 
soc_camera_set_fmt().

OMAP1 camera host driver has never been providing these parameters, so 
it no longer works correctly. Fix it by adding suitable assignments to 
omap1_cam_set_fmt().

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
 drivers/media/video/omap1_camera.c |    6 ++++++
 1 file changed, 6 insertions(+)

--- linux-2.6.39-rc2/drivers/media/video/omap1_camera.c.orig	2011-04-06 14:30:37.000000000 +0200
+++ linux-2.6.39-rc2/drivers/media/video/omap1_camera.c	2011-04-09 00:16:36.000000000 +0200
@@ -1292,6 +1292,12 @@ static int omap1_cam_set_fmt(struct soc_
 	pix->colorspace  = mf.colorspace;
 	icd->current_fmt = xlate;
 
+	pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
+						    xlate->host_fmt);
+	if (pix->bytesperline < 0)
+		return pix->bytesperline;
+	pix->sizeimage = pix->height * pix->bytesperline;
+
 	return 0;
 }
 

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Guennadi Liakhovetski]

Hi Janusz

On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:

> Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline and 
> sizeimage memebers of v4l2_pix_format structure have no longer been 
> calculated inside soc_camera_g_fmt_vid_cap(), but rather passed via 
> soc_camera_device structure from a host driver callback invoked by 
> soc_camera_set_fmt().
> 
> OMAP1 camera host driver has never been providing these parameters, so 
> it no longer works correctly. Fix it by adding suitable assignments to 
> omap1_cam_set_fmt().

Thanks for the patch, but now it looks like many soc-camera host drivers 
are re-implementing this very same calculation in different parts of their 
code - in try_fmt, set_fmt, get_fmt. Why don't we unify them all, 
implement this centrally in soc_camera.c and remove all those 
calculations? Could you cook up a patch or maybe several patches - for 
soc_camera.c and all drivers?

Thanks
Guennadi

> 
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> ---
>  drivers/media/video/omap1_camera.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- linux-2.6.39-rc2/drivers/media/video/omap1_camera.c.orig	2011-04-06 14:30:37.000000000 +0200
> +++ linux-2.6.39-rc2/drivers/media/video/omap1_camera.c	2011-04-09 00:16:36.000000000 +0200
> @@ -1292,6 +1292,12 @@ static int omap1_cam_set_fmt(struct soc_
>  	pix->colorspace  = mf.colorspace;
>  	icd->current_fmt = xlate;
>  
> +	pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
> +						    xlate->host_fmt);
> +	if (pix->bytesperline < 0)
> +		return pix->bytesperline;
> +	pix->sizeimage = pix->height * pix->bytesperline;
> +
>  	return 0;
>  }
>  
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Janusz Krzysztofik]

Dnia niedziela 10 kwiecień 2011 o 18:00:14 Guennadi Liakhovetski 
napisał(a):
> Hi Janusz
> 
> On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
> > Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline
> > and sizeimage memebers of v4l2_pix_format structure have no longer
> > been calculated inside soc_camera_g_fmt_vid_cap(), but rather
> > passed via soc_camera_device structure from a host driver callback
> > invoked by soc_camera_set_fmt().
> > 
> > OMAP1 camera host driver has never been providing these parameters,
> > so it no longer works correctly. Fix it by adding suitable
> > assignments to omap1_cam_set_fmt().
> 
> Thanks for the patch, but now it looks like many soc-camera host
> drivers are re-implementing this very same calculation in different
> parts of their code - in try_fmt, set_fmt, get_fmt. Why don't we
> unify them all, implement this centrally in soc_camera.c and remove
> all those calculations? 

Wasn't it already unified before commit in question?

> Could you cook up a patch or maybe several
> patches - for soc_camera.c and all drivers?

Perhaps I could, as soon as I found some spare time, but first I'd have 
to really understand why we need bytesperline or sizeimage handling 
being changed from how they worked before commit 
0e4c180d3e2cc11e248f29d4c604b6194739d05a was introduced. I never had a 
need to customize bytesperline or sizeimage calculations in my driver. 

But even then, I think these new patches would rather qualify for next 
merge window, while the OMAP1 driver case is just a regression, caused 
by an alredy applied, unrelated change to the underlying framework, and 
requires a fix if that change is not going to be reverted.

Maybe the author of the change, Sergio Aguirre form TI (CCing him), 
could rework his patch in a way which wouldn't impose, as a side effect, 
the new requirement of those structure members being passed from host 
drivers?

Thanks,
Janusz

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Guennadi Liakhovetski]

On Mon, 11 Apr 2011, Janusz Krzysztofik wrote:

> Dnia niedziela 10 kwiecień 2011 o 18:00:14 Guennadi Liakhovetski 
> napisał(a):
> > Hi Janusz
> > 
> > On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
> > > Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline
> > > and sizeimage memebers of v4l2_pix_format structure have no longer
> > > been calculated inside soc_camera_g_fmt_vid_cap(), but rather
> > > passed via soc_camera_device structure from a host driver callback
> > > invoked by soc_camera_set_fmt().
> > > 
> > > OMAP1 camera host driver has never been providing these parameters,
> > > so it no longer works correctly. Fix it by adding suitable
> > > assignments to omap1_cam_set_fmt().
> > 
> > Thanks for the patch, but now it looks like many soc-camera host
> > drivers are re-implementing this very same calculation in different
> > parts of their code - in try_fmt, set_fmt, get_fmt. Why don't we
> > unify them all, implement this centrally in soc_camera.c and remove
> > all those calculations? 
> 
> Wasn't it already unified before commit in question?

It was, but it was inconsistent. It was done centrally, and it was done in 
several other drivers additionally, creating a mess.

> > Could you cook up a patch or maybe several
> > patches - for soc_camera.c and all drivers?
> 
> Perhaps I could, as soon as I found some spare time, but first I'd have 
> to really understand why we need bytesperline or sizeimage handling 
> being changed from how they worked before commit 
> 0e4c180d3e2cc11e248f29d4c604b6194739d05a was introduced. I never had a 
> need to customize bytesperline or sizeimage calculations in my driver. 
> 
> But even then, I think these new patches would rather qualify for next 
> merge window, while the OMAP1 driver case is just a regression, caused 
> by an alredy applied, unrelated change to the underlying framework, and 
> requires a fix if that change is not going to be reverted.

Sure, we want this fixed for the current merge window. But I think it is 
possible with a relatively easy patch to soc_camera.c. I asked you, 
whether you'd be able to make a patch of that kind, if you don't have 
enough time ATM, I can try to make one and just ask you to test it on 
omap1. I'll have a look at it tomorrow.

> Maybe the author of the change, Sergio Aguirre form TI (CCing him), 
> could rework his patch in a way which wouldn't impose, as a side effect, 
> the new requirement of those structure members being passed from host 
> drivers?

I think we can just make an incremental patch to fill in those fields 
centrally, if the driver didn't do it for us.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Guennadi Liakhovetski]

On Mon, 11 Apr 2011, Janusz Krzysztofik wrote:

> Dnia niedziela 10 kwiecień 2011 o 18:00:14 Guennadi Liakhovetski 
> napisał(a):
> > Hi Janusz
> > 
> > On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
> > > Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline
> > > and sizeimage memebers of v4l2_pix_format structure have no longer
> > > been calculated inside soc_camera_g_fmt_vid_cap(), but rather
> > > passed via soc_camera_device structure from a host driver callback
> > > invoked by soc_camera_set_fmt().
> > > 
> > > OMAP1 camera host driver has never been providing these parameters,
> > > so it no longer works correctly. Fix it by adding suitable
> > > assignments to omap1_cam_set_fmt().
> > 
> > Thanks for the patch, but now it looks like many soc-camera host
> > drivers are re-implementing this very same calculation in different
> > parts of their code - in try_fmt, set_fmt, get_fmt. Why don't we
> > unify them all, implement this centrally in soc_camera.c and remove
> > all those calculations? 
> 
> Wasn't it already unified before commit in question?

Please test the patch, I've sent a minute ago.

Thanks
Guennadi

> 
> > Could you cook up a patch or maybe several
> > patches - for soc_camera.c and all drivers?
> 
> Perhaps I could, as soon as I found some spare time, but first I'd have 
> to really understand why we need bytesperline or sizeimage handling 
> being changed from how they worked before commit 
> 0e4c180d3e2cc11e248f29d4c604b6194739d05a was introduced. I never had a 
> need to customize bytesperline or sizeimage calculations in my driver. 
> 
> But even then, I think these new patches would rather qualify for next 
> merge window, while the OMAP1 driver case is just a regression, caused 
> by an alredy applied, unrelated change to the underlying framework, and 
> requires a fix if that change is not going to be reverted.
> 
> Maybe the author of the change, Sergio Aguirre form TI (CCing him), 
> could rework his patch in a way which wouldn't impose, as a side effect, 
> the new requirement of those structure members being passed from host 
> drivers?
> 
> Thanks,
> Janusz
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Kassey Lee]

hi, Guennadi:
    a lot of sensors support JPEG output.
    1) bytesperline is defined by sensor timing.
    2) and sizeimage is unknow for jpeg.

  how about for JPEG
   1) host driver gets bytesperline from sensor driver.
   2) sizeimage refilled by host driver after dma transfer done( a
frame is received)
  thanks.


2011/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> Hi Janusz
>
> On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
>
>> Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline and
>> sizeimage memebers of v4l2_pix_format structure have no longer been
>> calculated inside soc_camera_g_fmt_vid_cap(), but rather passed via
>> soc_camera_device structure from a host driver callback invoked by
>> soc_camera_set_fmt().
>>
>> OMAP1 camera host driver has never been providing these parameters, so
>> it no longer works correctly. Fix it by adding suitable assignments to
>> omap1_cam_set_fmt().
>
> Thanks for the patch, but now it looks like many soc-camera host drivers
> are re-implementing this very same calculation in different parts of their
> code - in try_fmt, set_fmt, get_fmt. Why don't we unify them all,
> implement this centrally in soc_camera.c and remove all those
> calculations? Could you cook up a patch or maybe several patches - for
> soc_camera.c and all drivers?
>
> Thanks
> Guennadi
>
>>
>> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
>> ---
>>  drivers/media/video/omap1_camera.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> --- linux-2.6.39-rc2/drivers/media/video/omap1_camera.c.orig  2011-04-06 14:30:37.000000000 +0200
>> +++ linux-2.6.39-rc2/drivers/media/video/omap1_camera.c       2011-04-09 00:16:36.000000000 +0200
>> @@ -1292,6 +1292,12 @@ static int omap1_cam_set_fmt(struct soc_
>>       pix->colorspace  = mf.colorspace;
>>       icd->current_fmt = xlate;
>>
>> +     pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
>> +                                                 xlate->host_fmt);
>> +     if (pix->bytesperline < 0)
>> +             return pix->bytesperline;
>> +     pix->sizeimage = pix->height * pix->bytesperline;
>> +
>>       return 0;
>>  }
>>
>>
>
> ---
> 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
>

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Guennadi Liakhovetski]

Hi

On Tue, 12 Apr 2011, Kassey Lee wrote:

> hi, Guennadi:
>     a lot of sensors support JPEG output.
>     1) bytesperline is defined by sensor timing.
>     2) and sizeimage is unknow for jpeg.
> 
>   how about for JPEG
>    1) host driver gets bytesperline from sensor driver.
>    2) sizeimage refilled by host driver after dma transfer done( a
> frame is received)
>   thanks.

How is this done currently on other V4L2 drivers? To transfer a frame you 
usually first do at least one of S_FMT and G_FMT, at which time you 
already have to report sizeimage to the user - before any transfer has 
taken place. Currently with soc-camera it is already possible to override 
sizeimage and bytesperline from the host driver. Just set them to whatever 
you need in your try_fmt and they will be kept. Not sure how you want to 
do that, if you need to first read in a frame - do you want to perform 
some dummy frame transfer? You might not even have any buffers queued yet, 
so, it has to be a read without writing to RAM. Don't such compressed 
formats just put a value in sizeimage, that is a calculated maximum size?

Thanks
Guennadi

> 2011/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > Hi Janusz
> >
> > On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
> >
> >> Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline and
> >> sizeimage memebers of v4l2_pix_format structure have no longer been
> >> calculated inside soc_camera_g_fmt_vid_cap(), but rather passed via
> >> soc_camera_device structure from a host driver callback invoked by
> >> soc_camera_set_fmt().
> >>
> >> OMAP1 camera host driver has never been providing these parameters, so
> >> it no longer works correctly. Fix it by adding suitable assignments to
> >> omap1_cam_set_fmt().
> >
> > Thanks for the patch, but now it looks like many soc-camera host drivers
> > are re-implementing this very same calculation in different parts of their
> > code - in try_fmt, set_fmt, get_fmt. Why don't we unify them all,
> > implement this centrally in soc_camera.c and remove all those
> > calculations? Could you cook up a patch or maybe several patches - for
> > soc_camera.c and all drivers?
> >
> > Thanks
> > Guennadi
> >
> >>
> >> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> >> ---
> >>  drivers/media/video/omap1_camera.c |    6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> --- linux-2.6.39-rc2/drivers/media/video/omap1_camera.c.orig  2011-04-06 14:30:37.000000000 +0200
> >> +++ linux-2.6.39-rc2/drivers/media/video/omap1_camera.c       2011-04-09 00:16:36.000000000 +0200
> >> @@ -1292,6 +1292,12 @@ static int omap1_cam_set_fmt(struct soc_
> >>       pix->colorspace  = mf.colorspace;
> >>       icd->current_fmt = xlate;
> >>
> >> +     pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
> >> +                                                 xlate->host_fmt);
> >> +     if (pix->bytesperline < 0)
> >> +             return pix->bytesperline;
> >> +     pix->sizeimage = pix->height * pix->bytesperline;
> >> +
> >>       return 0;
> >>  }
> >>
> >>
> >
> > ---
> > 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
> >
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Sylwester Nawrocki]

Hi,

On 04/12/2011 08:28 AM, Guennadi Liakhovetski wrote:
> Hi
> 
> On Tue, 12 Apr 2011, Kassey Lee wrote:
> 
>> hi, Guennadi:
>>     a lot of sensors support JPEG output.
>>     1) bytesperline is defined by sensor timing.

Im not sure whether this is the case. Doesn't bytesperline refer only 
to the data layout in memory buffer written by the DMA? 
i.e. does padding really makes sens for JPEG files?

>>     2) and sizeimage is unknow for jpeg.
>>
>>   how about for JPEG
>>    1) host driver gets bytesperline from sensor driver.
>>    2) sizeimage refilled by host driver after dma transfer done( a
>> frame is received)

You might want to use v4l2_buffer::bytesused to inform user space about
the actual size of the captured frame, which would be set before a buffer
is dequeued from the driver. The size of JPEG file will depend on the content,
so IMHO you could not use v4l2_pix_fmt::sizeimage in such way.


>>   thanks.
> 
> How is this done currently on other V4L2 drivers? To transfer a frame you
> usually first do at least one of S_FMT and G_FMT, at which time you 
> already have to report sizeimage to the user - before any transfer has 
> taken place. Currently with soc-camera it is already possible to override 
> sizeimage and bytesperline from the host driver. Just set them to whatever 
> you need in your try_fmt and they will be kept. Not sure how you want to 
> do that, if you need to first read in a frame - do you want to perform 
> some dummy frame transfer? You might not even have any buffers queued yet, 
> so, it has to be a read without writing to RAM. Don't such compressed 
> formats just put a value in sizeimage, that is a calculated maximum size?

I the S5P FIMC driver I used to set sizeimage to some arbitrary value,
(it's not yet in mainline kernel), e.g.
sizeimage = width * height * C,  where C = 1
bytesperline = width.

However it would be useful to make the C coefficient dependent on JPEG
compression quality, not to make the image buffer unnecessary large.
I thought about creating a separate control class for JPEG but the quality
control was so far everything I would need to put in this class. It's on my
to do list to figure out what controls set would cover the standard.


Regards,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Kassey Lee]

2011/4/12 Kassey Lee <kassey1216@gmail.com>:
> Hi, Guennadi;
>           for sizeimage , I agree with you. that we can overwrite it
> after a frame is done.
>
>    for byteperline: on Marvell soc.
>    it needs to know the bytesperline before receive frame from sensor.
>    what we did now is hardcode  in host driver for bytesperline.
>
>    since different sensors have different timing for JPEG, and
> bytesperline is different.
>    while  soc_mbus_bytes_per_line does not support JPEG.
>
>    So, we want that host driver can get byteperline from sensor
> driver (sub dev) before transfer a frame for JPEG format.
>    a way to do this:
>    soc_mbus_bytes_per_line return 0 for JPEG, and host driver will
> try another API to get bytesperline for JPEG from sensor driver.
>     the effort is new API or reused other API.
>
>    Is that reasonable ?
>
>
>
> 2011/4/12 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> Hi
>>
>> On Tue, 12 Apr 2011, Kassey Lee wrote:
>>
>>> hi, Guennadi:
>>>     a lot of sensors support JPEG output.
>>>     1) bytesperline is defined by sensor timing.
>>>     2) and sizeimage is unknow for jpeg.
>>>
>>>   how about for JPEG
>>>    1) host driver gets bytesperline from sensor driver.
>>>    2) sizeimage refilled by host driver after dma transfer done( a
>>> frame is received)
>>>   thanks.
>>
>> How is this done currently on other V4L2 drivers? To transfer a frame you
>> usually first do at least one of S_FMT and G_FMT, at which time you
>> already have to report sizeimage to the user - before any transfer has
>> taken place. Currently with soc-camera it is already possible to override
>> sizeimage and bytesperline from the host driver. Just set them to whatever
>> you need in your try_fmt and they will be kept. Not sure how you want to
>> do that, if you need to first read in a frame - do you want to perform
>> some dummy frame transfer? You might not even have any buffers queued yet,
>> so, it has to be a read without writing to RAM. Don't such compressed
>> formats just put a value in sizeimage, that is a calculated maximum size?
>>
>> Thanks
>> Guennadi
>>
>>> 2011/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>>> > Hi Janusz
>>> >
>>> > On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
>>> >
>>> >> Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline and
>>> >> sizeimage memebers of v4l2_pix_format structure have no longer been
>>> >> calculated inside soc_camera_g_fmt_vid_cap(), but rather passed via
>>> >> soc_camera_device structure from a host driver callback invoked by
>>> >> soc_camera_set_fmt().
>>> >>
>>> >> OMAP1 camera host driver has never been providing these parameters, so
>>> >> it no longer works correctly. Fix it by adding suitable assignments to
>>> >> omap1_cam_set_fmt().
>>> >
>>> > Thanks for the patch, but now it looks like many soc-camera host drivers
>>> > are re-implementing this very same calculation in different parts of their
>>> > code - in try_fmt, set_fmt, get_fmt. Why don't we unify them all,
>>> > implement this centrally in soc_camera.c and remove all those
>>> > calculations? Could you cook up a patch or maybe several patches - for
>>> > soc_camera.c and all drivers?
>>> >
>>> > Thanks
>>> > Guennadi
>>> >
>>> >>
>>> >> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
>>> >> ---
>>> >>  drivers/media/video/omap1_camera.c |    6 ++++++
>>> >>  1 file changed, 6 insertions(+)
>>> >>
>>> >> --- linux-2.6.39-rc2/drivers/media/video/omap1_camera.c.orig  2011-04-06 14:30:37.000000000 +0200
>>> >> +++ linux-2.6.39-rc2/drivers/media/video/omap1_camera.c       2011-04-09 00:16:36.000000000 +0200
>>> >> @@ -1292,6 +1292,12 @@ static int omap1_cam_set_fmt(struct soc_
>>> >>       pix->colorspace  = mf.colorspace;
>>> >>       icd->current_fmt = xlate;
>>> >>
>>> >> +     pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
>>> >> +                                                 xlate->host_fmt);
>>> >> +     if (pix->bytesperline < 0)
>>> >> +             return pix->bytesperline;
>>> >> +     pix->sizeimage = pix->height * pix->bytesperline;
>>> >> +
>>> >>       return 0;
>>> >>  }
>>> >>
>>> >>
>>> >
>>> > ---
>>> > 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
>>> >
>>>
>>
>> ---
>> 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
>>
>

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Guennadi Liakhovetski]

On Tue, 12 Apr 2011, Kassey Lee wrote:

> 2011/4/12 Kassey Lee <kassey1216@gmail.com>:
> > Hi, Guennadi;
> >           for sizeimage , I agree with you. that we can overwrite it
> > after a frame is done.
> >
> >    for byteperline: on Marvell soc.
> >    it needs to know the bytesperline before receive frame from sensor.
> >    what we did now is hardcode  in host driver for bytesperline.
> >
> >    since different sensors have different timing for JPEG, and
> > bytesperline is different.
> >    while  soc_mbus_bytes_per_line does not support JPEG.
> >
> >    So, we want that host driver can get byteperline from sensor
> > driver (sub dev) before transfer a frame for JPEG format.
> >    a way to do this:
> >    soc_mbus_bytes_per_line return 0 for JPEG, and host driver will
> > try another API to get bytesperline for JPEG from sensor driver.
> >     the effort is new API or reused other API.
> >
> >    Is that reasonable ?

If you mean this your patch

http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/31323/match=

then I've queued it for 2.6.40.

Thanks
Guennadi

> >
> >
> >
> > 2011/4/12 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> >> Hi
> >>
> >> On Tue, 12 Apr 2011, Kassey Lee wrote:
> >>
> >>> hi, Guennadi:
> >>>     a lot of sensors support JPEG output.
> >>>     1) bytesperline is defined by sensor timing.
> >>>     2) and sizeimage is unknow for jpeg.
> >>>
> >>>   how about for JPEG
> >>>    1) host driver gets bytesperline from sensor driver.
> >>>    2) sizeimage refilled by host driver after dma transfer done( a
> >>> frame is received)
> >>>   thanks.
> >>
> >> How is this done currently on other V4L2 drivers? To transfer a frame you
> >> usually first do at least one of S_FMT and G_FMT, at which time you
> >> already have to report sizeimage to the user - before any transfer has
> >> taken place. Currently with soc-camera it is already possible to override
> >> sizeimage and bytesperline from the host driver. Just set them to whatever
> >> you need in your try_fmt and they will be kept. Not sure how you want to
> >> do that, if you need to first read in a frame - do you want to perform
> >> some dummy frame transfer? You might not even have any buffers queued yet,
> >> so, it has to be a read without writing to RAM. Don't such compressed
> >> formats just put a value in sizeimage, that is a calculated maximum size?
> >>
> >> Thanks
> >> Guennadi
> >>
> >>> 2011/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> >>> > Hi Janusz
> >>> >
> >>> > On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
> >>> >
> >>> >> Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline and
> >>> >> sizeimage memebers of v4l2_pix_format structure have no longer been
> >>> >> calculated inside soc_camera_g_fmt_vid_cap(), but rather passed via
> >>> >> soc_camera_device structure from a host driver callback invoked by
> >>> >> soc_camera_set_fmt().
> >>> >>
> >>> >> OMAP1 camera host driver has never been providing these parameters, so
> >>> >> it no longer works correctly. Fix it by adding suitable assignments to
> >>> >> omap1_cam_set_fmt().
> >>> >
> >>> > Thanks for the patch, but now it looks like many soc-camera host drivers
> >>> > are re-implementing this very same calculation in different parts of their
> >>> > code - in try_fmt, set_fmt, get_fmt. Why don't we unify them all,
> >>> > implement this centrally in soc_camera.c and remove all those
> >>> > calculations? Could you cook up a patch or maybe several patches - for
> >>> > soc_camera.c and all drivers?
> >>> >
> >>> > Thanks
> >>> > Guennadi
> >>> >
> >>> >>
> >>> >> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> >>> >> ---
> >>> >>  drivers/media/video/omap1_camera.c |    6 ++++++
> >>> >>  1 file changed, 6 insertions(+)
> >>> >>
> >>> >> --- linux-2.6.39-rc2/drivers/media/video/omap1_camera.c.orig  2011-04-06 14:30:37.000000000 +0200
> >>> >> +++ linux-2.6.39-rc2/drivers/media/video/omap1_camera.c       2011-04-09 00:16:36.000000000 +0200
> >>> >> @@ -1292,6 +1292,12 @@ static int omap1_cam_set_fmt(struct soc_
> >>> >>       pix->colorspace  = mf.colorspace;
> >>> >>       icd->current_fmt = xlate;
> >>> >>
> >>> >> +     pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
> >>> >> +                                                 xlate->host_fmt);
> >>> >> +     if (pix->bytesperline < 0)
> >>> >> +             return pix->bytesperline;
> >>> >> +     pix->sizeimage = pix->height * pix->bytesperline;
> >>> >> +
> >>> >>       return 0;
> >>> >>  }
> >>> >>
> >>> >>
> >>> >
> >>> > ---
> >>> > 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
> >>> >
> >>>
> >>
> >> ---
> >> 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
> >>
> >
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2.6.39] soc_camera: OMAP1: fix missing bytesperline and sizeimage initialization

[Kassey Lee]

Yes, thank you very much!

2011/4/12 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Tue, 12 Apr 2011, Kassey Lee wrote:
>
>> 2011/4/12 Kassey Lee <kassey1216@gmail.com>:
>> > Hi, Guennadi;
>> >           for sizeimage , I agree with you. that we can overwrite it
>> > after a frame is done.
>> >
>> >    for byteperline: on Marvell soc.
>> >    it needs to know the bytesperline before receive frame from sensor.
>> >    what we did now is hardcode  in host driver for bytesperline.
>> >
>> >    since different sensors have different timing for JPEG, and
>> > bytesperline is different.
>> >    while  soc_mbus_bytes_per_line does not support JPEG.
>> >
>> >    So, we want that host driver can get byteperline from sensor
>> > driver (sub dev) before transfer a frame for JPEG format.
>> >    a way to do this:
>> >    soc_mbus_bytes_per_line return 0 for JPEG, and host driver will
>> > try another API to get bytesperline for JPEG from sensor driver.
>> >     the effort is new API or reused other API.
>> >
>> >    Is that reasonable ?
>
> If you mean this your patch
>
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/31323/match=
>
> then I've queued it for 2.6.40.
>
> Thanks
> Guennadi
>
>> >
>> >
>> >
>> > 2011/4/12 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> >> Hi
>> >>
>> >> On Tue, 12 Apr 2011, Kassey Lee wrote:
>> >>
>> >>> hi, Guennadi:
>> >>>     a lot of sensors support JPEG output.
>> >>>     1) bytesperline is defined by sensor timing.
>> >>>     2) and sizeimage is unknow for jpeg.
>> >>>
>> >>>   how about for JPEG
>> >>>    1) host driver gets bytesperline from sensor driver.
>> >>>    2) sizeimage refilled by host driver after dma transfer done( a
>> >>> frame is received)
>> >>>   thanks.
>> >>
>> >> How is this done currently on other V4L2 drivers? To transfer a frame you
>> >> usually first do at least one of S_FMT and G_FMT, at which time you
>> >> already have to report sizeimage to the user - before any transfer has
>> >> taken place. Currently with soc-camera it is already possible to override
>> >> sizeimage and bytesperline from the host driver. Just set them to whatever
>> >> you need in your try_fmt and they will be kept. Not sure how you want to
>> >> do that, if you need to first read in a frame - do you want to perform
>> >> some dummy frame transfer? You might not even have any buffers queued yet,
>> >> so, it has to be a read without writing to RAM. Don't such compressed
>> >> formats just put a value in sizeimage, that is a calculated maximum size?
>> >>
>> >> Thanks
>> >> Guennadi
>> >>
>> >>> 2011/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> >>> > Hi Janusz
>> >>> >
>> >>> > On Sat, 9 Apr 2011, Janusz Krzysztofik wrote:
>> >>> >
>> >>> >> Since commit 0e4c180d3e2cc11e248f29d4c604b6194739d05a, bytesperline and
>> >>> >> sizeimage memebers of v4l2_pix_format structure have no longer been
>> >>> >> calculated inside soc_camera_g_fmt_vid_cap(), but rather passed via
>> >>> >> soc_camera_device structure from a host driver callback invoked by
>> >>> >> soc_camera_set_fmt().
>> >>> >>
>> >>> >> OMAP1 camera host driver has never been providing these parameters, so
>> >>> >> it no longer works correctly. Fix it by adding suitable assignments to
>> >>> >> omap1_cam_set_fmt().
>> >>> >
>> >>> > Thanks for the patch, but now it looks like many soc-camera host drivers
>> >>> > are re-implementing this very same calculation in different parts of their
>> >>> > code - in try_fmt, set_fmt, get_fmt. Why don't we unify them all,
>> >>> > implement this centrally in soc_camera.c and remove all those
>> >>> > calculations? Could you cook up a patch or maybe several patches - for
>> >>> > soc_camera.c and all drivers?
>> >>> >
>> >>> > Thanks
>> >>> > Guennadi
>> >>> >
>> >>> >>
>> >>> >> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
>> >>> >> ---
>> >>> >>  drivers/media/video/omap1_camera.c |    6 ++++++
>> >>> >>  1 file changed, 6 insertions(+)
>> >>> >>
>> >>> >> --- linux-2.6.39-rc2/drivers/media/video/omap1_camera.c.orig  2011-04-06 14:30:37.000000000 +0200
>> >>> >> +++ linux-2.6.39-rc2/drivers/media/video/omap1_camera.c       2011-04-09 00:16:36.000000000 +0200
>> >>> >> @@ -1292,6 +1292,12 @@ static int omap1_cam_set_fmt(struct soc_
>> >>> >>       pix->colorspace  = mf.colorspace;
>> >>> >>       icd->current_fmt = xlate;
>> >>> >>
>> >>> >> +     pix->bytesperline = soc_mbus_bytes_per_line(pix->width,
>> >>> >> +                                                 xlate->host_fmt);
>> >>> >> +     if (pix->bytesperline < 0)
>> >>> >> +             return pix->bytesperline;
>> >>> >> +     pix->sizeimage = pix->height * pix->bytesperline;
>> >>> >> +
>> >>> >>       return 0;
>> >>> >>  }
>> >>> >>
>> >>> >>
>> >>> >
>> >>> > ---
>> >>> > 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
>> >>> >
>> >>>
>> >>
>> >> ---
>> >> 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
>> >>
>> >
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>