diff mbox

v4l: soc-camera: Store negotiated buffer settings

Message ID 1299545388-717-1-git-send-email-saaguirre@ti.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Aguirre Rodriguez, Sergio Alberto March 8, 2011, 12:49 a.m. UTC 
  This fixes the problem in which a host driver
sets a personalized sizeimage or bytesperline field,
and gets ignored when doing G_FMT.

Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
 drivers/media/video/soc_camera.c |    9 ++++-----
 include/media/soc_camera.h       |    2 ++
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

Guennadi Liakhovetski March 8, 2011, 7:19 a.m. UTC | #1 
On Mon, 7 Mar 2011, Sergio Aguirre wrote:

> This fixes the problem in which a host driver
> sets a personalized sizeimage or bytesperline field,
> and gets ignored when doing G_FMT.

Can you tell what that personalised value is? Is it not covered by 
soc_mbus_bytes_per_line()? Maybe something like a JPEG format?

Thanks
Guennadi

> 
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> ---
>  drivers/media/video/soc_camera.c |    9 ++++-----
>  include/media/soc_camera.h       |    2 ++
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index a66811b..59dc71d 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
> @@ -363,6 +363,8 @@ static int soc_camera_set_fmt(struct soc_camera_device *icd,
>  	icd->user_width		= pix->width;
>  	icd->user_height	= pix->height;
>  	icd->colorspace		= pix->colorspace;
> +	icd->bytesperline	= pix->bytesperline;
> +	icd->sizeimage		= pix->sizeimage;
>  	icd->vb_vidq.field	=
>  		icd->field	= pix->field;
>  
> @@ -608,12 +610,9 @@ static int soc_camera_g_fmt_vid_cap(struct file *file, void *priv,
>  	pix->height		= icd->user_height;
>  	pix->field		= icd->vb_vidq.field;
>  	pix->pixelformat	= icd->current_fmt->host_fmt->fourcc;
> -	pix->bytesperline	= soc_mbus_bytes_per_line(pix->width,
> -						icd->current_fmt->host_fmt);
> +	pix->bytesperline	= icd->bytesperline;
>  	pix->colorspace		= icd->colorspace;
> -	if (pix->bytesperline < 0)
> -		return pix->bytesperline;
> -	pix->sizeimage		= pix->height * pix->bytesperline;
> +	pix->sizeimage		= icd->sizeimage;
>  	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
>  		icd->current_fmt->host_fmt->fourcc);
>  	return 0;
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 9386db8..de81370 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -30,6 +30,8 @@ struct soc_camera_device {
>  	s32 user_width;
>  	s32 user_height;
>  	enum v4l2_colorspace colorspace;
> +	__u32 bytesperline;	/* for padding, zero if unused */
> +	__u32 sizeimage;
>  	unsigned char iface;		/* Host number */
>  	unsigned char devnum;		/* Device number per host */
>  	struct soc_camera_sense *sense;	/* See comment in struct definition */
> -- 
> 1.7.1
> 

---
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
Aguirre Rodriguez, Sergio Alberto March 8, 2011, 1:07 p.m. UTC | #2 
Hi Guennadi,

On 03/08/2011 01:19 AM, Guennadi Liakhovetski wrote:
> On Mon, 7 Mar 2011, Sergio Aguirre wrote:
>
>> This fixes the problem in which a host driver
>> sets a personalized sizeimage or bytesperline field,
>> and gets ignored when doing G_FMT.
>
> Can you tell what that personalised value is? Is it not covered by
> soc_mbus_bytes_per_line()? Maybe something like a JPEG format?

In my case, my omap4_camera driver requires to have a bytesperline which 
is a multiple of 32, and sometimes (depending on the internal HW blocks 
used) a page aligned byte offset between lines.

For example, I want to use such configuration that, for an NV12 buffer, 
I require a 4K offset between lines, so the vaues are:

pix->bytesperline = PAGE_SIZE;
pix->sizeimage = pix->bytesperline * height * 3 / 2;

Which I filled in TRY_FMT/S_FMT ioctl calls.

So, next time a driver tries a G_FMT, it currently gets recalculated by
a prefixed table (which comes from soc_mbus_bytes_per_line), which won't 
give me what i had set before. And it will also recalculate a size image 
based on this wrong bytesperline * height, which is also wrong, (lacks 
the * 3 / 2 for NV12).

Regards,
Sergio

>
> Thanks
> Guennadi
>
>>
>> Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
>> ---
>>   drivers/media/video/soc_camera.c |    9 ++++-----
>>   include/media/soc_camera.h       |    2 ++
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
>> index a66811b..59dc71d 100644
>> --- a/drivers/media/video/soc_camera.c
>> +++ b/drivers/media/video/soc_camera.c
>> @@ -363,6 +363,8 @@ static int soc_camera_set_fmt(struct soc_camera_device *icd,
>>   	icd->user_width		= pix->width;
>>   	icd->user_height	= pix->height;
>>   	icd->colorspace		= pix->colorspace;
>> +	icd->bytesperline	= pix->bytesperline;
>> +	icd->sizeimage		= pix->sizeimage;
>>   	icd->vb_vidq.field	=
>>   		icd->field	= pix->field;
>>
>> @@ -608,12 +610,9 @@ static int soc_camera_g_fmt_vid_cap(struct file *file, void *priv,
>>   	pix->height		= icd->user_height;
>>   	pix->field		= icd->vb_vidq.field;
>>   	pix->pixelformat	= icd->current_fmt->host_fmt->fourcc;
>> -	pix->bytesperline	= soc_mbus_bytes_per_line(pix->width,
>> -						icd->current_fmt->host_fmt);
>> +	pix->bytesperline	= icd->bytesperline;
>>   	pix->colorspace		= icd->colorspace;
>> -	if (pix->bytesperline<  0)
>> -		return pix->bytesperline;
>> -	pix->sizeimage		= pix->height * pix->bytesperline;
>> +	pix->sizeimage		= icd->sizeimage;
>>   	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
>>   		icd->current_fmt->host_fmt->fourcc);
>>   	return 0;
>> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
>> index 9386db8..de81370 100644
>> --- a/include/media/soc_camera.h
>> +++ b/include/media/soc_camera.h
>> @@ -30,6 +30,8 @@ struct soc_camera_device {
>>   	s32 user_width;
>>   	s32 user_height;
>>   	enum v4l2_colorspace colorspace;
>> +	__u32 bytesperline;	/* for padding, zero if unused */
>> +	__u32 sizeimage;
>>   	unsigned char iface;		/* Host number */
>>   	unsigned char devnum;		/* Device number per host */
>>   	struct soc_camera_sense *sense;	/* See comment in struct definition */
>> --
>> 1.7.1
>>
>
> ---
> 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 March 11, 2011, 4:36 p.m. UTC | #3 
On Tue, 8 Mar 2011, Sergio Aguirre wrote:

> Hi Guennadi,
> 
> On 03/08/2011 01:19 AM, Guennadi Liakhovetski wrote:
> > On Mon, 7 Mar 2011, Sergio Aguirre wrote:
> > 
> > > This fixes the problem in which a host driver
> > > sets a personalized sizeimage or bytesperline field,
> > > and gets ignored when doing G_FMT.
> > 
> > Can you tell what that personalised value is? Is it not covered by
> > soc_mbus_bytes_per_line()? Maybe something like a JPEG format?
> 
> In my case, my omap4_camera driver requires to have a bytesperline which is a
> multiple of 32, and sometimes (depending on the internal HW blocks used) a
> page aligned byte offset between lines.
> 
> For example, I want to use such configuration that, for an NV12 buffer, I
> require a 4K offset between lines, so the vaues are:
> 
> pix->bytesperline = PAGE_SIZE;
> pix->sizeimage = pix->bytesperline * height * 3 / 2;
> 
> Which I filled in TRY_FMT/S_FMT ioctl calls.

Ok, I think, I agree with this. Until now we didn't have drivers, that 
wanted to pad data. Even the pxa270 driver adjusts the frame format (see 
pxa_camera.c::pxa_camera_try_fmt() and the call to v4l_bound_align_image() 
there in the YUV422P case) to avoid padding, even though that's a 
different kind of padding - between planes. So, if line padding - as in 
your case - is indeed needed, I agree, that the correct way to support 
that is to implement driver-specific bytesperline and sizeimage 
calculations and, logically, those values should be used in 
soc_camera_g_fmt_vid_cap().

I'll just change your patch a bit - I'll use "u32" types instead of 
"__u32" - this is a kernel internal struct and we don't need user-space 
exported types.

Thanks
Guennadi

> So, next time a driver tries a G_FMT, it currently gets recalculated by
> a prefixed table (which comes from soc_mbus_bytes_per_line), which won't give
> me what i had set before. And it will also recalculate a size image based on
> this wrong bytesperline * height, which is also wrong, (lacks the * 3 / 2 for
> NV12).
> 
> Regards,
> Sergio
> 
> > 
> > Thanks
> > Guennadi
> > 
> > > 
> > > Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
> > > ---
> > >   drivers/media/video/soc_camera.c |    9 ++++-----
> > >   include/media/soc_camera.h       |    2 ++
> > >   2 files changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/soc_camera.c
> > > b/drivers/media/video/soc_camera.c
> > > index a66811b..59dc71d 100644
> > > --- a/drivers/media/video/soc_camera.c
> > > +++ b/drivers/media/video/soc_camera.c
> > > @@ -363,6 +363,8 @@ static int soc_camera_set_fmt(struct soc_camera_device
> > > *icd,
> > >   	icd->user_width		= pix->width;
> > >   	icd->user_height	= pix->height;
> > >   	icd->colorspace		= pix->colorspace;
> > > +	icd->bytesperline	= pix->bytesperline;
> > > +	icd->sizeimage		= pix->sizeimage;
> > >   	icd->vb_vidq.field	=
> > >   		icd->field	= pix->field;
> > > 
> > > @@ -608,12 +610,9 @@ static int soc_camera_g_fmt_vid_cap(struct file
> > > *file, void *priv,
> > >   	pix->height		= icd->user_height;
> > >   	pix->field		= icd->vb_vidq.field;
> > >   	pix->pixelformat	= icd->current_fmt->host_fmt->fourcc;
> > > -	pix->bytesperline	= soc_mbus_bytes_per_line(pix->width,
> > > -						icd->current_fmt->host_fmt);
> > > +	pix->bytesperline	= icd->bytesperline;
> > >   	pix->colorspace		= icd->colorspace;
> > > -	if (pix->bytesperline<  0)
> > > -		return pix->bytesperline;
> > > -	pix->sizeimage		= pix->height * pix->bytesperline;
> > > +	pix->sizeimage		= icd->sizeimage;
> > >   	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
> > >   		icd->current_fmt->host_fmt->fourcc);
> > >   	return 0;
> > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > > index 9386db8..de81370 100644
> > > --- a/include/media/soc_camera.h
> > > +++ b/include/media/soc_camera.h
> > > @@ -30,6 +30,8 @@ struct soc_camera_device {
> > >   	s32 user_width;
> > >   	s32 user_height;
> > >   	enum v4l2_colorspace colorspace;
> > > +	__u32 bytesperline;	/* for padding, zero if unused */
> > > +	__u32 sizeimage;
> > >   	unsigned char iface;		/* Host number */
> > >   	unsigned char devnum;		/* Device number per host */
> > >   	struct soc_camera_sense *sense;	/* See comment in struct definition */
> > > --
> > > 1.7.1

---
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
Aguirre Rodriguez, Sergio Alberto March 11, 2011, 4:40 p.m. UTC | #4 
Hi Guennadi,

On 03/11/2011 10:36 AM, Guennadi Liakhovetski wrote:
> On Tue, 8 Mar 2011, Sergio Aguirre wrote:
>
>> Hi Guennadi,
>>
>> On 03/08/2011 01:19 AM, Guennadi Liakhovetski wrote:
>>> On Mon, 7 Mar 2011, Sergio Aguirre wrote:
>>>
>>>> This fixes the problem in which a host driver
>>>> sets a personalized sizeimage or bytesperline field,
>>>> and gets ignored when doing G_FMT.
>>>
>>> Can you tell what that personalised value is? Is it not covered by
>>> soc_mbus_bytes_per_line()? Maybe something like a JPEG format?
>>
>> In my case, my omap4_camera driver requires to have a bytesperline which is a
>> multiple of 32, and sometimes (depending on the internal HW blocks used) a
>> page aligned byte offset between lines.
>>
>> For example, I want to use such configuration that, for an NV12 buffer, I
>> require a 4K offset between lines, so the vaues are:
>>
>> pix->bytesperline = PAGE_SIZE;
>> pix->sizeimage = pix->bytesperline * height * 3 / 2;
>>
>> Which I filled in TRY_FMT/S_FMT ioctl calls.
>
> Ok, I think, I agree with this. Until now we didn't have drivers, that
> wanted to pad data. Even the pxa270 driver adjusts the frame format (see
> pxa_camera.c::pxa_camera_try_fmt() and the call to v4l_bound_align_image()
> there in the YUV422P case) to avoid padding, even though that's a
> different kind of padding - between planes. So, if line padding - as in
> your case - is indeed needed, I agree, that the correct way to support
> that is to implement driver-specific bytesperline and sizeimage
> calculations and, logically, those values should be used in
> soc_camera_g_fmt_vid_cap().
>
> I'll just change your patch a bit - I'll use "u32" types instead of
> "__u32" - this is a kernel internal struct and we don't need user-space
> exported types.

Ok, thanks.

You're right about u32 type... my bad.

So, I'll change that, rebase to:

git://linuxtv.org/media_tree.git	staging/for_v2.6.39

And resubmit for review. No problem.

Regards,
Sergio

>
> Thanks
> Guennadi
>
>> So, next time a driver tries a G_FMT, it currently gets recalculated by
>> a prefixed table (which comes from soc_mbus_bytes_per_line), which won't give
>> me what i had set before. And it will also recalculate a size image based on
>> this wrong bytesperline * height, which is also wrong, (lacks the * 3 / 2 for
>> NV12).
>>
>> Regards,
>> Sergio
>>
>>>
>>> Thanks
>>> Guennadi
>>>
>>>>
>>>> Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
>>>> ---
>>>>    drivers/media/video/soc_camera.c |    9 ++++-----
>>>>    include/media/soc_camera.h       |    2 ++
>>>>    2 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/soc_camera.c
>>>> b/drivers/media/video/soc_camera.c
>>>> index a66811b..59dc71d 100644
>>>> --- a/drivers/media/video/soc_camera.c
>>>> +++ b/drivers/media/video/soc_camera.c
>>>> @@ -363,6 +363,8 @@ static int soc_camera_set_fmt(struct soc_camera_device
>>>> *icd,
>>>>    	icd->user_width		= pix->width;
>>>>    	icd->user_height	= pix->height;
>>>>    	icd->colorspace		= pix->colorspace;
>>>> +	icd->bytesperline	= pix->bytesperline;
>>>> +	icd->sizeimage		= pix->sizeimage;
>>>>    	icd->vb_vidq.field	=
>>>>    		icd->field	= pix->field;
>>>>
>>>> @@ -608,12 +610,9 @@ static int soc_camera_g_fmt_vid_cap(struct file
>>>> *file, void *priv,
>>>>    	pix->height		= icd->user_height;
>>>>    	pix->field		= icd->vb_vidq.field;
>>>>    	pix->pixelformat	= icd->current_fmt->host_fmt->fourcc;
>>>> -	pix->bytesperline	= soc_mbus_bytes_per_line(pix->width,
>>>> -						icd->current_fmt->host_fmt);
>>>> +	pix->bytesperline	= icd->bytesperline;
>>>>    	pix->colorspace		= icd->colorspace;
>>>> -	if (pix->bytesperline<   0)
>>>> -		return pix->bytesperline;
>>>> -	pix->sizeimage		= pix->height * pix->bytesperline;
>>>> +	pix->sizeimage		= icd->sizeimage;
>>>>    	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
>>>>    		icd->current_fmt->host_fmt->fourcc);
>>>>    	return 0;
>>>> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
>>>> index 9386db8..de81370 100644
>>>> --- a/include/media/soc_camera.h
>>>> +++ b/include/media/soc_camera.h
>>>> @@ -30,6 +30,8 @@ struct soc_camera_device {
>>>>    	s32 user_width;
>>>>    	s32 user_height;
>>>>    	enum v4l2_colorspace colorspace;
>>>> +	__u32 bytesperline;	/* for padding, zero if unused */
>>>> +	__u32 sizeimage;
>>>>    	unsigned char iface;		/* Host number */
>>>>    	unsigned char devnum;		/* Device number per host */
>>>>    	struct soc_camera_sense *sense;	/* See comment in struct definition */
>>>> --
>>>> 1.7.1
>
> ---
> 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 March 11, 2011, 4:48 p.m. UTC | #5 
On Fri, 11 Mar 2011, Sergio Aguirre wrote:

> Hi Guennadi,
> 
> On 03/11/2011 10:36 AM, Guennadi Liakhovetski wrote:
> > On Tue, 8 Mar 2011, Sergio Aguirre wrote:
> > 
> > > Hi Guennadi,
> > > 
> > > On 03/08/2011 01:19 AM, Guennadi Liakhovetski wrote:
> > > > On Mon, 7 Mar 2011, Sergio Aguirre wrote:
> > > > 
> > > > > This fixes the problem in which a host driver
> > > > > sets a personalized sizeimage or bytesperline field,
> > > > > and gets ignored when doing G_FMT.
> > > > 
> > > > Can you tell what that personalised value is? Is it not covered by
> > > > soc_mbus_bytes_per_line()? Maybe something like a JPEG format?
> > > 
> > > In my case, my omap4_camera driver requires to have a bytesperline which
> > > is a
> > > multiple of 32, and sometimes (depending on the internal HW blocks used) a
> > > page aligned byte offset between lines.
> > > 
> > > For example, I want to use such configuration that, for an NV12 buffer, I
> > > require a 4K offset between lines, so the vaues are:
> > > 
> > > pix->bytesperline = PAGE_SIZE;
> > > pix->sizeimage = pix->bytesperline * height * 3 / 2;
> > > 
> > > Which I filled in TRY_FMT/S_FMT ioctl calls.
> > 
> > Ok, I think, I agree with this. Until now we didn't have drivers, that
> > wanted to pad data. Even the pxa270 driver adjusts the frame format (see
> > pxa_camera.c::pxa_camera_try_fmt() and the call to v4l_bound_align_image()
> > there in the YUV422P case) to avoid padding, even though that's a
> > different kind of padding - between planes. So, if line padding - as in
> > your case - is indeed needed, I agree, that the correct way to support
> > that is to implement driver-specific bytesperline and sizeimage
> > calculations and, logically, those values should be used in
> > soc_camera_g_fmt_vid_cap().
> > 
> > I'll just change your patch a bit - I'll use "u32" types instead of
> > "__u32" - this is a kernel internal struct and we don't need user-space
> > exported types.
> 
> Ok, thanks.
> 
> You're right about u32 type... my bad.
> 
> So, I'll change that, rebase to:
> 
> git://linuxtv.org/media_tree.git	staging/for_v2.6.39
> 
> And resubmit for review. No problem.

Np, I've already committed (locally, not pushed yet) this patch with only 
changed field types and a bit shuffled lines to group size-related and 
format-related assignments together.

Thanks
Guennadi

> 
> Regards,
> Sergio
> 
> > 
> > Thanks
> > Guennadi
> > 
> > > So, next time a driver tries a G_FMT, it currently gets recalculated by
> > > a prefixed table (which comes from soc_mbus_bytes_per_line), which won't
> > > give
> > > me what i had set before. And it will also recalculate a size image based
> > > on
> > > this wrong bytesperline * height, which is also wrong, (lacks the * 3 / 2
> > > for
> > > NV12).
> > > 
> > > Regards,
> > > Sergio
> > > 
> > > > 
> > > > Thanks
> > > > Guennadi
> > > > 
> > > > > 
> > > > > Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
> > > > > ---
> > > > >    drivers/media/video/soc_camera.c |    9 ++++-----
> > > > >    include/media/soc_camera.h       |    2 ++
> > > > >    2 files changed, 6 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/video/soc_camera.c
> > > > > b/drivers/media/video/soc_camera.c
> > > > > index a66811b..59dc71d 100644
> > > > > --- a/drivers/media/video/soc_camera.c
> > > > > +++ b/drivers/media/video/soc_camera.c
> > > > > @@ -363,6 +363,8 @@ static int soc_camera_set_fmt(struct
> > > > > soc_camera_device
> > > > > *icd,
> > > > >    	icd->user_width		= pix->width;
> > > > >    	icd->user_height	= pix->height;
> > > > >    	icd->colorspace		= pix->colorspace;
> > > > > +	icd->bytesperline	= pix->bytesperline;
> > > > > +	icd->sizeimage		= pix->sizeimage;
> > > > >    	icd->vb_vidq.field	=
> > > > >    		icd->field	= pix->field;
> > > > > 
> > > > > @@ -608,12 +610,9 @@ static int soc_camera_g_fmt_vid_cap(struct file
> > > > > *file, void *priv,
> > > > >    	pix->height		= icd->user_height;
> > > > >    	pix->field		= icd->vb_vidq.field;
> > > > >    	pix->pixelformat	= icd->current_fmt->host_fmt->fourcc;
> > > > > -	pix->bytesperline	= soc_mbus_bytes_per_line(pix->width,
> > > > > -
> > > > > icd->current_fmt->host_fmt);
> > > > > +	pix->bytesperline	= icd->bytesperline;
> > > > >    	pix->colorspace		= icd->colorspace;
> > > > > -	if (pix->bytesperline<   0)
> > > > > -		return pix->bytesperline;
> > > > > -	pix->sizeimage		= pix->height * pix->bytesperline;
> > > > > +	pix->sizeimage		= icd->sizeimage;
> > > > >    	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
> > > > >    		icd->current_fmt->host_fmt->fourcc);
> > > > >    	return 0;
> > > > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > > > > index 9386db8..de81370 100644
> > > > > --- a/include/media/soc_camera.h
> > > > > +++ b/include/media/soc_camera.h
> > > > > @@ -30,6 +30,8 @@ struct soc_camera_device {
> > > > >    	s32 user_width;
> > > > >    	s32 user_height;
> > > > >    	enum v4l2_colorspace colorspace;
> > > > > +	__u32 bytesperline;	/* for padding, zero if unused */
> > > > > +	__u32 sizeimage;
> > > > >    	unsigned char iface;		/* Host number */
> > > > >    	unsigned char devnum;		/* Device number per host */
> > > > >    	struct soc_camera_sense *sense;	/* See comment in struct
> > > > > definition */
> > > > > --
> > > > > 1.7.1
> > 
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> 

---
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
Aguirre Rodriguez, Sergio Alberto March 11, 2011, 4:50 p.m. UTC | #6 
On 03/11/2011 10:48 AM, Guennadi Liakhovetski wrote:
> On Fri, 11 Mar 2011, Sergio Aguirre wrote:
>
>> Hi Guennadi,
>>
>> On 03/11/2011 10:36 AM, Guennadi Liakhovetski wrote:
>>> On Tue, 8 Mar 2011, Sergio Aguirre wrote:
>>>
>>>> Hi Guennadi,
>>>>
>>>> On 03/08/2011 01:19 AM, Guennadi Liakhovetski wrote:
>>>>> On Mon, 7 Mar 2011, Sergio Aguirre wrote:
>>>>>
>>>>>> This fixes the problem in which a host driver
>>>>>> sets a personalized sizeimage or bytesperline field,
>>>>>> and gets ignored when doing G_FMT.
>>>>>
>>>>> Can you tell what that personalised value is? Is it not covered by
>>>>> soc_mbus_bytes_per_line()? Maybe something like a JPEG format?
>>>>
>>>> In my case, my omap4_camera driver requires to have a bytesperline which
>>>> is a
>>>> multiple of 32, and sometimes (depending on the internal HW blocks used) a
>>>> page aligned byte offset between lines.
>>>>
>>>> For example, I want to use such configuration that, for an NV12 buffer, I
>>>> require a 4K offset between lines, so the vaues are:
>>>>
>>>> pix->bytesperline = PAGE_SIZE;
>>>> pix->sizeimage = pix->bytesperline * height * 3 / 2;
>>>>
>>>> Which I filled in TRY_FMT/S_FMT ioctl calls.
>>>
>>> Ok, I think, I agree with this. Until now we didn't have drivers, that
>>> wanted to pad data. Even the pxa270 driver adjusts the frame format (see
>>> pxa_camera.c::pxa_camera_try_fmt() and the call to v4l_bound_align_image()
>>> there in the YUV422P case) to avoid padding, even though that's a
>>> different kind of padding - between planes. So, if line padding - as in
>>> your case - is indeed needed, I agree, that the correct way to support
>>> that is to implement driver-specific bytesperline and sizeimage
>>> calculations and, logically, those values should be used in
>>> soc_camera_g_fmt_vid_cap().
>>>
>>> I'll just change your patch a bit - I'll use "u32" types instead of
>>> "__u32" - this is a kernel internal struct and we don't need user-space
>>> exported types.
>>
>> Ok, thanks.
>>
>> You're right about u32 type... my bad.
>>
>> So, I'll change that, rebase to:
>>
>> git://linuxtv.org/media_tree.git	staging/for_v2.6.39
>>
>> And resubmit for review. No problem.
>
> Np, I've already committed (locally, not pushed yet) this patch with only
> changed field types and a bit shuffled lines to group size-related and
> format-related assignments together.

Oh ok. That's good! Thanks for doing that. :)

Thanks,
Sergio


>
> Thanks
> Guennadi
>

<snip>

>
> ---
> 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
diff mbox

Patch

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index a66811b..59dc71d 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -363,6 +363,8 @@  static int soc_camera_set_fmt(struct soc_camera_device *icd,
 	icd->user_width		= pix->width;
 	icd->user_height	= pix->height;
 	icd->colorspace		= pix->colorspace;
+	icd->bytesperline	= pix->bytesperline;
+	icd->sizeimage		= pix->sizeimage;
 	icd->vb_vidq.field	=
 		icd->field	= pix->field;
 
@@ -608,12 +610,9 @@  static int soc_camera_g_fmt_vid_cap(struct file *file, void *priv,
 	pix->height		= icd->user_height;
 	pix->field		= icd->vb_vidq.field;
 	pix->pixelformat	= icd->current_fmt->host_fmt->fourcc;
-	pix->bytesperline	= soc_mbus_bytes_per_line(pix->width,
-						icd->current_fmt->host_fmt);
+	pix->bytesperline	= icd->bytesperline;
 	pix->colorspace		= icd->colorspace;
-	if (pix->bytesperline < 0)
-		return pix->bytesperline;
-	pix->sizeimage		= pix->height * pix->bytesperline;
+	pix->sizeimage		= icd->sizeimage;
 	dev_dbg(&icd->dev, "current_fmt->fourcc: 0x%08x\n",
 		icd->current_fmt->host_fmt->fourcc);
 	return 0;
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 9386db8..de81370 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -30,6 +30,8 @@  struct soc_camera_device {
 	s32 user_width;
 	s32 user_height;
 	enum v4l2_colorspace colorspace;
+	__u32 bytesperline;	/* for padding, zero if unused */
+	__u32 sizeimage;
 	unsigned char iface;		/* Host number */
 	unsigned char devnum;		/* Device number per host */
 	struct soc_camera_sense *sense;	/* See comment in struct definition */