Date:	Wed, 3 Aug 2016 09:39:12 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Robert Jarzmik <robert.jarzmik@...e.fr>
Cc:	Guennadi Liakhovetski <g.liakhovetski@....de>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: platform: pxa_camera: convert to vb2



On 08/02/2016 08:03 PM, Robert Jarzmik wrote:
> Hans Verkuil <hverkuil@...all.nl> writes:
> 
> Hi Hans,
> 
> Working further on the pxa_camera conversion out of soc_camera, I hit a problem
> of dma being very long, in running v4l2-compliance -f.
> 
> After digging a bit, I realized that the video buffer queued was a 640x480
> format, while the s_fmt_vid_cap() lastly called was a 48x32 one.
> 
> Digging a bit further, I remembered this conversation we had :
>>> +/*
>>> + * Please check the DMA prepared buffer structure in :
>>> + *   Documentation/video4linux/pxa_camera.txt
>>> + * Please check also in pxa_camera_check_link_miss() to understand why DMA chain
>>> + * modification while DMA chain is running will work anyway.
>>> + */
>>> +static int pxa_vb2_prepare(struct vb2_buffer *vb)
> ...zip...
>>> +	for (i = 0; i < pcdev->channels && vb2_plane_vaddr(vb, i); i++)
>>> +		if (vb2_get_plane_payload(vb, i) > vb2_plane_size(vb, i))
>>> +			return -EINVAL;
>>
>> No need for this check, this can't happen. This is checked when the buffers are
>> allocated, and once allocated icd->sizeimage can no longer change.
>>
>>> +
>>> +	if ((pcdev->channels != buf->nb_planes) ||
>>> +	    (vb2_get_plane_payload(vb, 0) != buf->plane_sizes[0])) {
>>> +		pxa_buffer_cleanup(buf);
>>> +		ret = pxa_buffer_init(pcdev, buf);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>
>> Ditto, this can't happen on the fly.
> 
> This last assumption is incorrect I think, at least that's what my tests show
> me.  As a proof, as attached in [1] the logs of pxa_camera. You will see that a
> 640x480 is initialized, but when the buffer is prepared and queued, the capture
> format had been changed to 48x32.
> 
> The consequence in my case is that the scatter-gather prepared for the DMA is
> still 640x480x2 bytes long and not 48x32x2 !!! That means I will capture many
> many frames before declaring the video-buffer done.
> The previous test was recalculating the scatter-gather, which would have taken
> care of this case.
> 
> Unless I'm wrong, I will re-add the above test.
> 
> Cheers.
> 
> --
> Robert
> 
> [1] Logs of pxa_camera under v4l2-compliance -f
>     I put "RJK" annotation for speed reading
> 
> [ 1509.751011] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue_setup(vq=c312f290 nbufs=2 num_planes=0 size=614400)
> [ 1509.754861] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1)
> [ 1509.754989] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0  dir=2
> [ 1509.755024] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41
> [ 1509.755206] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6400[0] flags=0x41
> [ 1509.758558] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1)
> [ 1509.758603] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0  dir=2
> [ 1509.758634] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41
> [ 1509.758797] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6000[0] flags=0x41
> [ 1509.759296] pxa27x-camera pxa27x-camera.0: pxac_vb2_cleanup(vb=c30e6600)
> [ 1509.759392] pxa-dma pxa-dma.0: vchan c39b0c30: txd c30e6400[0]: freeing
> [ 1509.759530] pxa27x-camera pxa27x-camera.0: pxac_vb2_cleanup(vb=c30e6a00)
> [ 1509.759560] pxa-dma pxa-dma.0: vchan c39b0c30: txd c30e6000[0]: freeing
> [ 1509.760730] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=640x480:50424752)
> [ 1509.764971] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752
> [ 1509.765077] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=640x480:50424752)
> [ 1509.768771] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752
> [ 1509.768945] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x50424752
> [ 1509.769009] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=48x32:56595559)
> [ 1509.772931] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x56595559
> [ 1509.773051] pxa27x-camera pxa27x-camera.0: s_fmt_vid_cap(pix=48x32:56595559)
> [ 1509.777213] pxa27x-camera pxa27x-camera.0: current_fmt->fourcc: 0x56595559
> 	RJK: Here we switch to 48x32 format
> 
> [ 1509.777386] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue_setup(vq=c312f290 nbufs=3 num_planes=0 size=614400)

But this debug line indicates that pcdev->current_pix.sizeimage is still the old value: this
should have been updated by the S_FMT call. You'd have to debug that a bit more, it looks like
there is a bug somewhere in the driver.

I suspect this line in pxac_vidioc_try_fmt_vid_cap:

pix->sizeimage = max_t(u32, pix->sizeimage, ret);

This should just be pix->sizeimage = ret.

Regards,

	Hans

> [ 1509.780789] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1)
> [ 1509.780869] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0  dir=2
> [ 1509.780902] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41
> [ 1509.781088] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6600[0] flags=0x41
> [ 1509.784948] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1)
> [ 1509.785065] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0  dir=2
> [ 1509.785100] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41
> [ 1509.785281] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6400[0] flags=0x41
> [ 1509.788611] pxa27x-camera pxa27x-camera.0: pxac_vb2_init(nb_channels=1)
> [ 1509.788658] dma dma0chan2: pxad_get_config(): dev_addr=0x50000028 maxburst=8 width=0  dir=2
> [ 1509.788689] dma dma0chan2: pxad_prep_slave_sg(): dir=2 flags=41
> [ 1509.788853] dma dma0chan2: pxad_tx_prep(): vc=c39b0c30 txd=c30e6200[0] flags=0x41
> [ 1509.802079] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6a00) nb_channels=1 size=614400
> 	RJK: Here we prepare a 614400 bytes buffer for 48x32 format
>              The scatter-gather chain is way too big for a 48x32x2 bytes capture
> 
> [ 1509.802207] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6000) nb_channels=1 size=614400
> [ 1509.802248] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6c00) nb_channels=1 size=614400
> [ 1509.802512] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6a00) nb_channels=1 size=614400 active=  (null)
> [ 1509.802564] dma dma0chan2: pxad_tx_submit(): txd c30e6600[7]: submitted (not linked)
> [ 1509.802600] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6a00 cookie=7
> [ 1509.802823] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6000) nb_channels=1 size=614400 active=  (null)
> [ 1509.802857] dma dma0chan2: pxad_tx_submit(): txd c30e6400[8]: submitted (cold linked)
> [ 1509.802889] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6000 cookie=8
> [ 1509.803110] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6c00) nb_channels=1 size=614400 active=  (null)
> [ 1509.803142] dma dma0chan2: pxad_tx_submit(): txd c30e6200[9]: submitted (cold linked)
> [ 1509.803172] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6c00 cookie=9
> [ 1509.803205] pxa27x-camera pxa27x-camera.0: pxac_vb2_start_streaming(count=3) active=  (null)
> [ 1509.803236] pxa27x-camera pxa27x-camera.0: pxa_camera_start_capture
> [ 1509.976619] pxa27x-camera pxa27x-camera.0: Camera interrupt status 0x9119
> [ 1509.976723] pxa27x-camera pxa27x-camera.0: Camera interrupt status 0x0
> [ 1509.976760] pxa27x-camera pxa27x-camera.0: pxa_dma_start_channels (channel=0)
> [ 1509.976793] dma dma0chan2: pxad_issue_pending(): txd c30e6600[7]
> [ 1509.976823] dma dma0chan2: pxad_launch_chan(): desc=c30e6600
> [ 1509.976856] dma dma0chan2: lookup_phy(): phy=c3a7f810(0)
> [ 1509.976885] dma dma0chan2: phy_enable(); phy=c3a7f810(0) misaligned=0
> [ 1592.777757] dma dma0chan2: pxad_chan_handler(): checking txd c30e6600[7]: completed=1
> [ 1592.777834] dma dma0chan2: pxad_chan_handler(): checking txd c30e6400[8]: completed=0
> [ 1592.777913] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1
> [ 1592.778057] dma dma0chan2: pxad_residue(): txd c30e6200[9] sw_desc=c30e6200: 614400
> [ 1592.778216] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6a00)
> [ 1592.778253] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6000, is_dma_stopped=0
> [ 1592.779099] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6a00) nb_channels=1 size=614400
> [ 1592.779403] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6a00) nb_channels=1 size=614400 active=c30e6000
> [ 1592.779455] dma dma0chan2: pxad_tx_submit(): txd c30e6600[a]: submitted (hot linked)
> [ 1592.779490] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6a00 cookie=10
> [ 1675.855782] dma dma0chan2: pxad_chan_handler(): checking txd c30e6400[8]: completed=1
> [ 1675.855861] dma dma0chan2: pxad_chan_handler(): checking txd c30e6200[9]: completed=0
> [ 1675.855953] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1
> [ 1675.856097] dma dma0chan2: pxad_residue(): txd c30e6600[a] sw_desc=c30e6600: 614400
> [ 1675.856251] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6000)
> [ 1675.856286] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6c00, is_dma_stopped=0
> [ 1675.857074] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6000) nb_channels=1 size=614400
> [ 1675.857376] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6000) nb_channels=1 size=614400 active=c30e6c00
> [ 1675.857428] dma dma0chan2: pxad_tx_submit(): txd c30e6400[b]: submitted (hot linked)
> [ 1675.857464] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6000 cookie=11
> [ 1758.933819] dma dma0chan2: pxad_chan_handler(): checking txd c30e6200[9]: completed=1
> [ 1758.933896] dma dma0chan2: pxad_chan_handler(): checking txd c30e6600[a]: completed=0
> [ 1758.933971] pxa27x-camera pxa27x-camera.0: camera dma irq, cisr=0x8318 dma=1
> [ 1758.934114] dma dma0chan2: pxad_residue(): txd c30e6400[b] sw_desc=c30e6400: 614400
> [ 1758.934271] pxa27x-camera pxa27x-camera.0: pxa_camera_wakeup dequeud buffer (buf=0xc30e6c00)
> [ 1758.934307] pxa27x-camera pxa27x-camera.0: pxa_camera_check_link_miss : top queued buffer=c30e6a00, is_dma_stopped=0
> [ 1758.935111] pxa27x-camera pxa27x-camera.0: pxac_vb2_prepare (vb=c30e6c00) nb_channels=1 size=614400
> [ 1758.935414] pxa27x-camera pxa27x-camera.0: pxac_vb2_queue(vb=c30e6c00) nb_channels=1 size=614400 active=c30e6a00
> [ 1758.935467] dma dma0chan2: pxad_tx_submit(): txd c30e6200[c]: submitted (hot linked)
> [ 1758.935503] pxa27x-camera pxa27x-camera.0: pxa_dma_add_tail_buf (channel=0) : submit vb=c30e6c00 cookie=12
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Powered by blists - more mailing lists