[Mesa-dev] [PATCH] st/dri: Add support for PIPE_FORMAT_RGBX8888_UNORM

[Lepton Wu]

The original dri2_format_to_pipe_format function just misses case branch
for __DRI_IMAGE_FORMAT_XBGR. I discovered this when debugging one google
map crash inside emulator.

Signed-off-by: Lepton Wu <lep...@chromium.org>
---
 src/gallium/state_trackers/dri/dri2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/gallium/state_trackers/dri/dri2.c 
b/src/gallium/state_trackers/dri/dri2.c
index c5e69d639b..f02ef30dd7 100644
--- a/src/gallium/state_trackers/dri/dri2.c
+++ b/src/gallium/state_trackers/dri/dri2.c
@@ -186,6 +186,9 @@ static enum pipe_format dri2_format_to_pipe_format (int 
format)
case __DRI_IMAGE_FORMAT_ARGB:
   pf = PIPE_FORMAT_BGRA_UNORM;
   break;
+   case __DRI_IMAGE_FORMAT_XBGR:
+  pf = PIPE_FORMAT_RGBX_UNORM;
+  break;
case __DRI_IMAGE_FORMAT_ABGR:
   pf = PIPE_FORMAT_RGBA_UNORM;
   break;
-- 
2.13.1.518.g3df882009-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/dri: Add support for PIPE_FORMAT_RGBX8888_UNORM

[Lepton Wu]

Hi All,

So it turns out someone else have a much complete fix for this issue and I
think I can wait that CL to land in mesa.

Feel free to drop this CL. Thanks all for your review.

On Tue, Jun 20, 2017 at 2:06 AM, Emil Velikov <emil.l.veli...@gmail.com>
wrote:

> Hi Lepton,
>
> On 19 June 2017 at 18:51, Lepton Wu <lep...@google.com> wrote:
> > The original dri2_format_to_pipe_format function just misses case branch
> > for __DRI_IMAGE_FORMAT_XBGR. I discovered this when debugging one
> google
> > map crash inside emulator.
> >
> > Signed-off-by: Lepton Wu <lep...@chromium.org>
> > ---
> >  src/gallium/state_trackers/dri/dri2.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/src/gallium/state_trackers/dri/dri2.c
> b/src/gallium/state_trackers/dri/dri2.c
> > index c5e69d639b..f02ef30dd7 100644
> > --- a/src/gallium/state_trackers/dri/dri2.c
> > +++ b/src/gallium/state_trackers/dri/dri2.c
> > @@ -186,6 +186,9 @@ static enum pipe_format dri2_format_to_pipe_format
> (int format)
> > case __DRI_IMAGE_FORMAT_ARGB:
> >pf = PIPE_FORMAT_BGRA_UNORM;
> >break;
> > +   case __DRI_IMAGE_FORMAT_XBGR:
> > +  pf = PIPE_FORMAT_RGBX_UNORM;
> > +  break;
>
> Can you provide some additional information here:
>  - How did you get here - a backtrace will be appreciated.
>  - Do you have additional patches that you apply on top of Mesa - can
> you share a link to them.
>
> I'm asking all this information since the commit looks deceptively
> close to an earlier one ccdcf91104a, which caused issues and had to be
> reverted see c0c6ca40a25.
>
> Thanks
> Emil
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/winsys/kms: Add support for multi-planes (v2)

[Lepton Wu]

Gentle ping. Thanks.

On Wed, Dec 27, 2017 at 11:35 PM, Lepton Wu <lep...@chromium.org> wrote:
> 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 <lep...@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, _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(>link, _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(_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(_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(_sw_dt->link, _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;
> - 

[Mesa-dev] [PATCH] gallium/winsys/kms: Add support for multi-planes (v2)

[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.

Signed-off-by: Lepton Wu <lep...@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, _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(>link, _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(_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(_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(_sw_dt->link, _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(_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_

Re: [Mesa-dev] [PATCH] gallium/winsys/kms: don't unmap what wasn't mapped

[Lepton Wu]

On Thu, Aug 16, 2018 at 1:37 PM Ray Strode  wrote:
>
> From: Ray Strode 
>
> At the moment, depending on pipe transfer flags, the dumb
> buffer map address can end up at either kms_sw_dt->ro_mapped
> or kms_sw_dt->mapped.
>
> When it's time to unmap the dumb buffer, both locations get unmapped,
> even though one is probably initialized to 0.
>
> That leads to the code segment getting unmapped at runtime and
> crashes when trying to call into unrelated code.
>
> This commit addresses the problem by using MAP_FAILED instead of
> NULL for ro_mapped and mapped when the dumb buffer is unmapped,
> and only unmapping mapped addresses at unmap time.
Does https://patchwork.freedesktop.org/patch/238931/ already fix this?
What's the advantage to use MAP_FAILED instead of NULL?

>
> Signed-off-by: Ray Strode 
> ---
>  .../winsys/sw/kms-dri/kms_dri_sw_winsys.c | 19 ++-
>  1 file changed, 14 insertions(+), 5 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 d842fe3257..effa3eb2c8 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
> @@ -149,63 +149,65 @@ static struct kms_sw_plane *get_plane(struct 
> kms_sw_displaytarget *kms_sw_dt,
> plane->width = width;
> plane->height = height;
> plane->stride = stride;
> plane->offset = offset;
> plane->dt = kms_sw_dt;
> list_add(>link, _sw_dt->planes);
> return plane;
>  }
>
>  static struct sw_displaytarget *
>  kms_sw_displaytarget_create(struct sw_winsys *ws,
>  unsigned tex_usage,
>  enum pipe_format format,
>  unsigned width, unsigned height,
>  unsigned alignment,
>  const void *front_private,
>  unsigned *stride)
>  {
> struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
> struct kms_sw_displaytarget *kms_sw_dt;
> struct drm_mode_create_dumb create_req;
> struct drm_mode_destroy_dumb destroy_req;
> int ret;
>
> kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
> if (!kms_sw_dt)
>goto no_dt;
>
> list_inithead(_sw_dt->planes);
> kms_sw_dt->ref_count = 1;
> +   kms_sw_dt->mapped = MAP_FAILED;
> +   kms_sw_dt->ro_mapped = MAP_FAILED;
>
> kms_sw_dt->format = format;
>
> memset(_req, 0, sizeof(create_req));
> create_req.bpp = 32;
> create_req.width = width;
> create_req.height = height;
> ret = drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_CREATE_DUMB, _req);
> if (ret)
>goto free_bo;
>
> 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(_sw_dt->link, _sw->bo_list);
>
> DEBUG_PRINT("KMS-DEBUG: created buffer %u (size %u)\n", 
> kms_sw_dt->handle, kms_sw_dt->size);
>
> *stride = create_req.pitch;
> return sw_displaytarget(plane);
>
>   free_bo:
> memset(_req, 0, sizeof destroy_req);
> destroy_req.handle = create_req.handle;
> drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _req);
> FREE(kms_sw_dt);
>   no_dt:
> return NULL;
>  }
> @@ -235,61 +237,61 @@ 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, _sw_dt->planes, link) {
>FREE(plane);
> }
>
> FREE(kms_sw_dt);
>  }
>
>  static void *
>  kms_sw_displaytarget_map(struct sw_winsys *ws,
>   struct sw_displaytarget *dt,
>   unsigned flags)
>  {
> struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
> 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;
>
> memset(_req, 0, sizeof map_req);
> map_req.handle = kms_sw_dt->handle;
> ret = drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_MAP_DUMB, _req);
> if (ret)
>return NULL;
>
> prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | 
> PROT_WRITE);
> void **ptr = (flags == PIPE_TRANSFER_READ) ? _sw_dt->ro_mapped : 
> _sw_dt->mapped;
> -   if (!*ptr) {
> +   if (*ptr == MAP_FAILED) {
>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, *ptr);
>
> kms_sw_dt->map_count++;
>
> return *ptr + plane->offset;
>  }
>
>  static struct kms_sw_displaytarget *
> 

Re: [Mesa-dev] [PATCH] gallium/winsys/kms: do not munmap NULL pointers

[Lepton Wu]

Hi Jan,
On Thu, Aug 16, 2018 at 10:37 AM Emil Velikov  wrote:
>
> Hi Jan,
>
> On 13 July 2018 at 20:57, Jan Palus  wrote:
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107098
> > Signed-off-by: Jan Palus 
> > ---
Thanks for fixing this, actually after digging in some kernel code, it
seems my original assumation that
munmap(NULL...) should be noop is false.  So linux kernel will dry to
munmap for   address space between
NULL  and NULL + len,  if you happen to have a "low" mapping which
starts before len, then it will unmap
some process memory.
> Thanks for fixing this issue. For the future, feel free to use a fixes
> tag (+cc) as below.
> It provides a nice reference when picking the fix for stable branches,
> plus the author is your first line reviewer ;-)
>
> Fixes: d891f28df9a ("gallium/winsys/kms: Fix possible leak in map/unmap.")
> Cc: Lepton Wu 
>
> Reviewed-by: Emil Velikov 
>
> Just heading out of the office, so I'll push this tomorrow morning.
>
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] virgl: Fix flush in virgl_encoder_inline_write.

[Lepton Wu]

The current code is buggy: if there are only 12 dwords left in cbuf,
we emit a zero data length command which will be rejected by virglrenderer.
Fix it by calling flush in this case.
---
 src/gallium/drivers/virgl/virgl_encode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/drivers/virgl/virgl_encode.c 
b/src/gallium/drivers/virgl/virgl_encode.c
index 6b800d3d07..e097fa83a9 100644
--- a/src/gallium/drivers/virgl/virgl_encode.c
+++ b/src/gallium/drivers/virgl/virgl_encode.c
@@ -528,7 +528,7 @@ int virgl_encoder_inline_write(struct virgl_context *ctx,
 
left_bytes = size;
while (left_bytes) {
-  if (ctx->cbuf->cdw + 12 > VIRGL_MAX_CMDBUF_DWORDS)
+  if (ctx->cbuf->cdw + 12 >= VIRGL_MAX_CMDBUF_DWORDS)
  ctx->base.flush(>base, NULL, 0);
 
   thispass = (VIRGL_MAX_CMDBUF_DWORDS - ctx->cbuf->cdw - 12) * 4;
-- 
2.18.0.203.gfac676dfb9-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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.

On Wed, Mar 7, 2018 at 7:14 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 6 March 2018 at 22:43, Lepton Wu <lep...@chromium.org> wrote:
>> 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 <lep...@chromium.org>
>> ---
>>  .../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 <emil.veli...@collabora.com>
>
> -Emil
>
> [1] $ git grep -20 -w displaytarget_[a-z]*map
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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.

On Wed, Mar 7, 2018 at 10:14 AM, Lepton Wu <lep...@chromium.org> wrote:
> 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.
>
> On Wed, Mar 7, 2018 at 7:14 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>> On 6 March 2018 at 22:43, Lepton Wu <lep...@chromium.org> wrote:
>>> 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 <lep...@chromium.org>
>>> ---
>>>  .../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 <emil.veli...@collabora.com>
>>
>> -Emil
>>
>> [1] $ git grep -20 -w displaytarget_[a-z]*map
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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.

On Wed, Mar 7, 2018 at 7:14 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 6 March 2018 at 22:43, Lepton Wu <lep...@chromium.org> wrote:
>> 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 <lep...@chromium.org>
>> ---
>>  .../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 <emil.veli...@collabora.com>
>
> -Emil
>
> [1] $ git grep -20 -w displaytarget_[a-z]*map
On Wed, Mar 7,
2018 at 7:14 AM, Emil Velikov mailto:emil.l.veli...@gmail.com;
target="_blank">emil.l.veli...@gmail.com
wrote:On 6 March 2018 at 22:43, Lepton Wu
mailto:lep...@chromium.org;>lep...@chromium.org
wrote:
 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 mailto:lep...@chromium.org;>lep...@chromium.org
 ---
 .../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 mailto:emil.veli...@collabora.com;>emil.veli...@collabora.com

-Emil

[1] $ git grep -20 -w displaytarget_[a-z]*map

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 2/2] gallium/winsys/kms: Add support for multi-planes

[Lepton Wu]

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.

On Wed, Mar 7, 2018 at 2:39 PM, Lepton Wu <lep...@chromium.org> wrote:
> 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 <lep...@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, _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(>link, _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(_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(_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(_sw_dt->link, _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 (s

[Mesa-dev] [PATCH v4 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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 <lep...@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) ? _sw_dt->ro_mapped : 
_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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 2/2] gallium/winsys/kms: Add support for multi-planes

[Lepton Wu]

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 <lep...@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, _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(>link, _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(_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(_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(_sw_dt->link, _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(_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, _sw_dt->pl

[Mesa-dev] [PATCH v3 2/2] gallium/winsys/kms: Add support for multi-planes

[Lepton Wu]

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 <lep...@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 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, _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(>link, _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(_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(_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(_sw_dt->link, _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(_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, _sw_dt->pl

Re: [Mesa-dev] [PATCH] gallium/winsys/kms: Add support for multi-planes (v2)

[Lepton Wu]

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.

On Wed, Feb 21, 2018 at 7:42 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> 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.
>
> On 28 December 2017 at 07:35, Lepton Wu <lep...@chromium.org> wrote:
>> 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.
>
>
>> @@ -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.
>
>> +   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.
>
>> +   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, _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.
>
>> +   } 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"
>
>> @@ -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.
>
>
>> @@ -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.
>
>
>> @@ -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) ? _sw_dt->ro_mapped : 
>> _sw_dt->mapped;
>> +   if (*ptr == NULL) {
> To prevent interment breakage, this NULL check must be part of the
> delayer munmap patch.
>
> HTH
> Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v3 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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 <lep...@chromium.org>
---
 .../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);
+
memset(_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _req);
@@ -198,16 +204,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) ? _sw_dt->ro_mapped : 
_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 +287,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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

Ping.  Any more comments or missing stuff to get this commited into master?

Thanks.

On Wed, Mar 7, 2018 at 2:39 PM, Lepton Wu <lep...@chromium.org> wrote:
> 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 <lep...@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) ? _sw_dt->ro_mapped : 
> _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
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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!

On Tue, Mar 13, 2018 at 8:41 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 13 March 2018 at 11:46, Tomasz Figa <tf...@chromium.org> wrote:
>> On Thu, Mar 8, 2018 at 7:39 AM, Lepton Wu <lep...@chromium.org> wrote:
>>> 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 <lep...@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) ? _sw_dt->ro_mapped : 
>>> _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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v2] virgl: disable virgl when no 3D for virtio gpu.

[Lepton Wu]

If users are running mesa under old version of qemu or have turned off
GL at runtime, virtio gpu driver actually doesn't work. Adds a detection
here so mesa can fall back to software rendering.

v2:
 - move detection from loader to virgl (Ilia, Emil)

Signed-off-by: Lepton Wu <lep...@chromium.org>
---
 src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
index cf3c3bac4b..4198ed7feb 100644
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
@@ -800,8 +800,15 @@ virgl_drm_winsys_create(int drmFD)
 {
struct virgl_drm_winsys *qdws;
int ret;
+   int gl = 0;
struct drm_virtgpu_getparam getparam = {0};
 
+   getparam.param = VIRTGPU_PARAM_3D_FEATURES;
+   getparam.value = (uint64_t)(uintptr_t)
+   ret = drmIoctl(drmFD, DRM_IOCTL_VIRTGPU_GETPARAM, );
+   if (ret < 0 || !gl)
+  return NULL;
+
qdws = CALLOC_STRUCT(virgl_drm_winsys);
if (!qdws)
   return NULL;
@@ -914,6 +921,10 @@ virgl_drm_screen_create(int fd)
   int dup_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
 
   vws = virgl_drm_winsys_create(dup_fd);
+  if (!vws) {
+ close(dup_fd);
+ goto unlock;
+  }
 
   pscreen = virgl_create_screen(vws);
   if (pscreen) {
-- 
2.17.0.484.g0c8726318c-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [RFC] Disable virgl driver when no 3D for virtio.

[Lepton Wu]

I know this looks like a hack. But just want to send this out for comments
to see how can I address this issue in mesa:
virgl driver enabled image can't work on legacy qemu or GL disabled qemu.

It seems another choice could be playing with MESA_LOADER_DRIVER_OVERRIDE to
override driver name, but that means everybody have to do something similar.
It should be better to fix this in mesa. Another possible solution
could be to add detection in virgl driver and let virgl handle the fallback
itself. Then it seems we have to duplicate code from kms_swrast or do some
non trivial refactoring?

Any thoughts? Thanks!


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] loader: disable virgl driver when no 3D for virtio

[Lepton Wu]

If user are running mesa under old version of qemu or have turned off
GL at runtime, virtio gpu driver actually doesn't work. Adding a detection
here can make sure same disk image work with both cases.

Signed-off-by: Lepton Wu <lep...@chromium.org>
---
 src/loader/loader.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/src/loader/loader.c b/src/loader/loader.c
index 43275484cc..2a689c52d6 100644
--- a/src/loader/loader.c
+++ b/src/loader/loader.c
@@ -381,6 +381,27 @@ out:
log_(driver ? _LOADER_DEBUG : _LOADER_WARNING,
  "pci id for fd %d: %04x:%04x, driver %s\n",
  fd, vendor_id, chip_id, driver);
+   if (!strcmp(driver, "virtio_gpu")) {
+  struct drm_virtgpu_getparam {
+ uint64_t param;
+ uint64_t value;
+  };
+  #define VIRTGPU_PARAM_3D_FEATURES 1 /* do we have 3D features in the hw 
*/
+  struct drm_virtgpu_getparam args;
+  uint32_t gl = 0;
+  args.param = VIRTGPU_PARAM_3D_FEATURES;
+  args.value = (uint64_t)(uintptr_t)
+  #define DRM_VIRTGPU_GETPARAM0x03
+  #define DRM_IOCTL_VIRTGPU_GETPARAM \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_GETPARAM,\
+  struct drm_virtgpu_getparam)
+  int ret = drmIoctl(fd, DRM_IOCTL_VIRTGPU_GETPARAM, );
+  if (ret || !gl) {
+ /* Actually there is no virtio_2d driver, mesa will
+  * fallback to software driver */
+ return strdup("virtio_gpu_2d");
+  }
+   }
return driver;
 }
 
-- 
2.17.0.484.g0c8726318c-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] virgl: disable virgl when no 3D for virtio gpu.

[Lepton Wu]

On Thu, Apr 5, 2018 at 12:38 PM, Lepton Wu <lep...@chromium.org> wrote:
> If users are running mesa under old version of qemu or have turned off
> GL at runtime, virtio gpu driver actually doesn't work. Adds a detection
> here so mesa can fall back to software rendering.
>
> v2:
>  - move detection from loader to virgl (Ilia, Emil)
Ping. Thanks.
>
> Signed-off-by: Lepton Wu <lep...@chromium.org>
> ---
>  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> index cf3c3bac4b..4198ed7feb 100644
> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> @@ -800,8 +800,15 @@ virgl_drm_winsys_create(int drmFD)
>  {
> struct virgl_drm_winsys *qdws;
> int ret;
> +   int gl = 0;
> struct drm_virtgpu_getparam getparam = {0};
>
> +   getparam.param = VIRTGPU_PARAM_3D_FEATURES;
> +   getparam.value = (uint64_t)(uintptr_t)
> +   ret = drmIoctl(drmFD, DRM_IOCTL_VIRTGPU_GETPARAM, );
> +   if (ret < 0 || !gl)
> +  return NULL;
> +
> qdws = CALLOC_STRUCT(virgl_drm_winsys);
> if (!qdws)
>return NULL;
> @@ -914,6 +921,10 @@ virgl_drm_screen_create(int fd)
>int dup_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
>
>vws = virgl_drm_winsys_create(dup_fd);
> +  if (!vws) {
> + close(dup_fd);
> + goto unlock;
> +  }
>
>pscreen = virgl_create_screen(vws);
>if (pscreen) {
> --
> 2.17.0.484.g0c8726318c-goog
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 2/2] gallium/winsys/kms: Add support for multi-planes

[Lepton Wu]

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 <lep...@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, _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(>link, _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(_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(_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(_sw_dt->link, _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(_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, _sw_dt->pl

[Mesa-dev] [PATCH v5 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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 <lep...@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(_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _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) ? _sw_dt->ro_mapped : 
_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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v6 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

On Tue, Mar 20, 2018 at 9:26 AM, Tomasz Figa <tf...@chromium.org> wrote:
> On Wed, Mar 21, 2018 at 12:58 AM, Emil Velikov <emil.l.veli...@gmail.com> 
> wrote:
>> On 20 March 2018 at 14:24, Tomasz Figa <tf...@chromium.org> wrote:
>>> On Tue, Mar 20, 2018 at 10:44 PM, Emil Velikov <emil.l.veli...@gmail.com> 
>>> wrote:
>>>> On 20 March 2018 at 04:40, Tomasz Figa <tf...@chromium.org> wrote:
>>>>> On Tue, Mar 20, 2018 at 2:55 AM, Emil Velikov <emil.l.veli...@gmail.com> 
>>>>> wrote:
>>>>>> Hi Lepton,
>>>>>>
>>>>>> On 19 March 2018 at 17:33, Lepton Wu <lep...@chromium.org> wrote:
>>>>>>> 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 <emil.veli...@collabora.com>
>>>>>>> Reviewed-by: Tomasz Figa <tf...@chromium.org>
>>>>>>> Signed-off-by: Lepton Wu <lep...@chromium.org>
>>>>>>
>>>>>> 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?
>>>>>
>>>>> 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.
>>>
>> 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?
>
> Best regards,
> Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v6 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

On Mon, Mar 19, 2018 at 10:55 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> Hi Lepton,
>
> On 19 March 2018 at 17:33, Lepton Wu <lep...@chromium.org> wrote:
>> 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 <emil.veli...@collabora.com>
>> Reviewed-by: Tomasz Figa <tf...@chromium.org>
>> Signed-off-by: Lepton Wu <lep...@chromium.org>
>
> 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)
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]...
> ...
>
>> @@ -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?
>
> Am I missing something?
>
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v6 2/2] gallium/winsys/kms: Add support for multi-planes

[Lepton Wu]

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 <tf...@chromium.org>
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
Signed-off-by: Lepton Wu <lep...@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, _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(>link, _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(_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(_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(_sw_dt->link, _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(_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

[Mesa-dev] [PATCH v6 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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 <emil.veli...@collabora.com>
Reviewed-by: Tomasz Figa <tf...@chromium.org>
Signed-off-by: Lepton Wu <lep...@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(_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _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) ? _sw_dt->ro_mapped : 
_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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v7 2/2] gallium/winsys/kms: Add support for multi-planes

[Lepton Wu]

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 <tf...@chromium.org>
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
Signed-off-by: Lepton Wu <lep...@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, _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(>link, _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(_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(_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(_sw_dt->link, _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(_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;

[Mesa-dev] [PATCH v7 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

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 <emil.veli...@collabora.com>
Reviewed-by: Tomasz Figa <tf...@chromium.org>
Signed-off-by: Lepton Wu <lep...@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(_req, 0, sizeof destroy_req);
destroy_req.handle = kms_sw_dt->handle;
drmIoctl(kms_sw->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _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) ? _sw_dt->ro_mapped : 
_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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] virgl: close drm fd when destroying virgl screen.

[Lepton Wu]

On Wed, Mar 20, 2019 at 2:26 PM Chia-I Wu  wrote:

> Reviewed-by: Chia-I Wu 
>
Anything else to need for merging this? I think this is a straightforward
leaking fix.


>
> On Mon, Mar 18, 2019 at 4:40 PM Lepton Wu  wrote:
>
>> This fd was create in virgl_drm_screen_create and should be closed
>> in virgl_drm_screen_destroy.
>>
>> Signed-off-by: Lepton Wu 
>> ---
>>  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> index 01811a0e997..5501fe3ed48 100644
>> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> @@ -973,6 +973,7 @@ virgl_drm_screen_destroy(struct pipe_screen *pscreen)
>> if (destroy) {
>>int fd = virgl_drm_winsys(screen->vws)->fd;
>>util_hash_table_remove(fd_tab, intptr_to_pointer(fd));
>> +  close(fd);
>> }
>> mtx_unlock(_screen_mutex);
>>
>> --
>> 2.21.0.225.g810b269d1ac-goog
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.

[Lepton Wu]

On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu  wrote:

> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu  wrote:
>
>> The old code could use gem name as key when inserting it to bo_handles
>> hash table while trying to remove it from hash table with bo_handle as
>> key in virgl_hw_res_destroy. This triggers use after free. Also, we
>> should only reuse resource from bo_handle hash when the handle type is
>> FD.
>>
> Reuse is not very accurate.  Opening a shared handle (flink name) twice
> gives two GEM handles.  Importing an fd handle (prime fd) twice gives the
> same GEM handle.  In all cases, within a virgl_winsys, we want only one GEM
> handle and only one virgl_resource for each kernel GEM object.
>
> I think the logic should go like:
>
>   if (HANDLE_TYPE_SHARED) {
> if (bo_names.has(flink_name))
>   return bo_names[flink_name];
> gem_handle = gem_open(flink_name);
>   } else {
> gem_handle = drmPrimeFDToHandle(prime_fd);
>   }
>
>
  if (bo_handles.has(gem_handle))
> return bo_handles[gem_handle];
>   bo_handles[gem_handle] = create_new_resource();
>
> Hi, the current patch did most of what you said with only one difference:
it didn't insert to bo_handles[]   hash when the type
is  HANDLE_TYPE_SHARED.
I think this is reasonable since opening a shared handle always get a new
gem handle very time and I think it doesn't worth to insert it to
bo_handles[] hash.
What do you think?

>
>> Signed-off-by: Lepton Wu 
>> ---
>>  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 15 ---
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> index 120e8eda2cd..01811a0e997 100644
>> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> @@ -424,13 +424,13 @@ virgl_drm_winsys_resource_create_handle(struct
>> virgl_winsys *qws,
>>   res = NULL;
>>   goto done;
>>}
>> -   }
>>
>> -   res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);
>> -   if (res) {
>> -  struct virgl_hw_res *r = NULL;
>> -  virgl_drm_resource_reference(qdws, , res);
>> -  goto done;
>> +  res = util_hash_table_get(qdws->bo_handles,
>> (void*)(uintptr_t)handle);
>> +  if (res) {
>> +struct virgl_hw_res *r = NULL;
>> +virgl_drm_resource_reference(qdws, , res);
>> +goto done;
>> +  }
>> }
>>
>> res = CALLOC_STRUCT(virgl_hw_res);
>> @@ -469,7 +469,8 @@ virgl_drm_winsys_resource_create_handle(struct
>> virgl_winsys *qws,
>> res->num_cs_references = 0;
>> res->fence_fd = -1;
>>
>> -   util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)handle, res);
>> +   util_hash_table_set(qdws->bo_handles, (void
>> *)(uintptr_t)res->bo_handle,
>> +   res);
>>
>>  done:
>> mtx_unlock(>bo_handles_mutex);
>> --
>> 2.21.0.225.g810b269d1ac-goog
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] virgl: Set bind when creating temp resource.

[Lepton Wu]

On Tue, Mar 19, 2019 at 4:29 AM Erik Faye-Lund 
wrote:

> On Mon, 2019-03-18 at 14:44 -0700, Lepton Wu wrote:
> > virgl render complains about "Illegal resource" when running
> > dEQP-EGL.functional.color_clears.single_context.gles2.rgb888_window,
> > the reason is that a zero bind value was given for temp resource.
> >
> > Signed-off-by: Lepton Wu 
> > ---
> >  src/gallium/drivers/virgl/virgl_texture.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/gallium/drivers/virgl/virgl_texture.c
> > b/src/gallium/drivers/virgl/virgl_texture.c
> > index 231319899e0..563dbacba7e 100644
> > --- a/src/gallium/drivers/virgl/virgl_texture.c
> > +++ b/src/gallium/drivers/virgl/virgl_texture.c
> > @@ -66,6 +66,7 @@ static void
> > virgl_init_temp_resource_from_box(struct pipe_resource *res,
> >unsigned level,
> > unsigned flags)
> >  {
> > memset(res, 0, sizeof(*res));
> > +   res->bind = orig->bind;
> > res->format = orig->format;
> > res->width0 = box->width;
> > res->height0 = box->height;
>
> I have a similar-ish patch for the same issue in a branch I'll be
> sending out soon:
>
>
> https://gitlab.freedesktop.org/kusma/mesa/commit/6c19b6b98025a1be31eabdb559709b18eecdbafa#note_132855
>
> Now, as Dave pointed out, there might be some more cases missing in my
> patch. I also tried your approach, and it works for me. But I'm not
> entirely sure it's the right one; for instance I don't think we'd ever
> want to carry flags like PIPE_BIND_DISPLAY_TARGET and
> PIPE_BIND_BLENDABLE forward.
>
> Perhaps the right thing is to do something like:
>
> res->bind = orig->bind & (PIPE_BIND_DEPTH_STENCIL |
>   PIPE_BIND_RENDER_TARGET);
>
> But I'm not sure if that's enough; what if we get a surface with
> PIPE_BIND_SHADER_IMAGE set? We probably still want to use
> PIPE_BIND_RENDER_TARGET for the blit then...
>
How about this, let's deal with  PIPE_BIND_DEPTH_STENCIL
and PIPE_BIND_RENDER_TARGET first in this patch, also add a warning here
if we see any other bind so later people  can fix things based on real
cases. Then at least we fix something in this patch.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v2] virgl: Use right key to insert resource to hash.

[Lepton Wu]

The old code could use gem name as key when inserting it to bo_handles
hash table while trying to remove it from hash table with bo_handle as
key in virgl_hw_res_destroy and then it fail to remove it from bo_handles
hash table. This triggers use after free. Also, we should insert resource
to bo_names hash table when handle type is SHARED.

Signed-off-by: Lepton Wu 
---
 .../winsys/virgl/drm/virgl_drm_winsys.c   | 24 +--
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
index 2cf8b4ba076..af92b6a98fc 100644
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
@@ -406,6 +406,12 @@ virgl_drm_winsys_resource_create_handle(struct 
virgl_winsys *qws,
   return NULL;
}
 
+   if (whandle->type != WINSYS_HANDLE_TYPE_FD &&
+   whandle->type != WINSYS_HANDLE_TYPE_SHARED) {
+  fprintf(stderr, "Unexpected handle type: %d\n", whandle->type);
+  return NULL;
+   }
+
mtx_lock(>bo_handles_mutex);
 
if (whandle->type == WINSYS_HANDLE_TYPE_SHARED) {
@@ -424,13 +430,13 @@ virgl_drm_winsys_resource_create_handle(struct 
virgl_winsys *qws,
  res = NULL;
  goto done;
   }
-   }
 
-   res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);
-   if (res) {
-  struct virgl_hw_res *r = NULL;
-  virgl_drm_resource_reference(qdws, , res);
-  goto done;
+  res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);
+  if (res) {
+ struct virgl_hw_res *r = NULL;
+ virgl_drm_resource_reference(qdws, , res);
+ goto done;
+  }
}
 
res = CALLOC_STRUCT(virgl_hw_res);
@@ -448,6 +454,8 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys 
*qws,
  goto done;
   }
   res->bo_handle = open_arg.handle;
+  res->flinked = true;
+  res->flink = whandle->handle;
}
res->name = handle;
 
@@ -469,7 +477,9 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys 
*qws,
res->num_cs_references = 0;
res->fence_fd = -1;
 
-   util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)handle, res);
+   util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)res->bo_handle, 
res);
+   if (whandle->type == WINSYS_HANDLE_TYPE_SHARED)
+  util_hash_table_set(qdws->bo_names, (void *)(uintptr_t)res->flink, res);
 
 done:
mtx_unlock(>bo_handles_mutex);
-- 
2.21.0.392.gf8f6787159e-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v2] virgl: Set bind when creating temp resource.

[Lepton Wu]

virgl render complains about "Illegal resource" when running
dEQP-EGL.functional.color_clears.single_context.gles2.rgb888_window,
the reason is that a zero bind value was given for temp resource.

Signed-off-by: Lepton Wu 
---
 src/gallium/drivers/virgl/virgl_texture.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/gallium/drivers/virgl/virgl_texture.c 
b/src/gallium/drivers/virgl/virgl_texture.c
index 231319899e0..12b05b52508 100644
--- a/src/gallium/drivers/virgl/virgl_texture.c
+++ b/src/gallium/drivers/virgl/virgl_texture.c
@@ -60,12 +60,22 @@ static void virgl_copy_region_with_blit(struct pipe_context 
*pipe,
   pipe->blit(pipe, );
}
 }
+
+static unsigned int temp_bind(unsigned int orig)
+{
+   if (orig & ~(PIPE_BIND_RENDER_TARGET | PIPE_BIND_DEPTH_STENCIL |
+PIPE_BIND_SAMPLER_VIEW))
+  fprintf(stderr, "Waring, possibly unhandled bind: %x\n", orig);
+   return orig & (PIPE_BIND_DEPTH_STENCIL | PIPE_BIND_RENDER_TARGET);
+}
+
 static void virgl_init_temp_resource_from_box(struct pipe_resource *res,
   struct pipe_resource *orig,
   const struct pipe_box *box,
   unsigned level, unsigned flags)
 {
memset(res, 0, sizeof(*res));
+   res->bind = temp_bind(orig->bind);
res->format = orig->format;
res->width0 = box->width;
res->height0 = box->height;
-- 
2.21.0.392.gf8f6787159e-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.

[Lepton Wu]

The old code could use gem name as key when inserting it to bo_handles
hash table while trying to remove it from hash table with bo_handle as
key in virgl_hw_res_destroy. This triggers use after free. Also, we
should only reuse resource from bo_handle hash when the handle type is
FD.

Signed-off-by: Lepton Wu 
---
 src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
index 120e8eda2cd..01811a0e997 100644
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
@@ -424,13 +424,13 @@ virgl_drm_winsys_resource_create_handle(struct 
virgl_winsys *qws,
  res = NULL;
  goto done;
   }
-   }
 
-   res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);
-   if (res) {
-  struct virgl_hw_res *r = NULL;
-  virgl_drm_resource_reference(qdws, , res);
-  goto done;
+  res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);
+  if (res) {
+struct virgl_hw_res *r = NULL;
+virgl_drm_resource_reference(qdws, , res);
+goto done;
+  }
}
 
res = CALLOC_STRUCT(virgl_hw_res);
@@ -469,7 +469,8 @@ virgl_drm_winsys_resource_create_handle(struct virgl_winsys 
*qws,
res->num_cs_references = 0;
res->fence_fd = -1;
 
-   util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)handle, res);
+   util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)res->bo_handle,
+   res);
 
 done:
mtx_unlock(>bo_handles_mutex);
-- 
2.21.0.225.g810b269d1ac-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] virgl: Set bind when creating temp resource.

[Lepton Wu]

virgl render complains about "Illegal resource" when running
dEQP-EGL.functional.color_clears.single_context.gles2.rgb888_window,
the reason is that a zero bind value was given for temp resource.

Signed-off-by: Lepton Wu 
---
 src/gallium/drivers/virgl/virgl_texture.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/gallium/drivers/virgl/virgl_texture.c 
b/src/gallium/drivers/virgl/virgl_texture.c
index 231319899e0..563dbacba7e 100644
--- a/src/gallium/drivers/virgl/virgl_texture.c
+++ b/src/gallium/drivers/virgl/virgl_texture.c
@@ -66,6 +66,7 @@ static void virgl_init_temp_resource_from_box(struct 
pipe_resource *res,
   unsigned level, unsigned flags)
 {
memset(res, 0, sizeof(*res));
+   res->bind = orig->bind;
res->format = orig->format;
res->width0 = box->width;
res->height0 = box->height;
-- 
2.21.0.225.g810b269d1ac-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] virgl: close drm fd when destroying virgl screen.

[Lepton Wu]

This fd was create in virgl_drm_screen_create and should be closed
in virgl_drm_screen_destroy.

Signed-off-by: Lepton Wu 
---
 src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
index 01811a0e997..5501fe3ed48 100644
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
@@ -973,6 +973,7 @@ virgl_drm_screen_destroy(struct pipe_screen *pscreen)
if (destroy) {
   int fd = virgl_drm_winsys(screen->vws)->fd;
   util_hash_table_remove(fd_tab, intptr_to_pointer(fd));
+  close(fd);
}
mtx_unlock(_screen_mutex);
 
-- 
2.21.0.225.g810b269d1ac-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] virgl: Use right key to insert resource to hash.

[Lepton Wu]

On Mon, Apr 8, 2019 at 11:10 AM Chia-I Wu  wrote:
>
>
>
> On Mon, Apr 8, 2019 at 9:34 AM Lepton Wu  wrote:
>>
>> The old code could use gem name as key when inserting it to bo_handles
>> hash table while trying to remove it from hash table with bo_handle as
>> key in virgl_hw_res_destroy and then it fail to remove it from bo_handles
>> hash table. This triggers use after free. Also, we should insert resource
>> to bo_names hash table when handle type is SHARED.
>>
>> Signed-off-by: Lepton Wu 
>> ---
>>  .../winsys/virgl/drm/virgl_drm_winsys.c   | 24 +--
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
>> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> index 2cf8b4ba076..af92b6a98fc 100644
>> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> @@ -406,6 +406,12 @@ virgl_drm_winsys_resource_create_handle(struct 
>> virgl_winsys *qws,
>>return NULL;
>> }
>>
>> +   if (whandle->type != WINSYS_HANDLE_TYPE_FD &&
>> +   whandle->type != WINSYS_HANDLE_TYPE_SHARED) {
>> +  fprintf(stderr, "Unexpected handle type: %d\n", whandle->type);
>> +  return NULL;
>> +   }
>> +
>> mtx_lock(>bo_handles_mutex);
>>
>> if (whandle->type == WINSYS_HANDLE_TYPE_SHARED) {
>> @@ -424,13 +430,13 @@ virgl_drm_winsys_resource_create_handle(struct 
>> virgl_winsys *qws,
>>   res = NULL;
>>   goto done;
>>}
>> -   }
>>
>> -   res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);
>> -   if (res) {
>> -  struct virgl_hw_res *r = NULL;
>> -  virgl_drm_resource_reference(qdws, , res);
>> -  goto done;
>> +  res = util_hash_table_get(qdws->bo_handles, (void*)(uintptr_t)handle);
>> +  if (res) {
>> + struct virgl_hw_res *r = NULL;
>> + virgl_drm_resource_reference(qdws, , res);
>> + goto done;
>> +  }
>> }
>>
> You can still run into troubles when asked to import a buffer by both its 
> prime fd and its flink name.
But it seems there is no way to fix it at user space, right? Every
time we got a flink name for
the first time, kernel will give a new handle and no way to  reuse any
res from hash table.
>
>> res = CALLOC_STRUCT(virgl_hw_res);
>> @@ -448,6 +454,8 @@ virgl_drm_winsys_resource_create_handle(struct 
>> virgl_winsys *qws,
>>   goto done;
>>}
>>res->bo_handle = open_arg.handle;
>> +  res->flinked = true;
>> +  res->flink = whandle->handle;
>> }
>> res->name = handle;
>
> res->name has no user.  Remove it.
>
> It is also less confusing to make sure `handle' means GEM handle at this 
> point, by calling either GEM_OPEN or drmPrimeFDToHandle depending on the 
> handle type.
>
>>
>> @@ -469,7 +477,9 @@ virgl_drm_winsys_resource_create_handle(struct 
>> virgl_winsys *qws,
>> res->num_cs_references = 0;
>> res->fence_fd = -1;
>>
>> -   util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)handle, res);
>> +   util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)res->bo_handle, 
>> res);
>> +   if (whandle->type == WINSYS_HANDLE_TYPE_SHARED)
>> +  util_hash_table_set(qdws->bo_names, (void *)(uintptr_t)res->flink, 
>> res);
>>
>>  done:
>> mtx_unlock(>bo_handles_mutex);
>> --
>> 2.21.0.392.gf8f6787159e-goog
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] virgl: Use right key to insert resource to hash.

[Lepton Wu]

The kernel code will create a new gem handle for SHARED handle every
time, see code here:

https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_gem.c#L796


On Mon, Apr 8, 2019 at 11:12 AM Chia-I Wu  wrote:
>
>
>
> On Wed, Apr 3, 2019 at 8:17 PM Dave Airlie  wrote:
>>
>> On Thu, 4 Apr 2019 at 06:54, Chia-I Wu  wrote:
>> >
>> > You could end up having two virgl_hw_res with two different GEM handles 
>> > pointing to the same kernel GEM object.  That might break some assumptions 
>> > about dependency tracking.
>> >
>> > For example, when the cmdbuf being built uses a buffer and you want to 
>> > transfer some more data into the buffer, you normally need to submit the 
>> > cmdbuf first before starting the transfer.  The current code detects that 
>> > with virgl_drm_res_is_ref, which assumes each kernel GEM object has a 
>> > unique virgl_hw_res.
>> >
>> > On Mon, Apr 1, 2019 at 12:37 PM Lepton Wu  wrote:
>> >>
>> >>
>> >>
>> >>
>> >> On Wed, Mar 20, 2019 at 3:03 PM Chia-I Wu  wrote:
>> >>>
>> >>> On Mon, Mar 18, 2019 at 2:22 PM Lepton Wu  wrote:
>> >>>>
>> >>>> The old code could use gem name as key when inserting it to bo_handles
>> >>>> hash table while trying to remove it from hash table with bo_handle as
>> >>>> key in virgl_hw_res_destroy. This triggers use after free. Also, we
>> >>>> should only reuse resource from bo_handle hash when the handle type is
>> >>>> FD.
>> >>>
>> >>> Reuse is not very accurate.  Opening a shared handle (flink name) twice 
>> >>> gives two GEM handles.  Importing an fd handle (prime fd) twice gives 
>> >>> the same GEM handle.  In all cases, within a virgl_winsys, we want only 
>> >>> one GEM handle and only one virgl_resource for each kernel GEM object.
>> >>>
>> >>> I think the logic should go like:
>> >>>
>> >>>   if (HANDLE_TYPE_SHARED) {
>> >>> if (bo_names.has(flink_name))
>> >>>   return bo_names[flink_name];
>> >>> gem_handle = gem_open(flink_name);
>> >>>   } else {
>> >>> gem_handle = drmPrimeFDToHandle(prime_fd);
>> >>>   }
>> >>>
>> >>>
>> >>>   if (bo_handles.has(gem_handle))
>> >>> return bo_handles[gem_handle];
>> >>>   bo_handles[gem_handle] = create_new_resource();
>> >>>
>> >> Hi, the current patch did most of what you said with only one difference: 
>> >>  it didn't insert to bo_handles[]   hash when the type is  
>> >> HANDLE_TYPE_SHARED.
>> >> I think this is reasonable since opening a shared handle always get a new 
>> >> gem handle very time and I think it doesn't worth to insert it to 
>> >> bo_handles[] hash.
>> >> What do you think?
>>
>> Just to reinforce this, we can only have one GEM handle for a kernel
>> object, validation will go wrong and deadlock if we submit two handles
>> pointing at the same bo.
>>
>> Opening a shared handle should not get a new gem handle, if should
>> return any gem handle that already exists.
>
> I just tried and that is not the case.  Sounds like a kernel bug?
The kernel code will create a new gem handle for SHARED handle every
time, see code here:

https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_gem.c#L796
>
>>
>>
>> Dave.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support

[Lepton Wu]

Any concern for this CL? I think it's pretty safe to merge this since
any way android egl wrapper
doesn't like HAL_PIXEL_FORMAT_BGRA_ and won't return it for native
window format
and then won't set it as native window format.

https://android.googlesource.com/platform/frameworks/native/+/refs/heads/master/opengl/libs/EGL/eglApi.cpp#608

On Wed, Jul 31, 2019 at 3:50 PM Lepton Wu  wrote:
>
> From: Emil Velikov 
>
> As said in the EGL_KHR_platform_android extensions
>
> For each EGLConfig that belongs to the Android platform, the
> EGL_NATIVE_VISUAL_ID attribute is an Android window format, such as
> WINDOW_FORMAT_RGBA_.
>
> Although it should be applicable overall.
>
> Even though we use HAL_PIXEL_FORMAT here, those are numerically
> identical to the  WINDOW_FORMAT_ and AHARDWAREBUFFER_FORMAT_ ones.
>
> Barring the said format of course. That one is only listed in HAL.
>
> Keep in mind that even if we try to use the said format, you'll get
> caught by droid_create_surface(). The function compares the format of
> the underlying window, against the NATIVE_VISUAL_ID of the config.
>
> Unfortunatelly it only prints a warning, rather than error out, likely
> leading to visual corruption.
>
> While SDL will even call ANativeWindow_setBuffersGeometry() with the
> wrong format, and conviniently ignore the [expected] failure.
>
> Signed-off-by: Emil Velikov 
> Acked-by: Tomasz Figa 
> (tfiga: Remove only respective EGL config, leave EGL image as is.)
> Signed-off-by: Tomasz Figa 
> Signed-off-by: Lepton Wu 
> ---
>  src/egl/drivers/dri2/platform_android.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index d37f6b8e447..6c54fc4f1fe 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -1193,7 +1193,6 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
> _EGLDisplay *disp)
>{ HAL_PIXEL_FORMAT_RGBA_, { 0x00ff, 0xff00, 0x00ff, 
> 0xff00 } },
>{ HAL_PIXEL_FORMAT_RGBX_, { 0x00ff, 0xff00, 0x00ff, 
> 0x } },
>{ HAL_PIXEL_FORMAT_RGB_565,   { 0xf800, 0x07e0, 0x001f, 
> 0x } },
> -  { HAL_PIXEL_FORMAT_BGRA_, { 0x00ff, 0xff00, 0x00ff, 
> 0xff00 } },
> };
>
> unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
> --
> 2.22.0.770.g0f2c4a37fd-goog
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Concern about proposed patch "egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support"

[Lepton Wu]

That's interesting. The android frame work will hard coded to use
RGBA_ and set it as window format here:

https://android.googlesource.com/platform/frameworks/native/+/refs/heads/master/opengl/libs/EGL/eglApi.cpp#738

Do you have a custom version of android which do different code here?

For other GPU, like intel, if we choose a EGL config with BGRA ,
finally we got a wrong color on screen.

On Thu, Aug 15, 2019 at 12:49 PM Mauro Rossi  wrote:
>
> Hi,
>
> sorry if I'm communicating in dedicated thread, but I don't know how to reply 
> to existing one.
>
> The proposed patch is based on the wrong assumption that all drivers used 
> with Android have RGBA_, which is not correct, for example nouveau does 
> not support RGBA on primary planes, infact we have a fallback to simpler EGL 
> query in android-x86 and we use BGRA_ (pixel format = 5)
>
> If patch does not solve a specific problem and it will cause a regression at 
> least in nouveau, it is recommended to abandon the patch
>
> Thanks for your feedbacks
>
> Mauro
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Concern about proposed patch "egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support"

[Lepton Wu]

OK,

Actually android-x86 has a "workaround" at android side to enable BRGA here:

http://git.osdn.net/view?p=android-x86/frameworks-native.git;a=blobdiff;f=opengl/libs/EGL/eglApi.cpp;h=01eeb6489dd1db98fde2e8c5cda2e0bd36e83f39;hp=94dfe6a9de4b7d3a0064b8cc60e8b856da1be5d3;hb=519828c5b649e5e83f18444666f0672ab7852518;hpb=792c8dc009bd3a0c44eb39e757a95e099c03b54c

Not sure how difficult it is to push this to aosp android.

On Thu, Aug 15, 2019 at 3:31 PM Mauro Rossi  wrote:
>
> Hi Lepton,
>
> On Thu, Aug 15, 2019 at 9:58 PM Lepton Wu  wrote:
>>
>> That's interesting. The android frame work will hard coded to use
>> RGBA_ and set it as window format here:
>>
>> https://android.googlesource.com/platform/frameworks/native/+/refs/heads/master/opengl/libs/EGL/eglApi.cpp#738
>>
>> Do you have a custom version of android which do different code here?
>
>
> Affirmative, you may check [1] for reference
>
> [1] 
> http://git.osdn.net/view?p=android-x86/frameworks-native.git;a=commit;h=792c8dc009bd3a0c44eb39e757a95e099c03b54c
>
>>
>>
>> For other GPU, like intel, if we choose a EGL config with BGRA ,
>> finally we got a wrong color on screen.
>
>
> android-x86 implementation selects BGRA_ only when RGBA_ not 
> available,
> in a similar way to a pre-existing Workaround 10194508 that was removed from 
> AOSP code
>
> Removing HAL_PIXEL_FORMAT_BGRA_ support from egl/android
> SurfaceFlinger will just crash with SIGABRT on nouveau
>
>>
>>
>> On Thu, Aug 15, 2019 at 12:49 PM Mauro Rossi  wrote:
>> >
>> > Hi,
>> >
>> > sorry if I'm communicating in dedicated thread, but I don't know how to 
>> > reply to existing one.
>> >
>> > The proposed patch is based on the wrong assumption that all drivers used 
>> > with Android have RGBA_, which is not correct, for example nouveau 
>> > does not support RGBA on primary planes, infact we have a fallback to 
>> > simpler EGL query in android-x86 and we use BGRA_ (pixel format = 5)
>> >
>> > If patch does not solve a specific problem and it will cause a regression 
>> > at least in nouveau, it is recommended to abandon the patch
>> >
>> > Thanks for your feedbacks
>> >
>> > Mauro
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Concern about proposed patch "egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support"

[Lepton Wu]

On Thu, Aug 15, 2019 at 3:31 PM Mauro Rossi  wrote:
>
> Hi Lepton,
>
> On Thu, Aug 15, 2019 at 9:58 PM Lepton Wu  wrote:
>>
>> That's interesting. The android frame work will hard coded to use
>> RGBA_ and set it as window format here:
>>
>> https://android.googlesource.com/platform/frameworks/native/+/refs/heads/master/opengl/libs/EGL/eglApi.cpp#738
>>
>> Do you have a custom version of android which do different code here?
>
>
> Affirmative, you may check [1] for reference
>
> [1] 
> http://git.osdn.net/view?p=android-x86/frameworks-native.git;a=commit;h=792c8dc009bd3a0c44eb39e757a95e099c03b54c
>
>>
>>
>> For other GPU, like intel, if we choose a EGL config with BGRA ,
>> finally we got a wrong color on screen.
>
>
> android-x86 implementation selects BGRA_ only when RGBA_ not 
> available,
> in a similar way to a pre-existing Workaround 10194508 that was removed from 
> AOSP code
>
> Removing HAL_PIXEL_FORMAT_BGRA_ support from egl/android
> SurfaceFlinger will just crash with SIGABRT on nouveau
Then how about this: since aosp android really doesn't support
HAL_PIXEL_FORMAT_BGRA_, let me guard  HAL_PIXEL_FORMAT_BGRA_
with
a macro, maybe something like #ifdef ENABLE_ANDROID_EGL_CONFIG_BGRA,
and also send a patch at android-x86 side to set it when compiling
mesa.
This make sure it works for aosp android tree and also android-x86
>
>>
>>
>> On Thu, Aug 15, 2019 at 12:49 PM Mauro Rossi  wrote:
>> >
>> > Hi,
>> >
>> > sorry if I'm communicating in dedicated thread, but I don't know how to 
>> > reply to existing one.
>> >
>> > The proposed patch is based on the wrong assumption that all drivers used 
>> > with Android have RGBA_, which is not correct, for example nouveau 
>> > does not support RGBA on primary planes, infact we have a fallback to 
>> > simpler EGL query in android-x86 and we use BGRA_ (pixel format = 5)
>> >
>> > If patch does not solve a specific problem and it will cause a regression 
>> > at least in nouveau, it is recommended to abandon the patch
>> >
>> > Thanks for your feedbacks
>> >
>> > Mauro
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Concern about proposed patch "egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support"

[Lepton Wu]

On Tue, Aug 27, 2019 at 7:14 PM Lepton Wu  wrote:
>
> On Thu, Aug 15, 2019 at 3:31 PM Mauro Rossi  wrote:
> >
> > Hi Lepton,
> >
> > On Thu, Aug 15, 2019 at 9:58 PM Lepton Wu  wrote:
> >>
> >> That's interesting. The android frame work will hard coded to use
> >> RGBA_ and set it as window format here:
> >>
> >> https://android.googlesource.com/platform/frameworks/native/+/refs/heads/master/opengl/libs/EGL/eglApi.cpp#738
> >>
> >> Do you have a custom version of android which do different code here?
> >
> >
> > Affirmative, you may check [1] for reference
> >
> > [1] 
> > http://git.osdn.net/view?p=android-x86/frameworks-native.git;a=commit;h=792c8dc009bd3a0c44eb39e757a95e099c03b54c
> >
> >>
> >>
> >> For other GPU, like intel, if we choose a EGL config with BGRA ,
> >> finally we got a wrong color on screen.
> >
> >
> > android-x86 implementation selects BGRA_ only when RGBA_ not 
> > available,
> > in a similar way to a pre-existing Workaround 10194508 that was removed 
> > from AOSP code
> >
> > Removing HAL_PIXEL_FORMAT_BGRA_ support from egl/android
> > SurfaceFlinger will just crash with SIGABRT on nouveau
> Then how about this: since aosp android really doesn't support
> HAL_PIXEL_FORMAT_BGRA_, let me guard  HAL_PIXEL_FORMAT_BGRA_
> with
> a macro, maybe something like #ifdef ENABLE_ANDROID_EGL_CONFIG_BGRA,
> and also send a patch at android-x86 side to set it when compiling
> mesa.
> This make sure it works for aosp android tree and also android-x86
Another idea could be only dropping  BGRA config if there is RGBA
config, would this work for android-x86?
Basically I will check format_count[0], if it's non zero, skip all BGRA configs.
> >
> >>
> >>
> >> On Thu, Aug 15, 2019 at 12:49 PM Mauro Rossi  wrote:
> >> >
> >> > Hi,
> >> >
> >> > sorry if I'm communicating in dedicated thread, but I don't know how to 
> >> > reply to existing one.
> >> >
> >> > The proposed patch is based on the wrong assumption that all drivers 
> >> > used with Android have RGBA_, which is not correct, for example 
> >> > nouveau does not support RGBA on primary planes, infact we have a 
> >> > fallback to simpler EGL query in android-x86 and we use BGRA_ (pixel 
> >> > format = 5)
> >> >
> >> > If patch does not solve a specific problem and it will cause a 
> >> > regression at least in nouveau, it is recommended to abandon the patch
> >> >
> >> > Thanks for your feedbacks
> >> >
> >> > Mauro
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Concern about proposed patch "egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support"

[Lepton Wu]

On Tue, Aug 27, 2019 at 7:35 PM Lepton Wu  wrote:
>
> On Tue, Aug 27, 2019 at 7:14 PM Lepton Wu  wrote:
> >
> > On Thu, Aug 15, 2019 at 3:31 PM Mauro Rossi  wrote:
> > >
> > > Hi Lepton,
> > >
> > > On Thu, Aug 15, 2019 at 9:58 PM Lepton Wu  wrote:
> > >>
> > >> That's interesting. The android frame work will hard coded to use
> > >> RGBA_ and set it as window format here:
> > >>
> > >> https://android.googlesource.com/platform/frameworks/native/+/refs/heads/master/opengl/libs/EGL/eglApi.cpp#738
> > >>
> > >> Do you have a custom version of android which do different code here?
> > >
> > >
> > > Affirmative, you may check [1] for reference
> > >
> > > [1] 
> > > http://git.osdn.net/view?p=android-x86/frameworks-native.git;a=commit;h=792c8dc009bd3a0c44eb39e757a95e099c03b54c
> > >
> > >>
> > >>
> > >> For other GPU, like intel, if we choose a EGL config with BGRA ,
> > >> finally we got a wrong color on screen.
> > >
> > >
> > > android-x86 implementation selects BGRA_ only when RGBA_ not 
> > > available,
> > > in a similar way to a pre-existing Workaround 10194508 that was removed 
> > > from AOSP code
> > >
> > > Removing HAL_PIXEL_FORMAT_BGRA_ support from egl/android
> > > SurfaceFlinger will just crash with SIGABRT on nouveau
> > Then how about this: since aosp android really doesn't support
> > HAL_PIXEL_FORMAT_BGRA_, let me guard  HAL_PIXEL_FORMAT_BGRA_
> > with
> > a macro, maybe something like #ifdef ENABLE_ANDROID_EGL_CONFIG_BGRA,
> > and also send a patch at android-x86 side to set it when compiling
> > mesa.
> > This make sure it works for aosp android tree and also android-x86
> Another idea could be only dropping  BGRA config if there is RGBA
> config, would this work for android-x86?
> Basically I will check format_count[0], if it's non zero, skip all BGRA 
> configs.
FYI, I draft a new CL and send it as merge request and hope it works for all.

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1865

> > >
> > >>
> > >>
> > >> On Thu, Aug 15, 2019 at 12:49 PM Mauro Rossi  
> > >> wrote:
> > >> >
> > >> > Hi,
> > >> >
> > >> > sorry if I'm communicating in dedicated thread, but I don't know how 
> > >> > to reply to existing one.
> > >> >
> > >> > The proposed patch is based on the wrong assumption that all drivers 
> > >> > used with Android have RGBA_, which is not correct, for example 
> > >> > nouveau does not support RGBA on primary planes, infact we have a 
> > >> > fallback to simpler EGL query in android-x86 and we use BGRA_ 
> > >> > (pixel format = 5)
> > >> >
> > >> > If patch does not solve a specific problem and it will cause a 
> > >> > regression at least in nouveau, it is recommended to abandon the patch
> > >> >
> > >> > Thanks for your feedbacks
> > >> >
> > >> > Mauro
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support

[Lepton Wu]

From: Emil Velikov 

As said in the EGL_KHR_platform_android extensions

For each EGLConfig that belongs to the Android platform, the
EGL_NATIVE_VISUAL_ID attribute is an Android window format, such as
WINDOW_FORMAT_RGBA_.

Although it should be applicable overall.

Even though we use HAL_PIXEL_FORMAT here, those are numerically
identical to the  WINDOW_FORMAT_ and AHARDWAREBUFFER_FORMAT_ ones.

Barring the said format of course. That one is only listed in HAL.

Keep in mind that even if we try to use the said format, you'll get
caught by droid_create_surface(). The function compares the format of
the underlying window, against the NATIVE_VISUAL_ID of the config.

Unfortunatelly it only prints a warning, rather than error out, likely
leading to visual corruption.

While SDL will even call ANativeWindow_setBuffersGeometry() with the
wrong format, and conviniently ignore the [expected] failure.

Signed-off-by: Emil Velikov 
Acked-by: Tomasz Figa 
(tfiga: Remove only respective EGL config, leave EGL image as is.)
Signed-off-by: Tomasz Figa 
Signed-off-by: Lepton Wu 
---
 src/egl/drivers/dri2/platform_android.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index d37f6b8e447..6c54fc4f1fe 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1193,7 +1193,6 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *disp)
   { HAL_PIXEL_FORMAT_RGBA_, { 0x00ff, 0xff00, 0x00ff, 
0xff00 } },
   { HAL_PIXEL_FORMAT_RGBX_, { 0x00ff, 0xff00, 0x00ff, 
0x } },
   { HAL_PIXEL_FORMAT_RGB_565,   { 0xf800, 0x07e0, 0x001f, 
0x } },
-  { HAL_PIXEL_FORMAT_BGRA_, { 0x00ff, 0xff00, 0x00ff, 
0xff00 } },
};
 
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
-- 
2.22.0.770.g0f2c4a37fd-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] virgl: Set meta data for textures from handle.

[Lepton Wu]

The set of meta data was removed by commit 8083464. It broke lots of
dEQP tests when running with pbuffer surface type.

Fixes: 80834640137 ("virgl: remove dead code")
Signed-off-by: Lepton Wu 
---
 src/gallium/drivers/virgl/virgl_resource.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/gallium/drivers/virgl/virgl_resource.c 
b/src/gallium/drivers/virgl/virgl_resource.c
index c22a78a4731..909deb774c7 100644
--- a/src/gallium/drivers/virgl/virgl_resource.c
+++ b/src/gallium/drivers/virgl/virgl_resource.c
@@ -515,6 +515,7 @@ static struct pipe_resource 
*virgl_resource_from_handle(struct pipe_screen *scre
res->u.b = *templ;
res->u.b.screen = >base;
pipe_reference_init(>u.b.reference, 1);
+   virgl_resource_layout(>u.b, >metadata);
 
res->hw_res = vs->vws->resource_create_from_handle(vs->vws, whandle);
if (!res->hw_res) {
-- 
2.22.0.510.g264f2c817a-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] virgl: Set meta data for textures from handle.

[Lepton Wu]

On Wed, Jul 17, 2019 at 11:26 AM Chia-I Wu  wrote:
>
> On Wed, Jul 17, 2019 at 10:14 AM Erik Faye-Lund
>  wrote:
> >
> > On Wed, 2019-07-17 at 10:02 -0700, Lepton Wu wrote:
> > > The set of meta data was removed by commit 8083464. It broke lots of
> > > dEQP tests when running with pbuffer surface type.
> > >
> > > Fixes: 80834640137 ("virgl: remove dead code")
> > > Signed-off-by: Lepton Wu 
> > > ---
> > >  src/gallium/drivers/virgl/virgl_resource.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/src/gallium/drivers/virgl/virgl_resource.c
> > > b/src/gallium/drivers/virgl/virgl_resource.c
> > > index c22a78a4731..909deb774c7 100644
> > > --- a/src/gallium/drivers/virgl/virgl_resource.c
> > > +++ b/src/gallium/drivers/virgl/virgl_resource.c
> > > @@ -515,6 +515,7 @@ static struct pipe_resource
> > > *virgl_resource_from_handle(struct pipe_screen *scre
> > > res->u.b = *templ;
> > > res->u.b.screen = >base;
> > > pipe_reference_init(>u.b.reference, 1);
> > > +   virgl_resource_layout(>u.b, >metadata);
> There was a similar MR for this
>
>   https://gitlab.freedesktop.org/mesa/mesa/merge_requests/965
>
> Can you add a check to make sure the stride is compatible?
I think this kind of check should be in "framework" side instead of
inside virgl driver.
The check what you are said is basically to check if  stride info n
whandle is comptabile
with value in pipe_resource, I think if we need this check, we should
put it in dri2_allocate_textures
and dri2_create_image_from_winsys? and that should be another CL?
>
>   if (res->metadata->stride[0] != whandle->stride) reject the whandle;
>
> > >
> > > res->hw_res = vs->vws->resource_create_from_handle(vs->vws,
> > > whandle);
> > > if (!res->hw_res) {
> >
> > Whoops! Good catch, sorry for the mess!
> >
> > Reviewed-by: Erik Faye-Lund 
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] virgl: Set meta data for textures from handle.

[Lepton Wu]

metadata->stride[0] is calculated from
util_format_get_stride(pt->format, pt->width0);
So basically you are asking to check if
util_format_get_stride(pt->format, pt->width0) == whandle->stride
Should this be something done by framework?

On Wed, Jul 17, 2019 at 12:25 PM Chia-I Wu  wrote:
>
> On Wed, Jul 17, 2019 at 11:44 AM Lepton Wu  wrote:
> >
> > On Wed, Jul 17, 2019 at 11:26 AM Chia-I Wu  wrote:
> > >
> > > On Wed, Jul 17, 2019 at 10:14 AM Erik Faye-Lund
> > >  wrote:
> > > >
> > > > On Wed, 2019-07-17 at 10:02 -0700, Lepton Wu wrote:
> > > > > The set of meta data was removed by commit 8083464. It broke lots of
> > > > > dEQP tests when running with pbuffer surface type.
> > > > >
> > > > > Fixes: 80834640137 ("virgl: remove dead code")
> > > > > Signed-off-by: Lepton Wu 
> > > > > ---
> > > > >  src/gallium/drivers/virgl/virgl_resource.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/src/gallium/drivers/virgl/virgl_resource.c
> > > > > b/src/gallium/drivers/virgl/virgl_resource.c
> > > > > index c22a78a4731..909deb774c7 100644
> > > > > --- a/src/gallium/drivers/virgl/virgl_resource.c
> > > > > +++ b/src/gallium/drivers/virgl/virgl_resource.c
> > > > > @@ -515,6 +515,7 @@ static struct pipe_resource
> > > > > *virgl_resource_from_handle(struct pipe_screen *scre
> > > > > res->u.b = *templ;
> > > > > res->u.b.screen = >base;
> > > > > pipe_reference_init(>u.b.reference, 1);
> > > > > +   virgl_resource_layout(>u.b, >metadata);
> > > There was a similar MR for this
> > >
> > >   https://gitlab.freedesktop.org/mesa/mesa/merge_requests/965
> > >
> > > Can you add a check to make sure the stride is compatible?
> > I think this kind of check should be in "framework" side instead of
> > inside virgl driver.
> > The check what you are said is basically to check if  stride info n
> > whandle is comptabile
> > with value in pipe_resource, I think if we need this check, we should
> > put it in dri2_allocate_textures
> > and dri2_create_image_from_winsys? and that should be another CL?
> The framework does not know the stride of a pipe resource.
>
> > >
> > >   if (res->metadata->stride[0] != whandle->stride) reject the whandle;
> > >
> > > > >
> > > > > res->hw_res = vs->vws->resource_create_from_handle(vs->vws,
> > > > > whandle);
> > > > > if (!res->hw_res) {
> > > >
> > > > Whoops! Good catch, sorry for the mess!
> > > >
> > > > Reviewed-by: Erik Faye-Lund 
> > > >
> > > > ___
> > > > mesa-dev mailing list
> > > > mesa-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] virgl: Set meta data for textures from handle.

[Lepton Wu]

OK, actually struct winsys_handle is an obscure structure for virgl
driver so we can't access whandle->stride here...
So maybe just leave this CL as it is?

On Wed, Jul 17, 2019 at 1:12 PM Chia-I Wu  wrote:
>
> On Wed, Jul 17, 2019 at 12:45 PM Lepton Wu  wrote:
> > metadata->stride[0] is calculated from
> > util_format_get_stride(pt->format, pt->width0);
> > So basically you are asking to check if
> > util_format_get_stride(pt->format, pt->width0) == whandle->stride
> > Should this be something done by framework?
> The framework does not know that is how virgl decides the stride for a
> resource.  That is also not the case for most drivers.
>
> The framework asks virgl to import a buffer with the specified stride.
> virgl should either accept it, or reject it if virgl does not want to
> handle the unexpected stride.  Not that I believe that is going to
> happen, but still...
>
> >
> > On Wed, Jul 17, 2019 at 12:25 PM Chia-I Wu  wrote:
> > >
> > > On Wed, Jul 17, 2019 at 11:44 AM Lepton Wu  wrote:
> > > >
> > > > On Wed, Jul 17, 2019 at 11:26 AM Chia-I Wu  wrote:
> > > > >
> > > > > On Wed, Jul 17, 2019 at 10:14 AM Erik Faye-Lund
> > > > >  wrote:
> > > > > >
> > > > > > On Wed, 2019-07-17 at 10:02 -0700, Lepton Wu wrote:
> > > > > > > The set of meta data was removed by commit 8083464. It broke lots 
> > > > > > > of
> > > > > > > dEQP tests when running with pbuffer surface type.
> > > > > > >
> > > > > > > Fixes: 80834640137 ("virgl: remove dead code")
> > > > > > > Signed-off-by: Lepton Wu 
> > > > > > > ---
> > > > > > >  src/gallium/drivers/virgl/virgl_resource.c | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/src/gallium/drivers/virgl/virgl_resource.c
> > > > > > > b/src/gallium/drivers/virgl/virgl_resource.c
> > > > > > > index c22a78a4731..909deb774c7 100644
> > > > > > > --- a/src/gallium/drivers/virgl/virgl_resource.c
> > > > > > > +++ b/src/gallium/drivers/virgl/virgl_resource.c
> > > > > > > @@ -515,6 +515,7 @@ static struct pipe_resource
> > > > > > > *virgl_resource_from_handle(struct pipe_screen *scre
> > > > > > > res->u.b = *templ;
> > > > > > > res->u.b.screen = >base;
> > > > > > > pipe_reference_init(>u.b.reference, 1);
> > > > > > > +   virgl_resource_layout(>u.b, >metadata);
> > > > > There was a similar MR for this
> > > > >
> > > > >   https://gitlab.freedesktop.org/mesa/mesa/merge_requests/965
> > > > >
> > > > > Can you add a check to make sure the stride is compatible?
> > > > I think this kind of check should be in "framework" side instead of
> > > > inside virgl driver.
> > > > The check what you are said is basically to check if  stride info n
> > > > whandle is comptabile
> > > > with value in pipe_resource, I think if we need this check, we should
> > > > put it in dri2_allocate_textures
> > > > and dri2_create_image_from_winsys? and that should be another CL?
> > > The framework does not know the stride of a pipe resource.
> > >
> > > > >
> > > > >   if (res->metadata->stride[0] != whandle->stride) reject the whandle;
> > > > >
> > > > > > >
> > > > > > > res->hw_res = vs->vws->resource_create_from_handle(vs->vws,
> > > > > > > whandle);
> > > > > > > if (!res->hw_res) {
> > > > > >
> > > > > > Whoops! Good catch, sorry for the mess!
> > > > > >
> > > > > > Reviewed-by: Erik Faye-Lund 
> > > > > >
> > > > > > ___
> > > > > > mesa-dev mailing list
> > > > > > mesa-dev@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] A question about order of st_update_*p

[Lepton Wu]

In src/mesa/state_tracker/st_atom_list.h,

Now it's this order:

ST_STATE(ST_NEW_FS_STATE, st_update_fp)
ST_STATE(ST_NEW_GS_STATE, st_update_gp)
ST_STATE(ST_NEW_TES_STATE, st_update_tep)
ST_STATE(ST_NEW_TCS_STATE, st_update_tcp)
ST_STATE(ST_NEW_VS_STATE, st_update_vp)

While code in
src/mesa/state_tracker/st_atom.c:

while (dirty_lo)
 update_functions[u_bit_scan(_lo)](st);

That means if will call st_update_fp first and then st_update_gp... etc.

But this is inconsistent with opengl pipeline: should we reverse the
order here or I missed something?

Background:

https://gitlab.freedesktop.org/virgl/virglrenderer/issues/114
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] A question about order of st_update_*p

[Lepton Wu]

I am confused, for example,  according to:

https://www.khronos.org/opengl/wiki/Geometry_Shader, it come after
Vertex shader, that mean, it depends on vertex shader, right?
So st_update_gp should be after st_update_vp, but now it is in the
front of st_update_vp.

On Fri, Jul 26, 2019 at 1:07 PM Marek Olšák  wrote:
>
>
>
> On Fri., Jul. 26, 2019, 15:58 Lepton Wu,  wrote:
>>
>> If shader A depends on shader B,  should we put st_update_shaderB  in
>> front of st_update_shaderA?
>
>
> That's the current order. The order of GPU execution doesn't matter.
>
> M.
>
>>
>> Now the order looks like reversed...
>>
>> On Fri, Jul 26, 2019 at 12:34 PM Marek Olšák  wrote:
>> >
>> > The order shouldn't matter, but there can be a reason behind it, e.g. if a 
>> > shader depends on the update of the following shader.
>> >
>> > Marek
>> >
>> > On Wed, Jul 24, 2019 at 7:19 PM Lepton Wu  wrote:
>> >>
>> >> In src/mesa/state_tracker/st_atom_list.h,
>> >>
>> >> Now it's this order:
>> >>
>> >> ST_STATE(ST_NEW_FS_STATE, st_update_fp)
>> >> ST_STATE(ST_NEW_GS_STATE, st_update_gp)
>> >> ST_STATE(ST_NEW_TES_STATE, st_update_tep)
>> >> ST_STATE(ST_NEW_TCS_STATE, st_update_tcp)
>> >> ST_STATE(ST_NEW_VS_STATE, st_update_vp)
>> >>
>> >> While code in
>> >> src/mesa/state_tracker/st_atom.c:
>> >>
>> >> while (dirty_lo)
>> >>  update_functions[u_bit_scan(_lo)](st);
>> >>
>> >> That means if will call st_update_fp first and then st_update_gp... etc.
>> >>
>> >> But this is inconsistent with opengl pipeline: should we reverse the
>> >> order here or I missed something?
>> >>
>> >> Background:
>> >>
>> >> https://gitlab.freedesktop.org/virgl/virglrenderer/issues/114
>> >> ___
>> >> mesa-dev mailing list
>> >> mesa-dev@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] A question about order of st_update_*p

[Lepton Wu]

If shader A depends on shader B,  should we put st_update_shaderB  in
front of st_update_shaderA?

Now the order looks like reversed...

On Fri, Jul 26, 2019 at 12:34 PM Marek Olšák  wrote:
>
> The order shouldn't matter, but there can be a reason behind it, e.g. if a 
> shader depends on the update of the following shader.
>
> Marek
>
> On Wed, Jul 24, 2019 at 7:19 PM Lepton Wu  wrote:
>>
>> In src/mesa/state_tracker/st_atom_list.h,
>>
>> Now it's this order:
>>
>> ST_STATE(ST_NEW_FS_STATE, st_update_fp)
>> ST_STATE(ST_NEW_GS_STATE, st_update_gp)
>> ST_STATE(ST_NEW_TES_STATE, st_update_tep)
>> ST_STATE(ST_NEW_TCS_STATE, st_update_tcp)
>> ST_STATE(ST_NEW_VS_STATE, st_update_vp)
>>
>> While code in
>> src/mesa/state_tracker/st_atom.c:
>>
>> while (dirty_lo)
>>  update_functions[u_bit_scan(_lo)](st);
>>
>> That means if will call st_update_fp first and then st_update_gp... etc.
>>
>> But this is inconsistent with opengl pipeline: should we reverse the
>> order here or I missed something?
>>
>> Background:
>>
>> https://gitlab.freedesktop.org/virgl/virglrenderer/issues/114
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] A question about order of st_update_*p

[Lepton Wu]

OK. Would you mind give some link to some document what's the exact
meaning of "dependence"
in st/mesa since it's unrelated with gpu execution. Thanks.

On Fri, Jul 26, 2019 at 1:27 PM Marek Olšák  wrote:
>
> The order of GPU execution doesn't matter here.
>
> M.
>
> On Fri., Jul. 26, 2019, 16:11 Lepton Wu,  wrote:
>>
>> I am confused, for example,  according to:
>>
>> https://www.khronos.org/opengl/wiki/Geometry_Shader, it come after
>> Vertex shader, that mean, it depends on vertex shader, right?
>> So st_update_gp should be after st_update_vp, but now it is in the
>> front of st_update_vp.
>>
>> On Fri, Jul 26, 2019 at 1:07 PM Marek Olšák  wrote:
>> >
>> >
>> >
>> > On Fri., Jul. 26, 2019, 15:58 Lepton Wu,  wrote:
>> >>
>> >> If shader A depends on shader B,  should we put st_update_shaderB  in
>> >> front of st_update_shaderA?
>> >
>> >
>> > That's the current order. The order of GPU execution doesn't matter.
>> >
>> > M.
>> >
>> >>
>> >> Now the order looks like reversed...
>> >>
>> >> On Fri, Jul 26, 2019 at 12:34 PM Marek Olšák  wrote:
>> >> >
>> >> > The order shouldn't matter, but there can be a reason behind it, e.g. 
>> >> > if a shader depends on the update of the following shader.
>> >> >
>> >> > Marek
>> >> >
>> >> > On Wed, Jul 24, 2019 at 7:19 PM Lepton Wu  wrote:
>> >> >>
>> >> >> In src/mesa/state_tracker/st_atom_list.h,
>> >> >>
>> >> >> Now it's this order:
>> >> >>
>> >> >> ST_STATE(ST_NEW_FS_STATE, st_update_fp)
>> >> >> ST_STATE(ST_NEW_GS_STATE, st_update_gp)
>> >> >> ST_STATE(ST_NEW_TES_STATE, st_update_tep)
>> >> >> ST_STATE(ST_NEW_TCS_STATE, st_update_tcp)
>> >> >> ST_STATE(ST_NEW_VS_STATE, st_update_vp)
>> >> >>
>> >> >> While code in
>> >> >> src/mesa/state_tracker/st_atom.c:
>> >> >>
>> >> >> while (dirty_lo)
>> >> >>  update_functions[u_bit_scan(_lo)](st);
>> >> >>
>> >> >> That means if will call st_update_fp first and then st_update_gp... 
>> >> >> etc.
>> >> >>
>> >> >> But this is inconsistent with opengl pipeline: should we reverse the
>> >> >> order here or I missed something?
>> >> >>
>> >> >> Background:
>> >> >>
>> >> >> https://gitlab.freedesktop.org/virgl/virglrenderer/issues/114
>> >> >> ___
>> >> >> mesa-dev mailing list
>> >> >> mesa-dev@lists.freedesktop.org
>> >> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] mapi: avoid text relocation in x86 tsd stubs

[Lepton Wu]

OK, I didn't know there is a patch in mail list.  BTW, it seems there is no
meaningful difference
with the generated code:  when it's trying to align to 32 bytes border,
since the actual code size
is between 32 and 64, actually the entry size is 64.   So in this case,
balign to 32 or 64 has same result

I heard that mesa will change to use libglvnd, if that will happen soon,
then we don't worry about code here
any more.

On Wed, Nov 27, 2019 at 5:57 AM Jonathan Gray  wrote:

> On Sun, Nov 11, 2018 at 03:47:35PM +1100, Jonathan Gray wrote:
> > Make similiar changes to libglvnd to avoid a text relocation in
> > x86 tsd stubs fixing the build with lld.
> >
> > v2:
> >   - store the address of the GOT in ebx required before calling PLT stub
> >   - change .balign values to match X86_ENTRY_SIZE
>
> When a different version of this patch was committed/pushed in
> 45206d7673adb1484cbdb3eadaf82e0849c9cdcf
> (with author rewritten to imply I wrote the commit message)
> it did not include the .balign changes to match the entry size change.
>
> Was this purposefully skipped or overlooked as the bugzilla patch was
> used?
>
> The diff to what was committed and this patch is
>
> diff --git a/src/mapi/entry_x86_tsd.h b/src/mapi/entry_x86_tsd.h
> index bd8db7b19f9..1dec3ed86c4 100644
> --- a/src/mapi/entry_x86_tsd.h
> +++ b/src/mapi/entry_x86_tsd.h
> @@ -34,18 +34,18 @@
>  #define X86_ENTRY_SIZE 64
>
>  __asm__(".text\n"
> -".balign 32\n"
> +".balign " U_STRINGIFY(X86_ENTRY_SIZE) "\n"
>  "x86_entry_start:");
>
>  #define STUB_ASM_ENTRY(func)\
> ".globl " func "\n"  \
> ".type " func ", @function\n"\
> -   ".balign 32\n"   \
> +   ".balign " U_STRINGIFY(X86_ENTRY_SIZE) "\n" \
> func ":"
>
>  #define STUB_ASM_CODE(slot) \
> "push %ebx\n\t"  \
> -   "call 1f\n\t"\
> +   "call 1f\n"  \
> "1:\n\t" \
> "popl %ebx\n\t"  \
> "addl $_GLOBAL_OFFSET_TABLE_+[.-1b], %ebx\n\t" \
> @@ -53,7 +53,7 @@ __asm__(".text\n"
> "mov (%eax), %eax\n\t"   \
> "testl %eax, %eax\n\t"   \
> "jne 1f\n\t" \
> -   "call " ENTRY_CURRENT_TABLE_GET "@PLT\n\t" \
> +   "call " ENTRY_CURRENT_TABLE_GET "@PLT\n" \
> "1:\n\t" \
> "pop %ebx\n\t"   \
> "jmp *(4 * " slot ")(%eax)"
> @@ -63,7 +63,7 @@ __asm__(".text\n"
>
>  #ifndef MAPI_MODE_BRIDGE
>
> -__asm__(".balign 32\n"
> +__asm__(".balign " U_STRINGIFY(X86_ENTRY_SIZE) "\n"
>  "x86_entry_end:");
>
>  #include 
>
> >
> > Signed-off-by: Jonathan Gray 
> > Cc: mesa-sta...@lists.freedesktop.org
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108541
> > ---
> >  src/mapi/entry_x86_tsd.h | 22 ++
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/mapi/entry_x86_tsd.h b/src/mapi/entry_x86_tsd.h
> > index 0c28c8ff068..1dec3ed86c4 100644
> > --- a/src/mapi/entry_x86_tsd.h
> > +++ b/src/mapi/entry_x86_tsd.h
> > @@ -31,25 +31,31 @@
> >  #define HIDDEN
> >  #endif
> >
> > -#define X86_ENTRY_SIZE 32
> > +#define X86_ENTRY_SIZE 64
> >
> >  __asm__(".text\n"
> > -".balign 32\n"
> > +".balign " U_STRINGIFY(X86_ENTRY_SIZE) "\n"
> >  "x86_entry_start:");
> >
> >  #define STUB_ASM_ENTRY(func)\
> > ".globl " func "\n"  \
> > ".type " func ", @function\n"\
> > -   ".balign 32\n"   \
> > +   ".balign " U_STRINGIFY(X86_ENTRY_SIZE) "\n" \
> > func ":"
> >
> >  #define STUB_ASM_CODE(slot) \
> > -   "movl " ENTRY_CURRENT_TABLE ", %eax\n\t" \
> > +   "push %ebx\n\t"  \
> > +   "call 1f\n"  \
> > +   "1:\n\t" \
> > +   "popl %ebx\n\t"  \
> > +   "addl $_GLOBAL_OFFSET_TABLE_+[.-1b], %ebx\n\t" \
> > +   "movl " ENTRY_CURRENT_TABLE "@GOT(%ebx), %eax\n\t" \
> > +   "mov (%eax), %eax\n\t"   \
> > "testl %eax, %eax\n\t"   \
> > -   "je 1f\n\t"  \
> > -   "jmp *(4 * " slot ")(%eax)\n"\
> > +   "jne 1f\n\t" \
> > +   "call " ENTRY_CURRENT_TABLE_GET "@PLT\n" \
> > "1:\n\t" \
> > -   "call " ENTRY_CURRENT_TABLE_GET "\n\t" \
> > +   "pop %ebx\n\t"   \
> > "jmp *(4 * " slot ")(%eax)"
> >
> >  #define MAPI_TMP_STUB_ASM_GCC
> > @@ -57,7 +63,7 @@ __asm__(".text\n"
> >
> >  #ifndef MAPI_MODE_BRIDGE
> >
> > -__asm__(".balign 32\n"
> > +__asm__(".balign " U_STRINGIFY(X86_ENTRY_SIZE) "\n"
> >  "x86_entry_end:");
> >
> >  #include 
> > --
> > 2.19.1
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org