[2/6] xf86drmMode: separate drmModeAtomicCommit() and drmModeAtomicCleanup()

Commit Message

Hyungwon Hwang Aug. 19, 2015, 12:58 a.m. UTC

This patch seprates the code, which sorts proprty sets and eliminates
duplicate properties, from drmModeAtomicCommit(). Now
drmModeAtomicCleanup() has to do the job before calling
drmModeAtomicCommit(), and drmModeAtomicCommit() just converts the cleaned
request to IOCTL argument.

Signed-off-by: Hyungwon Hwang <[e-mail]>
---
 xf86drmMode.c | 72 +++++++++++++++++++++++++++++++++--------------------------
 xf86drmMode.h |  1 +
 2 files changed, 41 insertions(+), 32 deletions(-)

Comments

Emil Velikov Aug. 20, 2015, 4:17 p.m. UTC | #1

Hi Hyungwon,

On 19 August 2015 at 01:58, Hyungwon Hwang <[e-mail]> wrote:
> This patch seprates the code, which sorts proprty sets and eliminates
> duplicate properties, from drmModeAtomicCommit(). Now
> drmModeAtomicCleanup() has to do the job before calling
> drmModeAtomicCommit(), and drmModeAtomicCommit() just converts the cleaned
> request to IOCTL argument.
>
Afaict the commit message should say why we want this, rather than
rewording what the patch does.

I'm not sure about the atomic status for wayland and others but this
commit might cause issues there. Additionally, with this patch we'll
send a lot of useless information to the kernel if one omits
drmModeAtomicCleanup(). The kernel will likely discard it but still
this doesn't seem like a good idea imho.

-Emil

Hyungwon Hwang Aug. 21, 2015, 4:54 a.m. UTC | #2

Hi Emil,

On Thu, 20 Aug 2015 17:17:27 +0100
Emil Velikov <[e-mail]> wrote:

> Hi Hyungwon,
> 
> On 19 August 2015 at 01:58, Hyungwon Hwang <[e-mail]>
> wrote:
> > This patch seprates the code, which sorts proprty sets and
> > eliminates duplicate properties, from drmModeAtomicCommit(). Now
> > drmModeAtomicCleanup() has to do the job before calling
> > drmModeAtomicCommit(), and drmModeAtomicCommit() just converts the
> > cleaned request to IOCTL argument.
> >
> Afaict the commit message should say why we want this, rather than
> rewording what the patch does.
> 
> I'm not sure about the atomic status for wayland and others but this
> commit might cause issues there. Additionally, with this patch we'll
> send a lot of useless information to the kernel if one omits
> drmModeAtomicCleanup(). The kernel will likely discard it but still
> this doesn't seem like a good idea imho.

Yes. I agree that this change burdens the userspace application to use
API correctly. In my case, for modetest, the function of cleaning up
the request is needed, so I thought that this separation would be
needed. Overall, I agree with you. So I will drop this patch, and find
another way which is specific for modetest.

Thanks for your review.

Best regards,
Hyungwon Hwang

> 
> -Emil

Pekka Paalanen Aug. 21, 2015, 6:42 a.m. UTC | #3

On Fri, 21 Aug 2015 13:54:49 +0900
Hyungwon Hwang <[e-mail]> wrote:

> Hi Emil,
> 
> On Thu, 20 Aug 2015 17:17:27 +0100
> Emil Velikov <[e-mail]> wrote:
> 
> > Hi Hyungwon,
> > 
> > On 19 August 2015 at 01:58, Hyungwon Hwang <[e-mail]>
> > wrote:
> > > This patch seprates the code, which sorts proprty sets and
> > > eliminates duplicate properties, from drmModeAtomicCommit(). Now
> > > drmModeAtomicCleanup() has to do the job before calling
> > > drmModeAtomicCommit(), and drmModeAtomicCommit() just converts the
> > > cleaned request to IOCTL argument.
> > >
> > Afaict the commit message should say why we want this, rather than
> > rewording what the patch does.
> > 
> > I'm not sure about the atomic status for wayland and others but this
> > commit might cause issues there. Additionally, with this patch we'll
> > send a lot of useless information to the kernel if one omits
> > drmModeAtomicCleanup(). The kernel will likely discard it but still
> > this doesn't seem like a good idea imho.
> 
> Yes. I agree that this change burdens the userspace application to use
> API correctly. In my case, for modetest, the function of cleaning up
> the request is needed, so I thought that this separation would be
> needed. Overall, I agree with you. So I will drop this patch, and find
> another way which is specific for modetest.

Hi,

why do you need that, exactly?


Thanks,
pq

Hyungwon Hwang Aug. 21, 2015, 7:18 a.m. UTC | #4

Hi Pekka,

On Fri, 21 Aug 2015 09:42:26 +0300
Pekka Paalanen <[e-mail]> wrote:

> On Fri, 21 Aug 2015 13:54:49 +0900
> Hyungwon Hwang <[e-mail]> wrote:
> 
> > Hi Emil,
> > 
> > On Thu, 20 Aug 2015 17:17:27 +0100
> > Emil Velikov <[e-mail]> wrote:
> > 
> > > Hi Hyungwon,
> > > 
> > > On 19 August 2015 at 01:58, Hyungwon Hwang
> > > <[e-mail]> wrote:
> > > > This patch seprates the code, which sorts proprty sets and
> > > > eliminates duplicate properties, from drmModeAtomicCommit(). Now
> > > > drmModeAtomicCleanup() has to do the job before calling
> > > > drmModeAtomicCommit(), and drmModeAtomicCommit() just converts
> > > > the cleaned request to IOCTL argument.
> > > >
> > > Afaict the commit message should say why we want this, rather than
> > > rewording what the patch does.
> > > 
> > > I'm not sure about the atomic status for wayland and others but
> > > this commit might cause issues there. Additionally, with this
> > > patch we'll send a lot of useless information to the kernel if
> > > one omits drmModeAtomicCleanup(). The kernel will likely discard
> > > it but still this doesn't seem like a good idea imho.
> > 
> > Yes. I agree that this change burdens the userspace application to
> > use API correctly. In my case, for modetest, the function of
> > cleaning up the request is needed, so I thought that this
> > separation would be needed. Overall, I agree with you. So I will
> > drop this patch, and find another way which is specific for
> > modetest.
> 
> Hi,
> 
> why do you need that, exactly?

To make the buffer for plane, I needed to figure out the width and the
height which the user set which are in the request, but not applied to
the kernel yet. To get the value from the request, I thought cleaning
the request before I try to getting the value from the request was
needed because the user could set the different values for the same
property.

> 
> 
> Thanks,
> pq

Pekka Paalanen Aug. 21, 2015, 10:08 a.m. UTC | #5

On Fri, 21 Aug 2015 16:18:59 +0900
Hyungwon Hwang <[e-mail]> wrote:

> Hi Pekka,
> 
> On Fri, 21 Aug 2015 09:42:26 +0300
> Pekka Paalanen <[e-mail]> wrote:
> 
> > On Fri, 21 Aug 2015 13:54:49 +0900
> > Hyungwon Hwang <[e-mail]> wrote:
> > 
> > > Hi Emil,
> > > 
> > > On Thu, 20 Aug 2015 17:17:27 +0100
> > > Emil Velikov <[e-mail]> wrote:
> > > 
> > > > Hi Hyungwon,
> > > > 
> > > > On 19 August 2015 at 01:58, Hyungwon Hwang
> > > > <[e-mail]> wrote:
> > > > > This patch seprates the code, which sorts proprty sets and
> > > > > eliminates duplicate properties, from drmModeAtomicCommit(). Now
> > > > > drmModeAtomicCleanup() has to do the job before calling
> > > > > drmModeAtomicCommit(), and drmModeAtomicCommit() just converts
> > > > > the cleaned request to IOCTL argument.
> > > > >
> > > > Afaict the commit message should say why we want this, rather than
> > > > rewording what the patch does.
> > > > 
> > > > I'm not sure about the atomic status for wayland and others but
> > > > this commit might cause issues there. Additionally, with this
> > > > patch we'll send a lot of useless information to the kernel if
> > > > one omits drmModeAtomicCleanup(). The kernel will likely discard
> > > > it but still this doesn't seem like a good idea imho.
> > > 
> > > Yes. I agree that this change burdens the userspace application to
> > > use API correctly. In my case, for modetest, the function of
> > > cleaning up the request is needed, so I thought that this
> > > separation would be needed. Overall, I agree with you. So I will
> > > drop this patch, and find another way which is specific for
> > > modetest.
> > 
> > Hi,
> > 
> > why do you need that, exactly?
> 
> To make the buffer for plane, I needed to figure out the width and the
> height which the user set which are in the request, but not applied to
> the kernel yet. To get the value from the request, I thought cleaning
> the request before I try to getting the value from the request was
> needed because the user could set the different values for the same
> property.

Hi,

I would say that that is completely out of the scope of the libdrm API.
The caller of that API set those values, so it knows them - why should
libdrm offer an API to query back the values you just put in the req?

The caller is storing those values somewhere else too in any case,
exactly because it wants to allocate buffers of the same size. So very
likely those values were already stored *before* the atomic request was
even created.

IMHO, it is the responsibility of the application to track what it
itself is doing. If you have to write some data structures to do that,
then that's what you should do. Libdrm is not a replacement for that.


Thanks,
pq

Patch

diff --git a/xf86drmMode.c b/xf86drmMode.c
index d4ed5c1..82c4c91 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -1303,10 +1303,39 @@  static int sort_req_list(const void *misc, const void *other)
 		return second->property_id - first->property_id;
 }
 
+void drmModeAtomicCleanup(drmModeAtomicReqPtr req)
+{
+	uint32_t last_obj_id = 0;
+	uint32_t i;
+
+	if (req->cursor == 0)
+		return;
+
+	/* Sort the list by object ID, then by property ID. */
+	qsort(req->items, req->cursor, sizeof(*req->items),
+	      sort_req_list);
+
+	/* Eliminate duplicate property sets. */
+	for (i = 0; i < req->cursor; i++) {
+		if (req->items[i].object_id != last_obj_id)
+			last_obj_id = req->items[i].object_id;
+
+		if (i == req->cursor - 1)
+			continue;
+
+		if (req->items[i].object_id != req->items[i + 1].object_id ||
+		    req->items[i].property_id != req->items[i + 1].property_id)
+			continue;
+
+		memmove(&req->items[i], &req->items[i + 1],
+			(req->cursor - i - 1) * sizeof(*req->items));
+		req->cursor--;
+	}
+}
+
 int drmModeAtomicCommit(int fd, drmModeAtomicReqPtr req, uint32_t flags,
 			void *user_data)
 {
-	drmModeAtomicReqPtr sorted;
 	struct drm_mode_atomic atomic;
 	uint32_t *objs_ptr = NULL;
 	uint32_t *count_props_ptr = NULL;
@@ -1320,33 +1349,13 @@  int drmModeAtomicCommit(int fd, drmModeAtomicReqPtr req, uint32_t flags,
 	if (req->cursor == 0)
 		return 0;
 
-	sorted = drmModeAtomicDuplicate(req);
-	if (sorted == NULL)
-		return -ENOMEM;
-
 	memclear(atomic);
 
-	/* Sort the list by object ID, then by property ID. */
-	qsort(sorted->items, sorted->cursor, sizeof(*sorted->items),
-	      sort_req_list);
-
-	/* Now the list is sorted, eliminate duplicate property sets. */
-	for (i = 0; i < sorted->cursor; i++) {
-		if (sorted->items[i].object_id != last_obj_id) {
+	for (i = 0; i < req->cursor; i++) {
+		if (req->items[i].object_id != last_obj_id) {
 			atomic.count_objs++;
-			last_obj_id = sorted->items[i].object_id;
+			last_obj_id = req->items[i].object_id;
 		}
-
-		if (i == sorted->cursor - 1)
-			continue;
-
-		if (sorted->items[i].object_id != sorted->items[i + 1].object_id ||
-		    sorted->items[i].property_id != sorted->items[i + 1].property_id)
-			continue;
-
-		memmove(&sorted->items[i], &sorted->items[i + 1],
-			(sorted->cursor - i - 1) * sizeof(*sorted->items));
-		sorted->cursor--;
 	}
 
 	objs_ptr = drmMalloc(atomic.count_objs * sizeof objs_ptr[0]);
@@ -1361,28 +1370,28 @@  int drmModeAtomicCommit(int fd, drmModeAtomicReqPtr req, uint32_t flags,
 		goto out;
 	}
 
-	props_ptr = drmMalloc(sorted->cursor * sizeof props_ptr[0]);
+	props_ptr = drmMalloc(req->cursor * sizeof props_ptr[0]);
 	if (!props_ptr) {
 		errno = ENOMEM;
 		goto out;
 	}
 
-	prop_values_ptr = drmMalloc(sorted->cursor * sizeof prop_values_ptr[0]);
+	prop_values_ptr = drmMalloc(req->cursor * sizeof prop_values_ptr[0]);
 	if (!prop_values_ptr) {
 		errno = ENOMEM;
 		goto out;
 	}
 
-	for (i = 0, last_obj_id = 0; i < sorted->cursor; i++) {
-		if (sorted->items[i].object_id != last_obj_id) {
+	for (i = 0, last_obj_id = 0; i < req->cursor; i++) {
+		if (req->items[i].object_id != last_obj_id) {
 			obj_idx++;
-			objs_ptr[obj_idx] = sorted->items[i].object_id;
+			objs_ptr[obj_idx] = req->items[i].object_id;
 			last_obj_id = objs_ptr[obj_idx];
 		}
 
 		count_props_ptr[obj_idx]++;
-		props_ptr[i] = sorted->items[i].property_id;
-		prop_values_ptr[i] = sorted->items[i].value;
+		props_ptr[i] = req->items[i].property_id;
+		prop_values_ptr[i] = req->items[i].value;
 
 	}
 
@@ -1400,7 +1409,6 @@  out:
 	drmFree(count_props_ptr);
 	drmFree(props_ptr);
 	drmFree(prop_values_ptr);
-	drmModeAtomicFree(sorted);
 
 	return ret;
 }
diff --git a/xf86drmMode.h b/xf86drmMode.h
index 4de7bbb..fe14078 100644
--- a/xf86drmMode.h
+++ b/xf86drmMode.h
@@ -498,6 +498,7 @@  extern int drmModeAtomicAddProperty(drmModeAtomicReqPtr req,
 				    uint32_t object_id,
 				    uint32_t property_id,
 				    uint64_t value);
+extern void drmModeAtomicCleanup(drmModeAtomicReqPtr req);
 extern int drmModeAtomicCommit(int fd,
 			       drmModeAtomicReqPtr req,
 			       uint32_t flags,