Guennadi Liakhovetski <g.liakhovetski@xxxxxx> writes:
>> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
>> index e3e6b29..54df071 100644
>> --- a/drivers/media/video/pxa_camera.c
>> +++ b/drivers/media/video/pxa_camera.c
>> @@ -242,14 +242,13 @@ static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
>> dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);
>>
>> /* planar capture requires Y, U and V buffers to be page aligned */
>> - if (pcdev->channels == 3) {
>> - *size = PAGE_ALIGN(icd->width * icd->height); /* Y pages */
>> - *size += PAGE_ALIGN(icd->width * icd->height / 2); /* U pages */
>> - *size += PAGE_ALIGN(icd->width * icd->height / 2); /* V pages */
>> - } else {
>> - *size = icd->width * icd->height *
>> - ((icd->current_fmt->depth + 7) >> 3);
>> - }
>> + if (pcdev->channels == 3)
>> + *size = roundup(icd->width * icd->height, 8) /* Y pages */
>> + + roundup(icd->width * icd->height / 2, 8) /* U pages */
>> + + roundup(icd->width * icd->height / 2, 8); /* V pages */
>> + else
>> + *size = roundup(icd->width * icd->height *
>> + ((icd->current_fmt->depth + 7) >> 3), 8);
>>
>> if (0 == *count)
>> *count = 32;
>
> Ok, this one will change I presume - new alignment calculations and
> line-breaking. In fact, if you adjust width and height earlier in set_fmt,
> maybe you'll just remove any rounding here completely.
Helas, not fully.
The problem is with passthrough and rgb formats, where I don't enforce
width/height. In the newest form of the patch I have this :
if (pcdev->channels == 3)
*size = icd->width * icd->height * 2;
else
*size = roundup(icd->width * icd->height *
((icd->current_fmt->depth + 7) >> 3), 8);
>
>> @@ -289,19 +288,63 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf)
>> buf->vb.state = VIDEOBUF_NEEDS_INIT;
>> }
>>
>> +static int calculate_dma_sglen(struct scatterlist *sglist, int sglen,
>> + int sg_first_ofs, int size)
>> +{
>> + int i, offset, dma_len, xfer_len;
>> + struct scatterlist *sg;
>> +
>> + offset = sg_first_ofs;
>> + for_each_sg(sglist, sg, sglen, i) {
>> + dma_len = sg_dma_len(sg);
>> +
>> + /* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
>> + xfer_len = roundup(min(dma_len - offset, size), 8);
>
> Ok, let's see. dma_len is PAGE_SIZE. offset is for Y-plane 0, for further
> planes it will be aligned after we recalculate width and height. size will
> be aligned too, so, roundup will disappear, right?
No, I don't think so.
Consider the case of a RGB565 image which size is 223*33 = 7359 bytes. This
makes a transfer of 4096 bytes and another of 3263 bytes.
But the QIF fifo will give 4096 + 3264 bytes (the last one beeing 0), and this
last byte has to be read from the fifo. As I understand it, the QIF fifo works
with 8 bytes permutations, and that why it's giving always a multiple of 8
bytes. Please, cross-check PXA developper manual, paragraph 27.4.4.1 to see if
you understand the same as I did.
So the roundup() is to be kept :(
> You might want to just
> just add a test for these. The calculation itself gives size >= xfer_len
>
>> +
>> + size = max(0, size - xfer_len);
>
> So, max is useless here, just "size -= xfer_len."
And so the max() still hold I think.
>> + size = max(0, size - xfer_len);
>
> Same here for roundup() and max().
Yes, same discussion.
>>
>> - pxa_dma->sg_cpu[sglen - 1].ddadr = DDADR_STOP;
>> - pxa_dma->sg_cpu[sglen - 1].dcmd |= DCMD_ENDIRQEN;
>> + pxa_dma->sg_cpu[sglen].ddadr = DDADR_STOP;
>> + pxa_dma->sg_cpu[sglen].dcmd = DCMD_FLOWSRC | DCMD_BURST8 | DCMD_ENDIRQEN;
>
> Why are you now always using the n+1'th element? Even if it is right, it
> rather belongs to the patch "2/4," not "1/4," right? In your earlier email
> you wrote:
>
>> - in the former pxa_videobuf_queue(), when a buffer was queued while another
>> was already active, a dummy descriptor was added, and then the new buffer was
>> chained with the actively running buffer. See code below :
>>
>> - } else {
>> - buf_dma->sg_cpu[nents].ddadr =
>> - DDADR(pcdev->dma_chans[i]);
>> - }
>> -
>> - /* The next descriptor is the dummy descriptor */
>> - DDADR(pcdev->dma_chans[i]) = buf_dma->sg_dma + nents *
>> - sizeof(struct pxa_dma_desc);
>>
>> The fix is in the code refactoring, as now the buffer is always added at the
>> tail of the queue through pxa_dma_add_tail_buf().
>
> I don't understand, what this is fixing. It would make a nice
> simplification, if it worked, but see my review to patch "2/4."
Let's discuss it in patch 2/4 review, yes.
>> +
>> + *sg_first_ofs = xfer_len;
>> + /*
>> + * Handle 1 special case :
>> + * - if we finish the DMA transfer in the last 7 bytes of a RAM page
>> + * then we return the sg element pointing on the next page
>> + */
>> + if (*sg_first_ofs >= dma_len) {
>> + *sg_first_ofs -= dma_len;
>> + *sg_first = sg_next(sg);
>> + } else {
>> + *sg_first = sg;
>> + }
>
> As we will not be rounding up any more, this special case shouldn't be
> needed either, right?
Same discussion as before. But here, as sg_first and sf_first_ofs only make
sense for 3 planes output, and the rounding takes care of the corner cases, this
part will be simplified, yes.
>
>>
>> return 0;
>> }
>> @@ -342,9 +409,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
>> struct pxa_camera_dev *pcdev = ici->priv;
>> struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
>> int ret;
>> - int sglen_y, sglen_yu = 0, sglen_u = 0, sglen_v = 0;
>> - int size_y, size_u = 0, size_v = 0;
>> -
>> + int size_y = 0, size_u = 0, size_v = 0;
>
> Isn't size_y always initialised?
Yes, I'll remove that.
Thanks for the review, let's keep that going on :)
Cheers.
--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
