Discussion:
[PATCH] gallium/winsys/kms: Add support for multi-planes.
(too old to reply)
Tao Wu
2017-12-24 04:59:27 UTC
Permalink
From: Lepton Wu <***@chromium.org>

Signed-off-by: Lepton Wu <***@chromium.org>

Change-Id: I0863f522976cc8863d6e95492d9346df35c066ec
---
src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 150 +++++++++++++++-------
1 file changed, 106 insertions(+), 44 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..514298aea4e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,13 +59,20 @@
#define DEBUG_PRINT(msg, ...)
#endif

+struct kms_sw_displaytarget;

-struct kms_sw_displaytarget
-{
- enum pipe_format format;
+struct kms_sw_plane {
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ struct kms_sw_displaytarget* dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;

uint32_t handle;
@@ -73,6 +80,7 @@ struct kms_sw_displaytarget

int ref_count;
struct list_head link;
+ struct list_head planes;
};

struct kms_sw_winsys
@@ -83,10 +91,10 @@ struct kms_sw_winsys
struct list_head bo_list;
};

-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct kms_sw_plane *)dt;
}

static inline struct kms_sw_winsys *
@@ -105,6 +113,39 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}

+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset) {
+ struct kms_sw_plane * tmp, * plane = NULL;
+ if (offset > kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: offset %d bigger then %d\n",
+ offset, kms_sw_dt->size);
+ return NULL;
+ }
+ LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
+ if (tmp->offset == offset) {
+ plane = tmp;
+ break;
+ }
+ }
+ if (plane) {
+ assert(plane->width == width);
+ assert(plane->height == height);
+ assert(plane->stride == stride);
+ assert(plane->dt == kms_sw_dt);
+ } else {
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (plane == NULL) return NULL;
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ }
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -124,11 +165,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;

+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;

kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;

memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -138,17 +178,20 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;

- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ if (get_plane(kms_sw_dt, width, height, create_req.pitch, create_req.size)
+ == NULL)
+ goto free_bo;

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);

- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
-
+ *stride = create_req.pitch;
+ return (struct sw_displaytarget *) list_first_entry(&kms_sw_dt->planes,
+ struct kms_sw_plane,
+ link);
free_bo:
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = create_req.handle;
@@ -163,7 +206,7 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = kms_sw_plane(dt)->dt;
struct drm_mode_destroy_dumb destroy_req;

kms_sw_dt->ref_count --;
@@ -178,6 +221,10 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,

DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);

+ struct kms_sw_plane * plane, * tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
FREE(kms_sw_dt);
}

@@ -187,7 +234,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *kms_sw_pl = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = kms_sw_pl->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;

@@ -204,10 +252,11 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
if (kms_sw_dt->mapped == MAP_FAILED)
return NULL;

- DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p %dx%d \n",
+ kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped,
+ kms_sw_pl->width, kms_sw_pl->height);

- return kms_sw_dt->mapped;
+ return kms_sw_dt->mapped + kms_sw_pl->offset;
}

static struct kms_sw_displaytarget *
@@ -230,10 +279,10 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}

-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -245,13 +294,20 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;

kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ if (kms_sw_dt) {
+ struct kms_sw_plane * pl = get_plane(kms_sw_dt, width, height,
+ stride, offset);
+ if (pl == NULL) {
+ kms_sw_dt->ref_count --;
+ }
+ return pl;
+ }

kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;

+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -260,22 +316,23 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;

lseek(fd, 0, SEEK_SET);
+ if (get_plane(kms_sw_dt, width, height, stride, offset) == NULL) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

- return kms_sw_dt;
+ return list_first_entry(&kms_sw_dt->planes, struct kms_sw_plane, link);
}

static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = kms_sw_plane(dt)->dt;

DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);

@@ -291,30 +348,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;

assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);

- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
case DRM_API_HANDLE_TYPE_FD:
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl) {
+ *stride = kms_sw_pl->stride;
+ }
+ return (struct sw_displaytarget *)kms_sw_pl;
case DRM_API_HANDLE_TYPE_KMS:
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane * plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return (struct sw_displaytarget *)plane;
+ }
+ }
+ kms_sw_dt->ref_count --;
+ return NULL;
}
/* fallthrough */
default:
@@ -331,19 +392,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *kms_sw_pl = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = kms_sw_pl->dt;

switch(whandle->type) {
case DRM_API_HANDLE_TYPE_KMS:
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = kms_sw_pl->stride;
+ whandle->offset = kms_sw_pl->offset;
return TRUE;
case DRM_API_HANDLE_TYPE_FD:
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = kms_sw_pl->stride;
+ whandle->offset = kms_sw_pl->offset;
return TRUE;
}
/* fallthrough */
--
2.13.5
Lepton Wu
2017-12-28 07:35:05 UTC
Permalink
v2: address comments from Tomasz Figa
a) Add more check for plane size.
b) Avoid duplicated mapping and leaked mapping.
c) Other minor changes.

Signed-off-by: Lepton Wu <***@chromium.org>

Change-Id: I0863f522976cc8863d6e95492d9346df35c066ec
---
src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 179 +++++++++++++++-------
1 file changed, 126 insertions(+), 53 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..69c05197081 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,20 +59,29 @@
#define DEBUG_PRINT(msg, ...)
#endif

+struct kms_sw_displaytarget;

-struct kms_sw_displaytarget
-{
- enum pipe_format format;
+struct kms_sw_plane {
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ struct kms_sw_displaytarget* dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;

uint32_t handle;
void *mapped;
+ void *ro_mapped;

int ref_count;
struct list_head link;
+ struct list_head planes;
};

struct kms_sw_winsys
@@ -83,10 +92,10 @@ struct kms_sw_winsys
struct list_head bo_list;
};

-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct kms_sw_plane *)dt;
}

static inline struct kms_sw_winsys *
@@ -105,6 +114,42 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}

+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset) {
+ struct kms_sw_plane * tmp, * plane = NULL;
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+ LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
+ if (tmp->offset == offset) {
+ plane = tmp;
+ break;
+ }
+ }
+ if (plane) {
+ assert(plane->width == width);
+ assert(plane->height == height);
+ assert(plane->stride == stride);
+ assert(plane->dt == kms_sw_dt);
+ } else {
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (plane == NULL) return NULL;
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ }
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -124,11 +169,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;

+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;

kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;

memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -138,17 +182,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;

- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane* plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (plane == NULL)
+ goto free_bo;

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);

- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
-
+ *stride = create_req.pitch;
+ return (struct sw_displaytarget *) plane;
free_bo:
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = create_req.handle;
@@ -163,13 +209,19 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;

kms_sw_dt->ref_count --;
if (kms_sw_dt->ref_count > 0)
return;

+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
@@ -178,6 +230,10 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,

DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);

+ struct kms_sw_plane * tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
FREE(kms_sw_dt);
}

@@ -187,7 +243,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;

@@ -198,16 +255,20 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;

prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (*ptr == NULL) {
+ void * tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }

- DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p %dx%d \n",
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr,
+ plane->width, plane->height);

- return kms_sw_dt->mapped;
+ return *ptr + plane->offset;
}

static struct kms_sw_displaytarget *
@@ -230,10 +291,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}

-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -245,13 +307,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;

kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane * plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (plane == NULL)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }

kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;

+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -260,27 +328,27 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;

lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (plane == NULL) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

- return kms_sw_dt;
+ return plane;
}

static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane * plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

- DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
-
- munmap(kms_sw_dt->mapped, kms_sw_dt->size);
- kms_sw_dt->mapped = NULL;
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap buffer %u \n", kms_sw_dt->handle);
}

static struct sw_displaytarget *
@@ -291,30 +359,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;

assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);

- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
case DRM_API_HANDLE_TYPE_FD:
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl) {
+ *stride = kms_sw_pl->stride;
+ }
+ return (struct sw_displaytarget *)kms_sw_pl;
case DRM_API_HANDLE_TYPE_KMS:
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane * plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return (struct sw_displaytarget *)plane;
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
default:
@@ -331,19 +403,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

switch(whandle->type) {
case DRM_API_HANDLE_TYPE_KMS:
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
case DRM_API_HANDLE_TYPE_FD:
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.15.1.620.gb9897f4670-goog
Lepton Wu
2018-01-09 06:52:52 UTC
Permalink
Gentle ping. Thanks.
Post by Lepton Wu
v2: address comments from Tomasz Figa
a) Add more check for plane size.
b) Avoid duplicated mapping and leaked mapping.
c) Other minor changes.
Change-Id: I0863f522976cc8863d6e95492d9346df35c066ec
---
src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 179 +++++++++++++++-------
1 file changed, 126 insertions(+), 53 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..69c05197081 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,20 +59,29 @@
#define DEBUG_PRINT(msg, ...)
#endif
+struct kms_sw_displaytarget;
-struct kms_sw_displaytarget
-{
- enum pipe_format format;
+struct kms_sw_plane {
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ struct kms_sw_displaytarget* dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
+ struct list_head planes;
};
struct kms_sw_winsys
@@ -83,10 +92,10 @@ struct kms_sw_winsys
struct list_head bo_list;
};
-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct kms_sw_plane *)dt;
}
static inline struct kms_sw_winsys *
@@ -105,6 +114,42 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}
+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset) {
+ struct kms_sw_plane * tmp, * plane = NULL;
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+ LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
+ if (tmp->offset == offset) {
+ plane = tmp;
+ break;
+ }
+ }
+ if (plane) {
+ assert(plane->width == width);
+ assert(plane->height == height);
+ assert(plane->stride == stride);
+ assert(plane->dt == kms_sw_dt);
+ } else {
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (plane == NULL) return NULL;
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ }
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -124,11 +169,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;
+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;
kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -138,17 +182,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;
- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane* plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (plane == NULL)
+ goto free_bo;
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
-
+ *stride = create_req.pitch;
+ return (struct sw_displaytarget *) plane;
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = create_req.handle;
@@ -163,13 +209,19 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;
kms_sw_dt->ref_count --;
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
@@ -178,6 +230,10 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);
+ struct kms_sw_plane * tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
FREE(kms_sw_dt);
}
@@ -187,7 +243,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;
@@ -198,16 +255,20 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (*ptr == NULL) {
+ void * tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }
- DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p %dx%d \n",
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr,
+ plane->width, plane->height);
- return kms_sw_dt->mapped;
+ return *ptr + plane->offset;
}
static struct kms_sw_displaytarget *
@@ -230,10 +291,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}
-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -245,13 +307,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane * plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (plane == NULL)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }
kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;
+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -260,27 +328,27 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;
lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (plane == NULL) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
- return kms_sw_dt;
+ return plane;
}
static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane * plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
- DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
-
- munmap(kms_sw_dt->mapped, kms_sw_dt->size);
- kms_sw_dt->mapped = NULL;
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap buffer %u \n", kms_sw_dt->handle);
}
static struct sw_displaytarget *
@@ -291,30 +359,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;
assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);
- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl) {
+ *stride = kms_sw_pl->stride;
+ }
+ return (struct sw_displaytarget *)kms_sw_pl;
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane * plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return (struct sw_displaytarget *)plane;
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
@@ -331,19 +403,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
switch(whandle->type) {
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.15.1.620.gb9897f4670-goog
Tomasz Figa
2018-01-09 09:15:44 UTC
Permalink
Adding some folks on CC for broader distribution.
Post by Lepton Wu
Gentle ping. Thanks.
Post by Lepton Wu
v2: address comments from Tomasz Figa
a) Add more check for plane size.
b) Avoid duplicated mapping and leaked mapping.
c) Other minor changes.
Change-Id: I0863f522976cc8863d6e95492d9346df35c066ec
---
src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 179 +++++++++++++++-------
1 file changed, 126 insertions(+), 53 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..69c05197081 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,20 +59,29 @@
#define DEBUG_PRINT(msg, ...)
#endif
+struct kms_sw_displaytarget;
-struct kms_sw_displaytarget
-{
- enum pipe_format format;
+struct kms_sw_plane {
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ struct kms_sw_displaytarget* dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
+ struct list_head planes;
};
struct kms_sw_winsys
@@ -83,10 +92,10 @@ struct kms_sw_winsys
struct list_head bo_list;
};
-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct kms_sw_plane *)dt;
}
static inline struct kms_sw_winsys *
@@ -105,6 +114,42 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}
+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset) {
+ struct kms_sw_plane * tmp, * plane = NULL;
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+ LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
+ if (tmp->offset == offset) {
+ plane = tmp;
+ break;
+ }
+ }
+ if (plane) {
+ assert(plane->width == width);
+ assert(plane->height == height);
+ assert(plane->stride == stride);
+ assert(plane->dt == kms_sw_dt);
+ } else {
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (plane == NULL) return NULL;
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ }
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -124,11 +169,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;
+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;
kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -138,17 +182,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;
- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane* plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (plane == NULL)
+ goto free_bo;
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
-
+ *stride = create_req.pitch;
+ return (struct sw_displaytarget *) plane;
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = create_req.handle;
@@ -163,13 +209,19 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;
kms_sw_dt->ref_count --;
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
@@ -178,6 +230,10 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);
+ struct kms_sw_plane * tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
FREE(kms_sw_dt);
}
@@ -187,7 +243,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;
@@ -198,16 +255,20 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (*ptr == NULL) {
+ void * tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }
- DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p %dx%d \n",
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr,
+ plane->width, plane->height);
- return kms_sw_dt->mapped;
+ return *ptr + plane->offset;
}
static struct kms_sw_displaytarget *
@@ -230,10 +291,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}
-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -245,13 +307,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane * plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (plane == NULL)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }
kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;
+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -260,27 +328,27 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;
lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (plane == NULL) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
- return kms_sw_dt;
+ return plane;
}
static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane * plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
- DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
-
- munmap(kms_sw_dt->mapped, kms_sw_dt->size);
- kms_sw_dt->mapped = NULL;
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap buffer %u \n", kms_sw_dt->handle);
}
static struct sw_displaytarget *
@@ -291,30 +359,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;
assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);
- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl) {
+ *stride = kms_sw_pl->stride;
+ }
+ return (struct sw_displaytarget *)kms_sw_pl;
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane * plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return (struct sw_displaytarget *)plane;
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
@@ -331,19 +403,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
switch(whandle->type) {
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.15.1.620.gb9897f4670-goog
Tomasz Figa
2018-02-01 04:39:43 UTC
Permalink
Post by Tomasz Figa
Adding some folks on CC for broader distribution.
Post by Lepton Wu
Gentle ping. Thanks.
Post by Lepton Wu
v2: address comments from Tomasz Figa
a) Add more check for plane size.
b) Avoid duplicated mapping and leaked mapping.
c) Other minor changes.
Change-Id: I0863f522976cc8863d6e95492d9346df35c066ec
---
src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 179 +++++++++++++++-------
1 file changed, 126 insertions(+), 53 deletions(-)
Well, if it's worth anything:

Reviewed-by: Tomasz Figa <***@chromium.org>

Would appreciate if some other user of kms_swrast took a look as well. :)

Best regards,
Tomasz
Emil Velikov
2018-02-21 15:42:43 UTC
Permalink
HI Lepton,

It seems that this has fallen through the cracks.

There's one important functionality change which should be split out
and documented.
The rest is just style nitpicks.
Post by Lepton Wu
v2: address comments from Tomasz Figa
a) Add more check for plane size.
b) Avoid duplicated mapping and leaked mapping.
c) Other minor changes.
Normally I omit saying "other minor changes" since it don't mean anything.
Post by Lepton Wu
@@ -105,6 +114,42 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}
+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset) {
Opening bracket should be at the start of next line.
Post by Lepton Wu
+ struct kms_sw_plane * tmp, * plane = NULL;
Through the patch: no space between * and variable name - example: s/* tmp/*tmp/

Add empty line between declarations and code.
Post by Lepton Wu
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+ LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
+ if (tmp->offset == offset) {
+ plane = tmp;
+ break;
+ }
+ }
+ if (plane) {
+ assert(plane->width == width);
+ assert(plane->height == height);
+ assert(plane->stride == stride);
+ assert(plane->dt == kms_sw_dt);
The only way to hit this if there's serious corruption happening. I'd
just drop it and "return tmp;" in the loop above.
Post by Lepton Wu
+ } else {
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (plane == NULL) return NULL;
The return statement should be on separate line.

Thorough the patch: swap "foo == NULL" with "!foo"
Post by Lepton Wu
@@ -138,17 +182,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
-
+ *stride = create_req.pitch;
+ return (struct sw_displaytarget *) plane;
Swap the cast with an inline wrapper analogous to the kms_sw_plane() one.
Post by Lepton Wu
@@ -163,13 +209,19 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;
kms_sw_dt->ref_count --;
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
This hunk moves the munmap from dt_unmap to dt_destroy.
It should be safe, although it is a subtle functionality change that
should be split out.
A commit message should explain why it's needed, etc.

Ideally, the ro_mapped introduction will be another patch.
Alternatively the commit message should mention it.
Post by Lepton Wu
@@ -198,16 +255,20 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (*ptr == NULL) {
To prevent interment breakage, this NULL check must be part of the
delayer munmap patch.

HTH
Emil
Emil Velikov
2018-03-07 15:14:55 UTC
Permalink
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 26 ++++++++++++++-----
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..30343222470 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
I'm not 100% sure about this. There's a reason why dt_unmap exists.

Skimming through the existing code [1] - there's a handful of cases
that indicate the leaks you're hitting.
I'd look into addressing those properly because as-is this looks alike
a duck-taping the problem.

With the hunk gone the patch is
Reviewed-by: Emil Velikov <***@collabora.com>

-Emil

[1] $ git grep -20 -w displaytarget_[a-z]*map
Lepton Wu
2018-03-07 18:14:52 UTC
Permalink
Actually the reason why I need this CL:

In multi plane patch I'd like to only mmap once for different planes
of same buffer. So actually I need some way
to reuse same mmap for different planes. Then it's natural to have
this CL. The fix to leak is a side effect of this CL.
dt_unmap still works with this CL, if user call dt_map for single
plane buffer multiple times, they will get same pointer, and if they
call dt_unmap
, with this CL, it still get unmapped.
Post by Emil Velikov
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 26 ++++++++++++++-----
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..30343222470 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
I'm not 100% sure about this. There's a reason why dt_unmap exists.
Skimming through the existing code [1] - there's a handful of cases
that indicate the leaks you're hitting.
I'd look into addressing those properly because as-is this looks alike
a duck-taping the problem.
With the hunk gone the patch is
-Emil
[1] $ git grep -20 -w displaytarget_[a-z]*map
Lepton Wu
2018-03-07 18:23:25 UTC
Permalink
I misunderstanding your point, now I got it and will do more test to
see if removing this hunk will cause some issue
in some cases of multi plane code path. Thanks.
Post by Lepton Wu
In multi plane patch I'd like to only mmap once for different planes
of same buffer. So actually I need some way
to reuse same mmap for different planes. Then it's natural to have
this CL. The fix to leak is a side effect of this CL.
dt_unmap still works with this CL, if user call dt_map for single
plane buffer multiple times, they will get same pointer, and if they
call dt_unmap
, with this CL, it still get unmapped.
Post by Emil Velikov
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 26 ++++++++++++++-----
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..30343222470 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
I'm not 100% sure about this. There's a reason why dt_unmap exists.
Skimming through the existing code [1] - there's a handful of cases
that indicate the leaks you're hitting.
I'd look into addressing those properly because as-is this looks alike
a duck-taping the problem.
With the hunk gone the patch is
-Emil
[1] $ git grep -20 -w displaytarget_[a-z]*map
Lepton Wu
2018-03-07 22:36:53 UTC
Permalink
OK, I will send out a new version which omit unmap in dt_destory.
Any way, even we need this code, it could be a separate patch.
Post by Emil Velikov
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 26 ++++++++++++++-----
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..30343222470 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
I'm not 100% sure about this. There's a reason why dt_unmap exists.
Skimming through the existing code [1] - there's a handful of cases
that indicate the leaks you're hitting.
I'd look into addressing those properly because as-is this looks alike
a duck-taping the problem.
With the hunk gone the patch is
-Emil
[1] $ git grep -20 -w displaytarget_[a-z]*map
<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 7,
2018 at 7:14 AM, Emil Velikov <span dir="ltr">&lt;<a
href="mailto:***@gmail.com"
target="_blank">***@gmail.com</a>&gt;</span>
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div
class="HOEnZb"><div class="h5">On 6 March 2018 at 22:43, Lepton Wu
&lt;<a href="mailto:***@chromium.org">***@chromium.org</a>&gt;
wrote:<br>
&gt; If user calls map twice for kms_sw_displaytarget, the first mapped<br>
&gt; buffer could get leaked. Instead of calling mmap every time, just<br>
&gt; reuse previous mapping. Since user could map same displaytarget with<br>
&gt; different flags, we have to keep two different pointers, one for rw<br>
&gt; mapping and one for ro mapping.<br>
&gt;<br>
&gt; Change-Id: I65308f0ff2640bd57b2577c6a3469<wbr>540c9722859<br>
&gt; Signed-off-by: Lepton Wu &lt;<a
href="mailto:***@chromium.org">***@chromium.org</a>&gt;<br>
&gt; ---<br>
&gt;&nbsp; .../winsys/sw/kms-dri/kms_dri_<wbr>sw_winsys.c&nbsp; &nbsp;
&nbsp;| 26 ++++++++++++++-----<br>
&gt;&nbsp; 1 file changed, 19 insertions(+), 7 deletions(-)<br>
&gt;<br>
&gt; diff --git
a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c
b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
&gt; index 22e1c936ac5..30343222470 100644<br>
&gt; --- a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
&gt; +++ b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
&gt; @@ -70,6 +70,7 @@ struct kms_sw_displaytarget<br>
&gt;<br>
&gt;&nbsp; &nbsp; &nbsp;uint32_t handle;<br>
&gt;&nbsp; &nbsp; &nbsp;void *mapped;<br>
&gt; +&nbsp; &nbsp;void *ro_mapped;<br>
&gt;<br>
&gt;&nbsp; &nbsp; &nbsp;int ref_count;<br>
&gt;&nbsp; &nbsp; &nbsp;struct list_head link;<br>
&gt; @@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(<wbr>struct
sw_winsys *ws,<br>
&gt;&nbsp; &nbsp; &nbsp;if (kms_sw_dt-&gt;ref_count &gt; 0)<br>
&gt;&nbsp; &nbsp; &nbsp; &nbsp; return;<br>
&gt;<br>
&gt; +&nbsp; &nbsp;if (kms_sw_dt-&gt;ro_mapped)<br>
&gt; +&nbsp; &nbsp; &nbsp; munmap(kms_sw_dt-&gt;ro_mapped,
kms_sw_dt-&gt;size);<br>
&gt; +&nbsp; &nbsp;if (kms_sw_dt-&gt;mapped)<br>
&gt; +&nbsp; &nbsp; &nbsp; munmap(kms_sw_dt-&gt;mapped, kms_sw_dt-&gt;size);<br>
&gt; +<br>
</div></div>I'm not 100% sure about this. There's a reason why
dt_unmap exists.<br>
<br>
Skimming through the existing code [1] - there's a handful of cases<br>
that indicate the leaks you're hitting.<br>
I'd look into addressing those properly because as-is this looks alike<br>
a duck-taping the problem.<br>
<br>
With the hunk gone the patch is<br>
Reviewed-by: Emil Velikov &lt;<a
href="mailto:***@collabora.com">***@collabora.com</a>&gt;<br>
<br>
-Emil<br>
<br>
[1] $ git grep -20 -w displaytarget_[a-z]*map<br>
</blockquote></div><br></div>
Lepton Wu
2018-03-07 22:39:18 UTC
Permalink
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.

Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..7fc40488c2e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget

uint32_t handle;
void *mapped;
+ void *ro_mapped;

int ref_count;
struct list_head link;
@@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;

prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }

DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);

- return kms_sw_dt->mapped;
+ return *ptr;
}

static struct kms_sw_displaytarget *
@@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);

DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);

munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}

static struct sw_displaytarget *
--
2.16.2.395.g2e18187dfd-goog
Lepton Wu
2018-03-07 22:39:19 UTC
Permalink
Add a new struct kms_sw_plane which delegate a plane and use it
in place of sw_displaytarget. Multiple planes share same underlying
kms_sw_displaytarget.

Change-Id: I0e9ca1d0ba0aa78c27dfdb50c30dc0c424fec172
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 162 +++++++++++++-----
1 file changed, 122 insertions(+), 40 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 7fc40488c2e..ec3c9d9d29e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,13 +59,22 @@
#define DEBUG_PRINT(msg, ...)
#endif

+struct kms_sw_displaytarget;

-struct kms_sw_displaytarget
+struct kms_sw_plane
{
- enum pipe_format format;
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ int mapped;
+ struct kms_sw_displaytarget *dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;

uint32_t handle;
@@ -74,6 +83,7 @@ struct kms_sw_displaytarget

int ref_count;
struct list_head link;
+ struct list_head planes;
};

struct kms_sw_winsys
@@ -84,10 +94,16 @@ struct kms_sw_winsys
struct list_head bo_list;
};

-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct kms_sw_plane *)dt;
+}
+
+static inline struct sw_displaytarget *
+sw_displaytarget( struct kms_sw_plane *pl)
+{
+ return (struct sw_displaytarget *)pl;
}

static inline struct kms_sw_winsys *
@@ -106,6 +122,39 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}

+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset)
+{
+ struct kms_sw_plane *plane = NULL;
+
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->offset == offset)
+ return plane;
+ }
+
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (!plane)
+ return NULL;
+
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -125,11 +174,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;

+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;

kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;

memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -139,16 +187,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;

- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane *plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (!plane)
+ goto free_bo;

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);

- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ *stride = create_req.pitch;
+ return sw_displaytarget(plane);

free_bo:
memset(&destroy_req, 0, sizeof destroy_req);
@@ -164,7 +215,8 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;

kms_sw_dt->ref_count --;
@@ -179,6 +231,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,

DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);

+ struct kms_sw_plane *tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
+
FREE(kms_sw_dt);
}

@@ -188,7 +245,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;

@@ -211,7 +269,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
kms_sw_dt->handle, kms_sw_dt->size, *ptr);

- return *ptr;
+ plane->mapped = 1;
+ return *ptr + plane->offset;
}

static struct kms_sw_displaytarget *
@@ -234,10 +293,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}

-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -249,13 +309,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;

kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane *plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }

kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;

+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -264,22 +330,33 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;

lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

- return kms_sw_dt;
+ return plane;
}

static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
+
+ plane->mapped = 0;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->mapped) {
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap buffer %u \n", kms_sw_dt->handle);
+ return;
+ }
+ }

DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
@@ -298,30 +375,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;
+

assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);

- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
case DRM_API_HANDLE_TYPE_FD:
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl)
+ *stride = kms_sw_pl->stride;
+ return sw_displaytarget(kms_sw_pl);
case DRM_API_HANDLE_TYPE_KMS:
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane *plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return sw_displaytarget(plane);
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
default:
@@ -338,19 +419,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

switch(whandle->type) {
case DRM_API_HANDLE_TYPE_KMS:
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
case DRM_API_HANDLE_TYPE_FD:
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.16.2.395.g2e18187dfd-goog
Lepton Wu
2018-03-07 22:42:01 UTC
Permalink
For this patch, actually it's as same as V3. But since it depends on
the 1st patch, I also update the title to V4.
Add a new struct kms_sw_plane which delegate a plane and use it
in place of sw_displaytarget. Multiple planes share same underlying
kms_sw_displaytarget.
Change-Id: I0e9ca1d0ba0aa78c27dfdb50c30dc0c424fec172
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 162 +++++++++++++-----
1 file changed, 122 insertions(+), 40 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 7fc40488c2e..ec3c9d9d29e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,13 +59,22 @@
#define DEBUG_PRINT(msg, ...)
#endif
+struct kms_sw_displaytarget;
-struct kms_sw_displaytarget
+struct kms_sw_plane
{
- enum pipe_format format;
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ int mapped;
+ struct kms_sw_displaytarget *dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;
uint32_t handle;
@@ -74,6 +83,7 @@ struct kms_sw_displaytarget
int ref_count;
struct list_head link;
+ struct list_head planes;
};
struct kms_sw_winsys
@@ -84,10 +94,16 @@ struct kms_sw_winsys
struct list_head bo_list;
};
-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct kms_sw_plane *)dt;
+}
+
+static inline struct sw_displaytarget *
+sw_displaytarget( struct kms_sw_plane *pl)
+{
+ return (struct sw_displaytarget *)pl;
}
static inline struct kms_sw_winsys *
@@ -106,6 +122,39 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}
+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset)
+{
+ struct kms_sw_plane *plane = NULL;
+
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->offset == offset)
+ return plane;
+ }
+
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (!plane)
+ return NULL;
+
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -125,11 +174,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;
+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;
kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -139,16 +187,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;
- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane *plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (!plane)
+ goto free_bo;
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ *stride = create_req.pitch;
+ return sw_displaytarget(plane);
memset(&destroy_req, 0, sizeof destroy_req);
@@ -164,7 +215,8 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;
kms_sw_dt->ref_count --;
@@ -179,6 +231,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);
+ struct kms_sw_plane *tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
+
FREE(kms_sw_dt);
}
@@ -188,7 +245,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;
@@ -211,7 +269,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
kms_sw_dt->handle, kms_sw_dt->size, *ptr);
- return *ptr;
+ plane->mapped = 1;
+ return *ptr + plane->offset;
}
static struct kms_sw_displaytarget *
@@ -234,10 +293,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}
-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -249,13 +309,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane *plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }
kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;
+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -264,22 +330,33 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;
lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
- return kms_sw_dt;
+ return plane;
}
static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
+
+ plane->mapped = 0;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->mapped) {
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap buffer %u \n", kms_sw_dt->handle);
+ return;
+ }
+ }
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
@@ -298,30 +375,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;
+
assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);
- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl)
+ *stride = kms_sw_pl->stride;
+ return sw_displaytarget(kms_sw_pl);
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane *plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return sw_displaytarget(plane);
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
@@ -338,19 +419,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
switch(whandle->type) {
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.16.2.395.g2e18187dfd-goog
<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 7,
2018 at 2:39 PM, Lepton Wu <span dir="ltr">&lt;<a
href="mailto:***@chromium.org"
target="_blank">***@chromium.org</a>&gt;</span>
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Add a new struct
kms_sw_plane which delegate a plane and use it<br>
in place of sw_displaytarget. Multiple planes share same underlying<br>
kms_sw_displaytarget.<br>
<br>
Change-Id: I0e9ca1d0ba0aa78c27dfdb50c30dc<wbr>0c424fec172<br>
Signed-off-by: Lepton Wu &lt;<a
href="mailto:***@chromium.org">***@chromium.org</a>&gt;<br>
---<br>
&nbsp;.../winsys/sw/kms-dri/kms_dri_<wbr>sw_winsys.c&nbsp; &nbsp;
&nbsp;| 162 +++++++++++++-----<br>
&nbsp;1 file changed, 122 insertions(+), 40 deletions(-)<br>
<br>
diff --git a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c
b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
index 7fc40488c2e..ec3c9d9d29e 100644<br>
--- a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
+++ b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br>
@@ -59,13 +59,22 @@<br>
&nbsp;#define DEBUG_PRINT(msg, ...)<br>
&nbsp;#endif<br>
<br>
+struct kms_sw_displaytarget;<br>
<br>
-struct kms_sw_displaytarget<br>
+struct kms_sw_plane<br>
&nbsp;{<br>
-&nbsp; &nbsp;enum pipe_format format;<br>
&nbsp; &nbsp; unsigned width;<br>
&nbsp; &nbsp; unsigned height;<br>
&nbsp; &nbsp; unsigned stride;<br>
+&nbsp; &nbsp;unsigned offset;<br>
+&nbsp; &nbsp;int mapped;<br>
+&nbsp; &nbsp;struct kms_sw_displaytarget *dt;<br>
+&nbsp; &nbsp;struct list_head link;<br>
+};<br>
+<br>
+struct kms_sw_displaytarget<br>
+{<br>
+&nbsp; &nbsp;enum pipe_format format;<br>
&nbsp; &nbsp; unsigned size;<br>
<br>
&nbsp; &nbsp; uint32_t handle;<br>
@@ -74,6 +83,7 @@ struct kms_sw_displaytarget<br>
<br>
&nbsp; &nbsp; int ref_count;<br>
&nbsp; &nbsp; struct list_head link;<br>
+&nbsp; &nbsp;struct list_head planes;<br>
&nbsp;};<br>
<br>
&nbsp;struct kms_sw_winsys<br>
@@ -84,10 +94,16 @@ struct kms_sw_winsys<br>
&nbsp; &nbsp; struct list_head bo_list;<br>
&nbsp;};<br>
<br>
-static inline struct kms_sw_displaytarget *<br>
-kms_sw_displaytarget( struct sw_displaytarget *dt )<br>
+static inline struct kms_sw_plane *<br>
+kms_sw_plane( struct sw_displaytarget *dt )<br>
&nbsp;{<br>
-&nbsp; &nbsp;return (struct kms_sw_displaytarget *)dt;<br>
+&nbsp; &nbsp;return (struct kms_sw_plane *)dt;<br>
+}<br>
+<br>
+static inline struct sw_displaytarget *<br>
+sw_displaytarget( struct kms_sw_plane *pl)<br>
+{<br>
+&nbsp; &nbsp;return (struct sw_displaytarget *)pl;<br>
&nbsp;}<br>
<br>
&nbsp;static inline struct kms_sw_winsys *<br>
@@ -106,6 +122,39 @@ kms_sw_is_displaytarget_<wbr>format_supported(
struct sw_winsys *ws,<br>
&nbsp; &nbsp; return TRUE;<br>
&nbsp;}<br>
<br>
+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget
*kms_sw_dt,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; enum
pipe_format format,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
unsigned width, unsigned height,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
unsigned stride, unsigned offset)<br>
+{<br>
+&nbsp; &nbsp;struct kms_sw_plane *plane = NULL;<br>
+<br>
+&nbsp; &nbsp;if (offset + util_format_get_2d_size(<wbr>format,
stride, height) &gt;<br>
+&nbsp; &nbsp; &nbsp; &nbsp;kms_sw_dt-&gt;size) {<br>
+&nbsp; &nbsp; &nbsp; DEBUG_PRINT("KMS-DEBUG: plane too big. format:
%d stride: %d height: %d "<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
"offset: %d size:%d\n", format, stride, height, offset,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
kms_sw_dt-&gt;size);<br>
+&nbsp; &nbsp; &nbsp; return NULL;<br>
+&nbsp; &nbsp;}<br>
+<br>
+&nbsp; &nbsp;LIST_FOR_EACH_ENTRY(plane, &amp;kms_sw_dt-&gt;planes, link) {<br>
+&nbsp; &nbsp; &nbsp; if (plane-&gt;offset == offset)<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;return plane;<br>
+&nbsp; &nbsp;}<br>
+<br>
+&nbsp; &nbsp;plane = CALLOC_STRUCT(kms_sw_plane);<br>
+&nbsp; &nbsp;if (!plane)<br>
+&nbsp; &nbsp; &nbsp; return NULL;<br>
+<br>
+&nbsp; &nbsp;plane-&gt;width = width;<br>
+&nbsp; &nbsp;plane-&gt;height = height;<br>
+&nbsp; &nbsp;plane-&gt;stride = stride;<br>
+&nbsp; &nbsp;plane-&gt;offset = offset;<br>
+&nbsp; &nbsp;plane-&gt;dt = kms_sw_dt;<br>
+&nbsp; &nbsp;list_add(&amp;plane-&gt;link, &amp;kms_sw_dt-&gt;planes);<br>
+&nbsp; &nbsp;return plane;<br>
+}<br>
+<br>
&nbsp;static struct sw_displaytarget *<br>
&nbsp;kms_sw_displaytarget_create(<wbr>struct sw_winsys *ws,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;unsigned tex_usage,<br>
@@ -125,11 +174,10 @@ kms_sw_displaytarget_create(<wbr>struct sw_winsys *ws,<br>
&nbsp; &nbsp; if (!kms_sw_dt)<br>
&nbsp; &nbsp; &nbsp; &nbsp;goto no_dt;<br>
<br>
+&nbsp; &nbsp;list_inithead(&amp;kms_sw_dt-&gt;<wbr>planes);<br>
&nbsp; &nbsp; kms_sw_dt-&gt;ref_count = 1;<br>
<br>
&nbsp; &nbsp; kms_sw_dt-&gt;format = format;<br>
-&nbsp; &nbsp;kms_sw_dt-&gt;width = width;<br>
-&nbsp; &nbsp;kms_sw_dt-&gt;height = height;<br>
<br>
&nbsp; &nbsp; memset(&amp;create_req, 0, sizeof(create_req));<br>
&nbsp; &nbsp; create_req.bpp = 32;<br>
@@ -139,16 +187,19 @@ kms_sw_displaytarget_create(<wbr>struct sw_winsys *ws,<br>
&nbsp; &nbsp; if (ret)<br>
&nbsp; &nbsp; &nbsp; &nbsp;goto free_bo;<br>
<br>
-&nbsp; &nbsp;kms_sw_dt-&gt;stride = create_req.pitch;<br>
&nbsp; &nbsp; kms_sw_dt-&gt;size = create_req.size;<br>
&nbsp; &nbsp; kms_sw_dt-&gt;handle = create_req.handle;<br>
+&nbsp; &nbsp;struct kms_sw_plane *plane = get_plane(kms_sw_dt,
format, width, height,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; create_req.pitch, 0);<br>
+&nbsp; &nbsp;if (!plane)<br>
+&nbsp; &nbsp; &nbsp; goto free_bo;<br>
<br>
&nbsp; &nbsp; list_add(&amp;kms_sw_dt-&gt;link, &amp;kms_sw-&gt;bo_list);<br>
<br>
&nbsp; &nbsp; DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n",
kms_sw_dt-&gt;handle, kms_sw_dt-&gt;size);<br>
<br>
-&nbsp; &nbsp;*stride = kms_sw_dt-&gt;stride;<br>
-&nbsp; &nbsp;return (struct sw_displaytarget *)kms_sw_dt;<br>
+&nbsp; &nbsp;*stride = create_req.pitch;<br>
+&nbsp; &nbsp;return sw_displaytarget(plane);<br>
<br>
&nbsp; free_bo:<br>
&nbsp; &nbsp; memset(&amp;destroy_req, 0, sizeof destroy_req);<br>
@@ -164,7 +215,8 @@ kms_sw_displaytarget_destroy(<wbr>struct sw_winsys *ws,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; struct sw_displaytarget *dt)<br>
&nbsp;{<br>
&nbsp; &nbsp; struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);<br>
-&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt =
kms_sw_displaytarget(dt);<br>
+&nbsp; &nbsp;struct kms_sw_plane *plane = kms_sw_plane(dt);<br>
+&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt = plane-&gt;dt;<br>
&nbsp; &nbsp; struct drm_mode_destroy_dumb destroy_req;<br>
<br>
&nbsp; &nbsp; kms_sw_dt-&gt;ref_count --;<br>
@@ -179,6 +231,11 @@ kms_sw_displaytarget_destroy(<wbr>struct sw_winsys *ws,<br>
<br>
&nbsp; &nbsp; DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n",
kms_sw_dt-&gt;handle);<br>
<br>
+&nbsp; &nbsp;struct kms_sw_plane *tmp;<br>
+&nbsp; &nbsp;LIST_FOR_EACH_ENTRY_SAFE(<wbr>plane, tmp,
&amp;kms_sw_dt-&gt;planes, link) {<br>
+&nbsp; &nbsp; &nbsp; FREE(plane);<br>
+&nbsp; &nbsp;}<br>
+<br>
&nbsp; &nbsp; FREE(kms_sw_dt);<br>
&nbsp;}<br>
<br>
@@ -188,7 +245,8 @@ kms_sw_displaytarget_map(<wbr>struct sw_winsys *ws,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; unsigned flags)<br>
&nbsp;{<br>
&nbsp; &nbsp; struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);<br>
-&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt =
kms_sw_displaytarget(dt);<br>
+&nbsp; &nbsp;struct kms_sw_plane *plane = kms_sw_plane(dt);<br>
+&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt = plane-&gt;dt;<br>
&nbsp; &nbsp; struct drm_mode_map_dumb map_req;<br>
&nbsp; &nbsp; int prot, ret;<br>
<br>
@@ -211,7 +269,8 @@ kms_sw_displaytarget_map(<wbr>struct sw_winsys *ws,<br>
&nbsp; &nbsp; DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; kms_sw_dt-&gt;handle,
kms_sw_dt-&gt;size, *ptr);<br>
<br>
-&nbsp; &nbsp;return *ptr;<br>
+&nbsp; &nbsp;plane-&gt;mapped = 1;<br>
+&nbsp; &nbsp;return *ptr + plane-&gt;offset;<br>
&nbsp;}<br>
<br>
&nbsp;static struct kms_sw_displaytarget *<br>
@@ -234,10 +293,11 @@ kms_sw_displaytarget_find_and_<wbr>ref(struct
kms_sw_winsys *kms_sw,<br>
&nbsp; &nbsp; return NULL;<br>
&nbsp;}<br>
<br>
-static struct kms_sw_displaytarget *<br>
+static struct kms_sw_plane *<br>
&nbsp;kms_sw_displaytarget_add_from_<wbr>prime(struct kms_sw_winsys
*kms_sw, int fd,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; enum
pipe_format format,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;unsigned
width, unsigned height,<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; unsigned
stride)<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; unsigned
stride, unsigned offset)<br>
&nbsp;{<br>
&nbsp; &nbsp; uint32_t handle = -1;<br>
&nbsp; &nbsp; struct kms_sw_displaytarget * kms_sw_dt;<br>
@@ -249,13 +309,19 @@ kms_sw_displaytarget_add_from_<wbr>prime(struct
kms_sw_winsys *kms_sw, int fd,<br>
&nbsp; &nbsp; &nbsp; &nbsp;return NULL;<br>
<br>
&nbsp; &nbsp; kms_sw_dt =
kms_sw_displaytarget_find_and_<wbr>ref(kms_sw, handle);<br>
-&nbsp; &nbsp;if (kms_sw_dt)<br>
-&nbsp; &nbsp; &nbsp; return kms_sw_dt;<br>
+&nbsp; &nbsp;struct kms_sw_plane *plane = NULL;<br>
+&nbsp; &nbsp;if (kms_sw_dt) {<br>
+&nbsp; &nbsp; &nbsp; plane = get_plane(kms_sw_dt, format, width,
height, stride, offset);<br>
+&nbsp; &nbsp; &nbsp; if (!plane)<br>
+&nbsp; &nbsp; &nbsp; &nbsp; kms_sw_dt-&gt;ref_count --;<br>
+&nbsp; &nbsp; &nbsp; return plane;<br>
+&nbsp; &nbsp;}<br>
<br>
&nbsp; &nbsp; kms_sw_dt = CALLOC_STRUCT(kms_sw_<wbr>displaytarget);<br>
&nbsp; &nbsp; if (!kms_sw_dt)<br>
&nbsp; &nbsp; &nbsp; &nbsp;return NULL;<br>
<br>
+&nbsp; &nbsp;list_inithead(&amp;kms_sw_dt-&gt;<wbr>planes);<br>
&nbsp; &nbsp; off_t lseek_ret = lseek(fd, 0, SEEK_END);<br>
&nbsp; &nbsp; if (lseek_ret == -1) {<br>
&nbsp; &nbsp; &nbsp; &nbsp;FREE(kms_sw_dt);<br>
@@ -264,22 +330,33 @@ kms_sw_displaytarget_add_from_<wbr>prime(struct
kms_sw_winsys *kms_sw, int fd,<br>
&nbsp; &nbsp; kms_sw_dt-&gt;size = lseek_ret;<br>
&nbsp; &nbsp; kms_sw_dt-&gt;ref_count = 1;<br>
&nbsp; &nbsp; kms_sw_dt-&gt;handle = handle;<br>
-&nbsp; &nbsp;kms_sw_dt-&gt;width = width;<br>
-&nbsp; &nbsp;kms_sw_dt-&gt;height = height;<br>
-&nbsp; &nbsp;kms_sw_dt-&gt;stride = stride;<br>
<br>
&nbsp; &nbsp; lseek(fd, 0, SEEK_SET);<br>
+&nbsp; &nbsp;plane = get_plane(kms_sw_dt, format, width, height,
stride, offset);<br>
+&nbsp; &nbsp;if (!plane) {<br>
+&nbsp; &nbsp; &nbsp; FREE(kms_sw_dt);<br>
+&nbsp; &nbsp; &nbsp; return NULL;<br>
+&nbsp; &nbsp;}<br>
<br>
&nbsp; &nbsp; list_add(&amp;kms_sw_dt-&gt;link, &amp;kms_sw-&gt;bo_list);<br>
<br>
-&nbsp; &nbsp;return kms_sw_dt;<br>
+&nbsp; &nbsp;return plane;<br>
&nbsp;}<br>
<br>
&nbsp;static void<br>
&nbsp;kms_sw_displaytarget_unmap(<wbr>struct sw_winsys *ws,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; struct sw_displaytarget *dt)<br>
&nbsp;{<br>
-&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt =
kms_sw_displaytarget(dt);<br>
+&nbsp; &nbsp;struct kms_sw_plane *plane = kms_sw_plane(dt);<br>
+&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt = plane-&gt;dt;<br>
+<br>
+&nbsp; &nbsp;plane-&gt;mapped = 0;<br>
+&nbsp; &nbsp;LIST_FOR_EACH_ENTRY(plane, &amp;kms_sw_dt-&gt;planes, link) {<br>
+&nbsp; &nbsp; &nbsp; if (plane-&gt;mapped) {<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;DEBUG_PRINT("KMS-DEBUG: ignore
unmap buffer %u \n", kms_sw_dt-&gt;handle);<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;return;<br>
+&nbsp; &nbsp; &nbsp; }<br>
+&nbsp; &nbsp;}<br>
<br>
&nbsp; &nbsp; DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n",
kms_sw_dt-&gt;handle, kms_sw_dt-&gt;mapped);<br>
&nbsp; &nbsp; DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n",
kms_sw_dt-&gt;handle, kms_sw_dt-&gt;ro_mapped);<br>
@@ -298,30 +375,34 @@ kms_sw_displaytarget_from_<wbr>handle(struct
sw_winsys *ws,<br>
&nbsp;{<br>
&nbsp; &nbsp; struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);<br>
&nbsp; &nbsp; struct kms_sw_displaytarget *kms_sw_dt;<br>
+&nbsp; &nbsp;struct kms_sw_plane *kms_sw_pl;<br>
+<br>
<br>
&nbsp; &nbsp; assert(whandle-&gt;type == DRM_API_HANDLE_TYPE_KMS ||<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;whandle-&gt;type ==
DRM_API_HANDLE_TYPE_FD);<br>
<br>
-&nbsp; &nbsp;if (whandle-&gt;offset != 0) {<br>
-&nbsp; &nbsp; &nbsp; DEBUG_PRINT("KMS-DEBUG: attempt to import
unsupported winsys offset %d\n",<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
whandle-&gt;offset);<br>
-&nbsp; &nbsp; &nbsp; return NULL;<br>
-&nbsp; &nbsp;}<br>
-<br>
&nbsp; &nbsp; switch(whandle-&gt;type) {<br>
&nbsp; &nbsp; case DRM_API_HANDLE_TYPE_FD:<br>
-&nbsp; &nbsp; &nbsp; kms_sw_dt =
kms_sw_displaytarget_add_from_<wbr>prime(kms_sw,
whandle-&gt;handle,<br>
+&nbsp; &nbsp; &nbsp; kms_sw_pl =
kms_sw_displaytarget_add_from_<wbr>prime(kms_sw,
whandle-&gt;handle,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; templ-&gt;format,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp;templ-&gt;width0,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp;templ-&gt;height0,<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
whandle-&gt;stride);<br>
-&nbsp; &nbsp; &nbsp; if (kms_sw_dt)<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;*stride = kms_sw_dt-&gt;stride;<br>
-&nbsp; &nbsp; &nbsp; return (struct sw_displaytarget *)kms_sw_dt;<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
whandle-&gt;stride,<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
whandle-&gt;offset);<br>
+&nbsp; &nbsp; &nbsp; if (kms_sw_pl)<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;*stride = kms_sw_pl-&gt;stride;<br>
+&nbsp; &nbsp; &nbsp; return sw_displaytarget(kms_sw_pl);<br>
&nbsp; &nbsp; case DRM_API_HANDLE_TYPE_KMS:<br>
&nbsp; &nbsp; &nbsp; &nbsp;kms_sw_dt =
kms_sw_displaytarget_find_and_<wbr>ref(kms_sw,
whandle-&gt;handle);<br>
&nbsp; &nbsp; &nbsp; &nbsp;if (kms_sw_dt) {<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;*stride = kms_sw_dt-&gt;stride;<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;return (struct sw_displaytarget
*)kms_sw_dt;<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;struct kms_sw_plane *plane;<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;LIST_FOR_EACH_ENTRY(plane,
&amp;kms_sw_dt-&gt;planes, link) {<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (whandle-&gt;offset ==
plane-&gt;offset) {<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;*stride =
plane-&gt;stride;<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;return
sw_displaytarget(plane);<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;}<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;kms_sw_dt-&gt;ref_count --;<br>
&nbsp; &nbsp; &nbsp; &nbsp;}<br>
&nbsp; &nbsp; &nbsp; &nbsp;/* fallthrough */<br>
&nbsp; &nbsp; default:<br>
@@ -338,19 +419,20 @@ kms_sw_displaytarget_get_<wbr>handle(struct
sw_winsys *winsys,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;struct winsys_handle
*whandle)<br>
&nbsp;{<br>
&nbsp; &nbsp; struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);<br>
-&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt =
kms_sw_displaytarget(dt);<br>
+&nbsp; &nbsp;struct kms_sw_plane *plane = kms_sw_plane(dt);<br>
+&nbsp; &nbsp;struct kms_sw_displaytarget *kms_sw_dt = plane-&gt;dt;<br>
<br>
&nbsp; &nbsp; switch(whandle-&gt;type) {<br>
&nbsp; &nbsp; case DRM_API_HANDLE_TYPE_KMS:<br>
&nbsp; &nbsp; &nbsp; &nbsp;whandle-&gt;handle = kms_sw_dt-&gt;handle;<br>
-&nbsp; &nbsp; &nbsp; whandle-&gt;stride = kms_sw_dt-&gt;stride;<br>
-&nbsp; &nbsp; &nbsp; whandle-&gt;offset = 0;<br>
+&nbsp; &nbsp; &nbsp; whandle-&gt;stride = plane-&gt;stride;<br>
+&nbsp; &nbsp; &nbsp; whandle-&gt;offset = plane-&gt;offset;<br>
&nbsp; &nbsp; &nbsp; &nbsp;return TRUE;<br>
&nbsp; &nbsp; case DRM_API_HANDLE_TYPE_FD:<br>
&nbsp; &nbsp; &nbsp; &nbsp;if (!drmPrimeHandleToFD(kms_sw-&gt;<wbr>fd,
kms_sw_dt-&gt;handle,<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; DRM_CLOEXEC,
(int*)&amp;whandle-&gt;handle)) {<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;whandle-&gt;stride =
kms_sw_dt-&gt;stride;<br>
-&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;whandle-&gt;offset = 0;<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;whandle-&gt;stride = plane-&gt;stride;<br>
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;whandle-&gt;offset = plane-&gt;offset;<br>
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; return TRUE;<br>
&nbsp; &nbsp; &nbsp; &nbsp;}<br>
&nbsp; &nbsp; &nbsp; &nbsp;/* fallthrough */<br>
<span class="HOEnZb"><font color="#888888">--<br>
2.16.2.395.g2e18187dfd-goog<br>
<br>
</font></span></blockquote></div><br></div>
Lepton Wu
2018-03-12 17:45:56 UTC
Permalink
Ping. Any more comments or missing stuff to get this commited into master?

Thanks.
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..7fc40488c2e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
- return kms_sw_dt->mapped;
+ return *ptr;
}
static struct kms_sw_displaytarget *
@@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}
static struct sw_displaytarget *
--
2.16.2.395.g2e18187dfd-goog
Emil Velikov
2018-03-12 18:10:43 UTC
Permalink
Post by Lepton Wu
Ping. Any more comments or missing stuff to get this commited into master?
As things have changed a bit (the original map/unmap behaviour is
preserved) I was hoping that Tomasz will give it another look.
If he prefers, I could add some revision summary and keep him as reviewer of v1?

Thanks
Emil
Tomasz Figa
2018-03-13 11:42:08 UTC
Permalink
Post by Emil Velikov
Post by Lepton Wu
Ping. Any more comments or missing stuff to get this commited into master?
As things have changed a bit (the original map/unmap behaviour is
preserved) I was hoping that Tomasz will give it another look.
If he prefers, I could add some revision summary and keep him as reviewer of v1?
Thanks Emil. I'll take a look. Sorry for being late to the party.

Best regards,
Tomasz
Tomasz Figa
2018-03-13 11:46:55 UTC
Permalink
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..7fc40488c2e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
- return kms_sw_dt->mapped;
+ return *ptr;
}
static struct kms_sw_displaytarget *
@@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}
If user calls map twice, wouldn't they also call unmap twice?
Moreover, wouldn't the pointer be expected to be still valid between
first and second unmap?

The answer obviously depends on how the API is designed, but i feels
really weird being asymmetrical like that. Typically the mapping would
be refcounted and maps would have to be balanced with unmaps to free
the mapping.

Best regards,
Tomasz
Emil Velikov
2018-03-13 15:41:09 UTC
Permalink
Post by Tomasz Figa
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..7fc40488c2e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
- return kms_sw_dt->mapped;
+ return *ptr;
}
static struct kms_sw_displaytarget *
@@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}
If user calls map twice, wouldn't they also call unmap twice?
Moreover, wouldn't the pointer be expected to be still valid between
first and second unmap?
The answer obviously depends on how the API is designed, but i feels
really weird being asymmetrical like that. Typically the mapping would
be refcounted and maps would have to be balanced with unmaps to free
the mapping.
Valid points.

If you guys prefer we could land 2/2 (multiplane support), since it
has no dependency of the mapping work.
And polish out ro/rw mappings (even the leaks) at later stage, as time permits?

-Emil
Lepton Wu
2018-03-14 01:20:27 UTC
Permalink
I am fine to add ref count for map pointer but then the code looks a
little complex:
We already have ref count for display target, and it seems most other
drivers don't have a
ref count for map pointer. (I checked dri_sw_displaytarget_map /
gdi_sw_displaytarget_map/hgl_winsys_displaytarget_map
/xlib_displaytarget_map)

If you really want a reference count for map pointer, I will add one.
But I am just feeling that introduce some unnecessary complex.
Just want to get confirmation before I begin to write code.

Thanks!
Post by Emil Velikov
Post by Tomasz Figa
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..7fc40488c2e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
- return kms_sw_dt->mapped;
+ return *ptr;
}
static struct kms_sw_displaytarget *
@@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}
If user calls map twice, wouldn't they also call unmap twice?
Moreover, wouldn't the pointer be expected to be still valid between
first and second unmap?
The answer obviously depends on how the API is designed, but i feels
really weird being asymmetrical like that. Typically the mapping would
be refcounted and maps would have to be balanced with unmaps to free
the mapping.
Valid points.
If you guys prefer we could land 2/2 (multiplane support), since it
has no dependency of the mapping work.
And polish out ro/rw mappings (even the leaks) at later stage, as time permits?
-Emil
Tomasz Figa
2018-03-16 07:06:20 UTC
Permalink
Hi Lepton,

Please avoid top-posting in mailing lists in the future. It's against
the netiquette.

On Wed, Mar 14, 2018 at 10:20 AM, Lepton Wu <***@chromium.org> wrote:
[Message moved to bottom]
Post by Lepton Wu
Post by Emil Velikov
Post by Tomasz Figa
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 21 ++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..7fc40488c2e 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,6 +70,7 @@ struct kms_sw_displaytarget
uint32_t handle;
void *mapped;
+ void *ro_mapped;
int ref_count;
struct list_head link;
@@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
- return kms_sw_dt->mapped;
+ return *ptr;
}
static struct kms_sw_displaytarget *
@@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}
If user calls map twice, wouldn't they also call unmap twice?
Moreover, wouldn't the pointer be expected to be still valid between
first and second unmap?
The answer obviously depends on how the API is designed, but i feels
really weird being asymmetrical like that. Typically the mapping would
be refcounted and maps would have to be balanced with unmaps to free
the mapping.
Valid points.
If you guys prefer we could land 2/2 (multiplane support), since it
has no dependency of the mapping work.
And polish out ro/rw mappings (even the leaks) at later stage, as time permits?
-Emil
I am fine to add ref count for map pointer but then the code looks a
We already have ref count for display target, and it seems most other
drivers don't have a
ref count for map pointer. (I checked dri_sw_displaytarget_map /
gdi_sw_displaytarget_map/hgl_winsys_displaytarget_map
/xlib_displaytarget_map)
If you really want a reference count for map pointer, I will add one.
But I am just feeling that introduce some unnecessary complex.
Just want to get confirmation before I begin to write code.
Looking at users, I found at least one that it's quite realistic to
have two separate maps created and expected to work, because of
semantics of pipe->transfer_map() API:

https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/softpipe/sp_texture.c#n355

You can see that every time it creates a new, independent
pipe_transfer object, which should be expected to be valid until
pipe->transfer_unmap() on this particular object is not called.

Also, looking at the implementations you mentioned:

1) dri_sw

The real buffer is not being mapped. There is a staging buffer
allocated with malloc() at display target creation time and data are
read-back to it in dri_sw_displaytarget_map() and written-back to the
real buffer in dri_sw_displaytarget_unmap(). The dri_sw_dt->mapped is
not needed for anything.

2) gdi_sw, hgl_winsys

The buffer is allocated by malloc_aligned() and so map() simply
returns that pointer and unmap() is a no-op.

3) xlib

The buffer is either an shmem or malloc buffer and the behavior is
essentially the same as 2). The xlib_dt->mapped variable is not needed
for anything.

So, if we are mapping the real buffer, we need to reference count the mapping.

Best regards,
Tomasz
Lepton Wu
2018-03-16 18:09:03 UTC
Permalink
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.

Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 44 +++++++++++++++----
1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac5..b4fb852833d 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,8 +70,10 @@ struct kms_sw_displaytarget

uint32_t handle;
void *mapped;
+ void *ro_mapped;

int ref_count;
+ int map_count;
struct list_head link;
};

@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;

+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
@@ -198,16 +208,21 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;

prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }

DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
+
+ kms_sw_dt->map_count++;

- return kms_sw_dt->mapped;
+ return *ptr;
}

static struct kms_sw_displaytarget *
@@ -277,10 +292,23 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
{
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);

- DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ if (!kms_sw_dt->map_count) {
+ DEBUG_PRINT("KMS-DEBUG: ignore duplicated unmap %u", kms_sw_dt->handle);
+ return;
+ }
+ kms_sw_dt->map_count--;
+ if (kms_sw_dt->map_count) {
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap for busy buffer %u", kms_sw_dt->handle);
+ return;
+ }

+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
+
munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}

static struct sw_displaytarget *
--
2.16.2.804.g6dcf76e118-goog
Lepton Wu
2018-03-16 18:09:04 UTC
Permalink
Add a new struct kms_sw_plane which delegate a plane and use it
in place of sw_displaytarget. Multiple planes share same underlying
kms_sw_displaytarget.

Change-Id: I0e9ca1d0ba0aa78c27dfdb50c30dc0c424fec172
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 152 +++++++++++++-----
1 file changed, 112 insertions(+), 40 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index b4fb852833d..04756ad3073 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,13 +59,21 @@
#define DEBUG_PRINT(msg, ...)
#endif

+struct kms_sw_displaytarget;

-struct kms_sw_displaytarget
+struct kms_sw_plane
{
- enum pipe_format format;
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ struct kms_sw_displaytarget *dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;

uint32_t handle;
@@ -75,6 +83,7 @@ struct kms_sw_displaytarget
int ref_count;
int map_count;
struct list_head link;
+ struct list_head planes;
};

struct kms_sw_winsys
@@ -85,10 +94,16 @@ struct kms_sw_winsys
struct list_head bo_list;
};

-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
+{
+ return (struct kms_sw_plane *)dt;
+}
+
+static inline struct sw_displaytarget *
+sw_displaytarget( struct kms_sw_plane *pl)
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct sw_displaytarget *)pl;
}

static inline struct kms_sw_winsys *
@@ -107,6 +122,39 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}

+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset)
+{
+ struct kms_sw_plane *plane = NULL;
+
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->offset == offset)
+ return plane;
+ }
+
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (!plane)
+ return NULL;
+
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -126,11 +174,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;

+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;

kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;

memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -140,16 +187,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;

- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane *plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (!plane)
+ goto free_bo;

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);

- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ *stride = create_req.pitch;
+ return sw_displaytarget(plane);

free_bo:
memset(&destroy_req, 0, sizeof destroy_req);
@@ -165,7 +215,8 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;

kms_sw_dt->ref_count --;
@@ -188,6 +239,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,

DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);

+ struct kms_sw_plane *tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
+
FREE(kms_sw_dt);
}

@@ -197,7 +253,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;

@@ -222,7 +279,7 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,

kms_sw_dt->map_count++;

- return *ptr;
+ return *ptr + plane->offset;
}

static struct kms_sw_displaytarget *
@@ -245,10 +302,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}

-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -260,13 +318,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;

kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane *plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }

kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;

+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -275,22 +339,25 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;

lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

- return kms_sw_dt;
+ return plane;
}

static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

if (!kms_sw_dt->map_count) {
DEBUG_PRINT("KMS-DEBUG: ignore duplicated unmap %u", kms_sw_dt->handle);
@@ -319,30 +386,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;
+

assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);

- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
case DRM_API_HANDLE_TYPE_FD:
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl)
+ *stride = kms_sw_pl->stride;
+ return sw_displaytarget(kms_sw_pl);
case DRM_API_HANDLE_TYPE_KMS:
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane *plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return sw_displaytarget(plane);
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
default:
@@ -359,19 +430,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

switch(whandle->type) {
case DRM_API_HANDLE_TYPE_KMS:
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
case DRM_API_HANDLE_TYPE_FD:
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.16.2.804.g6dcf76e118-goog
Tomasz Figa
2018-03-19 09:32:50 UTC
Permalink
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
Change-Id shouldn't be included in upstream patches.
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 44 +++++++++++++++----
1 file changed, 36 insertions(+), 8 deletions(-)
Other than that:

Reviewed-by: Tomasz Figa <***@chromium.org>

Thanks for addressing the comments.

Best regards,
Tomasz
Lepton Wu
2018-03-19 17:33:35 UTC
Permalink
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.

Reviewed-by: Emil Velikov <***@collabora.com>
Reviewed-by: Tomasz Figa <***@chromium.org>
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 42 +++++++++++++++----
1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac..8ee0b990ed 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,8 +70,10 @@ struct kms_sw_displaytarget

uint32_t handle;
void *mapped;
+ void *ro_mapped;

int ref_count;
+ int map_count;
struct list_head link;
};

@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;

+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
@@ -198,16 +208,21 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;

prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }

DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
+
+ kms_sw_dt->map_count++;

- return kms_sw_dt->mapped;
+ return *ptr;
}

static struct kms_sw_displaytarget *
@@ -277,10 +292,23 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
{
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);

+ if (!kms_sw_dt->map_count) {
+ DEBUG_PRINT("KMS-DEBUG: ignore duplicated unmap %u", kms_sw_dt->handle);
+ return;
+ }
+ kms_sw_dt->map_count--;
+ if (kms_sw_dt->map_count) {
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap for busy buffer %u", kms_sw_dt->handle);
+ return;
+ }
+
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);

munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}

static struct sw_displaytarget *
--
2.16.2.804.g6dcf76e118-goog
Lepton Wu
2018-03-19 17:33:36 UTC
Permalink
Add a new struct kms_sw_plane which delegate a plane and use it
in place of sw_displaytarget. Multiple planes share same underlying
kms_sw_displaytarget.

Reviewed-by: Tomasz Figa <***@chromium.org>
Reviewed-by: Emil Velikov <***@collabora.com>
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 152 +++++++++++++-----
1 file changed, 112 insertions(+), 40 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 8ee0b990ed..09c6c65c1d 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,13 +59,21 @@
#define DEBUG_PRINT(msg, ...)
#endif

+struct kms_sw_displaytarget;

-struct kms_sw_displaytarget
+struct kms_sw_plane
{
- enum pipe_format format;
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ struct kms_sw_displaytarget *dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;

uint32_t handle;
@@ -75,6 +83,7 @@ struct kms_sw_displaytarget
int ref_count;
int map_count;
struct list_head link;
+ struct list_head planes;
};

struct kms_sw_winsys
@@ -85,10 +94,16 @@ struct kms_sw_winsys
struct list_head bo_list;
};

-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
+{
+ return (struct kms_sw_plane *)dt;
+}
+
+static inline struct sw_displaytarget *
+sw_displaytarget( struct kms_sw_plane *pl)
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct sw_displaytarget *)pl;
}

static inline struct kms_sw_winsys *
@@ -107,6 +122,39 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}

+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset)
+{
+ struct kms_sw_plane *plane = NULL;
+
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->offset == offset)
+ return plane;
+ }
+
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (!plane)
+ return NULL;
+
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -126,11 +174,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;

+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;

kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;

memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -140,16 +187,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;

- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane *plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (!plane)
+ goto free_bo;

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);

- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ *stride = create_req.pitch;
+ return sw_displaytarget(plane);

free_bo:
memset(&destroy_req, 0, sizeof destroy_req);
@@ -165,7 +215,8 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;

kms_sw_dt->ref_count --;
@@ -188,6 +239,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,

DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);

+ struct kms_sw_plane *tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
+
FREE(kms_sw_dt);
}

@@ -197,7 +253,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;

@@ -222,7 +279,7 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,

kms_sw_dt->map_count++;

- return *ptr;
+ return *ptr + plane->offset;
}

static struct kms_sw_displaytarget *
@@ -245,10 +302,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}

-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -260,13 +318,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;

kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane *plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }

kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;

+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -275,22 +339,25 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;

lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

- return kms_sw_dt;
+ return plane;
}

static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

if (!kms_sw_dt->map_count) {
DEBUG_PRINT("KMS-DEBUG: ignore duplicated unmap %u", kms_sw_dt->handle);
@@ -319,30 +386,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;
+

assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);

- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
case DRM_API_HANDLE_TYPE_FD:
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl)
+ *stride = kms_sw_pl->stride;
+ return sw_displaytarget(kms_sw_pl);
case DRM_API_HANDLE_TYPE_KMS:
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane *plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return sw_displaytarget(plane);
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
default:
@@ -359,19 +430,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

switch(whandle->type) {
case DRM_API_HANDLE_TYPE_KMS:
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
case DRM_API_HANDLE_TYPE_FD:
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.16.2.804.g6dcf76e118-goog
Emil Velikov
2018-03-19 17:55:58 UTC
Permalink
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
new version:
v2:
- split from larger patch (Emil)
v3:
- remove munmap w/a from dt_destory(Emil)
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.

Am I missing something?

-Emil
Lepton Wu
2018-03-19 20:00:23 UTC
Permalink
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
Thanks, will send out new version to address this comment. One more question:
I have 2 CL, the later 1 actually depends on the first one, and you
can see, the actual
patch of the later one doesn't change across versions. Should I also
increase version
number for later one? If I don't increase version for later one, this
will looks inconsistent: v6 [1/2] and
v3 [2/2]...
Post by Tomasz Figa
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
How about just keeping the DEBUG_PRINT check?
Post by Tomasz Figa
Am I missing something?
-Emil
Emil Velikov
2018-03-19 21:16:26 UTC
Permalink
Post by Lepton Wu
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
I have 2 CL, the later 1 actually depends on the first one, and you
can see, the actual
patch of the later one doesn't change across versions. Should I also
increase version
number for later one? If I don't increase version for later one, this
will looks inconsistent: v6 [1/2] and
v3 [2/2]...
Feel free to say consistent (v6 [1/2], v6 [2/2]) and if there's no v6
changes for 2/2 then there's nothing to add.
Either way - it's a small nitpick.
Post by Lepton Wu
Post by Tomasz Figa
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
How about just keeping the DEBUG_PRINT check?
Sure thing.

Thanks
Emil
Lepton Wu
2018-03-19 22:01:30 UTC
Permalink
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.

v2:
- avoid duplicated mapping and leaked mapping (Tomasz)
v3:
- split from larger patch (Emil)
v4:
- remove munmap from dt_destory (Emil)
v5:
- introduce reference count for mapping (Tomasz)
- add back munmap in dt_destory
v6:
- remove change-id in commit message (Tomasz)
v7:
- remove munmap from dt_destory again (Emil)
- add revision history in commit message (Emil)

Reviewed-by: Emil Velikov <***@collabora.com>
Reviewed-by: Tomasz Figa <***@chromium.org>
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 38 +++++++++++++++----
1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 22e1c936ac..f7bad06edb 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -70,8 +70,10 @@ struct kms_sw_displaytarget

uint32_t handle;
void *mapped;
+ void *ro_mapped;

int ref_count;
+ int map_count;
struct list_head link;
};

@@ -170,6 +172,10 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;

+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: leaked map buffer %u\n", kms_sw_dt->handle);
+ }
+
memset(&destroy_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_req);
@@ -198,16 +204,21 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;

prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (!*ptr) {
+ void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
+ kms_sw->fd, map_req.offset);
+ if (tmp == MAP_FAILED)
+ return NULL;
+ *ptr = tmp;
+ }

DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
- kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
+ kms_sw_dt->handle, kms_sw_dt->size, *ptr);
+
+ kms_sw_dt->map_count++;

- return kms_sw_dt->mapped;
+ return *ptr;
}

static struct kms_sw_displaytarget *
@@ -277,10 +288,23 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
{
struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);

+ if (!kms_sw_dt->map_count) {
+ DEBUG_PRINT("KMS-DEBUG: ignore duplicated unmap %u", kms_sw_dt->handle);
+ return;
+ }
+ kms_sw_dt->map_count--;
+ if (kms_sw_dt->map_count) {
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap for busy buffer %u", kms_sw_dt->handle);
+ return;
+ }
+
DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
+ DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);

munmap(kms_sw_dt->mapped, kms_sw_dt->size);
kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
}

static struct sw_displaytarget *
--
2.16.2.804.g6dcf76e118-goog
Lepton Wu
2018-03-19 22:01:31 UTC
Permalink
Add a new struct kms_sw_plane which delegate a plane and use it
in place of sw_displaytarget. Multiple planes share same underlying
kms_sw_displaytarget.

v2:
- add more check for plane size (Tomasz)
v3:
- split from larger patch (Emil)
v4:
- no change from v3
v5:
- remove mapped field (Tomasz)
v6:
- remove change-id in commit message (Tomasz)
v7:
- add revision history in commit message (Emil)

Reviewed-by: Tomasz Figa <***@chromium.org>
Reviewed-by: Emil Velikov <***@collabora.com>
Signed-off-by: Lepton Wu <***@chromium.org>
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 152 +++++++++++++-----
1 file changed, 112 insertions(+), 40 deletions(-)

diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index f7bad06edb..d842fe3257 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,13 +59,21 @@
#define DEBUG_PRINT(msg, ...)
#endif

+struct kms_sw_displaytarget;

-struct kms_sw_displaytarget
+struct kms_sw_plane
{
- enum pipe_format format;
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ struct kms_sw_displaytarget *dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;

uint32_t handle;
@@ -75,6 +83,7 @@ struct kms_sw_displaytarget
int ref_count;
int map_count;
struct list_head link;
+ struct list_head planes;
};

struct kms_sw_winsys
@@ -85,10 +94,16 @@ struct kms_sw_winsys
struct list_head bo_list;
};

-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
+{
+ return (struct kms_sw_plane *)dt;
+}
+
+static inline struct sw_displaytarget *
+sw_displaytarget( struct kms_sw_plane *pl)
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct sw_displaytarget *)pl;
}

static inline struct kms_sw_winsys *
@@ -107,6 +122,39 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}

+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset)
+{
+ struct kms_sw_plane *plane = NULL;
+
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->offset == offset)
+ return plane;
+ }
+
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (!plane)
+ return NULL;
+
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -126,11 +174,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;

+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;

kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;

memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -140,16 +187,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;

- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane *plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (!plane)
+ goto free_bo;

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);

- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ *stride = create_req.pitch;
+ return sw_displaytarget(plane);

free_bo:
memset(&destroy_req, 0, sizeof destroy_req);
@@ -165,7 +215,8 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;

kms_sw_dt->ref_count --;
@@ -184,6 +235,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,

DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);

+ struct kms_sw_plane *tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
+
FREE(kms_sw_dt);
}

@@ -193,7 +249,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;

@@ -218,7 +275,7 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,

kms_sw_dt->map_count++;

- return *ptr;
+ return *ptr + plane->offset;
}

static struct kms_sw_displaytarget *
@@ -241,10 +298,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}

-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -256,13 +314,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;

kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane *plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }

kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;

+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -271,22 +335,25 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;

lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }

list_add(&kms_sw_dt->link, &kms_sw->bo_list);

- return kms_sw_dt;
+ return plane;
}

static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

if (!kms_sw_dt->map_count) {
DEBUG_PRINT("KMS-DEBUG: ignore duplicated unmap %u", kms_sw_dt->handle);
@@ -315,30 +382,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
struct kms_sw_displaytarget *kms_sw_dt;
+ struct kms_sw_plane *kms_sw_pl;
+

assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
whandle->type == DRM_API_HANDLE_TYPE_FD);

- if (whandle->offset != 0) {
- DEBUG_PRINT("KMS-DEBUG: attempt to import unsupported winsys offset %d\n",
- whandle->offset);
- return NULL;
- }
-
switch(whandle->type) {
case DRM_API_HANDLE_TYPE_FD:
- kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ kms_sw_pl = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
+ templ->format,
templ->width0,
templ->height0,
- whandle->stride);
- if (kms_sw_dt)
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ whandle->stride,
+ whandle->offset);
+ if (kms_sw_pl)
+ *stride = kms_sw_pl->stride;
+ return sw_displaytarget(kms_sw_pl);
case DRM_API_HANDLE_TYPE_KMS:
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, whandle->handle);
if (kms_sw_dt) {
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ struct kms_sw_plane *plane;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (whandle->offset == plane->offset) {
+ *stride = plane->stride;
+ return sw_displaytarget(plane);
+ }
+ }
+ kms_sw_dt->ref_count --;
}
/* fallthrough */
default:
@@ -355,19 +426,20 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys,
struct winsys_handle *whandle)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(winsys);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;

switch(whandle->type) {
case DRM_API_HANDLE_TYPE_KMS:
whandle->handle = kms_sw_dt->handle;
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
case DRM_API_HANDLE_TYPE_FD:
if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle,
DRM_CLOEXEC, (int*)&whandle->handle)) {
- whandle->stride = kms_sw_dt->stride;
- whandle->offset = 0;
+ whandle->stride = plane->stride;
+ whandle->offset = plane->offset;
return TRUE;
}
/* fallthrough */
--
2.16.2.804.g6dcf76e118-goog
Tomasz Figa
2018-03-20 04:40:31 UTC
Permalink
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
Am I missing something?
I think this is actually consistent with what other winsys
implementations do. They free the map (or shadow malloc/shm buffer) in
_destroy() callback, so we should probably do the same.

Best regards,
Tomasz
Emil Velikov
2018-03-20 13:44:14 UTC
Permalink
Post by Tomasz Figa
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
Am I missing something?
I think this is actually consistent with what other winsys
implementations do. They free the map (or shadow malloc/shm buffer) in
_destroy() callback, so we should probably do the same.
Looking at the SW winsys - none of them seem to unmap at destroy time.
Perhaps you meant that the HW ones do?

The code has explicit notes where map is leaked - see `git grep -10
"displaytarget_[un]*map" -- src/gallium/drivers/'

That said - no objections for changing the behaviour. Although lets:
a) have that as separate patch, and
b) drop the XXX/FIXME alongside it, and
c) optionally, fix the other sw winsys to do so

Thanks
Emil
Tomasz Figa
2018-03-20 14:24:09 UTC
Permalink
Post by Emil Velikov
Post by Tomasz Figa
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
Am I missing something?
I think this is actually consistent with what other winsys
implementations do. They free the map (or shadow malloc/shm buffer) in
_destroy() callback, so we should probably do the same.
Looking at the SW winsys - none of them seem to unmap at destroy time.
Perhaps you meant that the HW ones do?
dri:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/dri/dri_sw_winsys.c#n128

gdi:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/gdi/gdi_sw_winsys.c#n116

hgl:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/hgl/hgl_sw_winsys.c#n152

xlib:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n260
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n271

The don't do real mapping - they all work on locally allocated memory.
However, after destroy, no resources are leaked and the pointers
returned from _map() are not valid anymore.

On the other hand, wrapper only releases a reference to pipe_resource,
so the related transfer remains valid:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/wrapper/wrapper_sw_winsys.c#n266

Best regards,
Tomasz
Emil Velikov
2018-03-20 15:58:46 UTC
Permalink
Post by Tomasz Figa
Post by Emil Velikov
Post by Tomasz Figa
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
Am I missing something?
I think this is actually consistent with what other winsys
implementations do. They free the map (or shadow malloc/shm buffer) in
_destroy() callback, so we should probably do the same.
Looking at the SW winsys - none of them seem to unmap at destroy time.
Perhaps you meant that the HW ones do?
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/dri/dri_sw_winsys.c#n128
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/gdi/gdi_sw_winsys.c#n116
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/hgl/hgl_sw_winsys.c#n152
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n260
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n271
The don't do real mapping - they all work on locally allocated memory.
However, after destroy, no resources are leaked and the pointers
returned from _map() are not valid anymore.
As mentioned before - zero objections against changing that, but keep
it separate patch.
Pretty please?

-Emil
Tomasz Figa
2018-03-20 16:26:43 UTC
Permalink
Post by Emil Velikov
Post by Tomasz Figa
Post by Emil Velikov
Post by Tomasz Figa
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
Am I missing something?
I think this is actually consistent with what other winsys
implementations do. They free the map (or shadow malloc/shm buffer) in
_destroy() callback, so we should probably do the same.
Looking at the SW winsys - none of them seem to unmap at destroy time.
Perhaps you meant that the HW ones do?
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/dri/dri_sw_winsys.c#n128
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/gdi/gdi_sw_winsys.c#n116
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/hgl/hgl_sw_winsys.c#n152
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n260
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n271
The don't do real mapping - they all work on locally allocated memory.
However, after destroy, no resources are leaked and the pointers
returned from _map() are not valid anymore.
As mentioned before - zero objections against changing that, but keep
it separate patch.
Pretty please?
SGTM.

Best regards,
Tomasz
Lepton Wu
2018-03-21 18:00:03 UTC
Permalink
Post by Tomasz Figa
Post by Emil Velikov
Post by Tomasz Figa
Post by Emil Velikov
Post by Tomasz Figa
Post by Tomasz Figa
Hi Lepton,
If user calls map twice for kms_sw_displaytarget, the first mapped
buffer could get leaked. Instead of calling mmap every time, just
reuse previous mapping. Since user could map same displaytarget with
different flags, we have to keep two different pointers, one for rw
mapping and one for ro mapping. Also introduce reference count for
mapped buffer so we can unmap them at right time.
Nit: normally it's a good idea to have brief revision log when sending
- split from larger patch (Emil)
- remove munmap w/a from dt_destory(Emil)
...
@@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->map_count > 0) {
+ DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+ kms_sw_dt->mapped = NULL;
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ kms_sw_dt->ro_mapped = NULL;
+ }
+
I could swear this workaround was missing in earlier revisions. I
don't see anything in Tomasz' reply that suggesting we should bring it
back?
AFAICT the added refcounting makes no difference - the driver isn't
cleaning up after itself.
Am I missing something?
I think this is actually consistent with what other winsys
implementations do. They free the map (or shadow malloc/shm buffer) in
_destroy() callback, so we should probably do the same.
Looking at the SW winsys - none of them seem to unmap at destroy time.
Perhaps you meant that the HW ones do?
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/dri/dri_sw_winsys.c#n128
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/gdi/gdi_sw_winsys.c#n116
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/hgl/hgl_sw_winsys.c#n152
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n260
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n271
The don't do real mapping - they all work on locally allocated memory.
However, after destroy, no resources are leaked and the pointers
returned from _map() are not valid anymore.
As mentioned before - zero objections against changing that, but keep
it separate patch.
Pretty please?
SGTM.
Thanks all for review. Is there anything else missing for getting this
committed?
Post by Tomasz Figa
Best regards,
Tomasz
Emil Velikov
2018-03-22 18:18:13 UTC
Permalink
Post by Lepton Wu
Thanks all for review. Is there anything else missing for getting this
committed?
Some of us tend to give extra 24h or so for other devs to send final
comments/R-B.
Everything seems silent so the series is in master now. Thanks!

Tomasz can apply for commit access and with a bit more work so can you
- see [1].

-Emil
[1] https://www.mesa3d.org/repository.html#developer

Emil Velikov
2018-03-07 15:20:17 UTC
Permalink
Add a new struct kms_sw_plane which delegate a plane and use it
in place of sw_displaytarget. Multiple planes share same underlying
kms_sw_displaytarget.
Change-Id: I0e9ca1d0ba0aa78c27dfdb50c30dc0c424fec172
Looks great, thank you.
Reviewed-by: Emil Velikov <***@collabora.com>

-Emil
Lepton Wu
2018-03-06 22:49:22 UTC
Permalink
Thanks for reviewing, just back from vacation and send out V3 for review.
I split it to 2 patches now and I removed lazy unmap since it's unnecessary.
Post by Emil Velikov
HI Lepton,
It seems that this has fallen through the cracks.
There's one important functionality change which should be split out
and documented.
The rest is just style nitpicks.
Post by Lepton Wu
v2: address comments from Tomasz Figa
a) Add more check for plane size.
b) Avoid duplicated mapping and leaked mapping.
c) Other minor changes.
Normally I omit saying "other minor changes" since it don't mean anything.
Post by Lepton Wu
@@ -105,6 +114,42 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}
+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset) {
Opening bracket should be at the start of next line.
Post by Lepton Wu
+ struct kms_sw_plane * tmp, * plane = NULL;
Through the patch: no space between * and variable name - example: s/* tmp/*tmp/
Add empty line between declarations and code.
Post by Lepton Wu
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+ LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
+ if (tmp->offset == offset) {
+ plane = tmp;
+ break;
+ }
+ }
+ if (plane) {
+ assert(plane->width == width);
+ assert(plane->height == height);
+ assert(plane->stride == stride);
+ assert(plane->dt == kms_sw_dt);
The only way to hit this if there's serious corruption happening. I'd
just drop it and "return tmp;" in the loop above.
Post by Lepton Wu
+ } else {
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (plane == NULL) return NULL;
The return statement should be on separate line.
Thorough the patch: swap "foo == NULL" with "!foo"
Post by Lepton Wu
@@ -138,17 +182,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
-
+ *stride = create_req.pitch;
+ return (struct sw_displaytarget *) plane;
Swap the cast with an inline wrapper analogous to the kms_sw_plane() one.
Post by Lepton Wu
@@ -163,13 +209,19 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;
kms_sw_dt->ref_count --;
if (kms_sw_dt->ref_count > 0)
return;
+ if (kms_sw_dt->ro_mapped)
+ munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
+ if (kms_sw_dt->mapped)
+ munmap(kms_sw_dt->mapped, kms_sw_dt->size);
+
This hunk moves the munmap from dt_unmap to dt_destroy.
It should be safe, although it is a subtle functionality change that
should be split out.
A commit message should explain why it's needed, etc.
Ideally, the ro_mapped introduction will be another patch.
Alternatively the commit message should mention it.
Post by Lepton Wu
@@ -198,16 +255,20 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
return NULL;
prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
- kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
- kms_sw->fd, map_req.offset);
-
- if (kms_sw_dt->mapped == MAP_FAILED)
- return NULL;
+ void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
+ if (*ptr == NULL) {
To prevent interment breakage, this NULL check must be part of the
delayer munmap patch.
HTH
Emil
Tomasz Figa
2018-03-16 07:19:44 UTC
Permalink
Add a new struct kms_sw_plane which delegate a plane and use it
in place of sw_displaytarget. Multiple planes share same underlying
kms_sw_displaytarget.
Change-Id: I0e9ca1d0ba0aa78c27dfdb50c30dc0c424fec172
---
.../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 162 +++++++++++++-----
1 file changed, 122 insertions(+), 40 deletions(-)
diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
index 30343222470..d191d5c4987 100644
--- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
+++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
@@ -59,13 +59,22 @@
#define DEBUG_PRINT(msg, ...)
#endif
+struct kms_sw_displaytarget;
-struct kms_sw_displaytarget
+struct kms_sw_plane
{
- enum pipe_format format;
unsigned width;
unsigned height;
unsigned stride;
+ unsigned offset;
+ int mapped;
+ struct kms_sw_displaytarget *dt;
+ struct list_head link;
+};
+
+struct kms_sw_displaytarget
+{
+ enum pipe_format format;
unsigned size;
uint32_t handle;
@@ -74,6 +83,7 @@ struct kms_sw_displaytarget
int ref_count;
struct list_head link;
+ struct list_head planes;
};
struct kms_sw_winsys
@@ -84,10 +94,16 @@ struct kms_sw_winsys
struct list_head bo_list;
};
-static inline struct kms_sw_displaytarget *
-kms_sw_displaytarget( struct sw_displaytarget *dt )
+static inline struct kms_sw_plane *
+kms_sw_plane( struct sw_displaytarget *dt )
{
- return (struct kms_sw_displaytarget *)dt;
+ return (struct kms_sw_plane *)dt;
+}
+
+static inline struct sw_displaytarget *
+sw_displaytarget( struct kms_sw_plane *pl)
+{
+ return (struct sw_displaytarget *)pl;
}
static inline struct kms_sw_winsys *
@@ -106,6 +122,39 @@ kms_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
return TRUE;
}
+static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget *kms_sw_dt,
+ enum pipe_format format,
+ unsigned width, unsigned height,
+ unsigned stride, unsigned offset)
+{
+ struct kms_sw_plane *plane = NULL;
+
+ if (offset + util_format_get_2d_size(format, stride, height) >
+ kms_sw_dt->size) {
+ DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height: %d "
+ "offset: %d size:%d\n", format, stride, height, offset,
+ kms_sw_dt->size);
+ return NULL;
+ }
+
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->offset == offset)
+ return plane;
+ }
+
+ plane = CALLOC_STRUCT(kms_sw_plane);
+ if (!plane)
+ return NULL;
+
+ plane->width = width;
+ plane->height = height;
+ plane->stride = stride;
+ plane->offset = offset;
+ plane->dt = kms_sw_dt;
+ list_add(&plane->link, &kms_sw_dt->planes);
+ return plane;
+}
+
static struct sw_displaytarget *
kms_sw_displaytarget_create(struct sw_winsys *ws,
unsigned tex_usage,
@@ -125,11 +174,10 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (!kms_sw_dt)
goto no_dt;
+ list_inithead(&kms_sw_dt->planes);
kms_sw_dt->ref_count = 1;
kms_sw_dt->format = format;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
memset(&create_req, 0, sizeof(create_req));
create_req.bpp = 32;
@@ -139,16 +187,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
if (ret)
goto free_bo;
- kms_sw_dt->stride = create_req.pitch;
kms_sw_dt->size = create_req.size;
kms_sw_dt->handle = create_req.handle;
+ struct kms_sw_plane *plane = get_plane(kms_sw_dt, format, width, height,
+ create_req.pitch, 0);
+ if (!plane)
+ goto free_bo;
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);
- *stride = kms_sw_dt->stride;
- return (struct sw_displaytarget *)kms_sw_dt;
+ *stride = create_req.pitch;
+ return sw_displaytarget(plane);
memset(&destroy_req, 0, sizeof destroy_req);
@@ -164,7 +215,8 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_destroy_dumb destroy_req;
kms_sw_dt->ref_count --;
@@ -184,6 +236,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
DEBUG_PRINT("KMS-DEBUG: destroyed buffer %u\n", kms_sw_dt->handle);
+ struct kms_sw_plane *tmp;
+ LIST_FOR_EACH_ENTRY_SAFE(plane, tmp, &kms_sw_dt->planes, link) {
+ FREE(plane);
+ }
+
FREE(kms_sw_dt);
}
@@ -193,7 +250,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
unsigned flags)
{
struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
struct drm_mode_map_dumb map_req;
int prot, ret;
@@ -216,7 +274,8 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
kms_sw_dt->handle, kms_sw_dt->size, *ptr);
- return *ptr;
+ plane->mapped = 1;
If we refcount the mapping, we would just increment the mapping
refcount here instead and plane->mapped field wouldn't be necessary.
+ return *ptr + plane->offset;
}
static struct kms_sw_displaytarget *
@@ -239,10 +298,11 @@ kms_sw_displaytarget_find_and_ref(struct kms_sw_winsys *kms_sw,
return NULL;
}
-static struct kms_sw_displaytarget *
+static struct kms_sw_plane *
kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
+ enum pipe_format format,
unsigned width, unsigned height,
- unsigned stride)
+ unsigned stride, unsigned offset)
{
uint32_t handle = -1;
struct kms_sw_displaytarget * kms_sw_dt;
@@ -254,13 +314,19 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
return NULL;
kms_sw_dt = kms_sw_displaytarget_find_and_ref(kms_sw, handle);
- if (kms_sw_dt)
- return kms_sw_dt;
+ struct kms_sw_plane *plane = NULL;
+ if (kms_sw_dt) {
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane)
+ kms_sw_dt->ref_count --;
+ return plane;
+ }
kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
if (!kms_sw_dt)
return NULL;
+ list_inithead(&kms_sw_dt->planes);
off_t lseek_ret = lseek(fd, 0, SEEK_END);
if (lseek_ret == -1) {
FREE(kms_sw_dt);
@@ -269,22 +335,33 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
kms_sw_dt->size = lseek_ret;
kms_sw_dt->ref_count = 1;
kms_sw_dt->handle = handle;
- kms_sw_dt->width = width;
- kms_sw_dt->height = height;
- kms_sw_dt->stride = stride;
lseek(fd, 0, SEEK_SET);
+ plane = get_plane(kms_sw_dt, format, width, height, stride, offset);
+ if (!plane) {
+ FREE(kms_sw_dt);
+ return NULL;
+ }
list_add(&kms_sw_dt->link, &kms_sw->bo_list);
- return kms_sw_dt;
+ return plane;
}
static void
kms_sw_displaytarget_unmap(struct sw_winsys *ws,
struct sw_displaytarget *dt)
{
- struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
+ struct kms_sw_plane *plane = kms_sw_plane(dt);
+ struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
+
+ plane->mapped = 0;
+ LIST_FOR_EACH_ENTRY(plane, &kms_sw_dt->planes, link) {
+ if (plane->mapped) {
+ DEBUG_PRINT("KMS-DEBUG: ignore unmap buffer %u \n", kms_sw_dt->handle);
+ return;
+ }
+ }
This would be greatly simplified with mapping refcounting. We wouldn't
need to iterate through the list of planes. Just decrement the counter
and really unmap only if it goes to 0.

Best regards,
Tomasz
Loading...