Re: [Intel-gfx] [PATCH i-g-t v2 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

2016-12-16 Thread Brian Starkey

On Fri, Dec 16, 2016 at 03:21:45AM -0500, Robert Foss wrote:



On 2016-12-14 11:13 AM, Brian Starkey wrote:

Hi,

On Wed, Dec 14, 2016 at 04:05:04AM -0500, Robert Foss wrote:

From: Gustavo Padovan 

Add support for the OUT_FENCE_PTR property to enable setting out
fences for
atomic commits.

Signed-off-by: Gustavo Padovan 
Signed-off-by: Robert Foss 
---
lib/igt_kms.c | 21 -
lib/igt_kms.h |  3 +++
2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 8ca49d86..fe1b356d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -175,7 +175,8 @@ const char
*igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
   "DEGAMMA_LUT",
   "GAMMA_LUT",
   "MODE_ID",
-"ACTIVE"
+"ACTIVE",
+"OUT_FENCE_PTR"
};

const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
@@ -2103,6 +2104,10 @@ static void
igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
   igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE,
!!output);
   }

+if (pipe_obj->out_fence_ptr)
+igt_atomic_populate_crtc_req(req, pipe_obj,
IGT_CRTC_OUT_FENCE_PTR,
+(uint64_t)(uintptr_t) pipe_obj->out_fence_ptr);
+
   /*
*TODO: Add all crtc level properties here
*/
@@ -2683,6 +2688,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void
*ptr, size_t length)
}

/**
+ * igt_pipe_set_out_fence_ptr:
+ * @pipe: pipe pointer to which background color to be set
+ * @fence_ptr: out fence pointer
+ *
+ * Sets the out fence pointer that will be passed to the kernel in
+ * the atomic ioctl. When the kernel returns the out fence pointer
+ * will contain the fd number of the out fence created by KMS.
+ */
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int64_t *fence_ptr)
+{
+pipe->out_fence_ptr = fence_ptr;
+}
+
+/**
* igt_crtc_set_background:
* @pipe: pipe pointer to which background color to be set
* @background: background color value in BGR 16bpc
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 9766807c..8eb611c0 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
  IGT_CRTC_GAMMA_LUT,
  IGT_CRTC_MODE_ID,
  IGT_CRTC_ACTIVE,
+   IGT_CRTC_OUT_FENCE_PTR,
  IGT_NUM_CRTC_PROPS
};

@@ -316,6 +317,7 @@ struct igt_pipe {

   uint64_t mode_blob;
   bool mode_changed;
+int64_t *out_fence_ptr;


I prefer the interface that got suggested before - igt_pipe gets an
"int64_t out_fence;" member which tests can query to get the fence
value:

int igt_pipe_get_last_out_fence(igt_pipe_t *pipe);

..and the kernel only ever receives a pointer to pipe->out_fence.

The only reason I can see for a test to want to provide its own fence
pointer is to test invalid pointer values - and I think it's OK for
that test to set the property directly instead of making setting a
custom fence pointer the common case for all users.

If we don't want to get a fence for every commit then I guess there
could be a way for a test to request an out-fence for just the next
commit on a pipe (or the inverse - disable fencing for a particular
commit):

void igt_pipe_request_out_fence(igt_pipe_t *pipe);

-Brian


Now I see what you meant in the v1 discussion, I'll amend the 
implementation in v3 to be the one mentioned above.




Partly... my main point on v1 was just to make sure the pointer was to
a 64-bit type.

The only question I have is about igt_pipe_get_last_out_fence(), is 
it really necessary? I don't foresee a function like that ever doing 
more than just returning a struct member. Is it not a bit redundant?




I see two reasons for it:
 - It means tests only deal with plain ints, which I think Chris was
   fairly adamant about on v1.
 - The getter can set pipe->out_fence to -1 when its called, making
   the ownership of the fd obvious.

In this case I guess before each commit out_fence needs to be checked,
close()'d if it looks valid, and set to -1 too.

Cheers,
Brian



Rob.




};

typedef struct {
@@ -369,6 +371,7 @@ static inline bool
igt_plane_supports_rotation(igt_plane_t *plane)
void igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t
length);
void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length);
void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int64_t *fence_ptr);

void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd);
--
2.11.0


___
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 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

2016-12-16 Thread Robert Foss



On 2016-12-14 11:13 AM, Brian Starkey wrote:

Hi,

On Wed, Dec 14, 2016 at 04:05:04AM -0500, Robert Foss wrote:

From: Gustavo Padovan 

Add support for the OUT_FENCE_PTR property to enable setting out
fences for
atomic commits.

Signed-off-by: Gustavo Padovan 
Signed-off-by: Robert Foss 
---
lib/igt_kms.c | 21 -
lib/igt_kms.h |  3 +++
2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 8ca49d86..fe1b356d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -175,7 +175,8 @@ const char
*igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
"DEGAMMA_LUT",
"GAMMA_LUT",
"MODE_ID",
-"ACTIVE"
+"ACTIVE",
+"OUT_FENCE_PTR"
};

const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
@@ -2103,6 +2104,10 @@ static void
igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE,
!!output);
}

+if (pipe_obj->out_fence_ptr)
+igt_atomic_populate_crtc_req(req, pipe_obj,
IGT_CRTC_OUT_FENCE_PTR,
+(uint64_t)(uintptr_t) pipe_obj->out_fence_ptr);
+
/*
 *TODO: Add all crtc level properties here
 */
@@ -2683,6 +2688,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void
*ptr, size_t length)
}

/**
+ * igt_pipe_set_out_fence_ptr:
+ * @pipe: pipe pointer to which background color to be set
+ * @fence_ptr: out fence pointer
+ *
+ * Sets the out fence pointer that will be passed to the kernel in
+ * the atomic ioctl. When the kernel returns the out fence pointer
+ * will contain the fd number of the out fence created by KMS.
+ */
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int64_t *fence_ptr)
+{
+pipe->out_fence_ptr = fence_ptr;
+}
+
+/**
 * igt_crtc_set_background:
 * @pipe: pipe pointer to which background color to be set
 * @background: background color value in BGR 16bpc
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 9766807c..8eb611c0 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
   IGT_CRTC_GAMMA_LUT,
   IGT_CRTC_MODE_ID,
   IGT_CRTC_ACTIVE,
+   IGT_CRTC_OUT_FENCE_PTR,
   IGT_NUM_CRTC_PROPS
};

@@ -316,6 +317,7 @@ struct igt_pipe {

uint64_t mode_blob;
bool mode_changed;
+int64_t *out_fence_ptr;


I prefer the interface that got suggested before - igt_pipe gets an
"int64_t out_fence;" member which tests can query to get the fence
value:

int igt_pipe_get_last_out_fence(igt_pipe_t *pipe);

..and the kernel only ever receives a pointer to pipe->out_fence.

The only reason I can see for a test to want to provide its own fence
pointer is to test invalid pointer values - and I think it's OK for
that test to set the property directly instead of making setting a
custom fence pointer the common case for all users.

If we don't want to get a fence for every commit then I guess there
could be a way for a test to request an out-fence for just the next
commit on a pipe (or the inverse - disable fencing for a particular
commit):

void igt_pipe_request_out_fence(igt_pipe_t *pipe);

-Brian


Now I see what you meant in the v1 discussion, I'll amend the 
implementation in v3 to be the one mentioned above.


The only question I have is about igt_pipe_get_last_out_fence(), is it 
really necessary? I don't foresee a function like that ever doing more 
than just returning a struct member. Is it not a bit redundant?



Rob.




};

typedef struct {
@@ -369,6 +371,7 @@ static inline bool
igt_plane_supports_rotation(igt_plane_t *plane)
void igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t
length);
void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length);
void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int64_t *fence_ptr);

void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd);
--
2.11.0


___
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 07/12] lib/igt_kms: Add support for the OUT_FENCE_PTR property

2016-12-14 Thread Brian Starkey

Hi,

On Wed, Dec 14, 2016 at 04:05:04AM -0500, Robert Foss wrote:

From: Gustavo Padovan 

Add support for the OUT_FENCE_PTR property to enable setting out fences for
atomic commits.

Signed-off-by: Gustavo Padovan 
Signed-off-by: Robert Foss 
---
lib/igt_kms.c | 21 -
lib/igt_kms.h |  3 +++
2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 8ca49d86..fe1b356d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -175,7 +175,8 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
"DEGAMMA_LUT",
"GAMMA_LUT",
"MODE_ID",
-   "ACTIVE"
+   "ACTIVE",
+   "OUT_FENCE_PTR"
};

const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
@@ -2103,6 +2104,10 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t 
*pipe_obj, drmModeAtomicRe
igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_ACTIVE, 
!!output);
}

+   if (pipe_obj->out_fence_ptr)
+   igt_atomic_populate_crtc_req(req, pipe_obj, 
IGT_CRTC_OUT_FENCE_PTR,
+   (uint64_t)(uintptr_t) pipe_obj->out_fence_ptr);
+
/*
 *  TODO: Add all crtc level properties here
 */
@@ -2683,6 +2688,20 @@ igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, 
size_t length)
}

/**
+ * igt_pipe_set_out_fence_ptr:
+ * @pipe: pipe pointer to which background color to be set
+ * @fence_ptr: out fence pointer
+ *
+ * Sets the out fence pointer that will be passed to the kernel in
+ * the atomic ioctl. When the kernel returns the out fence pointer
+ * will contain the fd number of the out fence created by KMS.
+ */
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int64_t *fence_ptr)
+{
+   pipe->out_fence_ptr = fence_ptr;
+}
+
+/**
 * igt_crtc_set_background:
 * @pipe: pipe pointer to which background color to be set
 * @background: background color value in BGR 16bpc
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 9766807c..8eb611c0 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -110,6 +110,7 @@ enum igt_atomic_crtc_properties {
   IGT_CRTC_GAMMA_LUT,
   IGT_CRTC_MODE_ID,
   IGT_CRTC_ACTIVE,
+   IGT_CRTC_OUT_FENCE_PTR,
   IGT_NUM_CRTC_PROPS
};

@@ -316,6 +317,7 @@ struct igt_pipe {

uint64_t mode_blob;
bool mode_changed;
+   int64_t *out_fence_ptr;


I prefer the interface that got suggested before - igt_pipe gets an
"int64_t out_fence;" member which tests can query to get the fence
value:

int igt_pipe_get_last_out_fence(igt_pipe_t *pipe);

..and the kernel only ever receives a pointer to pipe->out_fence.

The only reason I can see for a test to want to provide its own fence
pointer is to test invalid pointer values - and I think it's OK for
that test to set the property directly instead of making setting a
custom fence pointer the common case for all users.

If we don't want to get a fence for every commit then I guess there
could be a way for a test to request an out-fence for just the next
commit on a pipe (or the inverse - disable fencing for a particular
commit):

void igt_pipe_request_out_fence(igt_pipe_t *pipe);

-Brian


};

typedef struct {
@@ -369,6 +371,7 @@ static inline bool igt_plane_supports_rotation(igt_plane_t 
*plane)
void igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
void igt_pipe_set_ctm_matrix(igt_pipe_t *pipe, void *ptr, size_t length);
void igt_pipe_set_gamma_lut(igt_pipe_t *pipe, void *ptr, size_t length);
+void igt_pipe_set_out_fence_ptr(igt_pipe_t *pipe, int64_t *fence_ptr);

void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
void igt_plane_set_fence_fd(igt_plane_t *plane, uint32_t fence_fd);
--
2.11.0


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