ov772x: add support S_CROP operation.

  ov772x_set_fmt had returned NULL when pixfmt is 0,
although it mean only geometry change.
This patch modify this problem.

Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
---
 drivers/media/video/ov772x.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)
  

On Fri, 23 Jan 2009, Kuninori Morimoto wrote:

> ov772x_set_fmt had returned NULL when pixfmt is 0,
> although it mean only geometry change.
> This patch modify this problem.
> 
> Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
> ---
>  drivers/media/video/ov772x.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
> index 681a11b..30eb80e 100644
> --- a/drivers/media/video/ov772x.c
> +++ b/drivers/media/video/ov772x.c
> @@ -792,12 +792,15 @@ static int ov772x_set_fmt(struct soc_camera_device *icd,
>  
>  	/*
>  	 * select format
> +	 * when pixfmt is 0, only geometry change
>  	 */
> -	priv->fmt = NULL;
> -	for (i = 0; i < ARRAY_SIZE(ov772x_cfmts); i++) {
> -		if (pixfmt == ov772x_cfmts[i].fourcc) {
> -			priv->fmt = ov772x_cfmts + i;
> -			break;
> +	if (pixfmt) {
> +		priv->fmt = NULL;
> +		for (i = 0; i < ARRAY_SIZE(ov772x_cfmts); i++) {
> +			if (pixfmt == ov772x_cfmts[i].fourcc) {
> +				priv->fmt = ov772x_cfmts + i;
> +				break;
> +			}
>  		}
>  	}
>  	if (!priv->fmt)

Have you tested with v4l-dvb/v4l2-apps/test/capture_example.c? I think it 
wouldn't work, because it first calls S_CROP, and then S_FMT, and even 
with this your patch you'd fail S_CROP if S_FMT hadn't been called before 
(priv->fmt == NULL). Am I right? 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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
  

Dear Guennadi

> Have you tested with v4l-dvb/v4l2-apps/test/capture_example.c? I think it 
> wouldn't work, because it first calls S_CROP, and then S_FMT, and even 
> with this your patch you'd fail S_CROP if S_FMT hadn't been called before 
> (priv->fmt == NULL). Am I right? 

hmm.. I would like to ask you about S_CROP on soc_camera.
There are a lot of problem for me.

at first, sh_mobile_ceu (and pxa_camera too) :: set_fmt is this
-----------------------------------------------
	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
	if (!xlate) {
		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
		return -EINVAL;
	}
-----------------------------------------------

and soc_camera_xlate_by_fourcc is this
-----------------------------------------------
const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
	struct soc_camera_device *icd, unsigned int fourcc)
{
	unsigned int i;

	for (i = 0; i < icd->num_user_formats; i++)
		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
			return icd->user_formats + i;
	return NULL;
}
-----------------------------------------------

If pixfmt is 0, it said "Format 0 not found" to me.

even if I fixed this problem (and ov772x set_fmt),
kernel will run BUG_ON.

call trace is this.
-------------
soc_camera_qbuf
 -> videobuf_qbuf
    -> videobuf_next_field
-------------

kernel will run BUG_ON when I use capture_example.
-------------------------------------------------
/* Locking: Caller holds q->vb_lock */
enum v4l2_field videobuf_next_field(struct videobuf_queue *q)
{
	enum v4l2_field field = q->field;

=>	BUG_ON(V4L2_FIELD_ANY == field);
-------------------------------------------------

If sh_mobile_ceu use V4L2_FIELD_NONE on sh_mobile_ceu_init_videobuf,
kernel will not run this BUG_ON.
But tw9910 and I want sh_mobile_ceu_init_videobuf to use V4L2_FIELD_ANY.
Do you remember this problem ?

And capture_example doesn't works even If
sh_mobile_ceu_init_videobuf use V4L2_FIELD_NONE.
It said "select timeout" to me.
I don't know why.

what is the best way to us ???
or do I miss understanding ???

> it first calls S_CROP, and then S_FMT

In this case, shoud ov772x use defaul color format
when S_CROP order ?

Best regards
--
Kuninori Morimoto
 
--
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, 26 Jan 2009, morimoto.kuninori@renesas.com wrote:

> 
> Dear Guennadi
> 
> > Have you tested with v4l-dvb/v4l2-apps/test/capture_example.c? I think it 
> > wouldn't work, because it first calls S_CROP, and then S_FMT, and even 
> > with this your patch you'd fail S_CROP if S_FMT hadn't been called before 
> > (priv->fmt == NULL). Am I right? 
> 
> hmm.. I would like to ask you about S_CROP on soc_camera.
> There are a lot of problem for me.
> 
> at first, sh_mobile_ceu (and pxa_camera too) :: set_fmt is this
> -----------------------------------------------
> 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> 	if (!xlate) {
> 		dev_warn(&ici->dev, "Format %x not found\n", pixfmt);
> 		return -EINVAL;
> 	}
> -----------------------------------------------
> 
> and soc_camera_xlate_by_fourcc is this
> -----------------------------------------------
> const struct soc_camera_format_xlate *soc_camera_xlate_by_fourcc(
> 	struct soc_camera_device *icd, unsigned int fourcc)
> {
> 	unsigned int i;
> 
> 	for (i = 0; i < icd->num_user_formats; i++)
> 		if (icd->user_formats[i].host_fmt->fourcc == fourcc)
> 			return icd->user_formats + i;
> 	return NULL;
> }
> -----------------------------------------------
> 
> If pixfmt is 0, it said "Format 0 not found" to me.

Yes, this will be fixed by this:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg00161.html

> even if I fixed this problem (and ov772x set_fmt),
> kernel will run BUG_ON.
> 
> call trace is this.
> -------------
> soc_camera_qbuf
>  -> videobuf_qbuf
>     -> videobuf_next_field
> -------------
> 
> kernel will run BUG_ON when I use capture_example.
> -------------------------------------------------
> /* Locking: Caller holds q->vb_lock */
> enum v4l2_field videobuf_next_field(struct videobuf_queue *q)
> {
> 	enum v4l2_field field = q->field;
> 
> =>	BUG_ON(V4L2_FIELD_ANY == field);
> -------------------------------------------------
> 
> If sh_mobile_ceu use V4L2_FIELD_NONE on sh_mobile_ceu_init_videobuf,
> kernel will not run this BUG_ON.
> But tw9910 and I want sh_mobile_ceu_init_videobuf to use V4L2_FIELD_ANY.
> Do you remember this problem ?

Well, I looked through the old emails, but chenged a few things since 
then, so I am not sure what the behaviour would be like now, if we do 
switch back to _NONE? It looks like you should be prepared to run without 
a call to S_FMT, which also means, your ->field should have a value other 
than _ANY. What would break if you use _ANY now?

> And capture_example doesn't works even If
> sh_mobile_ceu_init_videobuf use V4L2_FIELD_NONE.
> It said "select timeout" to me.
> I don't know why.

This is probably because you don't setup a default mode in 
sh_mobile_ceu_camera.c or ov772x.c if S_FMT is never called. This should 
be fixed. I'm not sure if other drivers would deliver anything meaningful 
without a call to S_FMT - has to be tested / fixed...

> what is the best way to us ???
> or do I miss understanding ???

Fix behaviour if no S_FMT is done.

> > it first calls S_CROP, and then S_FMT
> 
> In this case, shoud ov772x use defaul color format
> when S_CROP order ?

The thing is that older versions of capture.c always called S_FMT, newer 
ones don't unless forced per "-f" switch.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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
  

Dear Guennadi

> > If pixfmt is 0, it said "Format 0 not found" to me.
> 
> Yes, this will be fixed by this:
> 
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg00161.html

wow !!
I will use this. thank you.

> > If sh_mobile_ceu use V4L2_FIELD_NONE on sh_mobile_ceu_init_videobuf,
> > kernel will not run this BUG_ON.
> > But tw9910 and I want sh_mobile_ceu_init_videobuf to use V4L2_FIELD_ANY.
> > Do you remember this problem ?
> 
> Well, I looked through the old emails, but chenged a few things since 
> then, so I am not sure what the behaviour would be like now, if we do 
> switch back to _NONE? It looks like you should be prepared to run without 
> a call to S_FMT, which also means, your ->field should have a value other 
> than _ANY. What would break if you use _ANY now?

hmm. difficult English for me...

if sh_mobile_ceu use ANY
  -> ov772x can not use capture_example.
     -> run BUG_ON

if sh_mobile_ceu use NONE
  -> tw9910 can not works.
     -> try_fmt needs ANY or INTERLACED.

if sh_mobile_ceu use INTERLACED,
ov772x and tw9910 are works well now.
but this is niche fix and I don't know this is correct way or not.

if sh_mobile_ceu use NONE,
tw9910 needs some modify on try_fmt.

> > And capture_example doesn't works even If
> > sh_mobile_ceu_init_videobuf use V4L2_FIELD_NONE.
> > It said "select timeout" to me.
> > I don't know why.
> 
> This is probably because you don't setup a default mode in 
> sh_mobile_ceu_camera.c or ov772x.c if S_FMT is never called. This should 
> be fixed. I'm not sure if other drivers would deliver anything meaningful 
> without a call to S_FMT - has to be tested / fixed...
(snip)
> > In this case, shoud ov772x use defaul color format
> > when S_CROP order ?
> 
> The thing is that older versions of capture.c always called S_FMT, newer 
> ones don't unless forced per "-f" switch.

Yes. Now, sh_mobile_ceu :: set_bus_param must be called to use camera.
We need -f option. Thank you.


Best regards
--
Kuninori Morimoto
 
--
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
  

Dear Guennadi

> > This is probably because you don't setup a default mode in 
> > sh_mobile_ceu_camera.c or ov772x.c if S_FMT is never called. This should 
> > be fixed. I'm not sure if other drivers would deliver anything meaningful 
> > without a call to S_FMT - has to be tested / fixed...
> (snip)
> > > In this case, shoud ov772x use defaul color format
> > > when S_CROP order ?
> > 
> > The thing is that older versions of capture.c always called S_FMT, newer 
> > ones don't unless forced per "-f" switch.
> 
> Yes. Now, sh_mobile_ceu :: set_bus_param must be called to use camera.
> We need -f option. Thank you.

sorry. wrong understanding.
ov772x and tw9910 should works even if -f is not used.
is this correct ?
--
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, 26 Jan 2009, morimoto.kuninori@renesas.com wrote:

> 
> Dear Guennadi
> 
> > > This is probably because you don't setup a default mode in 
> > > sh_mobile_ceu_camera.c or ov772x.c if S_FMT is never called. This should 
> > > be fixed. I'm not sure if other drivers would deliver anything meaningful 
> > > without a call to S_FMT - has to be tested / fixed...
> > (snip)
> > > > In this case, shoud ov772x use defaul color format
> > > > when S_CROP order ?
> > > 
> > > The thing is that older versions of capture.c always called S_FMT, newer 
> > > ones don't unless forced per "-f" switch.
> > 
> > Yes. Now, sh_mobile_ceu :: set_bus_param must be called to use camera.
> > We need -f option. Thank you.
> 
> sorry. wrong understanding.
> ov772x and tw9910 should works even if -f is not used.
> is this correct ?

Yes.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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
  

Dear Guennadi

> > what is the best way to us ???
> > or do I miss understanding ???
> 
> Fix behaviour if no S_FMT is done.

I attached stupid 4 patches.
I would like to hear your opinion.
please check it.

I wonder is there any soc_camera that works without 
calling S_FMT though set_bus_param is not called ?

If soc_camera works without calling S_FMT, 
s_crop should call try_fmt_vid_cap
and set_bus_param like s_fmt_vid_cap I think.

And I think "current_fmt" is better than 0 to set_fmt
if user wants only geometry changes on s_crop.
it mean keep format.

These patches works well on my local environment.
ov772x and tw9910 work even if without -f option on capture_example.

If you can agree with this idea,
I will send these as formal patch.
Best regards
--
Kuninori Morimoto
  

On Tue, 27 Jan 2009, morimoto.kuninori@renesas.com wrote:

> Dear Guennadi
> 
> > > what is the best way to us ???
> > > or do I miss understanding ???
> > 
> > Fix behaviour if no S_FMT is done.
> 
> I attached stupid 4 patches.
> I would like to hear your opinion.
> please check it.
> 
> I wonder is there any soc_camera that works without 
> calling S_FMT though set_bus_param is not called ?

Don't know, never tested that way. Might well be they don't, in which case 
they need to be fixed.

> If soc_camera works without calling S_FMT, 
> s_crop should call try_fmt_vid_cap
> and set_bus_param like s_fmt_vid_cap I think.
> 
> And I think "current_fmt" is better than 0 to set_fmt
> if user wants only geometry changes on s_crop.
> it mean keep format.
> 
> These patches works well on my local environment.
> ov772x and tw9910 work even if without -f option on capture_example.
> 
> If you can agree with this idea,
> I will send these as formal patch.

Thanks for the patches, please, give me a couple of days for review.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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 Tue, 27 Jan 2009 08:53:23 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

Hi Guennadi,

I'm understanding that you're reviewing this patch and other ones for
soc_camera and will send me a PULL request after reviewing those stuff. I've
updated patchwork to reflect this.

Thanks,
Mauro

> On Tue, 27 Jan 2009, morimoto.kuninori@renesas.com wrote:
> 
> > Dear Guennadi
> > 
> > > > what is the best way to us ???
> > > > or do I miss understanding ???
> > > 
> > > Fix behaviour if no S_FMT is done.
> > 
> > I attached stupid 4 patches.
> > I would like to hear your opinion.
> > please check it.
> > 
> > I wonder is there any soc_camera that works without 
> > calling S_FMT though set_bus_param is not called ?
> 
> Don't know, never tested that way. Might well be they don't, in which case 
> they need to be fixed.
> 
> > If soc_camera works without calling S_FMT, 
> > s_crop should call try_fmt_vid_cap
> > and set_bus_param like s_fmt_vid_cap I think.
> > 
> > And I think "current_fmt" is better than 0 to set_fmt
> > if user wants only geometry changes on s_crop.
> > it mean keep format.
> > 
> > These patches works well on my local environment.
> > ov772x and tw9910 work even if without -f option on capture_example.
> > 
> > If you can agree with this idea,
> > I will send these as formal patch.
> 
> Thanks for the patches, please, give me a couple of days for review.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> --
> 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




Cheers,
Mauro
--
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 Thu, 29 Jan 2009, Mauro Carvalho Chehab wrote:

> On Tue, 27 Jan 2009 08:53:23 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> Hi Guennadi,
> 
> I'm understanding that you're reviewing this patch and other ones for
> soc_camera and will send me a PULL request after reviewing those stuff.

Yes, I'm (going to be) reviewing them, as soon as I find some time. Then 
I'll send you two pull requests - fixes for 2.6.29 and 2.6.30 material. 
AFAIK, unfortunately, mercurial doesn't support branches, so, I probably 
will end up first sending you a pull request with fixes, and after some 
time I'll also add 2.6.30 further development to the same tree and send 
another pull request. No idea what I do, if after that more 2.6.29 fixes 
come...

> I've updated patchwork to reflect this.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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 Thu, 29 Jan 2009 11:00:41 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> On Thu, 29 Jan 2009, Mauro Carvalho Chehab wrote:
> 
> > On Tue, 27 Jan 2009 08:53:23 +0100 (CET)
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > 
> > Hi Guennadi,
> > 
> > I'm understanding that you're reviewing this patch and other ones for
> > soc_camera and will send me a PULL request after reviewing those stuff.
> 
> Yes, I'm (going to be) reviewing them, as soon as I find some time. Then 
> I'll send you two pull requests - fixes for 2.6.29 and 2.6.30 material. 
> AFAIK, unfortunately, mercurial doesn't support branches, so, I probably 
> will end up first sending you a pull request with fixes, and after some 
> time I'll also add 2.6.30 further development to the same tree and send 
> another pull request. No idea what I do, if after that more 2.6.29 fixes 
> come...

Yes, this is another drawback of hg. For fixes, please add: 
	Priority: high

At the body of the description. My scripts will understand this as fix patches.

Cheers,
Mauro
--
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
  

Dear Guennadi

> > I attached stupid 4 patches.
(snip)
> Thanks for the patches, please, give me a couple of days for review.
(snip)
>> Yes, I'm (going to be) reviewing them, as soon as I find some time.

If you have not reviewed now, please use attached one.
It use more clever way I think.
Best regards
--
Kuninori Morimoto
  

Hi Morimoto-san,

On Fri, 30 Jan 2009, morimoto.kuninori@renesas.com wrote:

> > > I attached stupid 4 patches.
> (snip)
> > Thanks for the patches, please, give me a couple of days for review.
> (snip)
> >> Yes, I'm (going to be) reviewing them, as soon as I find some time.
> 
> If you have not reviewed now, please use attached one.
> It use more clever way I think.

I'll have to think about it more, but the first impression is - this 
wouldn't work.

At the moment we use the same soc-camera API call set_fmt for both S_FMT 
and S_CROP calls. But it you look in various instance drivers - host and 
camera - you will see, that almost all of them have a test "if (pixfmt)," 
i.e., they have to differentiate between the two cases. And not only 
because with pixfmt == 0 they cannot configure the stack completely, but 
because the meaning of these two calls even just with respect to geometry 
is different: S_FMT specifies scaling, whereas S_CROP preserves the 
current scaling and only specifies a window using the current scaled 
coordinates. So, you have to be able to differentiate. The original 
mt9m001 and mt9v022 drivers didn't support scaling, so, for they just used 
cropping for both, but the recently added mt9t031 does support scaling, 
so, it is indeed important. Not sure about mt9m111, and yours ov772x and 
tw9910.

Further, calling set_bus_param() from (or soc_camera_s_fmt_vid_cap()) from 
S_CROP is not enough too. This lets the capture.c example run, but, I 
think, we should be able to run with no configuration at all - even 
without a S_CROP. So, some default configuration has to be set up on 
open() or on STREAMON if none is set yet (current_fmt == NULL).

So, you can either wit until I find time to address this, or try to do 
something along these lines yourself. But I'm not sure if I manage to 
propose anything meaningful before FOSDEM (next weekend), so, this might 
take up to two weeks. But, I think, we have a bit of time before the 
2.6.30 merge window:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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
  

Dear Guennadi

Thank you for checking patch.

> camera - you will see, that almost all of them have a test "if (pixfmt)," 
> i.e., they have to differentiate between the two cases. And not only 
(snip)
> cropping for both, but the recently added mt9t031 does support scaling, 
> so, it is indeed important. Not sure about mt9m111, and yours ov772x and 
> tw9910.

hmm. I could understand.

> Further, calling set_bus_param() from (or soc_camera_s_fmt_vid_cap()) from 
> S_CROP is not enough too. This lets the capture.c example run, but, I 
> think, we should be able to run with no configuration at all - even 
> without a S_CROP. So, some default configuration has to be set up on 
> open() or on STREAMON if none is set yet (current_fmt == NULL).

I can agree with your opinion.

On my opinion, not only calling set_bus_param but also
try_fmt is important for tw9910.
Because tw9910 needs "INTERLACE" mode, and sh_mobile_ceu
sets "is_interlace flag" in try_fmt.
This is the reason why I would like to call try_fmt
(or s_fmt_vid_cap directly).

And
sh_mobile_ceu_dev :: camera_fmt is set in sh_mobile_ceu_set_fmt,
and it is used in set_bus_param.
but it need xlate->cam_fmt.
you know xlate need pixfmt now....
This is the reason why I would like to use current_fmt to set_fmt.
Therefore, I can agree with your
"some default configuration has to be set up on open()"

> So, you can either wit until I find time to address this, or try to do 
> something along these lines yourself. But I'm not sure if I manage to 
> propose anything meaningful before FOSDEM (next weekend), so, this might 
> take up to two weeks. But, I think, we have a bit of time before the 
> 2.6.30 merge window:-)

OK. no problem for me.
I can always help you if needed.

Best regards
--
Kuninori Morimoto
 
--
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 Fri, 30 Jan 2009, morimoto.kuninori@renesas.com wrote:

> On my opinion, not only calling set_bus_param but also
> try_fmt is important for tw9910.
> Because tw9910 needs "INTERLACE" mode, and sh_mobile_ceu
> sets "is_interlace flag" in try_fmt.

Ooh, this is wrong. As we discussed before - try_fmt shall not perform any 
configuration, it only "tries," i.e., tests, whether the specified 
configuration is possible. This has to be moved to S_FMT.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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
  

Dear Guennadi

> > On my opinion, not only calling set_bus_param but also
> > try_fmt is important for tw9910.
> > Because tw9910 needs "INTERLACE" mode, and sh_mobile_ceu
> > sets "is_interlace flag" in try_fmt.
> 
> Ooh, this is wrong. As we discussed before - try_fmt shall not perform any 
> configuration, it only "tries," i.e., tests, whether the specified 
> configuration is possible. This has to be moved to S_FMT.

Indeed.
But set_fmt doesn't called with "filed" now.
Can I fix it ?

Best regards
--
Kuninori Morimoto
 
--
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 Fri, 30 Jan 2009, morimoto.kuninori@renesas.com wrote:

> 
> Dear Guennadi
> 
> > > On my opinion, not only calling set_bus_param but also
> > > try_fmt is important for tw9910.
> > > Because tw9910 needs "INTERLACE" mode, and sh_mobile_ceu
> > > sets "is_interlace flag" in try_fmt.
> > 
> > Ooh, this is wrong. As we discussed before - try_fmt shall not perform any 
> > configuration, it only "tries," i.e., tests, whether the specified 
> > configuration is possible. This has to be moved to S_FMT.
> 
> Indeed.
> But set_fmt doesn't called with "filed" now.
> Can I fix it ?

Hm, ok, I was thinking about this several times, to use "struct 
v4l2_format *fmt" instead of "__u32 pixfmt" as a second parameter to 
set_fmt in both host and device structs. The disadvantage of this is, that 
then the information in the third parameter "struct v4l2_rect *rect" 
becomes redundant in case of S_FMT... I think, it might be better to 
finally separate S_FMT and S_CROP, i.e., add new set_crop methods to both 
host and device structs and switch all drivers to use them... Looks like 
this would be a cleaner solution than keeping them together and struggling 
to differentiate between the two... Specific drivers can then decide to 
implement them using the same function internally, like, e.g., mt9m001 
which doesn't use pixfmt at all, or mt9v022, which only uses it to check 
validity.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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