[PATCH 1/6] xf86drmMode: remove the trailing white spaces

[PATCH 1/6] xf86drmMode: remove the trailing white spaces

[Hyungwon Hwang]

This patch removes the trailing white spaces.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
 xf86drmMode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xf86drmMode.c b/xf86drmMode.c
index fc19504..d4ed5c1 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -879,7 +879,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
 	int len, i;
 	struct drm_event *e;
 	struct drm_event_vblank *vblank;
-	
+
 	/* The DRM read semantics guarantees that we always get only
 	 * complete events. */
 
@@ -899,7 +899,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
 				break;
 			vblank = (struct drm_event_vblank *) e;
 			evctx->vblank_handler(fd,
-					      vblank->sequence, 
+					      vblank->sequence,
 					      vblank->tv_sec,
 					      vblank->tv_usec,
 					      U642VOID (vblank->user_data));
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Hyungwon Hwang]

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 <human.hwang@samsung.com>
---
 xf86drmMode.c | 72 +++++++++++++++++++++++++++++++++--------------------------
 xf86drmMode.h |  1 +
 2 files changed, 41 insertions(+), 32 deletions(-)

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,
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

Hi Hyungwon,

On 19 August 2015 at 01:58, Hyungwon Hwang <human.hwang@samsung.com> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Hyungwon Hwang]

Hi Emil,

On Thu, 20 Aug 2015 17:17:27 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> Hi Hyungwon,
> 
> On 19 August 2015 at 01:58, Hyungwon Hwang <human.hwang@samsung.com>
> 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 1474 bytes --]

On Fri, 21 Aug 2015 13:54:49 +0900
Hyungwon Hwang <human.hwang@samsung.com> wrote:

> Hi Emil,
> 
> On Thu, 20 Aug 2015 17:17:27 +0100
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
> 
> > Hi Hyungwon,
> > 
> > On 19 August 2015 at 01:58, Hyungwon Hwang <human.hwang@samsung.com>
> > 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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Hyungwon Hwang]

Hi Pekka,

On Fri, 21 Aug 2015 09:42:26 +0300
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Fri, 21 Aug 2015 13:54:49 +0900
> Hyungwon Hwang <human.hwang@samsung.com> wrote:
> 
> > Hi Emil,
> > 
> > On Thu, 20 Aug 2015 17:17:27 +0100
> > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > 
> > > Hi Hyungwon,
> > > 
> > > On 19 August 2015 at 01:58, Hyungwon Hwang
> > > <human.hwang@samsung.com> 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2865 bytes --]

On Fri, 21 Aug 2015 16:18:59 +0900
Hyungwon Hwang <human.hwang@samsung.com> wrote:

> Hi Pekka,
> 
> On Fri, 21 Aug 2015 09:42:26 +0300
> Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > On Fri, 21 Aug 2015 13:54:49 +0900
> > Hyungwon Hwang <human.hwang@samsung.com> wrote:
> > 
> > > Hi Emil,
> > > 
> > > On Thu, 20 Aug 2015 17:17:27 +0100
> > > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > > 
> > > > Hi Hyungwon,
> > > > 
> > > > On 19 August 2015 at 01:58, Hyungwon Hwang
> > > > <human.hwang@samsung.com> 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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] xf86drmMode: Make atomic request structures visible

[Hyungwon Hwang]

This patch makes 'struct _drmModeAtomicReqItem' and 'struct
_drmModeAtomicReq' visible from outside. This is needed for userspace
applications to use those structures when calling drmModeAtomicCommit().

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
 xf86drmMode.c | 14 --------------
 xf86drmMode.h | 12 ++++++++++++
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/xf86drmMode.c b/xf86drmMode.c
index 82c4c91..cf3fa21 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -1164,20 +1164,6 @@ int drmModeObjectSetProperty(int fd, uint32_t object_id, uint32_t object_type,
 	return DRM_IOCTL(fd, DRM_IOCTL_MODE_OBJ_SETPROPERTY, &prop);
 }
 
-typedef struct _drmModeAtomicReqItem drmModeAtomicReqItem, *drmModeAtomicReqItemPtr;
-
-struct _drmModeAtomicReqItem {
-	uint32_t object_id;
-	uint32_t property_id;
-	uint64_t value;
-};
-
-struct _drmModeAtomicReq {
-	uint32_t cursor;
-	uint32_t size_items;
-	drmModeAtomicReqItemPtr items;
-};
-
 drmModeAtomicReqPtr drmModeAtomicAlloc(void)
 {
 	drmModeAtomicReqPtr req;
diff --git a/xf86drmMode.h b/xf86drmMode.h
index fe14078..ec05ed8 100644
--- a/xf86drmMode.h
+++ b/xf86drmMode.h
@@ -485,6 +485,18 @@ extern int drmModeObjectSetProperty(int fd, uint32_t object_id,
 				    uint64_t value);
 
 
+struct _drmModeAtomicReqItem {
+	uint32_t object_id;
+	uint32_t property_id;
+	uint64_t value;
+};
+typedef struct _drmModeAtomicReqItem drmModeAtomicReqItem, *drmModeAtomicReqItemPtr;
+
+struct _drmModeAtomicReq {
+	uint32_t cursor;
+	uint32_t size_items;
+	drmModeAtomicReqItemPtr items;
+};
 typedef struct _drmModeAtomicReq drmModeAtomicReq, *drmModeAtomicReqPtr;
 
 extern drmModeAtomicReqPtr drmModeAtomicAlloc(void);
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] xf86drmMode: Make atomic request structures visible

[Emil Velikov]

On 19 August 2015 at 01:58, Hyungwon Hwang <human.hwang@samsung.com> wrote:
> This patch makes 'struct _drmModeAtomicReqItem' and 'struct
> _drmModeAtomicReq' visible from outside. This is needed for userspace
> applications to use those structures when calling drmModeAtomicCommit().
>
Hmmm what is missing in the current API, that one needs direct access
to the structs ? If we expose these to the user we'll be putting a
(ABI) hedgehog down our pants (i.e. it might be ok, but will likely
result in a very painful experience).

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] xf86drmMode: Make atomic request structures visible

[Hyungwon Hwang]

Hi Emil,

On Thu, 20 Aug 2015 17:23:09 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> On 19 August 2015 at 01:58, Hyungwon Hwang <human.hwang@samsung.com>
> wrote:
> > This patch makes 'struct _drmModeAtomicReqItem' and 'struct
> > _drmModeAtomicReq' visible from outside. This is needed for
> > userspace applications to use those structures when calling
> > drmModeAtomicCommit().
> >
> Hmmm what is missing in the current API, that one needs direct access
> to the structs ? If we expose these to the user we'll be putting a
> (ABI) hedgehog down our pants (i.e. it might be ok, but will likely
> result in a very painful experience).

I also agree with you. I think I should drop this patch, and find
another way for modetest.

Thanks.

Best regards,
Hyungwon Hwang

> 
> Thanks
> Emil

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] xf86drmMode: Make atomic request structures visible

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 946 bytes --]

On Fri, 21 Aug 2015 15:06:58 +0900
Hyungwon Hwang <human.hwang@samsung.com> wrote:

> Hi Emil,
> 
> On Thu, 20 Aug 2015 17:23:09 +0100
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
> 
> > On 19 August 2015 at 01:58, Hyungwon Hwang <human.hwang@samsung.com>
> > wrote:
> > > This patch makes 'struct _drmModeAtomicReqItem' and 'struct
> > > _drmModeAtomicReq' visible from outside. This is needed for
> > > userspace applications to use those structures when calling
> > > drmModeAtomicCommit().

Yeah, this sounds like a very bad idea.


Thanks,
pq

> > >
> > Hmmm what is missing in the current API, that one needs direct access
> > to the structs ? If we expose these to the user we'll be putting a
> > (ABI) hedgehog down our pants (i.e. it might be ok, but will likely
> > result in a very painful experience).
> 
> I also agree with you. I think I should drop this patch, and find
> another way for modetest.

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] xf86drmMode: Make atomic request structures visible

[Hyungwon Hwang]

Dear,

On Fri, 21 Aug 2015 09:44:42 +0300
Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Fri, 21 Aug 2015 15:06:58 +0900
> Hyungwon Hwang <human.hwang@samsung.com> wrote:
> 
> > Hi Emil,
> > 
> > On Thu, 20 Aug 2015 17:23:09 +0100
> > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > 
> > > On 19 August 2015 at 01:58, Hyungwon Hwang
> > > <human.hwang@samsung.com> wrote:
> > > > This patch makes 'struct _drmModeAtomicReqItem' and 'struct
> > > > _drmModeAtomicReq' visible from outside. This is needed for
> > > > userspace applications to use those structures when calling
> > > > drmModeAtomicCommit().
> 
> Yeah, this sounds like a very bad idea.

Yes. Making it visible was not good. But then I think that new API for
getting the value of a request item in the request which are not
applied to the kernel. Because for preparing the buffer, the userspace
program needs width and height which are in the request.

The program can get the value before making it as a request. In that
case, the program such as modetest, which does not understand what the
object id or propery id means, have to be modified to understand them
for extracting the needed value.

How do you think about it? Is modetest just a special program for
testing, and is this support not needed for another real program?

> 
> 
> Thanks,
> pq
> 
> > > >
> > > Hmmm what is missing in the current API, that one needs direct
> > > access to the structs ? If we expose these to the user we'll be
> > > putting a (ABI) hedgehog down our pants (i.e. it might be ok, but
> > > will likely result in a very painful experience).
> > 
> > I also agree with you. I think I should drop this patch, and find
> > another way for modetest.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] modetest: remove the trailing white spaces

[Hyungwon Hwang]

This patch removes the trailing white spaces.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
 tests/modetest/modetest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 4eb9eac..43bd06f 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -1251,7 +1251,7 @@ static void test_page_flip(struct device *dev, struct pipe_arg *pipes, unsigned
 	evctx.version = DRM_EVENT_CONTEXT_VERSION;
 	evctx.vblank_handler = NULL;
 	evctx.page_flip_handler = page_flip_handler;
-	
+
 	while (1) {
 #if 0
 		struct pollfd pfd[2];
@@ -1565,7 +1565,7 @@ int main(int argc, char **argv)
 			if (parse_connector(&pipe_args[count], optarg) < 0)
 				usage(argv[0]);
 
-			count++;				      
+			count++;
 			break;
 		case 'C':
 			test_cursor = 1;
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] modetest: add atomic modeset support

[Hyungwon Hwang]

This patch adds support for atomic modeset. Using -a option, user can
make modeset to use DRM_IOCTL_MODE_ATOMIC instead of legacy IOCTLs.
Also, by using -W option, user can set the value of each object's property.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
 tests/modetest/modetest.c | 237 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 204 insertions(+), 33 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 43bd06f..bf9222d 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -1433,15 +1433,21 @@ static int parse_property(struct property_arg *p, const char *arg)
 
 static void usage(char *name)
 {
-	fprintf(stderr, "usage: %s [-cDdefMPpsCvw]\n", name);
+	fprintf(stderr, "usage: %s [-acDdefMPpsCvwW]\n", name);
+	fprintf(stderr, "\tA: supported in atomic modeset\n");
+	fprintf(stderr, "\tL: supported in legacy modeset\n");
 
-	fprintf(stderr, "\n Query options:\n\n");
+	fprintf(stderr, "\n Query options: [AL]\n\n");
 	fprintf(stderr, "\t-c\tlist connectors\n");
 	fprintf(stderr, "\t-e\tlist encoders\n");
 	fprintf(stderr, "\t-f\tlist framebuffers\n");
 	fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n");
 
-	fprintf(stderr, "\n Test options:\n\n");
+	fprintf(stderr, "\n Atomic Test options: [A]\n\n");
+	fprintf(stderr, "\t-a\tuse atomic modeset\n");
+	fprintf(stderr, "\t-W <obj_id>:<prop_name>:<value>\tset property\n");
+
+	fprintf(stderr, "\n Legacy test options: [L]\n\n");
 	fprintf(stderr, "\t-P <crtc_id>:<w>x<h>[+<x>+<y>][*<scale>][@<format>]\tset a plane\n");
 	fprintf(stderr, "\t-s <connector_id>[,<connector_id>][@<crtc_id>]:<mode>[-<vrefresh>][@<format>]\tset a mode\n");
 	fprintf(stderr, "\t-C\ttest hw cursor\n");
@@ -1449,9 +1455,9 @@ static void usage(char *name)
 	fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n");
 
 	fprintf(stderr, "\n Generic options:\n\n");
-	fprintf(stderr, "\t-d\tdrop master after mode set\n");
-	fprintf(stderr, "\t-M module\tuse the given driver\n");
-	fprintf(stderr, "\t-D device\tuse the given device\n");
+	fprintf(stderr, "\t-d\tdrop master after mode set [L]\n");
+	fprintf(stderr, "\t-M module\tuse the given driver [AL]\n");
+	fprintf(stderr, "\t-D device\tuse the given device [AL]\n");
 
 	fprintf(stderr, "\n\tDefault is to dump all info.\n");
 	exit(0);
@@ -1484,7 +1490,154 @@ static int cursor_supported(void)
 	return 1;
 }
 
-static char optstr[] = "cdD:efM:P:ps:Cvw:";
+static int parse_atomic_args(drmModeAtomicReqPtr req, const char *p)
+{
+	uint32_t object_id, property_id;
+	uint64_t value;
+
+	if (sscanf(p, "%u:%u:%llu", &object_id, &property_id, &value) != 3)
+		return -1;
+
+	drmModeAtomicAddProperty(req, object_id, property_id, value);
+
+	return 0;
+}
+
+static uint32_t get_atomic_plane_prop_id(struct resources *res, uint32_t obj_id,
+								const char *name)
+{
+	drmModePropertyRes *props_info;
+	struct plane *plane;
+	unsigned int i, j;
+
+	for (i = 0; i < res->plane_res->count_planes; i++) {
+		plane = &res->planes[i];
+		if (plane->plane->plane_id != obj_id)
+			continue;
+
+		for (j = 0; j < plane->props->count_props; j++) {
+			props_info = plane->props_info[j];
+			if (!strcmp(props_info->name, name))
+				return props_info->prop_id;
+		}
+	}
+
+	return 0;
+}
+
+static uint64_t get_atomic_plane_prop_value_in_request(drmModeAtomicReqPtr req,
+			struct resources *res, uint32_t obj_id, const char *name)
+{
+	uint32_t prop_id, i;
+	drmModeAtomicReqItemPtr item;
+
+	prop_id = get_atomic_plane_prop_id(res, obj_id, name);
+
+	for (i = 0; i < req->cursor; i++) {
+		item = &req->items[i];
+
+		if (item->object_id != obj_id)
+			continue;
+
+		if (item->property_id != prop_id)
+			continue;
+
+		return item->value;
+	}
+
+	return -1;
+}
+
+static bool is_atomic_obj_in_request(drmModeAtomicReqPtr req,
+						uint32_t obj_id)
+{
+	unsigned int i;
+
+	for (i = 0; i < req->cursor; i++)
+		if (req->items[i].object_id == obj_id)
+			return true;
+
+	return false;
+}
+
+static int allocate_fb(struct device *dev, drmModeAtomicReqPtr req, struct resources *res,
+			uint32_t width,	uint32_t height, uint32_t pixel_format,
+			int pattern, struct bo **bo, uint32_t *fb_id)
+{
+	uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0};
+	int ret;
+
+	*bo = bo_create(dev->fd, pixel_format, width, height,
+			handles, pitches, offsets, pattern);
+	if (*bo == NULL) {
+		fprintf(stderr, "failed to create bo (%ux%u): %s\n",
+				width, height, strerror(errno));
+		return -1;
+	}
+
+	ret = drmModeAddFB2(dev->fd, width, height, pixel_format,
+			handles, pitches, offsets, fb_id, 0);
+	if (ret) {
+		fprintf(stderr, "failed to add fb (%ux%u): %s\n",
+				width, height, strerror(errno));
+		bo_destroy(*bo);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int allocate_fbs(struct device *dev, drmModeAtomicReqPtr req, struct resources *res)
+{
+	uint32_t plane_id, fb_obj_id, fb_id, i, width,	height, pixel_format;
+	struct bo *bo;
+	int ret;
+
+	for (i = 0; i < res->plane_res->count_planes; i++) {
+		plane_id = res->planes[i].plane->plane_id;
+
+		if (!is_atomic_obj_in_request(req, plane_id))
+			continue;
+
+		fb_obj_id = get_atomic_plane_prop_id(res, plane_id, "FB_ID");
+		if (!fb_obj_id)
+			return -1;
+
+		width = get_atomic_plane_prop_value_in_request(req, res,
+							plane_id, "SRC_W");
+		height = get_atomic_plane_prop_value_in_request(req, res,
+							plane_id, "SRC_H");
+		pixel_format = DRM_FORMAT_XRGB8888;
+
+		ret = allocate_fb(dev, req, res, width, height, pixel_format,
+						PATTERN_SMPTE, &bo, &fb_id);
+		if (ret < 0)
+			return ret;
+
+		dev->mode.bo = bo;
+		dev->mode.fb_id = fb_id;
+
+		ret = drmModeAtomicAddProperty(req, plane_id, fb_obj_id, fb_id);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void atomic_modeset(struct device *dev, drmModeAtomicReqPtr req,
+		struct resources *res)
+{
+	uint32_t flags = 0;
+
+	drmModeAtomicCleanup(req);
+
+	allocate_fbs(dev, req, res);
+
+	drmModeAtomicCommit(dev->fd, req, flags, NULL);
+}
+
+static char optstr[] = "acdD:efM:P:ps:Cvw:W:";
 
 int main(int argc, char **argv)
 {
@@ -1504,6 +1657,7 @@ int main(int argc, char **argv)
 	struct pipe_arg *pipe_args = NULL;
 	struct plane_arg *plane_args = NULL;
 	struct property_arg *prop_args = NULL;
+	drmModeAtomicReqPtr req = NULL;
 	unsigned int args = 0;
 	int ret;
 
@@ -1514,6 +1668,11 @@ int main(int argc, char **argv)
 		args++;
 
 		switch (c) {
+		case 'a':
+			if (!req)
+				req = drmModeAtomicAlloc();
+			args--;
+			break;
 		case 'c':
 			connectors = 1;
 			break;
@@ -1587,6 +1746,10 @@ int main(int argc, char **argv)
 
 			prop_count++;
 			break;
+		case 'W':
+			if (!req || parse_atomic_args(req, optarg) < 0)
+				usage(argv[0]);
+			break;
 		default:
 			usage(argv[0]);
 			break;
@@ -1635,6 +1798,9 @@ int main(int argc, char **argv)
 		return -1;
 	}
 
+	if (req)
+		drmSetClientCap(dev.fd, DRM_CLIENT_CAP_ATOMIC, 1);
+
 	dev.resources = get_resources(&dev);
 	if (!dev.resources) {
 		drmClose(dev.fd);
@@ -1649,46 +1815,51 @@ int main(int argc, char **argv)
 	dump_resource(&dev, planes);
 	dump_resource(&dev, framebuffers);
 
-	for (i = 0; i < prop_count; ++i)
-		set_property(&dev, &prop_args[i]);
+	if (req) {
+		atomic_modeset(&dev, req, dev.resources);
 
-	if (count || plane_count) {
-		uint64_t cap = 0;
+		getchar();
+	} else {
+		for (i = 0; i < prop_count; ++i)
+			set_property(&dev, &prop_args[i]);
 
-		ret = drmGetCap(dev.fd, DRM_CAP_DUMB_BUFFER, &cap);
-		if (ret || cap == 0) {
-			fprintf(stderr, "driver doesn't support the dumb buffer API\n");
-			return 1;
-		}
+		if (count || plane_count) {
+			uint64_t cap = 0;
 
-		if (count)
-			set_mode(&dev, pipe_args, count);
+			ret = drmGetCap(dev.fd, DRM_CAP_DUMB_BUFFER, &cap);
+			if (ret || cap == 0) {
+				fprintf(stderr, "driver doesn't support the dumb buffer API\n");
+				return 1;
+			}
 
-		if (plane_count)
-			set_planes(&dev, plane_args, plane_count);
+			if (count)
+				set_mode(&dev, pipe_args, count);
 
-		if (test_cursor)
-			set_cursors(&dev, pipe_args, count);
+			if (plane_count)
+				set_planes(&dev, plane_args, plane_count);
 
-		if (test_vsync)
-			test_page_flip(&dev, pipe_args, count);
+			if (test_cursor)
+				set_cursors(&dev, pipe_args, count);
 
-		if (drop_master)
-			drmDropMaster(dev.fd);
+			if (test_vsync)
+				test_page_flip(&dev, pipe_args, count);
 
-		getchar();
+			if (drop_master)
+				drmDropMaster(dev.fd);
 
-		if (test_cursor)
-			clear_cursors(&dev);
+			getchar();
 
-		if (plane_count)
-			clear_planes(&dev, plane_args, plane_count);
+			if (test_cursor)
+				clear_cursors(&dev);
 
-		if (count)
-			clear_mode(&dev);
+			if (plane_count)
+				clear_planes(&dev, plane_args, plane_count);
+		}
 	}
 
+	clear_mode(&dev);
 	free_resources(dev.resources);
+	drmFree(req);
 
 	return 0;
 }
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] modetest: add atomic page flip support

[Hyungwon Hwang]

This patch adds support for atomic page flip. User can specify -W option
with the plane id for testing atomic page flipping.

Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
---
 tests/modetest/modetest.c | 150 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 3 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index bf9222d..d1b7dc4 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -719,6 +719,10 @@ struct pipe_arg {
 	struct timeval start;
 
 	int swap_count;
+
+	/* for atomic modeset */
+	uint32_t plane_id;
+	uint32_t fb_obj_id;
 };
 
 struct plane_arg {
@@ -1446,6 +1450,7 @@ static void usage(char *name)
 	fprintf(stderr, "\n Atomic Test options: [A]\n\n");
 	fprintf(stderr, "\t-a\tuse atomic modeset\n");
 	fprintf(stderr, "\t-W <obj_id>:<prop_name>:<value>\tset property\n");
+	fprintf(stderr, "\t-V <flipping_plane_id>\ttest vsynced page flipping\n");
 
 	fprintf(stderr, "\n Legacy test options: [L]\n\n");
 	fprintf(stderr, "\t-P <crtc_id>:<w>x<h>[+<x>+<y>][*<scale>][@<format>]\tset a plane\n");
@@ -1637,7 +1642,137 @@ static void atomic_modeset(struct device *dev, drmModeAtomicReqPtr req,
 	drmModeAtomicCommit(dev->fd, req, flags, NULL);
 }
 
-static char optstr[] = "acdD:efM:P:ps:Cvw:W:";
+static void
+atomic_page_flip_handler(int fd, unsigned int frame,
+		unsigned int sec, unsigned int usec, void *data)
+{
+	static drmModeAtomicReqPtr req = NULL;
+	unsigned int new_fb_id;
+	struct timeval end;
+	struct pipe_arg *pipe;
+	double t;
+	uint32_t flags = 0;
+
+	pipe = (struct pipe_arg *)(unsigned long)data;
+
+	if (pipe->current_fb_id == pipe->fb_id[0])
+		new_fb_id = pipe->fb_id[1];
+	else
+		new_fb_id = pipe->fb_id[0];
+
+	pipe->current_fb_id = new_fb_id;
+	pipe->swap_count++;
+
+	req = drmModeAtomicAlloc();
+
+	drmModeAtomicAddProperty(req, pipe->plane_id, pipe->fb_obj_id, new_fb_id);
+
+	flags = DRM_MODE_PAGE_FLIP_EVENT;
+	drmModeAtomicCommit(fd, req, flags, pipe);
+
+	if (pipe->swap_count == 60) {
+		gettimeofday(&end, NULL);
+		t = end.tv_sec + end.tv_usec * 1e-6 -
+			(pipe->start.tv_sec + pipe->start.tv_usec * 1e-6);
+		fprintf(stderr, "freq: %.02fHz\n", pipe->swap_count / t);
+		pipe->swap_count = 0;
+		pipe->start = end;
+	}
+
+	drmModeAtomicFree(req);
+}
+
+static int atomic_test_page_flip(struct device *dev, drmModeAtomicReqPtr req,
+		struct resources *res, unsigned int plane_id)
+{
+	struct bo *other_bo;
+	unsigned int other_fb_id;
+	struct pipe_arg *pipe = NULL;
+	drmEventContext evctx;
+	uint32_t flags = 0, fb_obj_id = 0, pixel_format;
+	uint64_t width, height;
+	int ret;
+
+	if (!is_atomic_obj_in_request(req, plane_id))
+		return -1;
+
+	fb_obj_id = get_atomic_plane_prop_id(res, plane_id, "FB_ID");
+	if (!fb_obj_id)
+		return -1;
+
+	width = get_atomic_plane_prop_value_in_request(req, res,
+			plane_id, "SRC_W");
+	height = get_atomic_plane_prop_value_in_request(req, res,
+			plane_id, "SRC_H");
+	pixel_format = DRM_FORMAT_XRGB8888;
+
+	ret = allocate_fb(dev, req, res, width, height, pixel_format,
+			PATTERN_TILES, &other_bo, &other_fb_id);
+	if (ret < 0)
+		return ret;
+
+	ret = drmModeAtomicAddProperty(req, plane_id, fb_obj_id,
+			other_fb_id);
+	if (ret < 0)
+		goto err;
+
+	if (!fb_obj_id) {
+		fprintf(stderr, "page flipping requires at least one plane setting.\n");
+		return -1;
+	}
+
+	pipe = drmMalloc(sizeof *pipe);
+
+	gettimeofday(&pipe->start, NULL);
+	pipe->swap_count = 0;
+	pipe->plane_id = plane_id;
+	pipe->fb_obj_id = fb_obj_id;
+	pipe->fb_id[0] = dev->mode.fb_id;
+	pipe->fb_id[1] = other_fb_id;
+	pipe->current_fb_id = other_fb_id;
+
+	flags = DRM_MODE_PAGE_FLIP_EVENT;
+
+	ret = drmModeAtomicCommit(dev->fd, req, flags, pipe);
+	if (ret < 0)
+		goto err_rmfb;
+
+	memset(&evctx, 0, sizeof evctx);
+	evctx.version = DRM_EVENT_CONTEXT_VERSION;
+	evctx.vblank_handler = NULL;
+	evctx.page_flip_handler = atomic_page_flip_handler;
+
+	while (1) {
+		struct timeval timeout = { .tv_sec = 3, .tv_usec = 0 };
+		fd_set fds;
+		int ret;
+
+		FD_ZERO(&fds);
+		FD_SET(0, &fds);
+		FD_SET(dev->fd, &fds);
+		ret = select(dev->fd + 1, &fds, NULL, NULL, &timeout);
+
+		if (ret <= 0) {
+			fprintf(stderr, "select timed out or error (ret %d)\n",
+					ret);
+			continue;
+		} else if (FD_ISSET(0, &fds)) {
+			break;
+		}
+
+		drmHandleEvent(dev->fd, &evctx);
+	}
+
+err_rmfb:
+	drmFree(pipe);
+	drmModeRmFB(dev->fd, other_fb_id);
+err:
+	bo_destroy(other_bo);
+
+	return 0;
+}
+
+static char optstr[] = "acdD:efM:P:ps:CvV:w:W:";
 
 int main(int argc, char **argv)
 {
@@ -1651,7 +1786,7 @@ int main(int argc, char **argv)
 	const char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos", "tilcdc", "msm", "sti", "tegra", "imx-drm", "rockchip", "atmel-hlcdc" };
 	char *device = NULL;
 	char *module = NULL;
-	unsigned int i;
+	unsigned int i, flip_plane_id = 0;
 	int count = 0, plane_count = 0;
 	unsigned int prop_count = 0;
 	struct pipe_arg *pipe_args = NULL;
@@ -1732,6 +1867,11 @@ int main(int argc, char **argv)
 		case 'v':
 			test_vsync = 1;
 			break;
+		case 'V':
+			if (sscanf(optarg, "%u", &flip_plane_id) != 1)
+				usage(argv[0]);
+			test_vsync = 1;
+			break;
 		case 'w':
 			prop_args = realloc(prop_args,
 					   (prop_count + 1) * sizeof *prop_args);
@@ -1788,7 +1928,7 @@ int main(int argc, char **argv)
 		return -1;
 	}
 
-	if (test_vsync && !count) {
+	if (test_vsync && (!count && !req)) {
 		fprintf(stderr, "page flipping requires at least one -s option.\n");
 		return -1;
 	}
@@ -1818,6 +1958,10 @@ int main(int argc, char **argv)
 	if (req) {
 		atomic_modeset(&dev, req, dev.resources);
 
+		if (test_vsync)
+			atomic_test_page_flip(&dev, req, dev.resources,
+								flip_plane_id);
+
 		getchar();
 	} else {
 		for (i = 0; i < prop_count; ++i)
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel