Re: [Intel-gfx] [PATCH i-g-t v2 03/14] lib/igt_kms: Rework pipe properties to be more atomic, v7.

2018-03-06 Thread Maarten Lankhorst 
Op 06-03-18 om 14:41 schreef Daniel Vetter:
> On Mon, Mar 05, 2018 at 03:37:30PM +0100, Maxime Ripard wrote:
>> Hi,
>>
>> On Thu, Oct 12, 2017 at 01:54:24PM +0200, Maarten Lankhorst wrote:
>>> In the future I want to allow tests to commit more properties,
>>> but for this to work I have to fix all properties to work better
>>> with atomic commit. Instead of special casing each
>>> property make a bitmask for all property changed flags, and try to
>>> commit all properties.
>>>
>>> This has been the most involved one, since legacy pipe commit still
>>> handles a lot of the properties differently from the rest.
>>>
>>> Changes since v1:
>>> - Dump all changed properties on commit.
>>> - Fix bug in igt_pipe_refresh().
>>> Changes since v2:
>>> - Set pipe ACTIVE property changed flag on init.
>>> Changes since v3:
>>> - Add a missing igt_pipe_refresh() to kms_atomic_interruptible.
>>> Changes since v4:
>>> - Perform error handling when setting custom crtc properties.
>>> Changes since v5:
>>> - Only attempt to commit changes properties.
>>> Changes since v6:
>>> - Clear OUT_FENCE_PTR on succesful commit.
>>>
>>> Signed-off-by: Maarten Lankhorst 
>> I'm a bit late to the party on this one, but this commit broke the
>> chamelium tests on vc4, with every kernel since at least 4.12.
>>
>> This is the error message:
>> http://code.bulix.org/32fw1l-293842
>>
>> From the stack trace, it looks like the atomic commit was failing, and
>> indeed it fails here:
>> https://elixir.bootlin.com/linux/v4.16-rc4/source/drivers/gpu/drm/drm_atomic.c#L2319
>>
>> with prop_id being 0 for some reason.
>>
>> I had a look at that patch, but I can't see anything wrong with it. Do
>> you have any ideas?
> No idea tbh, I guess we need to start tracing where the igt library tries
> to set property 0. Would probably be really good to catch that in the
> libdrm atomic support (same with trying to set a prop on obj 0, neither
> makes any sense at all).
> -Daniel

Sorry, I cc'd the original posters on it but already have a fix:

https://patchwork.freedesktop.org/patch/208058/

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v2 03/14] lib/igt_kms: Rework pipe properties to be more atomic, v7.

2018-03-06 Thread Daniel Vetter 
On Mon, Mar 05, 2018 at 03:37:30PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Oct 12, 2017 at 01:54:24PM +0200, Maarten Lankhorst wrote:
> > In the future I want to allow tests to commit more properties,
> > but for this to work I have to fix all properties to work better
> > with atomic commit. Instead of special casing each
> > property make a bitmask for all property changed flags, and try to
> > commit all properties.
> > 
> > This has been the most involved one, since legacy pipe commit still
> > handles a lot of the properties differently from the rest.
> > 
> > Changes since v1:
> > - Dump all changed properties on commit.
> > - Fix bug in igt_pipe_refresh().
> > Changes since v2:
> > - Set pipe ACTIVE property changed flag on init.
> > Changes since v3:
> > - Add a missing igt_pipe_refresh() to kms_atomic_interruptible.
> > Changes since v4:
> > - Perform error handling when setting custom crtc properties.
> > Changes since v5:
> > - Only attempt to commit changes properties.
> > Changes since v6:
> > - Clear OUT_FENCE_PTR on succesful commit.
> > 
> > Signed-off-by: Maarten Lankhorst 
> 
> I'm a bit late to the party on this one, but this commit broke the
> chamelium tests on vc4, with every kernel since at least 4.12.
> 
> This is the error message:
> http://code.bulix.org/32fw1l-293842
> 
> From the stack trace, it looks like the atomic commit was failing, and
> indeed it fails here:
> https://elixir.bootlin.com/linux/v4.16-rc4/source/drivers/gpu/drm/drm_atomic.c#L2319
> 
> with prop_id being 0 for some reason.
> 
> I had a look at that patch, but I can't see anything wrong with it. Do
> you have any ideas?

No idea tbh, I guess we need to start tracing where the igt library tries
to set property 0. Would probably be really good to catch that in the
libdrm atomic support (same with trying to set a prop on obj 0, neither
makes any sense at all).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v2 03/14] lib/igt_kms: Rework pipe properties to be more atomic, v7.

2018-03-05 Thread Maxime Ripard 
Hi,

On Thu, Oct 12, 2017 at 01:54:24PM +0200, Maarten Lankhorst wrote:
> In the future I want to allow tests to commit more properties,
> but for this to work I have to fix all properties to work better
> with atomic commit. Instead of special casing each
> property make a bitmask for all property changed flags, and try to
> commit all properties.
> 
> This has been the most involved one, since legacy pipe commit still
> handles a lot of the properties differently from the rest.
> 
> Changes since v1:
> - Dump all changed properties on commit.
> - Fix bug in igt_pipe_refresh().
> Changes since v2:
> - Set pipe ACTIVE property changed flag on init.
> Changes since v3:
> - Add a missing igt_pipe_refresh() to kms_atomic_interruptible.
> Changes since v4:
> - Perform error handling when setting custom crtc properties.
> Changes since v5:
> - Only attempt to commit changes properties.
> Changes since v6:
> - Clear OUT_FENCE_PTR on succesful commit.
> 
> Signed-off-by: Maarten Lankhorst 

I'm a bit late to the party on this one, but this commit broke the
chamelium tests on vc4, with every kernel since at least 4.12.

This is the error message:
http://code.bulix.org/32fw1l-293842

From the stack trace, it looks like the atomic commit was failing, and
indeed it fails here:
https://elixir.bootlin.com/linux/v4.16-rc4/source/drivers/gpu/drm/drm_atomic.c#L2319

with prop_id being 0 for some reason.

I had a look at that patch, but I can't see anything wrong with it. Do
you have any ideas?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v2 03/14] lib/igt_kms: Rework pipe properties to be more atomic, v7.

2017-10-19 Thread Mika Kahola 
Reviewed-by: Mika Kahola 

On Thu, 2017-10-12 at 13:54 +0200, Maarten Lankhorst wrote:
> In the future I want to allow tests to commit more properties,
> but for this to work I have to fix all properties to work better
> with atomic commit. Instead of special casing each
> property make a bitmask for all property changed flags, and try to
> commit all properties.
> 
> This has been the most involved one, since legacy pipe commit still
> handles a lot of the properties differently from the rest.
> 
> Changes since v1:
> - Dump all changed properties on commit.
> - Fix bug in igt_pipe_refresh().
> Changes since v2:
> - Set pipe ACTIVE property changed flag on init.
> Changes since v3:
> - Add a missing igt_pipe_refresh() to kms_atomic_interruptible.
> Changes since v4:
> - Perform error handling when setting custom crtc properties.
> Changes since v5:
> - Only attempt to commit changes properties.
> Changes since v6:
> - Clear OUT_FENCE_PTR on succesful commit.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  lib/igt_kms.c | 231 --
> 
>  lib/igt_kms.h |  77 +
>  tests/kms_atomic_interruptible.c  |   8 +-
>  tests/kms_atomic_transition.c |   2 +-
>  tests/kms_crtc_background_color.c |   2 +-
>  5 files changed, 159 insertions(+), 161 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e77ae5d696da..02de39b8fc7f 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -259,8 +259,8 @@ igt_atomic_fill_connector_props(igt_display_t
> *display, igt_output_t *output,
>  }
>  
>  static void
> -igt_atomic_fill_pipe_props(igt_display_t *display, igt_pipe_t *pipe,
> - int num_crtc_props, const char
> **crtc_prop_names)
> +igt_fill_pipe_props(igt_display_t *display, igt_pipe_t *pipe,
> + int num_crtc_props, const char
> **crtc_prop_names)
>  {
>   drmModeObjectPropertiesPtr props;
>   int i, j, fd;
> @@ -278,7 +278,7 @@ igt_atomic_fill_pipe_props(igt_display_t
> *display, igt_pipe_t *pipe,
>   if (strcmp(prop->name, crtc_prop_names[j])
> != 0)
>   continue;
>  
> - pipe->atomic_props_crtc[j] = props-
> >props[i];
> + pipe->props[j] = props->props[i];
>   break;
>   }
>  
> @@ -1620,13 +1620,6 @@ get_crtc_property(int drm_fd, uint32_t
> crtc_id, const char *name,
>   name, prop_id, value, prop);
>  }
>  
> -static void
> -igt_crtc_set_property(igt_pipe_t *pipe, uint32_t prop_id, uint64_t
> value)
> -{
> - drmModeObjectSetProperty(pipe->display->drm_fd,
> - pipe->crtc_id, DRM_MODE_OBJECT_CRTC, prop_id,
> value);
> -}
> -
>  /*
>   * Walk a plane's property list to determine its type.  If we don't
>   * find a type property, then the kernel doesn't support universal
> @@ -1690,7 +1683,6 @@ void igt_display_init(igt_display_t *display,
> int drm_fd)
>   int p = 1;
>   int j, type;
>   uint8_t last_plane = 0, n_planes = 0;
> - uint64_t prop_value;
>  
>   pipe->crtc_id = resources->crtcs[i];
>   pipe->display = display;
> @@ -1700,29 +1692,16 @@ void igt_display_init(igt_display_t *display,
> int drm_fd)
>   pipe->planes = NULL;
>   pipe->out_fence_fd = -1;
>  
> + igt_fill_pipe_props(display, pipe,
> IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
> +
> + /* Force modeset disable on first commit */
> + igt_pipe_obj_set_prop_changed(pipe,
> IGT_CRTC_MODE_ID);
> + igt_pipe_obj_set_prop_changed(pipe,
> IGT_CRTC_ACTIVE);
> +
>   get_crtc_property(display->drm_fd, pipe->crtc_id,
> - "background_color",
> - >background_property,
> - _value,
> + "background_color", NULL,
> + 
> >values[IGT_CRTC_BACKGROUND],
>   NULL);
> - pipe->background = (uint32_t)prop_value;
> - get_crtc_property(display->drm_fd, pipe->crtc_id,
> -   "DEGAMMA_LUT",
> -   >degamma_property,
> -   NULL,
> -   NULL);
> - get_crtc_property(display->drm_fd, pipe->crtc_id,
> -   "CTM",
> -   >ctm_property,
> -   NULL,
> -   NULL);
> - get_crtc_property(display->drm_fd, pipe->crtc_id,
> -   "GAMMA_LUT",
> -   >gamma_property,
> -   NULL,
> -   NULL);
> -
> -

[Intel-gfx] [PATCH i-g-t v2 03/14] lib/igt_kms: Rework pipe properties to be more atomic, v7.

2017-10-12 Thread Maarten Lankhorst 
In the future I want to allow tests to commit more properties,
but for this to work I have to fix all properties to work better
with atomic commit. Instead of special casing each
property make a bitmask for all property changed flags, and try to
commit all properties.

This has been the most involved one, since legacy pipe commit still
handles a lot of the properties differently from the rest.

Changes since v1:
- Dump all changed properties on commit.
- Fix bug in igt_pipe_refresh().
Changes since v2:
- Set pipe ACTIVE property changed flag on init.
Changes since v3:
- Add a missing igt_pipe_refresh() to kms_atomic_interruptible.
Changes since v4:
- Perform error handling when setting custom crtc properties.
Changes since v5:
- Only attempt to commit changes properties.
Changes since v6:
- Clear OUT_FENCE_PTR on succesful commit.

Signed-off-by: Maarten Lankhorst 
---
 lib/igt_kms.c | 231 --
 lib/igt_kms.h |  77 +
 tests/kms_atomic_interruptible.c  |   8 +-
 tests/kms_atomic_transition.c |   2 +-
 tests/kms_crtc_background_color.c |   2 +-
 5 files changed, 159 insertions(+), 161 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index e77ae5d696da..02de39b8fc7f 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -259,8 +259,8 @@ igt_atomic_fill_connector_props(igt_display_t *display, 
igt_output_t *output,
 }
 
 static void
-igt_atomic_fill_pipe_props(igt_display_t *display, igt_pipe_t *pipe,
-   int num_crtc_props, const char **crtc_prop_names)
+igt_fill_pipe_props(igt_display_t *display, igt_pipe_t *pipe,
+   int num_crtc_props, const char **crtc_prop_names)
 {
drmModeObjectPropertiesPtr props;
int i, j, fd;
@@ -278,7 +278,7 @@ igt_atomic_fill_pipe_props(igt_display_t *display, 
igt_pipe_t *pipe,
if (strcmp(prop->name, crtc_prop_names[j]) != 0)
continue;
 
-   pipe->atomic_props_crtc[j] = props->props[i];
+   pipe->props[j] = props->props[i];
break;
}
 
@@ -1620,13 +1620,6 @@ get_crtc_property(int drm_fd, uint32_t crtc_id, const 
char *name,
name, prop_id, value, prop);
 }
 
-static void
-igt_crtc_set_property(igt_pipe_t *pipe, uint32_t prop_id, uint64_t value)
-{
-   drmModeObjectSetProperty(pipe->display->drm_fd,
-   pipe->crtc_id, DRM_MODE_OBJECT_CRTC, prop_id, value);
-}
-
 /*
  * Walk a plane's property list to determine its type.  If we don't
  * find a type property, then the kernel doesn't support universal
@@ -1690,7 +1683,6 @@ void igt_display_init(igt_display_t *display, int drm_fd)
int p = 1;
int j, type;
uint8_t last_plane = 0, n_planes = 0;
-   uint64_t prop_value;
 
pipe->crtc_id = resources->crtcs[i];
pipe->display = display;
@@ -1700,29 +1692,16 @@ void igt_display_init(igt_display_t *display, int 
drm_fd)
pipe->planes = NULL;
pipe->out_fence_fd = -1;
 
+   igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, 
igt_crtc_prop_names);
+
+   /* Force modeset disable on first commit */
+   igt_pipe_obj_set_prop_changed(pipe, IGT_CRTC_MODE_ID);
+   igt_pipe_obj_set_prop_changed(pipe, IGT_CRTC_ACTIVE);
+
get_crtc_property(display->drm_fd, pipe->crtc_id,
-   "background_color",
-   >background_property,
-   _value,
+   "background_color", NULL,
+   >values[IGT_CRTC_BACKGROUND],
NULL);
-   pipe->background = (uint32_t)prop_value;
-   get_crtc_property(display->drm_fd, pipe->crtc_id,
- "DEGAMMA_LUT",
- >degamma_property,
- NULL,
- NULL);
-   get_crtc_property(display->drm_fd, pipe->crtc_id,
- "CTM",
- >ctm_property,
- NULL,
- NULL);
-   get_crtc_property(display->drm_fd, pipe->crtc_id,
- "GAMMA_LUT",
- >gamma_property,
- NULL,
- NULL);
-
-   igt_atomic_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, 
igt_crtc_prop_names);
 
/* count number of valid planes */
for (j = 0; j < plane_resources->count_planes; j++) {
@@ -1813,8 +1792,6 @@ void igt_display_init(igt_display_t *display, int