[PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Vivek Kasireddy]

From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>

The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be similar
to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.

While displaying the packed AYUV buffers generated by the Vivid driver
using v4l2-tpg on Weston, it was observed that these AYUV images were not
getting displayed correctly. Changing the memory layout makes them display
as expected.

Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
index d9a590ae7545..825667f67c92 100644
--- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
+++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
@@ -1269,7 +1269,6 @@ static void gen_twopix(struct tpg_data *tpg,
 	case V4L2_PIX_FMT_HSV32:
 		alpha = 0;
 		/* fall through */
-	case V4L2_PIX_FMT_YUV32:
 	case V4L2_PIX_FMT_ARGB32:
 		buf[0][offset] = alpha;
 		buf[0][offset + 1] = r_y_h;
@@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data *tpg,
 	case V4L2_PIX_FMT_XBGR32:
 		alpha = 0;
 		/* fall through */
+	case V4L2_PIX_FMT_YUV32:
 	case V4L2_PIX_FMT_ABGR32:
 		buf[0][offset] = b_v;
 		buf[0][offset + 1] = g_u_s;
-- 
2.14.5

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Hans Verkuil]

Hi Vivek,

On 1/29/19 3:32 AM, Vivek Kasireddy wrote:
> From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
> 
> The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be similar
> to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.
> 
> While displaying the packed AYUV buffers generated by the Vivid driver
> using v4l2-tpg on Weston, it was observed that these AYUV images were not
> getting displayed correctly. Changing the memory layout makes them display
> as expected.

Our YUV32 fourcc is defined as follows:

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html

As far as I see the format that the TPG generates is according to the V4L2 spec.

Philipp, can you check the YUV32 format that the imx-pxp driver uses?
Is that according to our spec?

At some point we probably want to add a VUY32 format which is what Weston
expects, but we certainly cannot change what the TPG generates for YUV32
since that is correct.

Regards,

	Hans

> 
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> index d9a590ae7545..825667f67c92 100644
> --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> @@ -1269,7 +1269,6 @@ static void gen_twopix(struct tpg_data *tpg,
>  	case V4L2_PIX_FMT_HSV32:
>  		alpha = 0;
>  		/* fall through */
> -	case V4L2_PIX_FMT_YUV32:
>  	case V4L2_PIX_FMT_ARGB32:
>  		buf[0][offset] = alpha;
>  		buf[0][offset + 1] = r_y_h;
> @@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data *tpg,
>  	case V4L2_PIX_FMT_XBGR32:
>  		alpha = 0;
>  		/* fall through */
> +	case V4L2_PIX_FMT_YUV32:
>  	case V4L2_PIX_FMT_ABGR32:
>  		buf[0][offset] = b_v;
>  		buf[0][offset + 1] = g_u_s;
> 

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Vivek Kasireddy]

On Thu, 31 Jan 2019 14:36:42 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:

Hi Hans,

> Hi Vivek,
> 
> On 1/29/19 3:32 AM, Vivek Kasireddy wrote:
> > From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
> > 
> > The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be
> > similar to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.
> > 
> > While displaying the packed AYUV buffers generated by the Vivid
> > driver using v4l2-tpg on Weston, it was observed that these AYUV
> > images were not getting displayed correctly. Changing the memory
> > layout makes them display as expected.  
> 
> Our YUV32 fourcc is defined as follows:
> 
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
> 
> As far as I see the format that the TPG generates is according to the
> V4L2 spec.

I looked into the above link, and I am now wondering whether YUV32 is
the same as the format referred to as AYUV here or not:

https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering#ayuv

If YUV32 is not the same as AYUV, should I send another patch adding a
new format named AYUV with the reversed memory layout?

> 
> Philipp, can you check the YUV32 format that the imx-pxp driver uses?
> Is that according to our spec?
> 
> At some point we probably want to add a VUY32 format which is what
> Weston expects, but we certainly cannot change what the TPG generates
> for YUV32 since that is correct.
Weston does not know much about the details of pixel formats and
instead relies on the Mesa i965 DRI driver to do the heavy lifting.
And, this driver implemented AYUV support looking at the above Microsoft
link. Also, is the description of V4l pixel formats mentioned in the
below link accurate:
https://afrantzis.com/pixel-format-guide/v4l2.html

Thanks,
Vivek

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> > b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index
> > d9a590ae7545..825667f67c92 100644 ---
> > a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++
> > b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -1269,7 +1269,6
> > @@ static void gen_twopix(struct tpg_data *tpg, case
> > V4L2_PIX_FMT_HSV32: alpha = 0;
> >  		/* fall through */
> > -	case V4L2_PIX_FMT_YUV32:
> >  	case V4L2_PIX_FMT_ARGB32:
> >  		buf[0][offset] = alpha;
> >  		buf[0][offset + 1] = r_y_h;
> > @@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data *tpg,
> >  	case V4L2_PIX_FMT_XBGR32:
> >  		alpha = 0;
> >  		/* fall through */
> > +	case V4L2_PIX_FMT_YUV32:
> >  	case V4L2_PIX_FMT_ABGR32:
> >  		buf[0][offset] = b_v;
> >  		buf[0][offset + 1] = g_u_s;
> >   
> 

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Hans Verkuil]

On 2/1/19 3:29 AM, Vivek Kasireddy wrote:
> On Thu, 31 Jan 2019 14:36:42 +0100
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> Hi Hans,
> 
>> Hi Vivek,
>>
>> On 1/29/19 3:32 AM, Vivek Kasireddy wrote:
>>> From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
>>>
>>> The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be
>>> similar to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.
>>>
>>> While displaying the packed AYUV buffers generated by the Vivid
>>> driver using v4l2-tpg on Weston, it was observed that these AYUV
>>> images were not getting displayed correctly. Changing the memory
>>> layout makes them display as expected.  
>>
>> Our YUV32 fourcc is defined as follows:
>>
>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
>>
>> As far as I see the format that the TPG generates is according to the
>> V4L2 spec.
> 
> I looked into the above link, and I am now wondering whether YUV32 is
> the same as the format referred to as AYUV here or not:
> 
> https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering#ayuv

It's not the same format.

> 
> If YUV32 is not the same as AYUV, should I send another patch adding a
> new format named AYUV with the reversed memory layout?

That can only be done if there is also a driver that uses it.

> 
>>
>> Philipp, can you check the YUV32 format that the imx-pxp driver uses?
>> Is that according to our spec?

Philipp, would it be possible to add such a format to imx-pxp? That might
be a nice approach because once imx-pxp can do it, then it can also be
added to the TPG.

>>
>> At some point we probably want to add a VUY32 format which is what
>> Weston expects, but we certainly cannot change what the TPG generates
>> for YUV32 since that is correct.
> Weston does not know much about the details of pixel formats and
> instead relies on the Mesa i965 DRI driver to do the heavy lifting.
> And, this driver implemented AYUV support looking at the above Microsoft
> link. Also, is the description of V4l pixel formats mentioned in the
> below link accurate:
> https://afrantzis.com/pixel-format-guide/v4l2.html

Don't use it, just use the V4L2 Specification, that's always kept up to date.

Regards,

	Hans

> 
> Thanks,
> Vivek
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> ---
>>>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index
>>> d9a590ae7545..825667f67c92 100644 ---
>>> a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++
>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -1269,7 +1269,6
>>> @@ static void gen_twopix(struct tpg_data *tpg, case
>>> V4L2_PIX_FMT_HSV32: alpha = 0;
>>>  		/* fall through */
>>> -	case V4L2_PIX_FMT_YUV32:
>>>  	case V4L2_PIX_FMT_ARGB32:
>>>  		buf[0][offset] = alpha;
>>>  		buf[0][offset + 1] = r_y_h;
>>> @@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data *tpg,
>>>  	case V4L2_PIX_FMT_XBGR32:
>>>  		alpha = 0;
>>>  		/* fall through */
>>> +	case V4L2_PIX_FMT_YUV32:
>>>  	case V4L2_PIX_FMT_ABGR32:
>>>  		buf[0][offset] = b_v;
>>>  		buf[0][offset + 1] = g_u_s;
>>>   
>>
> 

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Vivek Kasireddy]

On Fri, 1 Feb 2019 10:08:52 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:
Hi Hans,

> On 2/1/19 3:29 AM, Vivek Kasireddy wrote:
> > On Thu, 31 Jan 2019 14:36:42 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > 
> > Hi Hans,
> >   
> >> Hi Vivek,
> >>
> >> On 1/29/19 3:32 AM, Vivek Kasireddy wrote:  
> >>> From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
> >>>
> >>> The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be
> >>> similar to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.
> >>>
> >>> While displaying the packed AYUV buffers generated by the Vivid
> >>> driver using v4l2-tpg on Weston, it was observed that these AYUV
> >>> images were not getting displayed correctly. Changing the memory
> >>> layout makes them display as expected.    
> >>
> >> Our YUV32 fourcc is defined as follows:
> >>
> >> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
> >>
> >> As far as I see the format that the TPG generates is according to
> >> the V4L2 spec.  
> > 
> > I looked into the above link, and I am now wondering whether YUV32
> > is the same as the format referred to as AYUV here or not:
> > 
> > https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering#ayuv  
> 
> It's not the same format.
> 
> > 
> > If YUV32 is not the same as AYUV, should I send another patch
> > adding a new format named AYUV with the reversed memory layout?  
> 
> That can only be done if there is also a driver that uses it.
There are some drm drivers that already use the AYUV format defined
here:
https://git.linuxtv.org/media_tree.git/tree/drivers/gpu/drm/drm_fourcc.c#n228

> 
> >   
> >>
> >> Philipp, can you check the YUV32 format that the imx-pxp driver
> >> uses? Is that according to our spec?  
> 
> Philipp, would it be possible to add such a format to imx-pxp? That
> might be a nice approach because once imx-pxp can do it, then it can
> also be added to the TPG.
I was going to send in a patch to add the AYUV (and maybe XYUV) format
but I came across this line that leaves me confused:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-vid-common.c#n164

It appears that when YUV32 was added, the intention was to mimic AYUV.
And, unless I am utterly mistaken, the alpha_mask of 0x000000ff
suggests that the alpha component is expected to be found in the 
LSB (LE) bits indicating a memory layout of VUYA. Am I interpreting 
this incorrectly?

Thanks,
Vivek

> 
> >>
> >> At some point we probably want to add a VUY32 format which is what
> >> Weston expects, but we certainly cannot change what the TPG
> >> generates for YUV32 since that is correct.  
> > Weston does not know much about the details of pixel formats and
> > instead relies on the Mesa i965 DRI driver to do the heavy lifting.
> > And, this driver implemented AYUV support looking at the above
> > Microsoft link. Also, is the description of V4l pixel formats
> > mentioned in the below link accurate:
> > https://afrantzis.com/pixel-format-guide/v4l2.html  
> 
> Don't use it, just use the V4L2 Specification, that's always kept up
> to date.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Thanks,
> > Vivek
> >   
> >>
> >> Regards,
> >>
> >> 	Hans
> >>  
> >>>
> >>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>> ---
> >>>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> >>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index
> >>> d9a590ae7545..825667f67c92 100644 ---
> >>> a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++
> >>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -1269,7 +1269,6
> >>> @@ static void gen_twopix(struct tpg_data *tpg, case
> >>> V4L2_PIX_FMT_HSV32: alpha = 0;
> >>>  		/* fall through */
> >>> -	case V4L2_PIX_FMT_YUV32:
> >>>  	case V4L2_PIX_FMT_ARGB32:
> >>>  		buf[0][offset] = alpha;
> >>>  		buf[0][offset + 1] = r_y_h;
> >>> @@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data *tpg,
> >>>  	case V4L2_PIX_FMT_XBGR32:
> >>>  		alpha = 0;
> >>>  		/* fall through */
> >>> +	case V4L2_PIX_FMT_YUV32:
> >>>  	case V4L2_PIX_FMT_ABGR32:
> >>>  		buf[0][offset] = b_v;
> >>>  		buf[0][offset + 1] = g_u_s;
> >>>     
> >>  
> >   
> 

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Hans Verkuil]

On 02/02/2019 01:48 AM, Vivek Kasireddy wrote:
> On Fri, 1 Feb 2019 10:08:52 +0100
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Hans,
> 
>> On 2/1/19 3:29 AM, Vivek Kasireddy wrote:
>>> On Thu, 31 Jan 2019 14:36:42 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>
>>> Hi Hans,
>>>   
>>>> Hi Vivek,
>>>>
>>>> On 1/29/19 3:32 AM, Vivek Kasireddy wrote:  
>>>>> From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
>>>>>
>>>>> The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be
>>>>> similar to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.
>>>>>
>>>>> While displaying the packed AYUV buffers generated by the Vivid
>>>>> driver using v4l2-tpg on Weston, it was observed that these AYUV
>>>>> images were not getting displayed correctly. Changing the memory
>>>>> layout makes them display as expected.    
>>>>
>>>> Our YUV32 fourcc is defined as follows:
>>>>
>>>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
>>>>
>>>> As far as I see the format that the TPG generates is according to
>>>> the V4L2 spec.  
>>>
>>> I looked into the above link, and I am now wondering whether YUV32
>>> is the same as the format referred to as AYUV here or not:
>>>
>>> https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering#ayuv  
>>
>> It's not the same format.
>>
>>>
>>> If YUV32 is not the same as AYUV, should I send another patch
>>> adding a new format named AYUV with the reversed memory layout?  
>>
>> That can only be done if there is also a driver that uses it.
> There are some drm drivers that already use the AYUV format defined
> here:
> https://git.linuxtv.org/media_tree.git/tree/drivers/gpu/drm/drm_fourcc.c#n228

I would have to check this with Mauro whether this is a good enough excuse to
add a new format.

> 
>>
>>>   
>>>>
>>>> Philipp, can you check the YUV32 format that the imx-pxp driver
>>>> uses? Is that according to our spec?  
>>
>> Philipp, would it be possible to add such a format to imx-pxp? That
>> might be a nice approach because once imx-pxp can do it, then it can
>> also be added to the TPG.
> I was going to send in a patch to add the AYUV (and maybe XYUV) format
> but I came across this line that leaves me confused:
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-vid-common.c#n164

It indicates the order of the components in memory, which is indeed AYUV.
But it has nothing to do with the microsoft AYUV fourcc.

> It appears that when YUV32 was added, the intention was to mimic AYUV.
> And, unless I am utterly mistaken, the alpha_mask of 0x000000ff
> suggests that the alpha component is expected to be found in the 
> LSB (LE) bits indicating a memory layout of VUYA. Am I interpreting 
> this incorrectly?

Yes. When you write 0x000000ff to memory in a LE environment, it ends
up as 0xff 0x00 0x00 0x00 in memory (in increasing memory addresses).

The v4l2-tpg AYUV format is really, really correct given the V4L2 specification
of this format.

Regards,

	Hans

> 
> Thanks,
> Vivek
> 
>>
>>>>
>>>> At some point we probably want to add a VUY32 format which is what
>>>> Weston expects, but we certainly cannot change what the TPG
>>>> generates for YUV32 since that is correct.  
>>> Weston does not know much about the details of pixel formats and
>>> instead relies on the Mesa i965 DRI driver to do the heavy lifting.
>>> And, this driver implemented AYUV support looking at the above
>>> Microsoft link. Also, is the description of V4l pixel formats
>>> mentioned in the below link accurate:
>>> https://afrantzis.com/pixel-format-guide/v4l2.html  
>>
>> Don't use it, just use the V4L2 Specification, that's always kept up
>> to date.
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Thanks,
>>> Vivek
>>>   
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>  
>>>>>
>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>>> ---
>>>>>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
>>>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index
>>>>> d9a590ae7545..825667f67c92 100644 ---
>>>>> a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++
>>>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -1269,7 +1269,6
>>>>> @@ static void gen_twopix(struct tpg_data *tpg, case
>>>>> V4L2_PIX_FMT_HSV32: alpha = 0;
>>>>>  		/* fall through */
>>>>> -	case V4L2_PIX_FMT_YUV32:
>>>>>  	case V4L2_PIX_FMT_ARGB32:
>>>>>  		buf[0][offset] = alpha;
>>>>>  		buf[0][offset + 1] = r_y_h;
>>>>> @@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data *tpg,
>>>>>  	case V4L2_PIX_FMT_XBGR32:
>>>>>  		alpha = 0;
>>>>>  		/* fall through */
>>>>> +	case V4L2_PIX_FMT_YUV32:
>>>>>  	case V4L2_PIX_FMT_ABGR32:
>>>>>  		buf[0][offset] = b_v;
>>>>>  		buf[0][offset + 1] = g_u_s;
>>>>>     
>>>>  
>>>   
>>
> 

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Vivek Kasireddy]

On Sat, 2 Feb 2019 09:03:17 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:
Hi Hans,

> On 02/02/2019 01:48 AM, Vivek Kasireddy wrote:
> > On Fri, 1 Feb 2019 10:08:52 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > Hi Hans,
> >   
> >> On 2/1/19 3:29 AM, Vivek Kasireddy wrote:  
> >>> On Thu, 31 Jan 2019 14:36:42 +0100
> >>> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>
> >>> Hi Hans,
> >>>     
> >>>> Hi Vivek,
> >>>>
> >>>> On 1/29/19 3:32 AM, Vivek Kasireddy wrote:    
> >>>>> From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
> >>>>>
> >>>>> The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be
> >>>>> similar to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.
> >>>>>
> >>>>> While displaying the packed AYUV buffers generated by the Vivid
> >>>>> driver using v4l2-tpg on Weston, it was observed that these AYUV
> >>>>> images were not getting displayed correctly. Changing the memory
> >>>>> layout makes them display as expected.      
> >>>>
> >>>> Our YUV32 fourcc is defined as follows:
> >>>>
> >>>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
> >>>>
> >>>> As far as I see the format that the TPG generates is according to
> >>>> the V4L2 spec.    
> >>>
> >>> I looked into the above link, and I am now wondering whether YUV32
> >>> is the same as the format referred to as AYUV here or not:
> >>>
> >>> https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering#ayuv    
> >>
> >> It's not the same format.
> >>  
> >>>
> >>> If YUV32 is not the same as AYUV, should I send another patch
> >>> adding a new format named AYUV with the reversed memory
> >>> layout?    
> >>
> >> That can only be done if there is also a driver that uses it.  
> > There are some drm drivers that already use the AYUV format defined
> > here:
> > https://git.linuxtv.org/media_tree.git/tree/drivers/gpu/drm/drm_fourcc.c#n228  
> 
> I would have to check this with Mauro whether this is a good enough
> excuse to add a new format.
> 
> >   
> >>  
> >>>     
> >>>>
> >>>> Philipp, can you check the YUV32 format that the imx-pxp driver
> >>>> uses? Is that according to our spec?    
> >>
> >> Philipp, would it be possible to add such a format to imx-pxp? That
> >> might be a nice approach because once imx-pxp can do it, then it
> >> can also be added to the TPG.  
> > I was going to send in a patch to add the AYUV (and maybe XYUV)
> > format but I came across this line that leaves me confused:
> > https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-vid-common.c#n164  
> 
> It indicates the order of the components in memory, which is indeed
> AYUV. But it has nothing to do with the microsoft AYUV fourcc.
Does it mean that based on the V4L2 spec, the order of components in
memory is always guaranteed regardless of the endianness of the
underlying platform? Is it the Vivid/v4l2 core driver's responsibility
to make sure this happens?

> 
> > It appears that when YUV32 was added, the intention was to mimic
> > AYUV. And, unless I am utterly mistaken, the alpha_mask of
> > 0x000000ff suggests that the alpha component is expected to be
> > found in the LSB (LE) bits indicating a memory layout of VUYA. Am I
> > interpreting this incorrectly?  
> 
> Yes. When you write 0x000000ff to memory in a LE environment, it ends
> up as 0xff 0x00 0x00 0x00 in memory (in increasing memory addresses).
This would only work if the alpha component A is always in the MSB
which suggests endianness-independence. And, what would happen when
writing the alpha mask 0x000000ff in a BE environment which would end up
as 0x00 0x00 0x00 0xff in memory? 

> 
> The v4l2-tpg AYUV format is really, really correct given the V4L2
> specification of this format.
Ok, should I send a patch adding support for VUYA format? Should the
new fourcc be VUY4?

Thanks,
Vivek

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Thanks,
> > Vivek
> >   
> >>  
> >>>>
> >>>> At some point we probably want to add a VUY32 format which is
> >>>> what Weston expects, but we certainly cannot change what the TPG
> >>>> generates for YUV32 since that is correct.    
> >>> Weston does not know much about the details of pixel formats and
> >>> instead relies on the Mesa i965 DRI driver to do the heavy
> >>> lifting. And, this driver implemented AYUV support looking at the
> >>> above Microsoft link. Also, is the description of V4l pixel
> >>> formats mentioned in the below link accurate:
> >>> https://afrantzis.com/pixel-format-guide/v4l2.html    
> >>
> >> Don't use it, just use the V4L2 Specification, that's always kept
> >> up to date.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>  
> >>>
> >>> Thanks,
> >>> Vivek
> >>>     
> >>>>
> >>>> Regards,
> >>>>
> >>>> 	Hans
> >>>>    
> >>>>>
> >>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>>>> ---
> >>>>>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> >>>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index
> >>>>> d9a590ae7545..825667f67c92 100644 ---
> >>>>> a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++
> >>>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -1269,7
> >>>>> +1269,6 @@ static void gen_twopix(struct tpg_data *tpg, case
> >>>>> V4L2_PIX_FMT_HSV32: alpha = 0;
> >>>>>  		/* fall through */
> >>>>> -	case V4L2_PIX_FMT_YUV32:
> >>>>>  	case V4L2_PIX_FMT_ARGB32:
> >>>>>  		buf[0][offset] = alpha;
> >>>>>  		buf[0][offset + 1] = r_y_h;
> >>>>> @@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data
> >>>>> *tpg, case V4L2_PIX_FMT_XBGR32:
> >>>>>  		alpha = 0;
> >>>>>  		/* fall through */
> >>>>> +	case V4L2_PIX_FMT_YUV32:
> >>>>>  	case V4L2_PIX_FMT_ABGR32:
> >>>>>  		buf[0][offset] = b_v;
> >>>>>  		buf[0][offset + 1] = g_u_s;
> >>>>>       
> >>>>    
> >>>     
> >>  
> >   
> 

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Hans Verkuil]

On 2/5/19 3:04 AM, Vivek Kasireddy wrote:
> On Sat, 2 Feb 2019 09:03:17 +0100
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Hans,
> 
>> On 02/02/2019 01:48 AM, Vivek Kasireddy wrote:
>>> On Fri, 1 Feb 2019 10:08:52 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> Hi Hans,
>>>   
>>>> On 2/1/19 3:29 AM, Vivek Kasireddy wrote:  
>>>>> On Thu, 31 Jan 2019 14:36:42 +0100
>>>>> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>
>>>>> Hi Hans,
>>>>>     
>>>>>> Hi Vivek,
>>>>>>
>>>>>> On 1/29/19 3:32 AM, Vivek Kasireddy wrote:    
>>>>>>> From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
>>>>>>>
>>>>>>> The memory layout of AYUV buffers (V4L2_PIX_FMT_YUV32) should be
>>>>>>> similar to V4L2_PIX_FMT_ABGR32 instead of V4L2_PIX_FMT_ARGB32.
>>>>>>>
>>>>>>> While displaying the packed AYUV buffers generated by the Vivid
>>>>>>> driver using v4l2-tpg on Weston, it was observed that these AYUV
>>>>>>> images were not getting displayed correctly. Changing the memory
>>>>>>> layout makes them display as expected.      
>>>>>>
>>>>>> Our YUV32 fourcc is defined as follows:
>>>>>>
>>>>>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
>>>>>>
>>>>>> As far as I see the format that the TPG generates is according to
>>>>>> the V4L2 spec.    
>>>>>
>>>>> I looked into the above link, and I am now wondering whether YUV32
>>>>> is the same as the format referred to as AYUV here or not:
>>>>>
>>>>> https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering#ayuv    
>>>>
>>>> It's not the same format.
>>>>  
>>>>>
>>>>> If YUV32 is not the same as AYUV, should I send another patch
>>>>> adding a new format named AYUV with the reversed memory
>>>>> layout?    
>>>>
>>>> That can only be done if there is also a driver that uses it.  
>>> There are some drm drivers that already use the AYUV format defined
>>> here:
>>> https://git.linuxtv.org/media_tree.git/tree/drivers/gpu/drm/drm_fourcc.c#n228  
>>
>> I would have to check this with Mauro whether this is a good enough
>> excuse to add a new format.
>>
>>>   
>>>>  
>>>>>     
>>>>>>
>>>>>> Philipp, can you check the YUV32 format that the imx-pxp driver
>>>>>> uses? Is that according to our spec?    
>>>>
>>>> Philipp, would it be possible to add such a format to imx-pxp? That
>>>> might be a nice approach because once imx-pxp can do it, then it
>>>> can also be added to the TPG.  
>>> I was going to send in a patch to add the AYUV (and maybe XYUV)
>>> format but I came across this line that leaves me confused:
>>> https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/vivid/vivid-vid-common.c#n164  
>>
>> It indicates the order of the components in memory, which is indeed
>> AYUV. But it has nothing to do with the microsoft AYUV fourcc.
> Does it mean that based on the V4L2 spec, the order of components in
> memory is always guaranteed regardless of the endianness of the
> underlying platform? Is it the Vivid/v4l2 core driver's responsibility
> to make sure this happens?

Correct. But I'm sure it is likely to be horribly broken in many drivers
since they are rarely if ever tested on a big endian system.

> 
>>
>>> It appears that when YUV32 was added, the intention was to mimic
>>> AYUV. And, unless I am utterly mistaken, the alpha_mask of
>>> 0x000000ff suggests that the alpha component is expected to be
>>> found in the LSB (LE) bits indicating a memory layout of VUYA. Am I
>>> interpreting this incorrectly?  
>>
>> Yes. When you write 0x000000ff to memory in a LE environment, it ends
>> up as 0xff 0x00 0x00 0x00 in memory (in increasing memory addresses).
> This would only work if the alpha component A is always in the MSB
> which suggests endianness-independence. And, what would happen when
> writing the alpha mask 0x000000ff in a BE environment which would end up
> as 0x00 0x00 0x00 0xff in memory? 

It would be wrong. But this alpha mask is used to test an esoteric corner
case of v4l2 (video overlays), and only for RGB 5:6:5 and 1:5:5:5 formats.

If I ever would test this on a big endian system I would have to make some
fixes, I'm sure.

> 
>>
>> The v4l2-tpg AYUV format is really, really correct given the V4L2
>> specification of this format.
> Ok, should I send a patch adding support for VUYA format? Should the
> new fourcc be VUY4?

I would prefer to have it along the line of how RGB32 works, so:

VUYX32 and VUYA32

(slightly different from how e.g. ABGR32 since that should have been
BGRA32, but let's do it right)

While you are at it, also add AYUV32 and XYUV32 for completeness. The
old YUV32 didn't indicate anything about whether the hardware would use
or ignore the A byte, we corrected that for RGB formats but never for
this YUV32 format.

Don't forget to update the documentation and to add support for this
to v4l2-tpg and vivid.

And mention that this allows you to use the tpg/vivid in combination with
drm drivers that use VUYX32.

Regards,

	Hans

> 
> Thanks,
> Vivek
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Thanks,
>>> Vivek
>>>   
>>>>  
>>>>>>
>>>>>> At some point we probably want to add a VUY32 format which is
>>>>>> what Weston expects, but we certainly cannot change what the TPG
>>>>>> generates for YUV32 since that is correct.    
>>>>> Weston does not know much about the details of pixel formats and
>>>>> instead relies on the Mesa i965 DRI driver to do the heavy
>>>>> lifting. And, this driver implemented AYUV support looking at the
>>>>> above Microsoft link. Also, is the description of V4l pixel
>>>>> formats mentioned in the below link accurate:
>>>>> https://afrantzis.com/pixel-format-guide/v4l2.html    
>>>>
>>>> Don't use it, just use the V4L2 Specification, that's always kept
>>>> up to date.
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>  
>>>>>
>>>>> Thanks,
>>>>> Vivek
>>>>>     
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> 	Hans
>>>>>>    
>>>>>>>
>>>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>>>>> ---
>>>>>>>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
>>>>>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index
>>>>>>> d9a590ae7545..825667f67c92 100644 ---
>>>>>>> a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++
>>>>>>> b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -1269,7
>>>>>>> +1269,6 @@ static void gen_twopix(struct tpg_data *tpg, case
>>>>>>> V4L2_PIX_FMT_HSV32: alpha = 0;
>>>>>>>  		/* fall through */
>>>>>>> -	case V4L2_PIX_FMT_YUV32:
>>>>>>>  	case V4L2_PIX_FMT_ARGB32:
>>>>>>>  		buf[0][offset] = alpha;
>>>>>>>  		buf[0][offset + 1] = r_y_h;
>>>>>>> @@ -1280,6 +1279,7 @@ static void gen_twopix(struct tpg_data
>>>>>>> *tpg, case V4L2_PIX_FMT_XBGR32:
>>>>>>>  		alpha = 0;
>>>>>>>  		/* fall through */
>>>>>>> +	case V4L2_PIX_FMT_YUV32:
>>>>>>>  	case V4L2_PIX_FMT_ABGR32:
>>>>>>>  		buf[0][offset] = b_v;
>>>>>>>  		buf[0][offset + 1] = g_u_s;
>>>>>>>       
>>>>>>    
>>>>>     
>>>>  
>>>   
>>
> 

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Philipp Zabel]

Hi Hans,

On Thu, 2019-01-31 at 14:36 +0100, Hans Verkuil wrote:
[...]
>
> Our YUV32 fourcc is defined as follows:
> 
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
> 
> As far as I see the format that the TPG generates is according to the V4L2 spec.
> 
> Philipp, can you check the YUV32 format that the imx-pxp driver uses?
> Is that according to our spec?
> 
> At some point we probably want to add a VUY32 format which is what Weston
> expects, but we certainly cannot change what the TPG generates for YUV32
> since that is correct.

I hadn't noticed as YUV32 doesn't show up in GStreamer, but testing with
v4l2-ctl, it seems to be incorrect. This script:

  #!/bin/sh
  function check() {
      PATTERN="$1"
      NAME="$2"
      echo -ne "${NAME}:\t"
      v4l2-ctl \
          --set-fmt-video-out=width=8,height=8,pixelformat=RGBP \
          --set-fmt-video=width=8,height=8,pixelformat=YUV4 \
          --stream-count 1 \
          --stream-poll \
          --stream-out-pattern "${PATTERN}" \
          --stream-out-mmap 3 \
          --stream-mmap 3 \
          --stream-to - 2>/dev/null | hexdump -v -n4 -e '/1 "%02x "'
      echo
  }
  check 6 "100% white"
  check 7 "100% red"
  check 9 "100% blue"

results in the following output:

  100% white:	80 80 ea ff 
  100% red:	f0 66 3e ff 
  100% blue:	74 f0 23 ff 

That looks like 32-bit VUYA 8-8-8-8.

regards
Philipp

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Hans Verkuil]

On 2/5/19 3:38 PM, Philipp Zabel wrote:
> Hi Hans,
> 
> On Thu, 2019-01-31 at 14:36 +0100, Hans Verkuil wrote:
> [...]
>>
>> Our YUV32 fourcc is defined as follows:
>>
>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
>>
>> As far as I see the format that the TPG generates is according to the V4L2 spec.
>>
>> Philipp, can you check the YUV32 format that the imx-pxp driver uses?
>> Is that according to our spec?
>>
>> At some point we probably want to add a VUY32 format which is what Weston
>> expects, but we certainly cannot change what the TPG generates for YUV32
>> since that is correct.
> 
> I hadn't noticed as YUV32 doesn't show up in GStreamer, but testing with
> v4l2-ctl, it seems to be incorrect. This script:
> 
>   #!/bin/sh
>   function check() {
>       PATTERN="$1"
>       NAME="$2"
>       echo -ne "${NAME}:\t"
>       v4l2-ctl \
>           --set-fmt-video-out=width=8,height=8,pixelformat=RGBP \
>           --set-fmt-video=width=8,height=8,pixelformat=YUV4 \
>           --stream-count 1 \
>           --stream-poll \
>           --stream-out-pattern "${PATTERN}" \
>           --stream-out-mmap 3 \
>           --stream-mmap 3 \
>           --stream-to - 2>/dev/null | hexdump -v -n4 -e '/1 "%02x "'
>       echo
>   }
>   check 6 "100% white"
>   check 7 "100% red"
>   check 9 "100% blue"
> 
> results in the following output:
> 
>   100% white:	80 80 ea ff 
>   100% red:	f0 66 3e ff 
>   100% blue:	74 f0 23 ff 
> 
> That looks like 32-bit VUYA 8-8-8-8.

Right. So Vivek, can you make the patches to add a proper VUYA pixelformat?

And a final patch updating imx-pxp so it uses the right pixelformat?

Since there is now a driver using it, it is also not a problem anymore to
get the new pixelformat patches merged.

Regards,

	Hans

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Nicolas Dufresne]

Le mardi 05 février 2019 à 15:38 +0100, Philipp Zabel a écrit :
> Hi Hans,
> 
> On Thu, 2019-01-31 at 14:36 +0100, Hans Verkuil wrote:
> [...]
> > Our YUV32 fourcc is defined as follows:
> > 
> > https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
> > 
> > As far as I see the format that the TPG generates is according to the V4L2 spec.
> > 
> > Philipp, can you check the YUV32 format that the imx-pxp driver uses?
> > Is that according to our spec?
> > 
> > At some point we probably want to add a VUY32 format which is what Weston
> > expects, but we certainly cannot change what the TPG generates for YUV32
> > since that is correct.
> 
> I hadn't noticed as YUV32 doesn't show up in GStreamer, but testing with
> v4l2-ctl, it seems to be incorrect. This script:

Oops, noted, I have filed an issue for that.

https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/565

It was skipped because I always assumed this was not a real format. It
was added in GStreamer for software color conversion as a YUV unpack
format (same for AYUV64 16-16-16-16). This is used in the generic color
conversion path, or the slow path.

> 
>   #!/bin/sh
>   function check() {
>       PATTERN="$1"
>       NAME="$2"
>       echo -ne "${NAME}:\t"
>       v4l2-ctl \
>           --set-fmt-video-out=width=8,height=8,pixelformat=RGBP \
>           --set-fmt-video=width=8,height=8,pixelformat=YUV4 \
>           --stream-count 1 \
>           --stream-poll \
>           --stream-out-pattern "${PATTERN}" \
>           --stream-out-mmap 3 \
>           --stream-mmap 3 \
>           --stream-to - 2>/dev/null | hexdump -v -n4 -e '/1 "%02x "'
>       echo
>   }
>   check 6 "100% white"
>   check 7 "100% red"
>   check 9 "100% blue"
> 
> results in the following output:
> 
>   100% white:	80 80 ea ff 
>   100% red:	f0 66 3e ff 
>   100% blue:	74 f0 23 ff 
> 
> That looks like 32-bit VUYA 8-8-8-8.
> 
> regards
> Philipp

Re: [PATCH] media: v4l2-tpg: Fix the memory layout of AYUV buffers

[Vivek Kasireddy]

On Wed, 6 Feb 2019 11:46:21 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:
Hi Hans,

> On 2/5/19 3:38 PM, Philipp Zabel wrote:
> > Hi Hans,
> > 
> > On Thu, 2019-01-31 at 14:36 +0100, Hans Verkuil wrote:
> > [...]  
> >>
> >> Our YUV32 fourcc is defined as follows:
> >>
> >> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-packed-yuv.html
> >>
> >> As far as I see the format that the TPG generates is according to
> >> the V4L2 spec.
> >>
> >> Philipp, can you check the YUV32 format that the imx-pxp driver
> >> uses? Is that according to our spec?
> >>
> >> At some point we probably want to add a VUY32 format which is what
> >> Weston expects, but we certainly cannot change what the TPG
> >> generates for YUV32 since that is correct.  
> > 
> > I hadn't noticed as YUV32 doesn't show up in GStreamer, but testing
> > with v4l2-ctl, it seems to be incorrect. This script:
> > 
> >   #!/bin/sh
> >   function check() {
> >       PATTERN="$1"
> >       NAME="$2"
> >       echo -ne "${NAME}:\t"
> >       v4l2-ctl \
> >           --set-fmt-video-out=width=8,height=8,pixelformat=RGBP \
> >           --set-fmt-video=width=8,height=8,pixelformat=YUV4 \
> >           --stream-count 1 \
> >           --stream-poll \
> >           --stream-out-pattern "${PATTERN}" \
> >           --stream-out-mmap 3 \
> >           --stream-mmap 3 \
> >           --stream-to - 2>/dev/null | hexdump -v -n4 -e '/1 "%02x "'
> >       echo
> >   }
> >   check 6 "100% white"
> >   check 7 "100% red"
> >   check 9 "100% blue"
> > 
> > results in the following output:
> > 
> >   100% white:	80 80 ea ff 
> >   100% red:	f0 66 3e ff 
> >   100% blue:	74 f0 23 ff 
> > 
> > That looks like 32-bit VUYA 8-8-8-8.  
> 
> Right. So Vivek, can you make the patches to add a proper VUYA
> pixelformat?
Sure, let me send the patches soon.
> 
> And a final patch updating imx-pxp so it uses the right pixelformat?
Ok, will do.

Thanks,
Vivek

> 
> Since there is now a driver using it, it is also not a problem
> anymore to get the new pixelformat patches merged.
> 
> Regards,
> 
> 	Hans