* [Nouveau] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes
@ 2021-03-17 22:44 Lyude
2021-03-18 6:41 ` [Nouveau] [igt-dev] " Martin Peres
0 siblings, 1 reply; 5+ messages in thread
From: Lyude @ 2021-03-17 22:44 UTC (permalink / raw)
To: igt-dev, nouveau; +Cc: Ben Skeggs
From: Lyude Paul <lyude@redhat.com>
Currently, nouveau doesn't support having a CRTC without a primary FB set. We
won't reject such configurations, but the behavior is undefined which will cause
CRCs to become undefined. So, avoid clearing the primary FB while we're testing
non-primary planes.
Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Martin Peres <martin.peres@free.fr>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Jeremy Cline <jcline@redhat.com>
---
tests/kms_plane.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/kms_plane.c b/tests/kms_plane.c
index 298a9375..c87983a4 100644
--- a/tests/kms_plane.c
+++ b/tests/kms_plane.c
@@ -817,9 +817,10 @@ enum crc_set { PACKED_CRC_SET,
MAX_CRC_SET };
static bool test_format_plane(data_t *data, enum pipe pipe,
- igt_output_t *output, igt_plane_t *plane)
+ igt_output_t *output, igt_plane_t *plane, igt_fb_t *primary_fb)
{
struct igt_fb fb = {};
+ struct igt_fb *clear_fb = plane->type == DRM_PLANE_TYPE_PRIMARY ? primary_fb : NULL;
drmModeModeInfo *mode;
uint32_t format, ref_format;
uint64_t modifier, ref_modifier;
@@ -879,7 +880,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
height = test_fb.height;
}
- igt_plane_set_fb(plane, NULL);
+ igt_plane_set_fb(plane, clear_fb);
igt_remove_fb(data->drm_fd, &test_fb);
}
@@ -937,7 +938,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
igt_pipe_crc_stop(data->pipe_crc);
- igt_plane_set_fb(plane, NULL);
+ igt_plane_set_fb(plane, clear_fb);
igt_remove_fb(data->drm_fd, &fb);
return result;
@@ -1010,7 +1011,7 @@ test_pixel_formats(data_t *data, enum pipe pipe)
for_each_plane_on_pipe(&data->display, pipe, plane) {
if (skip_plane(data, plane))
continue;
- result &= test_format_plane(data, pipe, output, plane);
+ result &= test_format_plane(data, pipe, output, plane, &primary_fb);
}
test_fini(data);
--
2.29.2
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Nouveau] [igt-dev] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes
2021-03-17 22:44 [Nouveau] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes Lyude
@ 2021-03-18 6:41 ` Martin Peres
2021-03-21 20:02 ` Juha-Pekka Heikkila
0 siblings, 1 reply; 5+ messages in thread
From: Martin Peres @ 2021-03-18 6:41 UTC (permalink / raw)
To: Lyude, igt-dev, nouveau; +Cc: Ben Skeggs
On 18/03/2021 00:44, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
>
> Currently, nouveau doesn't support having a CRTC without a primary FB set. We
> won't reject such configurations, but the behavior is undefined which will cause
> CRCs to become undefined. So, avoid clearing the primary FB while we're testing
> non-primary planes.
Looks good to me:
Reviewed-by: Martin Peres <martin.peres@mupuf.org>
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Martin Peres <martin.peres@free.fr>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Jeremy Cline <jcline@redhat.com>
> ---
> tests/kms_plane.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index 298a9375..c87983a4 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -817,9 +817,10 @@ enum crc_set { PACKED_CRC_SET,
> MAX_CRC_SET };
>
> static bool test_format_plane(data_t *data, enum pipe pipe,
> - igt_output_t *output, igt_plane_t *plane)
> + igt_output_t *output, igt_plane_t *plane, igt_fb_t *primary_fb)
> {
> struct igt_fb fb = {};
> + struct igt_fb *clear_fb = plane->type == DRM_PLANE_TYPE_PRIMARY ? primary_fb : NULL;
> drmModeModeInfo *mode;
> uint32_t format, ref_format;
> uint64_t modifier, ref_modifier;
> @@ -879,7 +880,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
> height = test_fb.height;
> }
>
> - igt_plane_set_fb(plane, NULL);
> + igt_plane_set_fb(plane, clear_fb);
>
> igt_remove_fb(data->drm_fd, &test_fb);
> }
> @@ -937,7 +938,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
>
> igt_pipe_crc_stop(data->pipe_crc);
>
> - igt_plane_set_fb(plane, NULL);
> + igt_plane_set_fb(plane, clear_fb);
> igt_remove_fb(data->drm_fd, &fb);
>
> return result;
> @@ -1010,7 +1011,7 @@ test_pixel_formats(data_t *data, enum pipe pipe)
> for_each_plane_on_pipe(&data->display, pipe, plane) {
> if (skip_plane(data, plane))
> continue;
> - result &= test_format_plane(data, pipe, output, plane);
> + result &= test_format_plane(data, pipe, output, plane, &primary_fb);
> }
>
> test_fini(data);
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Nouveau] [igt-dev] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes
2021-03-18 6:41 ` [Nouveau] [igt-dev] " Martin Peres
@ 2021-03-21 20:02 ` Juha-Pekka Heikkila
2021-03-22 6:53 ` Martin Peres
2021-03-22 8:12 ` Saarinen, Jani
0 siblings, 2 replies; 5+ messages in thread
From: Juha-Pekka Heikkila @ 2021-03-21 20:02 UTC (permalink / raw)
To: Martin Peres, Lyude, igt-dev, nouveau; +Cc: Lisovskiy, Stanislav, Ben Skeggs
On 18.3.2021 8.41, Martin Peres wrote:
> On 18/03/2021 00:44, Lyude wrote:
>> From: Lyude Paul <lyude@redhat.com>
>>
>> Currently, nouveau doesn't support having a CRTC without a primary FB
>> set. We
>> won't reject such configurations, but the behavior is undefined which
>> will cause
>> CRCs to become undefined. So, avoid clearing the primary FB while
>> we're testing
>> non-primary planes.
>
> Looks good to me:
>
> Reviewed-by: Martin Peres <martin.peres@mupuf.org>
>
This doesn't look good at all. Code changes were never run on ci, see this:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5614/shards-all.html?testfilter=kms_plane@pixel-format
This happen because of:
igt-gpu-tools$ git blame tests/intel-ci/blacklist-pre-merge.txt -L 173,+1
3e686098d9 (Martin Peres 2020-02-21 11:00:47 +0200 173)
igt@kms_plane@pixel-format-pipe-[a-d]-planes(-source-clamping)?
You'll need to include something like this to test and review what did
change on pixel format tests:
https://patchwork.freedesktop.org/patch/404706/?series=84370&rev=1
but as this patch seems to be already merged as untested we now have
pixel format tests failing reliably ever since:
https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=kms_plane@pixel-format&hosts=tgl%7Cicl
I have only intel icl at hand so I have no suggestions for fix. Though,
looking at those failures Stan (CCd) may have some interest on them.
/Juha-Pekka
>>
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>> Cc: Martin Peres <martin.peres@free.fr>
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: Jeremy Cline <jcline@redhat.com>
>> ---
>> tests/kms_plane.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
>> index 298a9375..c87983a4 100644
>> --- a/tests/kms_plane.c
>> +++ b/tests/kms_plane.c
>> @@ -817,9 +817,10 @@ enum crc_set { PACKED_CRC_SET,
>> MAX_CRC_SET };
>> static bool test_format_plane(data_t *data, enum pipe pipe,
>> - igt_output_t *output, igt_plane_t *plane)
>> + igt_output_t *output, igt_plane_t *plane, igt_fb_t
>> *primary_fb)
>> {
>> struct igt_fb fb = {};
>> + struct igt_fb *clear_fb = plane->type == DRM_PLANE_TYPE_PRIMARY ?
>> primary_fb : NULL;
>> drmModeModeInfo *mode;
>> uint32_t format, ref_format;
>> uint64_t modifier, ref_modifier;
>> @@ -879,7 +880,7 @@ static bool test_format_plane(data_t *data, enum
>> pipe pipe,
>> height = test_fb.height;
>> }
>> - igt_plane_set_fb(plane, NULL);
>> + igt_plane_set_fb(plane, clear_fb);
>> igt_remove_fb(data->drm_fd, &test_fb);
>> }
>> @@ -937,7 +938,7 @@ static bool test_format_plane(data_t *data, enum
>> pipe pipe,
>> igt_pipe_crc_stop(data->pipe_crc);
>> - igt_plane_set_fb(plane, NULL);
>> + igt_plane_set_fb(plane, clear_fb);
>> igt_remove_fb(data->drm_fd, &fb);
>> return result;
>> @@ -1010,7 +1011,7 @@ test_pixel_formats(data_t *data, enum pipe pipe)
>> for_each_plane_on_pipe(&data->display, pipe, plane) {
>> if (skip_plane(data, plane))
>> continue;
>> - result &= test_format_plane(data, pipe, output, plane);
>> + result &= test_format_plane(data, pipe, output, plane,
>> &primary_fb);
>> }
>> test_fini(data);
>>
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Nouveau] [igt-dev] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes
2021-03-21 20:02 ` Juha-Pekka Heikkila
@ 2021-03-22 6:53 ` Martin Peres
2021-03-22 8:12 ` Saarinen, Jani
1 sibling, 0 replies; 5+ messages in thread
From: Martin Peres @ 2021-03-22 6:53 UTC (permalink / raw)
To: juhapekka.heikkila, Lyude, igt-dev, nouveau
Cc: Lisovskiy, Stanislav, Ben Skeggs
On 21/03/2021 22:02, Juha-Pekka Heikkila wrote:
> On 18.3.2021 8.41, Martin Peres wrote:
>> On 18/03/2021 00:44, Lyude wrote:
>>> From: Lyude Paul <lyude@redhat.com>
>>>
>>> Currently, nouveau doesn't support having a CRTC without a primary FB
>>> set. We
>>> won't reject such configurations, but the behavior is undefined which
>>> will cause
>>> CRCs to become undefined. So, avoid clearing the primary FB while
>>> we're testing
>>> non-primary planes.
>>
>> Looks good to me:
>>
>> Reviewed-by: Martin Peres <martin.peres@mupuf.org>
>>
>
> This doesn't look good at all. Code changes were never run on ci, see this:
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5614/shards-all.html?testfilter=kms_plane@pixel-format
>
>
> This happen because of:
> igt-gpu-tools$ git blame tests/intel-ci/blacklist-pre-merge.txt -L 173,+1
> 3e686098d9 (Martin Peres 2020-02-21 11:00:47 +0200 173)
> igt@kms_plane@pixel-format-pipe-[a-d]-planes(-source-clamping)?
Killed by my own creation, isn't it quaint? Thanks a lot for
investigating the regression, and I'll be wearing my brown bag today :s
>
> You'll need to include something like this to test and review what did
> change on pixel format tests:
> https://patchwork.freedesktop.org/patch/404706/?series=84370&rev=1
>
> but as this patch seems to be already merged as untested we now have
> pixel format tests failing reliably ever since:
> https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=kms_plane@pixel-format&hosts=tgl%7Cicl
>
>
> I have only intel icl at hand so I have no suggestions for fix. Though,
> looking at those failures Stan (CCd) may have some interest on them.
I would indeed like to hear if this sounds like a kernel bug, a
violation of the KMS spec, or the spec is not clear-enough on this.
If I don't hear back on this today, I'll send a revert while the
situation is investigated.
Martin
>
> /Juha-Pekka
>
>>>
>>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>>> Cc: Martin Peres <martin.peres@free.fr>
>>> Cc: Ben Skeggs <bskeggs@redhat.com>
>>> Cc: Jeremy Cline <jcline@redhat.com>
>>> ---
>>> tests/kms_plane.c | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
>>> index 298a9375..c87983a4 100644
>>> --- a/tests/kms_plane.c
>>> +++ b/tests/kms_plane.c
>>> @@ -817,9 +817,10 @@ enum crc_set { PACKED_CRC_SET,
>>> MAX_CRC_SET };
>>> static bool test_format_plane(data_t *data, enum pipe pipe,
>>> - igt_output_t *output, igt_plane_t *plane)
>>> + igt_output_t *output, igt_plane_t *plane, igt_fb_t
>>> *primary_fb)
>>> {
>>> struct igt_fb fb = {};
>>> + struct igt_fb *clear_fb = plane->type == DRM_PLANE_TYPE_PRIMARY
>>> ? primary_fb : NULL;
>>> drmModeModeInfo *mode;
>>> uint32_t format, ref_format;
>>> uint64_t modifier, ref_modifier;
>>> @@ -879,7 +880,7 @@ static bool test_format_plane(data_t *data, enum
>>> pipe pipe,
>>> height = test_fb.height;
>>> }
>>> - igt_plane_set_fb(plane, NULL);
>>> + igt_plane_set_fb(plane, clear_fb);
>>> igt_remove_fb(data->drm_fd, &test_fb);
>>> }
>>> @@ -937,7 +938,7 @@ static bool test_format_plane(data_t *data, enum
>>> pipe pipe,
>>> igt_pipe_crc_stop(data->pipe_crc);
>>> - igt_plane_set_fb(plane, NULL);
>>> + igt_plane_set_fb(plane, clear_fb);
>>> igt_remove_fb(data->drm_fd, &fb);
>>> return result;
>>> @@ -1010,7 +1011,7 @@ test_pixel_formats(data_t *data, enum pipe pipe)
>>> for_each_plane_on_pipe(&data->display, pipe, plane) {
>>> if (skip_plane(data, plane))
>>> continue;
>>> - result &= test_format_plane(data, pipe, output, plane);
>>> + result &= test_format_plane(data, pipe, output, plane,
>>> &primary_fb);
>>> }
>>> test_fini(data);
>>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Nouveau] [igt-dev] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes
2021-03-21 20:02 ` Juha-Pekka Heikkila
2021-03-22 6:53 ` Martin Peres
@ 2021-03-22 8:12 ` Saarinen, Jani
1 sibling, 0 replies; 5+ messages in thread
From: Saarinen, Jani @ 2021-03-22 8:12 UTC (permalink / raw)
To: juhapekka.heikkila, Martin Peres, Lyude, igt-dev, nouveau; +Cc: Ben Skeggs
Hi,
> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Juha-Pekka
> Heikkila
> Sent: sunnuntai 21. maaliskuuta 2021 22.03
> To: Martin Peres <martin.peres@mupuf.org>; Lyude <lyude@redhat.com>; igt-
> dev@lists.freedesktop.org; nouveau@lists.freedesktop.org
> Cc: Martin Peres <martin.peres@free.fr>; Ben Skeggs <bskeggs@redhat.com>
> Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when
> testing other planes
>
> On 18.3.2021 8.41, Martin Peres wrote:
> > On 18/03/2021 00:44, Lyude wrote:
> >> From: Lyude Paul <lyude@redhat.com>
> >>
> >> Currently, nouveau doesn't support having a CRTC without a primary FB
> >> set. We won't reject such configurations, but the behavior is
> >> undefined which will cause CRCs to become undefined. So, avoid
> >> clearing the primary FB while we're testing non-primary planes.
> >
> > Looks good to me:
> >
> > Reviewed-by: Martin Peres <martin.peres@mupuf.org>
> >
>
> This doesn't look good at all. Code changes were never run on ci, see this:
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5614/shards-
> all.html?testfilter=kms_plane@pixel-format
>
> This happen because of:
> igt-gpu-tools$ git blame tests/intel-ci/blacklist-pre-merge.txt -L 173,+1
> 3e686098d9 (Martin Peres 2020-02-21 11:00:47 +0200 173) igt@kms_plane@pixel-
> format-pipe-[a-d]-planes(-source-clamping)?
>
> You'll need to include something like this to test and review what did change on pixel
> format tests:
> https://patchwork.freedesktop.org/patch/404706/?series=84370&rev=1
>
> but as this patch seems to be already merged as untested we now have pixel format
> tests failing reliably ever since:
> https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=kms_plane@pixel-
> format&hosts=tgl%7Cicl
>
> I have only intel icl at hand so I have no suggestions for fix. Though, looking at those
> failures Stan (CCd) may have some interest on them.
Can we just simply revert until fixed properly?
>
> /Juha-Pekka
>
> >>
> >> Signed-off-by: Lyude Paul <lyude@redhat.com>
> >> Cc: Martin Peres <martin.peres@free.fr>
> >> Cc: Ben Skeggs <bskeggs@redhat.com>
> >> Cc: Jeremy Cline <jcline@redhat.com>
> >> ---
> >> tests/kms_plane.c | 9 +++++----
> >> 1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tests/kms_plane.c b/tests/kms_plane.c index
> >> 298a9375..c87983a4 100644
> >> --- a/tests/kms_plane.c
> >> +++ b/tests/kms_plane.c
> >> @@ -817,9 +817,10 @@ enum crc_set { PACKED_CRC_SET,
> >> MAX_CRC_SET };
> >> static bool test_format_plane(data_t *data, enum pipe pipe,
> >> - igt_output_t *output, igt_plane_t *plane)
> >> + igt_output_t *output, igt_plane_t *plane, igt_fb_t
> >> *primary_fb)
> >> {
> >> struct igt_fb fb = {};
> >> + struct igt_fb *clear_fb = plane->type == DRM_PLANE_TYPE_PRIMARY ?
> >> primary_fb : NULL;
> >> drmModeModeInfo *mode;
> >> uint32_t format, ref_format;
> >> uint64_t modifier, ref_modifier; @@ -879,7 +880,7 @@ static
> >> bool test_format_plane(data_t *data, enum pipe pipe,
> >> height = test_fb.height;
> >> }
> >> - igt_plane_set_fb(plane, NULL);
> >> + igt_plane_set_fb(plane, clear_fb);
> >> igt_remove_fb(data->drm_fd, &test_fb);
> >> }
> >> @@ -937,7 +938,7 @@ static bool test_format_plane(data_t *data, enum
> >> pipe pipe,
> >> igt_pipe_crc_stop(data->pipe_crc);
> >> - igt_plane_set_fb(plane, NULL);
> >> + igt_plane_set_fb(plane, clear_fb);
> >> igt_remove_fb(data->drm_fd, &fb);
> >> return result;
> >> @@ -1010,7 +1011,7 @@ test_pixel_formats(data_t *data, enum pipe
> >> pipe)
> >> for_each_plane_on_pipe(&data->display, pipe, plane) {
> >> if (skip_plane(data, plane))
> >> continue;
> >> - result &= test_format_plane(data, pipe, output, plane);
> >> + result &= test_format_plane(data, pipe, output, plane,
> >> &primary_fb);
> >> }
> >> test_fini(data);
> >>
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-22 13:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-17 22:44 [Nouveau] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes Lyude
2021-03-18 6:41 ` [Nouveau] [igt-dev] " Martin Peres
2021-03-21 20:02 ` Juha-Pekka Heikkila
2021-03-22 6:53 ` Martin Peres
2021-03-22 8:12 ` Saarinen, Jani
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).