[PATCH v2] modetest: initialize handles/pitches in set_plane()

[PATCH v2] modetest: initialize handles/pitches in set_plane()

[Tobias Jakobi]

Only the 'offsets' array was initialized to zero.
Since bo_create only sets the handles which are
necessary, were we passing garbage data to the
kernel when calling drmModeAddFB2 later.

The issue only seems to appear when passing e.g.
NV12 data to the kernel, a case where not only
handles[0] is used. I therefore also removed the
corresponding comment.

v2: Do the same for set_mode(), set_cursors()
    and test_page_flip().

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 tests/modetest/modetest.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 5f46efd..27ce380 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -964,7 +964,7 @@ page_flip_handler(int fd, unsigned int frame,
 static int set_plane(struct device *dev, struct plane_arg *p)
 {
 	drmModePlane *ovr;
-	uint32_t handles[4], pitches[4], offsets[4] = {0}; /* we only use [0] */
+	uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0};
 	uint32_t plane_id = 0;
 	struct bo *plane_bo;
 	uint32_t plane_flags = 0;
@@ -1046,7 +1046,7 @@ static int set_plane(struct device *dev, struct plane_arg *p)
 
 static void set_mode(struct device *dev, struct pipe_arg *pipes, unsigned int count)
 {
-	uint32_t handles[4], pitches[4], offsets[4] = {0}; /* we only use [0] */
+	uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0};
 	unsigned int fb_id;
 	struct bo *bo;
 	unsigned int i;
@@ -1125,7 +1125,7 @@ static void set_planes(struct device *dev, struct plane_arg *p, unsigned int cou
 
 static void set_cursors(struct device *dev, struct pipe_arg *pipes, unsigned int count)
 {
-	uint32_t handles[4], pitches[4], offsets[4] = {0}; /* we only use [0] */
+	uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0};
 	struct bo *bo;
 	unsigned int i;
 	int ret;
@@ -1165,7 +1165,7 @@ static void clear_cursors(struct device *dev)
 
 static void test_page_flip(struct device *dev, struct pipe_arg *pipes, unsigned int count)
 {
-	uint32_t handles[4], pitches[4], offsets[4] = {0}; /* we only use [0] */
+	uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0};
 	unsigned int other_fb_id;
 	struct bo *other_bo;
 	drmEventContext evctx;
-- 
2.0.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] modetest: initialize handles/pitches in set_plane()

[Emil Velikov]

Hi Tobias,

On 20/04/15 19:50, Tobias Jakobi wrote:
> Only the 'offsets' array was initialized to zero.
> Since bo_create only sets the handles which are
> necessary, were we passing garbage data to the
> kernel when calling drmModeAddFB2 later.
> 
> The issue only seems to appear when passing e.g.
> NV12 data to the kernel, a case where not only
> handles[0] is used. I therefore also removed the
> corresponding comment.
> 
> v2: Do the same for set_mode(), set_cursors()
>     and test_page_flip().
> 
Nice catch. I will push this in a day or so, unless someone objects.

This and the other patches from Joonyoung Shim make me question how many
people have seriously used this tool.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] modetest: initialize handles/pitches in set_plane()

[Ilia Mirkin]

On Tue, Apr 21, 2015 at 4:10 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi Tobias,
>
> On 20/04/15 19:50, Tobias Jakobi wrote:
>> Only the 'offsets' array was initialized to zero.
>> Since bo_create only sets the handles which are
>> necessary, were we passing garbage data to the
>> kernel when calling drmModeAddFB2 later.
>>
>> The issue only seems to appear when passing e.g.
>> NV12 data to the kernel, a case where not only
>> handles[0] is used. I therefore also removed the
>> corresponding comment.
>>
>> v2: Do the same for set_mode(), set_cursors()
>>     and test_page_flip().
>>
> Nice catch. I will push this in a day or so, unless someone objects.
>
> This and the other patches from Joonyoung Shim make me question how many
> people have seriously used this tool.

I know it was immensely useful to me when I was adding YUV plane
support to nouveau. Seemed to work as advertised at the time (1.5y
ago) for YUYV, UYVY, and NV12.

  -ilia
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] modetest: initialize handles/pitches in set_plane()

[Tobias Jakobi]

Hello Ilia,

On 2015-04-21 21:15, Ilia Mirkin wrote:
> I know it was immensely useful to me when I was adding YUV plane
> support to nouveau. Seemed to work as advertised at the time (1.5y
> ago) for YUYV, UYVY, and NV12.
> 
>   -ilia
maybe you can help me with that question.

Let's consider a user of the DRM interface that wants to feed NV12 data 
to it. NV12 is bi-planar, so the user should provide two 
handles/pitches/offsets describing chroma and luma plane respectively. 
But most of the time chroma and luma is contiguous in memory, with 
nothing in between.

I was wondering if it is an allowed setup to request NV12 as 
pixelformat, but only to provide _one_ handle/pitch/offset? (implying 
that we are in the contiguous setting)


With best wishes,
Tobias

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] modetest: initialize handles/pitches in set_plane()

[Ilia Mirkin]

On Thu, Apr 23, 2015 at 9:39 AM, Tobias Jakobi
<tjakobi@math.uni-bielefeld.de> wrote:
> Hello Ilia,
>
> On 2015-04-21 21:15, Ilia Mirkin wrote:
>>
>> I know it was immensely useful to me when I was adding YUV plane
>> support to nouveau. Seemed to work as advertised at the time (1.5y
>> ago) for YUYV, UYVY, and NV12.
>>
>>   -ilia
>
> maybe you can help me with that question.
>
> Let's consider a user of the DRM interface that wants to feed NV12 data to
> it. NV12 is bi-planar, so the user should provide two
> handles/pitches/offsets describing chroma and luma plane respectively. But
> most of the time chroma and luma is contiguous in memory, with nothing in
> between.
>
> I was wondering if it is an allowed setup to request NV12 as pixelformat,
> but only to provide _one_ handle/pitch/offset? (implying that we are in the
> contiguous setting)

Uhm... I'm no authority on the matter, merely vouching for the
usefulness of the modetest tool :) However I was never aware of any
contiguousness assumptions in NV12, afaik the two different planes are
different :) It could also cause issues if you had, a, say, 32x30
image but whatever hw produced it wanted to make it 32x32. You'd end
up with an offset between the two planes which wouldn't be specified.
FWIW on the (much older) NVIDIA gpu's that I added support for, it
assumes a separate offset:

                nvif_wr32(dev, NV_PVIDEO_UVPLANE_OFFSET_BUFF(flip),
                        nv_fb->nvbo->bo.offset + fb->offsets[1]);

Note that as far as the HW is concerned, it's an entirely separate
memory location, not even an offset from the Y plane -- it could be 2
totally separate bo's for all it cares.

Also, as another datapoint, the VP3 and newer video decoding units on
NVIDIA cards (generally speaking GeForce 200+) have firmware that
produces the Y and UV data as completely separate pieces of data as
well. On VP2 they had to be in the same buffer, but you could provide
an explicit offset to the UV bit.

  -ilia
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] modetest: initialize handles/pitches in set_plane()

[Tobias Jakobi]

Hello Ilia!


On 2015-04-23 16:32, Ilia Mirkin wrote:
> On Thu, Apr 23, 2015 at 9:39 AM, Tobias Jakobi
> <tjakobi@math.uni-bielefeld.de> wrote:
>> Hello Ilia,
>> 
>> On 2015-04-21 21:15, Ilia Mirkin wrote:
>>> 
>>> I know it was immensely useful to me when I was adding YUV plane
>>> support to nouveau. Seemed to work as advertised at the time (1.5y
>>> ago) for YUYV, UYVY, and NV12.
>>> 
>>>   -ilia
>> 
>> maybe you can help me with that question.
>> 
>> Let's consider a user of the DRM interface that wants to feed NV12 
>> data to
>> it. NV12 is bi-planar, so the user should provide two
>> handles/pitches/offsets describing chroma and luma plane respectively. 
>> But
>> most of the time chroma and luma is contiguous in memory, with nothing 
>> in
>> between.
>> 
>> I was wondering if it is an allowed setup to request NV12 as 
>> pixelformat,
>> but only to provide _one_ handle/pitch/offset? (implying that we are 
>> in the
>> contiguous setting)
> 
> Uhm... I'm no authority on the matter, merely vouching for the
> usefulness of the modetest tool :) However I was never aware of any
> contiguousness assumptions in NV12, afaik the two different planes are
> different :) It could also cause issues if you had, a, say, 32x30
> image but whatever hw produced it wanted to make it 32x32. You'd end
> up with an offset between the two planes which wouldn't be specified.
> FWIW on the (much older) NVIDIA gpu's that I added support for, it
> assumes a separate offset:
> 
>                 nvif_wr32(dev, NV_PVIDEO_UVPLANE_OFFSET_BUFF(flip),
>                         nv_fb->nvbo->bo.offset + fb->offsets[1]);
> 
> Note that as far as the HW is concerned, it's an entirely separate
> memory location, not even an offset from the Y plane -- it could be 2
> totally separate bo's for all it cares.
Thanks for the insight! That's kind of what I expected.

What confused me though is that the v4l2 API has this:
http://www.hep.by/gnu/kernel/media/V4L2-PIX-FMT-NV12M.html

Maybe pixelformats are passed around differently in v4l2, but as far as 
I can see, the difference between v4l2-NV12 and v4l2-NV12M doesn't exist 
in DRM land. As soon as NV12 is used, we always have two planes given 
explicitly.



> Also, as another datapoint, the VP3 and newer video decoding units on
> NVIDIA cards (generally speaking GeForce 200+) have firmware that
> produces the Y and UV data as completely separate pieces of data as
> well. On VP2 they had to be in the same buffer, but you could provide
> an explicit offset to the UV bit.
OK, so the Exynos video processor kinda does the same here. It needs 
separate pointers to chroma and luma.


> 
>   -ilia


With best wishes,
Tobias


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] modetest: initialize handles/pitches in set_plane()

[Emil Velikov]

Hi Ilia,

On 21/04/15 19:15, Ilia Mirkin wrote:
> On Tue, Apr 21, 2015 at 4:10 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> Hi Tobias,
>>
>> On 20/04/15 19:50, Tobias Jakobi wrote:
>>> Only the 'offsets' array was initialized to zero.
>>> Since bo_create only sets the handles which are
>>> necessary, were we passing garbage data to the
>>> kernel when calling drmModeAddFB2 later.
>>>
>>> The issue only seems to appear when passing e.g.
>>> NV12 data to the kernel, a case where not only
>>> handles[0] is used. I therefore also removed the
>>> corresponding comment.
>>>
>>> v2: Do the same for set_mode(), set_cursors()
>>>     and test_page_flip().
>>>
>> Nice catch. I will push this in a day or so, unless someone objects.
>>
>> This and the other patches from Joonyoung Shim make me question how many
>> people have seriously used this tool.
> 
> I know it was immensely useful to me when I was adding YUV plane
> support to nouveau. Seemed to work as advertised at the time (1.5y
> ago) for YUYV, UYVY, and NV12.
> 
Hah... your reply was authored 55 minutes before mine, according to
Blunderbird :)

Back to the original topic - most likely I'm underestimating the shear
permutation of ways the tool can be used. Glad to hear that it was
useful for your work - perhaps we changed something that "broke" in
after that :-\


-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel