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@xxxxxxxxxxxxxxx> >>> >>>Add support for the OUT_FENCE_PTR property to enable setting out fences for >>>atomic commits. >>> >>>Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx> >>>--- >>>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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
