diff mbox series

[v2,4/5] media: staging: rkisp1: cap: support uv swapped plane formats

Message ID 20200402190419.15155-5-dafna.hirschfeld@collabora.com (mailing list archive)
State New, archived
Headers show
Series media: staging: rkisp1: cap: various fixes for capture formats | expand

Commit Message

Dafna Hirschfeld April 2, 2020, 7:04 p.m. UTC 
Plane formats with the u and v planes swapped can be
supported by changing the address of the cb and cr in
the buffer.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Laurent Pinchart April 5, 2020, 6:16 p.m. UTC | #1 
Hi Dafna,

Thank you for the patch.

On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
> Plane formats with the u and v planes swapped can be
> supported by changing the address of the cb and cr in
> the buffer.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index fa2849209433..2d274e8f565b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -41,6 +41,10 @@
>  	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
>  	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>  
> +#define RKISP1_IS_PLANAR(write_format)					\
> +	(((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||		\
> +	 ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
> +
>  enum rkisp1_plane {
>  	RKISP1_PLANE_Y	= 0,
>  	RKISP1_PLANE_CB	= 1,
> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>  			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>  	}
>  
> +	/*
> +	 * uv swap can be supported for plane formats by switching
> +	 * the address of cb and cr
> +	 */
> +	if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&

As commented on patch 3/5, could this be checked from the data in
v4l2_format_info ?

> +	    cap->pix.cfg->uv_swap) {
> +		ispbuf->buff_addr[RKISP1_PLANE_CR] =
> +			ispbuf->buff_addr[RKISP1_PLANE_CB];
> +		ispbuf->buff_addr[RKISP1_PLANE_CB] =
> +			ispbuf->buff_addr[RKISP1_PLANE_CR] +
> +			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);

How about

		swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
		     ispbuf->buff_addr[RKISP1_PLANE_CB]);

?

> +	}
> +
>  	spin_lock_irqsave(&cap->buf.lock, flags);
>  
>  	/*
Dafna Hirschfeld April 6, 2020, 11:56 a.m. UTC | #2 
On 4/5/20 8:16 PM, Laurent Pinchart wrote:
> Hi Dafna,
> 
> Thank you for the patch.
> 
> On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
>> Plane formats with the u and v planes swapped can be
>> supported by changing the address of the cb and cr in
>> the buffer.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index fa2849209433..2d274e8f565b 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -41,6 +41,10 @@
>>   	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
>>   	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>>   
>> +#define RKISP1_IS_PLANAR(write_format)					\
>> +	(((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||		\
>> +	 ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
>> +
>>   enum rkisp1_plane {
>>   	RKISP1_PLANE_Y	= 0,
>>   	RKISP1_PLANE_CB	= 1,
>> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>>   			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>>   	}
>>   
>> +	/*
>> +	 * uv swap can be supported for plane formats by switching
>> +	 * the address of cb and cr
>> +	 */
>> +	if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
> 
> As commented on patch 3/5, could this be checked from the data in
> v4l2_format_info ?
yes
> 
>> +	    cap->pix.cfg->uv_swap) {
>> +		ispbuf->buff_addr[RKISP1_PLANE_CR] =
>> +			ispbuf->buff_addr[RKISP1_PLANE_CB];
>> +		ispbuf->buff_addr[RKISP1_PLANE_CB] =
>> +			ispbuf->buff_addr[RKISP1_PLANE_CR] +
>> +			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);
> 
> How about
> 
> 		swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
> 		     ispbuf->buff_addr[RKISP1_PLANE_CB]);
> 
> ?
This also works, theoretically if there was a format where the Cb, Cr planes
are not equal size then a swap will not work.

Thanks,
Dafna
> 
>> +	}
>> +
>>   	spin_lock_irqsave(&cap->buf.lock, flags);
>>   
>>   	/*
>
Helen Mae Koike Fornazier April 6, 2020, 12:27 p.m. UTC | #3 
Hi,

On 4/6/20 8:56 AM, Dafna Hirschfeld wrote:
> 
> 
> On 4/5/20 8:16 PM, Laurent Pinchart wrote:
>> Hi Dafna,
>>
>> Thank you for the patch.
>>
>> On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
>>> Plane formats with the u and v planes swapped can be
>>> supported by changing the address of the cb and cr in
>>> the buffer.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> index fa2849209433..2d274e8f565b 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> @@ -41,6 +41,10 @@
>>>       (((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||    \
>>>        ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>>>   +#define RKISP1_IS_PLANAR(write_format)                    \
>>> +    (((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||        \
>>> +     ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
>>> +
>>>   enum rkisp1_plane {
>>>       RKISP1_PLANE_Y    = 0,
>>>       RKISP1_PLANE_CB    = 1,
>>> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>>>               rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>>>       }
>>>   +    /*
>>> +     * uv swap can be supported for plane formats by switching
>>> +     * the address of cb and cr
>>> +     */
>>> +    if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
>>
>> As commented on patch 3/5, could this be checked from the data in
>> v4l2_format_info ?
> yes
>>
>>> +        cap->pix.cfg->uv_swap) {
>>> +        ispbuf->buff_addr[RKISP1_PLANE_CR] =
>>> +            ispbuf->buff_addr[RKISP1_PLANE_CB];
>>> +        ispbuf->buff_addr[RKISP1_PLANE_CB] =
>>> +            ispbuf->buff_addr[RKISP1_PLANE_CR] +
>>> +            rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);

Actually this is wrong if pixm->num_planes != 1, since they are different buffers.

>>
>> How about
>>
>>         swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
>>              ispbuf->buff_addr[RKISP1_PLANE_CB]);
>>
>> ?
> This also works, theoretically if there was a format where the Cb, Cr planes
> are not equal size then a swap will not work.

If you check rkisp1_fill_pixfmt(), you'll see that they are equal size.
hdiv and vdiv applies to both.

Thank you Laurent for the review and thank you Dafna for working on this.

Regards,
Helen

> 
> Thanks,
> Dafna
>>
>>> +    }
>>> +
>>>       spin_lock_irqsave(&cap->buf.lock, flags);
>>>         /*
>>
>
Dafna Hirschfeld April 6, 2020, 12:40 p.m. UTC | #4 
On 4/6/20 2:27 PM, Helen Koike wrote:
> Hi,
> 
> On 4/6/20 8:56 AM, Dafna Hirschfeld wrote:
>>
>>
>> On 4/5/20 8:16 PM, Laurent Pinchart wrote:
>>> Hi Dafna,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
>>>> Plane formats with the u and v planes swapped can be
>>>> supported by changing the address of the cb and cr in
>>>> the buffer.
>>>>
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>>> ---
>>>>    drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>>>>    1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>> index fa2849209433..2d274e8f565b 100644
>>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>> @@ -41,6 +41,10 @@
>>>>        (((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||    \
>>>>         ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>>>>    +#define RKISP1_IS_PLANAR(write_format)                    \
>>>> +    (((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||        \
>>>> +     ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
>>>> +
>>>>    enum rkisp1_plane {
>>>>        RKISP1_PLANE_Y    = 0,
>>>>        RKISP1_PLANE_CB    = 1,
>>>> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>>>>                rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>>>>        }
>>>>    +    /*
>>>> +     * uv swap can be supported for plane formats by switching
>>>> +     * the address of cb and cr
>>>> +     */
>>>> +    if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
>>>
>>> As commented on patch 3/5, could this be checked from the data in
>>> v4l2_format_info ?
>> yes
>>>
>>>> +        cap->pix.cfg->uv_swap) {
>>>> +        ispbuf->buff_addr[RKISP1_PLANE_CR] =
>>>> +            ispbuf->buff_addr[RKISP1_PLANE_CB];
>>>> +        ispbuf->buff_addr[RKISP1_PLANE_CB] =
>>>> +            ispbuf->buff_addr[RKISP1_PLANE_CR] +
>>>> +            rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);
> 
> Actually this is wrong if pixm->num_planes != 1, since they are different buffers.
Hi, right, I will change to swap

Thanks,
Dafna
> 
>>>
>>> How about
>>>
>>>          swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
>>>               ispbuf->buff_addr[RKISP1_PLANE_CB]);
>>>
>>> ?
>> This also works, theoretically if there was a format where the Cb, Cr planes
>> are not equal size then a swap will not work.
> 
> If you check rkisp1_fill_pixfmt(), you'll see that they are equal size.
> hdiv and vdiv applies to both.
> 
> Thank you Laurent for the review and thank you Dafna for working on this.
> 
> Regards,
> Helen
> 
>>
>> Thanks,
>> Dafna
>>>
>>>> +    }
>>>> +
>>>>        spin_lock_irqsave(&cap->buf.lock, flags);
>>>>          /*
>>>
>>
diff mbox series

Patch

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index fa2849209433..2d274e8f565b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -41,6 +41,10 @@ 
 	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
 	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
 
+#define RKISP1_IS_PLANAR(write_format)					\
+	(((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||		\
+	 ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
+
 enum rkisp1_plane {
 	RKISP1_PLANE_Y	= 0,
 	RKISP1_PLANE_CB	= 1,
@@ -788,6 +792,19 @@  static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
 			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
 	}
 
+	/*
+	 * uv swap can be supported for plane formats by switching
+	 * the address of cb and cr
+	 */
+	if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
+	    cap->pix.cfg->uv_swap) {
+		ispbuf->buff_addr[RKISP1_PLANE_CR] =
+			ispbuf->buff_addr[RKISP1_PLANE_CB];
+		ispbuf->buff_addr[RKISP1_PLANE_CB] =
+			ispbuf->buff_addr[RKISP1_PLANE_CR] +
+			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);
+	}
+
 	spin_lock_irqsave(&cap->buf.lock, flags);
 
 	/*