[i-g-t,3/7] lib/igt_kms: Make sure that default planes aren't overwritten.

Commit Message

Robert Foss April 20, 2016, 2:59 p.m. UTC

From: Robert Foss <[e-mail]>

Avoid overwriting planes with statically asigned indices
with planes that have dynamically assigned indices.

Signed-off-by: Robert Foss <[e-mail]>
---
 lib/igt_kms.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tomeu Vizoso April 21, 2016, 10:51 a.m. UTC | #1

On 20 April 2016 at 16:59,  <[e-mail]> wrote:
> From: Robert Foss <[e-mail]>
>
> Avoid overwriting planes with statically asigned indices
> with planes that have dynamically assigned indices.
>
> Signed-off-by: Robert Foss <[e-mail]>
> ---
>  lib/igt_kms.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 07fb73b..3f953ec 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>                                 display->has_universal_planes = 1;
>                                 break;
>                         default:
> +                               while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
> +                                       p++;

Had to read a fair bit of code to understand what's going on here, so
may make sense to add a comment.

Regards,

Tomeu

>                                 plane = &pipe->planes[p];
>                                 plane->index = p++;
>                                 break;
> --
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter April 21, 2016, 2:50 p.m. UTC | #2

On Wed, Apr 20, 2016 at 10:59:45AM -0400, [e-mail] wrote:
> From: Robert Foss <[e-mail]>
> 
> Avoid overwriting planes with statically asigned indices
> with planes that have dynamically assigned indices.
> 
> Signed-off-by: Robert Foss <[e-mail]>
> ---
>  lib/igt_kms.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 07fb73b..3f953ec 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  				display->has_universal_planes = 1;
>  				break;
>  			default:
> +				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
> +					p++;

What we might need to do is to actually make primary and cursor
dynamically assigned. Since with most drivers that's the case that
happens. Otoh we have a variable number of planes on i915 too, so not
entirely sure now what's going on here ...
-Daniel

>  				plane = &pipe->planes[p];
>  				plane->index = p++;
>  				break;
> -- 
> 2.5.0
>

Robert Foss April 21, 2016, 5:31 p.m. UTC | #3

On 04/21/2016 10:50 AM, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 10:59:45AM -0400, [e-mail] wrote:
>> From: Robert Foss <[e-mail]>
>>
>> Avoid overwriting planes with statically asigned indices
>> with planes that have dynamically assigned indices.
>>
>> Signed-off-by: Robert Foss <[e-mail]>
>> ---
>>   lib/igt_kms.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 07fb73b..3f953ec 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>>   				display->has_universal_planes = 1;
>>   				break;
>>   			default:
>> +				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
>> +					p++;
> What we might need to do is to actually make primary and cursor
> dynamically assigned. Since with most drivers that's the case that
> happens. Otoh we have a variable number of planes on i915 too, so not
> entirely sure now what's going on here ...
> -Daniel
Since PRIMARY and CURSOR planes are statically assigned and
that the other cases in this switch statement used the static
assignments, the loop simply makes sure the we don't overwrite
the PRIMARY or CURSOR plane.


>>   				plane = &pipe->planes[p];
>>   				plane->index = p++;
>>   				break;
>> -- 
>> 2.5.0
>>

Daniel Vetter April 22, 2016, 12:53 p.m. UTC | #4

On Thu, Apr 21, 2016 at 01:31:48PM -0400, Robert Foss wrote:
> 
> 
> On 04/21/2016 10:50 AM, Daniel Vetter wrote:
> >On Wed, Apr 20, 2016 at 10:59:45AM -0400, [e-mail] wrote:
> >>From: Robert Foss <[e-mail]>
> >>
> >>Avoid overwriting planes with statically asigned indices
> >>with planes that have dynamically assigned indices.
> >>
> >>Signed-off-by: Robert Foss <[e-mail]>
> >>---
> >>  lib/igt_kms.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >>index 07fb73b..3f953ec 100644
> >>--- a/lib/igt_kms.c
> >>+++ b/lib/igt_kms.c
> >>@@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> >>  				display->has_universal_planes = 1;
> >>  				break;
> >>  			default:
> >>+				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
> >>+					p++;
> >What we might need to do is to actually make primary and cursor
> >dynamically assigned. Since with most drivers that's the case that
> >happens. Otoh we have a variable number of planes on i915 too, so not
> >entirely sure now what's going on here ...
> >-Daniel
> Since PRIMARY and CURSOR planes are statically assigned and
> that the other cases in this switch statement used the static
> assignments, the loop simply makes sure the we don't overwrite
> the PRIMARY or CURSOR plane.

Well primary should be first, cursor last ... Are there drivers which
register planes in a funny order that we need this? If so a comment would
be good I think.
-Daniel

> 
> 
> >>  				plane = &pipe->planes[p];
> >>  				plane->index = p++;
> >>  				break;
> >>-- 
> >>2.5.0
> >>
>

Robert Foss April 22, 2016, 1:23 p.m. UTC | #5

On 04/22/2016 08:53 AM, Daniel Vetter wrote:
> On Thu, Apr 21, 2016 at 01:31:48PM -0400, Robert Foss wrote:
>>
>> On 04/21/2016 10:50 AM, Daniel Vetter wrote:
>>> On Wed, Apr 20, 2016 at 10:59:45AM -0400, [e-mail] wrote:
>>>> From: Robert Foss <[e-mail]>
>>>>
>>>> Avoid overwriting planes with statically asigned indices
>>>> with planes that have dynamically assigned indices.
>>>>
>>>> Signed-off-by: Robert Foss <[e-mail]>
>>>> ---
>>>>   lib/igt_kms.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>>> index 07fb73b..3f953ec 100644
>>>> --- a/lib/igt_kms.c
>>>> +++ b/lib/igt_kms.c
>>>> @@ -1326,6 +1326,8 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>>>>   				display->has_universal_planes = 1;
>>>>   				break;
>>>>   			default:
>>>> +				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
>>>> +					p++;
>>> What we might need to do is to actually make primary and cursor
>>> dynamically assigned. Since with most drivers that's the case that
>>> happens. Otoh we have a variable number of planes on i915 too, so not
>>> entirely sure now what's going on here ...
>>> -Daniel
>> Since PRIMARY and CURSOR planes are statically assigned and
>> that the other cases in this switch statement used the static
>> assignments, the loop simply makes sure the we don't overwrite
>> the PRIMARY or CURSOR plane.
> Well primary should be first, cursor last ... Are there drivers which
> register planes in a funny order that we need this? If so a comment would
> be good I think.
> -Daniel
I wasn't aware of the cursor plane always being last one.
I'll have the v2 patches reflect this.
>
>>
>>>>   				plane = &pipe->planes[p];
>>>>   				plane->index = p++;
>>>>   				break;
>>>> -- 
>>>> 2.5.0
>>>>

Patch

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 07fb73b..3f953ec 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1326,6 +1326,8 @@  void igt_display_init(igt_display_t *display, int drm_fd)
 				display->has_universal_planes = 1;
 				break;
 			default:
+				while (p == IGT_PLANE_PRIMARY || p == IGT_PLANE_CURSOR)
+					p++;
 				plane = &pipe->planes[p];
 				plane->index = p++;
 				break;