Re: [PATCH] Fix suspend/resume of pxa_camera driver

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Sun, 27 Jul 2008, Robert Jarzmik wrote:

> Yes, I have tested, with a complete suspend/resume cycle on a Mitac Mio A701
> smartphone. And yes, the PXA suspend is based on SDRAM being in self refresh
> state. I'm speaking of suspend, not standby, there's no confusion here.
> 
> Notice I always go into suspend while _not_ in active capturing. That could
> change things.

Yes, this is the difference. The sensor is attached to the camera host 
only on open. In fact, I am not sure, how video applications should behave 
during a suspend / resume cycle. If you suspend, while, say, recording 
from your camera, should you directly continue recording after a wake up? 
How do currect drivers implement this? Or, in general, for example with 
audio - if you suspend while listening to a stream over the net, or to a 
CD, or to a mp3-file on your local disk, should the sound resume after a 
wake up? I added linux-pm for some authoritative answers:-)

If you know how a v4l2 device should handle suspend/resume, or when we get 
some answers, let's try to do it completely-

> Have you previously tested the pxa_camera driver in suspend ?

No, I have not. I didn't have power-management enabled on my board, and I 
don't know how easy such tests would be on my hardware. That's why I just 
removed all suspend/resume code from the pxa270 driver completely.

> For history, my setup is :
>  - a pxa272 on a Mio A701 board
>  - a Micron MT9M111 chip (driver under construction)
> 
> For the camera part, by now, I'm using standard suspend/resume functions of the
> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> between the driver resume function and the availability of the I2C bus are not
> properly chained. I'm still working on it.

Yes, we have to clarify this too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Alan Stern]

On Sun, 27 Jul 2008, Guennadi Liakhovetski wrote:

> Yes, this is the difference. The sensor is attached to the camera host 
> only on open. In fact, I am not sure, how video applications should behave 
> during a suspend / resume cycle. If you suspend, while, say, recording 
> from your camera, should you directly continue recording after a wake up? 
> How do currect drivers implement this? Or, in general, for example with 
> audio - if you suspend while listening to a stream over the net, or to a 
> CD, or to a mp3-file on your local disk, should the sound resume after a 
> wake up? I added linux-pm for some authoritative answers:-)

IMO, suspend should be as transparent as possible for userspace.  So if
a user program has started an audio or video stream before suspend,
then after resume the stream should still be playing.

Alan Stern

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sun, 27 Jul 2008, Robert Jarzmik wrote:
>
> Yes, this is the difference. The sensor is attached to the camera host 
> only on open. In fact, I am not sure, how video applications should behave 
> during a suspend / resume cycle. If you suspend, while, say, recording 
> from your camera, should you directly continue recording after a wake up? 
> How do currect drivers implement this? Or, in general, for example with 
> audio - if you suspend while listening to a stream over the net, or to a 
> CD, or to a mp3-file on your local disk, should the sound resume after a 
> wake up? I added linux-pm for some authoritative answers:-)
AFAIK, on resume, sound streams continues. So the normal behaviour would be to
continue video stream too (as done in ALSA). This supposes the whole video chip
state is saved on suspend and restored on resume, of course.

> If you know how a v4l2 device should handle suspend/resume, or when we get 
> some answers, let's try to do it completely-
Of course. In a week or two, my mt9m111 driver will be ready for submission, and
in the review process I'll post a submission for complete suspend/resume.

>> For the camera part, by now, I'm using standard suspend/resume functions of the
>> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
>> between the driver resume function and the availability of the I2C bus are not
>> properly chained. I'm still working on it.
>
> Yes, we have to clarify this too.
Yes.

So, to sum up :
 - I finish the mt9m111 driver
 - I submit it
 - I cook up a clean suspend/resume (unless you did it first of course :)

--
Robert

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Sun, 27 Jul 2008, Guennadi Liakhovetski wrote:
>
> IMO, suspend should be as transparent as possible for userspace.  So if
> a user program has started an audio or video stream before suspend,
> then after resume the stream should still be playing.
Yes, that's what I think too.

The only little thing that should change will be timeout problem (userspace
waits on a poll to get a video frame within 1 second for example).
I'm not sure how well this is handled, I didn't check.

--
Robert

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Mon, 28 Jul 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > On Sun, 27 Jul 2008, Robert Jarzmik wrote:
> >
> > Yes, this is the difference. The sensor is attached to the camera host 
> > only on open. In fact, I am not sure, how video applications should behave 
> > during a suspend / resume cycle. If you suspend, while, say, recording 
> > from your camera, should you directly continue recording after a wake up? 
> > How do currect drivers implement this? Or, in general, for example with 
> > audio - if you suspend while listening to a stream over the net, or to a 
> > CD, or to a mp3-file on your local disk, should the sound resume after a 
> > wake up? I added linux-pm for some authoritative answers:-)
> AFAIK, on resume, sound streams continues. So the normal behaviour would be to
> continue video stream too (as done in ALSA). This supposes the whole video chip
> state is saved on suspend and restored on resume, of course.

Ok, that's what I also thought.

> > If you know how a v4l2 device should handle suspend/resume, or when we get 
> > some answers, let's try to do it completely-
> Of course. In a week or two, my mt9m111 driver will be ready for submission, and
> in the review process I'll post a submission for complete suspend/resume.
> 
> >> For the camera part, by now, I'm using standard suspend/resume functions of the
> >> platform driver (mt9m111.c). It does work, but it's not clean ATM. The chaining
> >> between the driver resume function and the availability of the I2C bus are not
> >> properly chained. I'm still working on it.
> >
> > Yes, we have to clarify this too.
> Yes.
> 
> So, to sum up :
>  - I finish the mt9m111 driver
>  - I submit it
>  - I cook up a clean suspend/resume (unless you did it first of course :)

Good plan! You don't have to worry - I will not do this before you:-)

And, I am still waiting for the patch to move .power and .reset to 
camera-link (see my another post on the V4L ML today). With that patch in 
place your power-management would also become easier and more logical.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Thu, 31 Jul 2008, Robert Jarzmik wrote:

> > So, to sum up :
> >  - I finish the mt9m111 driver
> >  - I submit it
> >  - I cook up a clean suspend/resume (unless you did it first of course :)
> 
> All right, I finished the pxa_camera part. The suspend/resume does work with a
> opened video stream. The capture begins before the suspend and finished after
> the resume.
> 
> I post the patch here attached for information. I'll submit later with the
> complete suspend/resume serie. This is just for preliminary comments. Of course,
> this patch superseeds the origin patch posted in this thread, which didn't work
> for an opened video stream.

Ok, some preliminary comments.

> >From fb38f10c233a5b4e13f5ad42cf1c381ecc4215e9 Mon Sep 17 00:00:00 2001
> From: Robert Jarzmik <robert.jarzmik@free.fr>
> Date: Sun, 27 Jul 2008 00:52:22 +0200
> Subject: [PATCH] Fix suspend/resume of pxa_camera driver
> 
> PXA suspend switches off DMA core, which looses all context

I think, you mean "loses" - with one "o".

> of previously assigned descriptors. As pxa_camera driver
> relies on DMA transfers, setup the lost descriptors on
> resume and retrigger frame acquisition if needed.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/pxa_camera.c |   49 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index efb2d19..f00844c 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -128,6 +128,8 @@ struct pxa_camera_dev {
>  
>  	struct pxa_buffer	*active;
>  	struct pxa_dma_desc	*sg_tail[3];
> +
> +	u32			save_CICR[5];

I think, it would look better in  plane lowercase, just name it "cicr" or 
even "save_cicr" if you prefer.

>  };
>  
>  static const char *pxa_cam_driver_description = "PXA_Camera";
> @@ -1017,6 +1019,51 @@ static struct soc_camera_host pxa_soc_camera_host = {
>  	.ops			= &pxa_soc_camera_host_ops,
>  };
>  
> +static int pxa_camera_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
> +	int i = 0;
> +
> +	pcdev->save_CICR[i++] = CICR0;
> +	pcdev->save_CICR[i++] = CICR1;
> +	pcdev->save_CICR[i++] = CICR2;
> +	pcdev->save_CICR[i++] = CICR3;
> +	pcdev->save_CICR[i++] = CICR4;
> +
> +	return 0;
> +}
> +
> +static int pxa_camera_resume(struct platform_device *pdev)
> +{
> +	struct pxa_camera_dev *pcdev = platform_get_drvdata(pdev);
> +	int i = 0;
> +
> +	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
> +	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
> +	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;
> +
> +	CICR0 = pcdev->save_CICR[i++] & ~CICR0_ENB;
> +	CICR1 = pcdev->save_CICR[i++];
> +	CICR2 = pcdev->save_CICR[i++];
> +	CICR3 = pcdev->save_CICR[i++];
> +	CICR4 = pcdev->save_CICR[i++];
> +
> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
> +		pcdev->icd->ops->resume(pcdev->icd);

Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
quite convinced by your argumentation in a previous email regarding in 
which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
far as I understood, devices get resumed simply in the order they got 
registered. This does guarantee, that children are resumed after parents, 
but otherwise there are no guarantees. I guess, you load pxa-camera after 
i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
almost prepared to bet, your resume will not work then:-)

I think, I have an idea. Our soc_camera_device is registered the last - it 
is registered after the respective i2c device (at least in all drivers so 
far, and future drivers better keep it this way), and after the camera 
host it is on (see soc_camera.c::device_register_link()). So, all we have 
to do is add a suspend and a resume to soc_camera_bus_type and to 
soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
from soc_camera_bus_type .resume and .suspend. Now this should work, what 
do you think?

> +
> +	/* Restart frame capture if active buffer exists */
> +	if (pcdev->active) {
> +		/* Reset the FIFOs */
> +		CIFR |= CIFR_RESET_F;
> +		/* Enable End-Of-Frame Interrupt */
> +		CICR0 &= ~CICR0_EOFM;
> +		/* Restart the Capture Interface */
> +		CICR0 |= CICR0_ENB;
> +	}
> +
> +	return 0;
> +}
> +
>  static int pxa_camera_probe(struct platform_device *pdev)
>  {
>  	struct pxa_camera_dev *pcdev;
> @@ -1188,6 +1235,8 @@ static struct platform_driver pxa_camera_driver = {
>  	},
>  	.probe		= pxa_camera_probe,
>  	.remove		= __exit_p(pxa_camera_remove),
> +	.suspend	= pxa_camera_suspend,
> +	.resume		= pxa_camera_resume,
>  };

If we agree on the above just move these two to pxa_soc_camera_host_ops.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I think, you mean "loses" - with one "o".
Typo. Will be fixed.

> I think, it would look better in  plane lowercase, just name it "cicr" or 
> even "save_cicr" if you prefer.
OK. Will be fixed.

>> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
>> +		pcdev->icd->ops->resume(pcdev->icd);
>
> Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
> quite convinced by your argumentation in a previous email regarding in 
> which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
> far as I understood, devices get resumed simply in the order they got 
> registered. This does guarantee, that children are resumed after parents, 
> but otherwise there are no guarantees. I guess, you load pxa-camera after 
> i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
> almost prepared to bet, your resume will not work then:-)

And you're probably right. I tested ... The order is bus before devices, parents
before childs, but I see no link between i2c-pxa and pxa-camera.

Yesterday, I moved that 2 lines into soc_camera_resume(), and
soc_camera_suspend() was added too. I came to the same conclusion, which is that
we can only be sure of the order if called from soc_camera_bus.

> I think, I have an idea. Our soc_camera_device is registered the last - it 
> is registered after the respective i2c device (at least in all drivers so 
> far, and future drivers better keep it this way), and after the camera 
> host it is on (see soc_camera.c::device_register_link()). So, all we have 
> to do is add a suspend and a resume to soc_camera_bus_type and to 
> soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
> from soc_camera_bus_type .resume and .suspend. Now this should work, what 
> do you think?

Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
brings me to another question, in which order :
 a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
 b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()

For me, the only working order can be (a), because I need
soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
camera. Do you think the same ?

> If we agree on the above just move these two to pxa_soc_camera_host_ops.
Agreed for me.

--
Robert

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I think, you mean "loses" - with one "o".
Typo. Will be fixed.

> I think, it would look better in  plane lowercase, just name it "cicr" or 
> even "save_cicr" if you prefer.
OK. Will be fixed.

>> +	if ((pcdev->icd) && (pcdev->icd->ops->resume))
>> +		pcdev->icd->ops->resume(pcdev->icd);
>
> Are we sure, that i2c has been woken up by now?... I am sorry, I wasn't 
> quite convinced by your argumentation in a previous email regarding in 
> which order the drivers will be resumed. So, I re-added pm to the cc:-) As 
> far as I understood, devices get resumed simply in the order they got 
> registered. This does guarantee, that children are resumed after parents, 
> but otherwise there are no guarantees. I guess, you load pxa-camera after 
> i2c-pxa, right? What if you first load pxa-camera and then i2c-pxa? I'm 
> almost prepared to bet, your resume will not work then:-)

And you're probably right. I tested ... The order is bus before devices, parents
before childs, but I see no link between i2c-pxa and pxa-camera.

Yesterday, I moved that 2 lines into soc_camera_resume(), and
soc_camera_suspend() was added too. I came to the same conclusion, which is that
we can only be sure of the order if called from soc_camera_bus.

> I think, I have an idea. Our soc_camera_device is registered the last - it 
> is registered after the respective i2c device (at least in all drivers so 
> far, and future drivers better keep it this way), and after the camera 
> host it is on (see soc_camera.c::device_register_link()). So, all we have 
> to do is add a suspend and a resume to soc_camera_bus_type and to 
> soc_camera_ops and to soc_camera_host_ops. Then just call the latter two 
> from soc_camera_bus_type .resume and .suspend. Now this should work, what 
> do you think?

Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
brings me to another question, in which order :
 a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
 b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()

For me, the only working order can be (a), because I need
soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
camera. Do you think the same ?

> If we agree on the above just move these two to pxa_soc_camera_host_ops.
Agreed for me.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Guennadi Liakhovetski]

On Fri, 1 Aug 2008, Robert Jarzmik wrote:

> Ah, I didn't thought of soc_camera_host_ops ... But I agree, it may be better to
> call soc_camera_host_ops->suspend() rather than pxa-camera::suspend(). Which
> brings me to another question, in which order :
>  a) soc_camera_ops->suspend() then soc_camera_hosts->suspend()
>  b) soc_camera_hosts->suspend() then soc_camera_ops->suspend()
> 
> For me, the only working order can be (a), because I need
> soc_camera_host->resume() first to enable QIF Clock, so that i2c interface is
> usable on Micron chip, so that soc_camera->resume() can send i2c commands to the
> camera. Do you think the same ?

On resume we have to do this exactly as you have done it in your last 
patch: first restore general parameters on the host, then resume the 
camera, and then continue with the FIFOs and activating the DMA. So, I 
think, we have no choice but to only call host's resume, passing it the 
camera device as a parameter, and let it decide when it wants to resume 
the camera. Similar on suspend. This will also be consistent with how 
pxa_camera_add_device() calls icd->ops->init(icd) and 
pxa_camera_remove_device() calls icd->ops->release(icd).

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] Fix suspend/resume of pxa_camera driver

[Pavel Machek]

On Mon 2008-07-28 20:37:25, Robert Jarzmik wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > On Sun, 27 Jul 2008, Guennadi Liakhovetski wrote:
> >
> > IMO, suspend should be as transparent as possible for userspace.  So if
> > a user program has started an audio or video stream before suspend,
> > then after resume the stream should still be playing.
> Yes, that's what I think too.
> 
> The only little thing that should change will be timeout problem (userspace
> waits on a poll to get a video frame within 1 second for example).
> I'm not sure how well this is handled, I didn't check.

Userrland problem. Could hit you if you go deeply into swap, for
example. Fix userland.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html