| Message ID | 1479117566-17709-10-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gustavo, A little late to the party here, but I was blocked by our internal contributions process... I didn't see these end up in my checkout yet though, so I guess they aren't picked up yet. On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >Add support for the OUT_FENCE_PTR property to enable setting out fences for >atomic commits. > >Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >--- > lib/igt_kms.c | 20 +++++++++++++++++++- > lib/igt_kms.h | 3 +++ > 2 files changed, 22 insertions(+), 1 deletion(-) > >diff --git a/lib/igt_kms.c b/lib/igt_kms.c >index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); >+ > /* > * TODO: Add all crtc level properties here > */ >@@ -2683,6 +2687,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 I don't think fence_ptr can be int *. It needs to be a pointer to a 64-bit type. >+ * >+ * 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, int *fence_ptr) >+{ >+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 > }; > >@@ -298,6 +299,7 @@ struct igt_pipe { > > uint64_t mode_blob; > bool mode_changed; >+ uint64_t out_fence_ptr; IMO this should be: int64_t *out_fence_ptr; That way there can be no confusion about the type. You can convert it to u64 just before giving it to the kernel. Thanks, -Brian > }; > > typedef struct { >@@ -351,6 +353,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, int *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.5.5 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: > Hi Gustavo, > > A little late to the party here, but I was blocked by our internal > contributions process... > > I didn't see these end up in my checkout yet though, so I guess they > aren't picked up yet. > > On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: > >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > >Add support for the OUT_FENCE_PTR property to enable setting out fences for > >atomic commits. > > > >Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >--- > >lib/igt_kms.c | 20 +++++++++++++++++++- > >lib/igt_kms.h | 3 +++ > >2 files changed, 22 insertions(+), 1 deletion(-) > > > >diff --git a/lib/igt_kms.c b/lib/igt_kms.c > >index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); > >+ > > /* > > * TODO: Add all crtc level properties here > > */ > >@@ -2683,6 +2687,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 > > I don't think fence_ptr can be int *. It needs to be a pointer to a > 64-bit type. > > >+ * > >+ * 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, int *fence_ptr) > >+{ > >+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 > >}; > > > >@@ -298,6 +299,7 @@ struct igt_pipe { > > > > uint64_t mode_blob; > > bool mode_changed; > >+ uint64_t out_fence_ptr; > > IMO this should be: > > int64_t *out_fence_ptr; In userspace, fences are *fd*, a plain int. It is only the uabi that we pass pointers as u64 to the kernel, and indeed that should be limited to the uabi wrapper. -Chris
On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote: >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: >> Hi Gustavo, >> >> A little late to the party here, but I was blocked by our internal >> contributions process... >> >> I didn't see these end up in my checkout yet though, so I guess they >> aren't picked up yet. >> >> On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: >> >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > >> >Add support for the OUT_FENCE_PTR property to enable setting out fences for >> >atomic commits. >> > >> >Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> >--- >> >lib/igt_kms.c | 20 +++++++++++++++++++- >> >lib/igt_kms.h | 3 +++ >> >2 files changed, 22 insertions(+), 1 deletion(-) >> > >> >diff --git a/lib/igt_kms.c b/lib/igt_kms.c >> >index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); >> >+ >> > /* >> > * TODO: Add all crtc level properties here >> > */ >> >@@ -2683,6 +2687,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 >> >> I don't think fence_ptr can be int *. It needs to be a pointer to a >> 64-bit type. >> >> >+ * >> >+ * 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, int *fence_ptr) >> >+{ >> >+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 >> >}; >> > >> >@@ -298,6 +299,7 @@ struct igt_pipe { >> > >> > uint64_t mode_blob; >> > bool mode_changed; >> >+ uint64_t out_fence_ptr; >> >> IMO this should be: >> >> int64_t *out_fence_ptr; > >In userspace, fences are *fd*, a plain int. It is only the uabi that we >pass pointers as u64 to the kernel, and indeed that should be limited to >the uabi wrapper. >-Chris Where's the uabi wrapper in this case? Wherever it is, afaik someone needs to have 64-bit type for the kernel to stash its fd in - on the kernel side out_fence_ptr is (s64 __user *), so if there's not a 64-bit variable on the other end of it then someone's going to have a bad day. -Brian > >-- >Chris Wilson, Intel Open Source Technology Centre
On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote: > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote: > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: > >>Hi Gustavo, > >> > >>A little late to the party here, but I was blocked by our internal > >>contributions process... > >> > >>I didn't see these end up in my checkout yet though, so I guess they > >>aren't picked up yet. > >> > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for > >>>atomic commits. > >>> > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>>--- > >>>lib/igt_kms.c | 20 +++++++++++++++++++- > >>>lib/igt_kms.h | 3 +++ > >>>2 files changed, 22 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c > >>>index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); > >>>+ > >>> /* > >>> * TODO: Add all crtc level properties here > >>> */ > >>>@@ -2683,6 +2687,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 > >> > >>I don't think fence_ptr can be int *. It needs to be a pointer to a > >>64-bit type. > >> > >>>+ * > >>>+ * 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, int *fence_ptr) > >>>+{ > >>>+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 > >>>}; > >>> > >>>@@ -298,6 +299,7 @@ struct igt_pipe { > >>> > >>> uint64_t mode_blob; > >>> bool mode_changed; > >>>+ uint64_t out_fence_ptr; > >> > >>IMO this should be: > >> > >> int64_t *out_fence_ptr; > > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we > >pass pointers as u64 to the kernel, and indeed that should be limited to > >the uabi wrapper. > >-Chris > > Where's the uabi wrapper in this case? > > Wherever it is, afaik someone needs to have 64-bit type for the kernel > to stash its fd in - on the kernel side out_fence_ptr is > (s64 __user *), so if there's not a 64-bit variable on the other end > of it then someone's going to have a bad day. We do not have pointers in the uabi because they are different sizes on different platforms. The uabi must be a u64 representation of a user address to store the result - that is what we pass to the crtc set property ioctl. That it has been futher managled not to pass around fd is an interesting twist, but ideally that sillyness should not make itself into our API. -Chris
On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote: >On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote: >> On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote: >> >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: >> >>Hi Gustavo, >> >> >> >>A little late to the party here, but I was blocked by our internal >> >>contributions process... >> >> >> >>I didn't see these end up in my checkout yet though, so I guess they >> >>aren't picked up yet. >> >> >> >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: >> >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> >>> >> >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for >> >>>atomic commits. >> >>> >> >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> >>>--- >> >>>lib/igt_kms.c | 20 +++++++++++++++++++- >> >>>lib/igt_kms.h | 3 +++ >> >>>2 files changed, 22 insertions(+), 1 deletion(-) >> >>> >> >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c >> >>>index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); >> >>>+ >> >>> /* >> >>> * TODO: Add all crtc level properties here >> >>> */ >> >>>@@ -2683,6 +2687,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 >> >> >> >>I don't think fence_ptr can be int *. It needs to be a pointer to a >> >>64-bit type. >> >> >> >>>+ * >> >>>+ * 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, int *fence_ptr) >> >>>+{ >> >>>+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 >> >>>}; >> >>> >> >>>@@ -298,6 +299,7 @@ struct igt_pipe { >> >>> >> >>> uint64_t mode_blob; >> >>> bool mode_changed; >> >>>+ uint64_t out_fence_ptr; >> >> >> >>IMO this should be: >> >> >> >> int64_t *out_fence_ptr; >> > >> >In userspace, fences are *fd*, a plain int. It is only the uabi that we >> >pass pointers as u64 to the kernel, and indeed that should be limited to >> >the uabi wrapper. >> >-Chris >> >> Where's the uabi wrapper in this case? >> >> Wherever it is, afaik someone needs to have 64-bit type for the kernel >> to stash its fd in - on the kernel side out_fence_ptr is >> (s64 __user *), so if there's not a 64-bit variable on the other end >> of it then someone's going to have a bad day. > >We do not have pointers in the uabi because they are different sizes on >different platforms. The uabi must be a u64 representation of a user >address to store the result - that is what we pass to the crtc set >property ioctl. Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here we're making it needlessly opaque what the value is actually meant to be - which is the address of a 64-bit signed integer. Regardless, tests cannot set out_fence_ptr to the address of an int, I hope we can agree on that. Where that detail gets taken care of I don't much mind - but this code as-is is incorrect. By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be letting the compiler warn anyone else away from incorrect code. >That it has been futher managled not to pass around fd >is an interesting twist, but ideally that sillyness should not make >itself into our API. Allowing the kernel and userspace to have different ideas about how big an int is doesn't sound so silly to me. It may not be a theoretical problem forever. -Brian >-Chris > >-- >Chris Wilson, Intel Open Source Technology Centre
On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote: > On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote: > > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote: > > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote: > > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: > > > >>Hi Gustavo, > > > >> > > > >>A little late to the party here, but I was blocked by our internal > > > >>contributions process... > > > >> > > > >>I didn't see these end up in my checkout yet though, so I guess they > > > >>aren't picked up yet. > > > >> > > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: > > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > >>> > > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for > > > >>>atomic commits. > > > >>> > > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > >>>--- > > > >>>lib/igt_kms.c | 20 +++++++++++++++++++- > > > >>>lib/igt_kms.h | 3 +++ > > > >>>2 files changed, 22 insertions(+), 1 deletion(-) > > > >>> > > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c > > > >>>index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); > > > >>>+ > > > >>> /* > > > >>> * TODO: Add all crtc level properties here > > > >>> */ > > > >>>@@ -2683,6 +2687,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 > > > >> > > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a > > > >>64-bit type. > > > >> > > > >>>+ * > > > >>>+ * 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, int *fence_ptr) > > > >>>+{ > > > >>>+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 > > > >>>}; > > > >>> > > > >>>@@ -298,6 +299,7 @@ struct igt_pipe { > > > >>> > > > >>> uint64_t mode_blob; > > > >>> bool mode_changed; > > > >>>+ uint64_t out_fence_ptr; > > > >> > > > >>IMO this should be: > > > >> > > > >> int64_t *out_fence_ptr; > > > > > > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we > > > >pass pointers as u64 to the kernel, and indeed that should be limited to > > > >the uabi wrapper. > > > >-Chris > > > > > > Where's the uabi wrapper in this case? > > > > > > Wherever it is, afaik someone needs to have 64-bit type for the kernel > > > to stash its fd in - on the kernel side out_fence_ptr is > > > (s64 __user *), so if there's not a 64-bit variable on the other end > > > of it then someone's going to have a bad day. > > > > We do not have pointers in the uabi because they are different sizes on > > different platforms. The uabi must be a u64 representation of a user > > address to store the result - that is what we pass to the crtc set > > property ioctl. > > Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here > we're making it needlessly opaque what the value is actually meant to > be - which is the address of a 64-bit signed integer. > > Regardless, tests cannot set out_fence_ptr to the address of an int, I > hope we can agree on that. Where that detail gets taken care of I > don't much mind - but this code as-is is incorrect. > > By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be > letting the compiler warn anyone else away from incorrect code. > > > That it has been futher managled not to pass around fd > > is an interesting twist, but ideally that sillyness should not make > > itself into our API. > > Allowing the kernel and userspace to have different ideas about how > big an int is doesn't sound so silly to me. It may not be a > theoretical problem forever. What Chris means is that you want to have an int out_fence in igt_pipe, and just pass the address of that into the OUT_FENCE_PTR property. In userspace we want to directly handle the fd, not a pointer to an fd. Like Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse the atomic ioctl as transport, it's not a reasonable interface within userspace. -Daniel
On Tue, Nov 22, 2016 at 02:12:59PM +0100, Daniel Vetter wrote: >On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote: >> On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote: >> > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote: >> > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote: >> > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: >> > > >>Hi Gustavo, >> > > >> >> > > >>A little late to the party here, but I was blocked by our internal >> > > >>contributions process... >> > > >> >> > > >>I didn't see these end up in my checkout yet though, so I guess they >> > > >>aren't picked up yet. >> > > >> >> > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: >> > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > > >>> >> > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for >> > > >>>atomic commits. >> > > >>> >> > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > > >>>--- >> > > >>>lib/igt_kms.c | 20 +++++++++++++++++++- >> > > >>>lib/igt_kms.h | 3 +++ >> > > >>>2 files changed, 22 insertions(+), 1 deletion(-) >> > > >>> >> > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c >> > > >>>index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); >> > > >>>+ >> > > >>> /* >> > > >>> * TODO: Add all crtc level properties here >> > > >>> */ >> > > >>>@@ -2683,6 +2687,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 >> > > >> >> > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a >> > > >>64-bit type. >> > > >> >> > > >>>+ * >> > > >>>+ * 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, int *fence_ptr) >> > > >>>+{ >> > > >>>+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 >> > > >>>}; >> > > >>> >> > > >>>@@ -298,6 +299,7 @@ struct igt_pipe { >> > > >>> >> > > >>> uint64_t mode_blob; >> > > >>> bool mode_changed; >> > > >>>+ uint64_t out_fence_ptr; >> > > >> >> > > >>IMO this should be: >> > > >> >> > > >> int64_t *out_fence_ptr; >> > > > >> > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we >> > > >pass pointers as u64 to the kernel, and indeed that should be limited to >> > > >the uabi wrapper. >> > > >-Chris >> > > >> > > Where's the uabi wrapper in this case? >> > > >> > > Wherever it is, afaik someone needs to have 64-bit type for the kernel >> > > to stash its fd in - on the kernel side out_fence_ptr is >> > > (s64 __user *), so if there's not a 64-bit variable on the other end >> > > of it then someone's going to have a bad day. >> > >> > We do not have pointers in the uabi because they are different sizes on >> > different platforms. The uabi must be a u64 representation of a user >> > address to store the result - that is what we pass to the crtc set >> > property ioctl. >> >> Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here >> we're making it needlessly opaque what the value is actually meant to >> be - which is the address of a 64-bit signed integer. >> >> Regardless, tests cannot set out_fence_ptr to the address of an int, I >> hope we can agree on that. Where that detail gets taken care of I >> don't much mind - but this code as-is is incorrect. >> >> By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be >> letting the compiler warn anyone else away from incorrect code. >> >> > That it has been futher managled not to pass around fd >> > is an interesting twist, but ideally that sillyness should not make >> > itself into our API. >> >> Allowing the kernel and userspace to have different ideas about how >> big an int is doesn't sound so silly to me. It may not be a >> theoretical problem forever. > >What Chris means is that you want to have an int out_fence in igt_pipe, >and just pass the address of that into the OUT_FENCE_PTR property. Storing the fence itself in igt_pipe instead of a pointer to it is a different matter (and it isn't what's implemented in this patch). It still doesn't change the fact that you can't do as you suggest - you cannot just pass the address of an int in the OUT_FENCE_PTR property. In the kernel, put_user(fd, fence_state[i].out_fence_ptr); is going to write 8 bytes. If out_fence_ptr is the address of a 4-byte variable, then obviously that's not going to work out so well. >In >userspace we want to directly handle the fd, not a pointer to an fd. Like >Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse >the atomic ioctl as transport, it's not a reasonable interface within >userspace. I don't really follow this bit. At some point, something in userspace is going to need to take care of the fact that the kernel needs to have an 8-byte container to write into. -Brian >-Daniel >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
On Tue, Nov 22, 2016 at 01:50:53PM +0000, Brian Starkey wrote: > On Tue, Nov 22, 2016 at 02:12:59PM +0100, Daniel Vetter wrote: > > On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote: > > > On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote: > > > > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote: > > > > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote: > > > > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: > > > > > >>Hi Gustavo, > > > > > >> > > > > > >>A little late to the party here, but I was blocked by our internal > > > > > >>contributions process... > > > > > >> > > > > > >>I didn't see these end up in my checkout yet though, so I guess they > > > > > >>aren't picked up yet. > > > > > >> > > > > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: > > > > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > >>> > > > > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for > > > > > >>>atomic commits. > > > > > >>> > > > > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > >>>--- > > > > > >>>lib/igt_kms.c | 20 +++++++++++++++++++- > > > > > >>>lib/igt_kms.h | 3 +++ > > > > > >>>2 files changed, 22 insertions(+), 1 deletion(-) > > > > > >>> > > > > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c > > > > > >>>index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); > > > > > >>>+ > > > > > >>> /* > > > > > >>> * TODO: Add all crtc level properties here > > > > > >>> */ > > > > > >>>@@ -2683,6 +2687,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 > > > > > >> > > > > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a > > > > > >>64-bit type. > > > > > >> > > > > > >>>+ * > > > > > >>>+ * 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, int *fence_ptr) > > > > > >>>+{ > > > > > >>>+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 > > > > > >>>}; > > > > > >>> > > > > > >>>@@ -298,6 +299,7 @@ struct igt_pipe { > > > > > >>> > > > > > >>> uint64_t mode_blob; > > > > > >>> bool mode_changed; > > > > > >>>+ uint64_t out_fence_ptr; > > > > > >> > > > > > >>IMO this should be: > > > > > >> > > > > > >> int64_t *out_fence_ptr; > > > > > > > > > > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we > > > > > >pass pointers as u64 to the kernel, and indeed that should be limited to > > > > > >the uabi wrapper. > > > > > >-Chris > > > > > > > > > > Where's the uabi wrapper in this case? > > > > > > > > > > Wherever it is, afaik someone needs to have 64-bit type for the kernel > > > > > to stash its fd in - on the kernel side out_fence_ptr is > > > > > (s64 __user *), so if there's not a 64-bit variable on the other end > > > > > of it then someone's going to have a bad day. > > > > > > > > We do not have pointers in the uabi because they are different sizes on > > > > different platforms. The uabi must be a u64 representation of a user > > > > address to store the result - that is what we pass to the crtc set > > > > property ioctl. > > > > > > Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here > > > we're making it needlessly opaque what the value is actually meant to > > > be - which is the address of a 64-bit signed integer. > > > > > > Regardless, tests cannot set out_fence_ptr to the address of an int, I > > > hope we can agree on that. Where that detail gets taken care of I > > > don't much mind - but this code as-is is incorrect. > > > > > > By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be > > > letting the compiler warn anyone else away from incorrect code. > > > > > > > That it has been futher managled not to pass around fd > > > > is an interesting twist, but ideally that sillyness should not make > > > > itself into our API. > > > > > > Allowing the kernel and userspace to have different ideas about how > > > big an int is doesn't sound so silly to me. It may not be a > > > theoretical problem forever. > > > > What Chris means is that you want to have an int out_fence in igt_pipe, > > and just pass the address of that into the OUT_FENCE_PTR property. > > Storing the fence itself in igt_pipe instead of a pointer to it is a > different matter (and it isn't what's implemented in this patch). > > It still doesn't change the fact that you can't do as you suggest - > you cannot just pass the address of an int in the OUT_FENCE_PTR > property. > > In the kernel, put_user(fd, fence_state[i].out_fence_ptr); is going to > write 8 bytes. If out_fence_ptr is the address of a 4-byte variable, > then obviously that's not going to work out so well. Oh right, s/int/int64_t/, but otherwise that's imo what we should do. > > In > > userspace we want to directly handle the fd, not a pointer to an fd. Like > > Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse > > the atomic ioctl as transport, it's not a reasonable interface within > > userspace. > > I don't really follow this bit. At some point, something in userspace > is going to need to take care of the fact that the kernel needs to > have an 8-byte container to write into. Hm, why can't we allow igt test to directly access igt_pipe->out_fence? And switch your function to igt_pipe_request_out_fence? -Daniel
On Tue, Nov 22, 2016 at 02:56:49PM +0100, Daniel Vetter wrote: >On Tue, Nov 22, 2016 at 01:50:53PM +0000, Brian Starkey wrote: >> On Tue, Nov 22, 2016 at 02:12:59PM +0100, Daniel Vetter wrote: >> > On Tue, Nov 22, 2016 at 12:37:47PM +0000, Brian Starkey wrote: >> > > On Tue, Nov 22, 2016 at 12:10:52PM +0000, Chris Wilson wrote: >> > > > On Tue, Nov 22, 2016 at 11:54:57AM +0000, Brian Starkey wrote: >> > > > > On Tue, Nov 22, 2016 at 11:06:00AM +0000, Chris Wilson wrote: >> > > > > >On Tue, Nov 22, 2016 at 10:53:51AM +0000, Brian Starkey wrote: >> > > > > >>Hi Gustavo, >> > > > > >> >> > > > > >>A little late to the party here, but I was blocked by our internal >> > > > > >>contributions process... >> > > > > >> >> > > > > >>I didn't see these end up in my checkout yet though, so I guess they >> > > > > >>aren't picked up yet. >> > > > > >> >> > > > > >>On Mon, Nov 14, 2016 at 06:59:21PM +0900, Gustavo Padovan wrote: >> > > > > >>>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > > > > >>> >> > > > > >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for >> > > > > >>>atomic commits. >> > > > > >>> >> > > > > >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > > > > >>>--- >> > > > > >>>lib/igt_kms.c | 20 +++++++++++++++++++- >> > > > > >>>lib/igt_kms.h | 3 +++ >> > > > > >>>2 files changed, 22 insertions(+), 1 deletion(-) >> > > > > >>> >> > > > > >>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c >> > > > > >>>index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); >> > > > > >>>+ >> > > > > >>> /* >> > > > > >>> * TODO: Add all crtc level properties here >> > > > > >>> */ >> > > > > >>>@@ -2683,6 +2687,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 >> > > > > >> >> > > > > >>I don't think fence_ptr can be int *. It needs to be a pointer to a >> > > > > >>64-bit type. >> > > > > >> >> > > > > >>>+ * >> > > > > >>>+ * 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, int *fence_ptr) >> > > > > >>>+{ >> > > > > >>>+ pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 >> > > > > >>>}; >> > > > > >>> >> > > > > >>>@@ -298,6 +299,7 @@ struct igt_pipe { >> > > > > >>> >> > > > > >>> uint64_t mode_blob; >> > > > > >>> bool mode_changed; >> > > > > >>>+ uint64_t out_fence_ptr; >> > > > > >> >> > > > > >>IMO this should be: >> > > > > >> >> > > > > >> int64_t *out_fence_ptr; >> > > > > > >> > > > > >In userspace, fences are *fd*, a plain int. It is only the uabi that we >> > > > > >pass pointers as u64 to the kernel, and indeed that should be limited to >> > > > > >the uabi wrapper. >> > > > > >-Chris >> > > > > >> > > > > Where's the uabi wrapper in this case? >> > > > > >> > > > > Wherever it is, afaik someone needs to have 64-bit type for the kernel >> > > > > to stash its fd in - on the kernel side out_fence_ptr is >> > > > > (s64 __user *), so if there's not a 64-bit variable on the other end >> > > > > of it then someone's going to have a bad day. >> > > > >> > > > We do not have pointers in the uabi because they are different sizes on >> > > > different platforms. The uabi must be a u64 representation of a user >> > > > address to store the result - that is what we pass to the crtc set >> > > > property ioctl. >> > > >> > > Sure, but igt_pipe is not a uabi structure. By storing a uint64_t here >> > > we're making it needlessly opaque what the value is actually meant to >> > > be - which is the address of a 64-bit signed integer. >> > > >> > > Regardless, tests cannot set out_fence_ptr to the address of an int, I >> > > hope we can agree on that. Where that detail gets taken care of I >> > > don't much mind - but this code as-is is incorrect. >> > > >> > > By making igt_pipe.out_fence_ptr an (int64_t *) I thought we'd be >> > > letting the compiler warn anyone else away from incorrect code. >> > > >> > > > That it has been futher managled not to pass around fd >> > > > is an interesting twist, but ideally that sillyness should not make >> > > > itself into our API. >> > > >> > > Allowing the kernel and userspace to have different ideas about how >> > > big an int is doesn't sound so silly to me. It may not be a >> > > theoretical problem forever. >> > >> > What Chris means is that you want to have an int out_fence in igt_pipe, >> > and just pass the address of that into the OUT_FENCE_PTR property. >> >> Storing the fence itself in igt_pipe instead of a pointer to it is a >> different matter (and it isn't what's implemented in this patch). >> >> It still doesn't change the fact that you can't do as you suggest - >> you cannot just pass the address of an int in the OUT_FENCE_PTR >> property. >> >> In the kernel, put_user(fd, fence_state[i].out_fence_ptr); is going to >> write 8 bytes. If out_fence_ptr is the address of a 4-byte variable, >> then obviously that's not going to work out so well. > >Oh right, s/int/int64_t/, but otherwise that's imo what we should do. > >> > In >> > userspace we want to directly handle the fd, not a pointer to an fd. Like >> > Chris explained, the pointer-to-fd-cast-to-u64 is just to be able to reuse >> > the atomic ioctl as transport, it's not a reasonable interface within >> > userspace. >> >> I don't really follow this bit. At some point, something in userspace >> is going to need to take care of the fact that the kernel needs to >> have an 8-byte container to write into. > >Hm, why can't we allow igt test to directly access igt_pipe->out_fence? >And switch your function to igt_pipe_request_out_fence? Not my function, but yeah that sounds fine to me. All I wanted was to make sure we weren't passing the address of an int for the kernel to clobber whatever was nearby. My suggestion to store an (int64_t *) was just to let the compiler prevent that clobbering from ever happening by accident. -Brian >-Daniel >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 4748c0a..f25e1eb 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,9 @@ 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, pipe_obj->out_fence_ptr); + /* * TODO: Add all crtc level properties here */ @@ -2683,6 +2687,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, int *fence_ptr) +{ + pipe->out_fence_ptr = (uint64_t) 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 344f931..02d7bd1 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 }; @@ -298,6 +299,7 @@ struct igt_pipe { uint64_t mode_blob; bool mode_changed; + uint64_t out_fence_ptr; }; typedef struct { @@ -351,6 +353,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, int *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);