[Mesa-dev] [PATCH] llvmpipe : Fixed an issue where the display target texture was mapped multiple times.

2018-04-18 Thread Seongchan Jeong 
The lp_setup_set_fragment_sampler_views function can be called
when the texture module is enabled. However, mapping can be
performed several times for one display target texture, but
unmapping does not proceed. So some logic have been added to
unmap the display target texture to prevent additional mappings
when the texture is already mapped.
---
 src/gallium/drivers/llvmpipe/lp_setup.c   | 9 +
 src/gallium/drivers/llvmpipe/lp_texture.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c 
b/src/gallium/drivers/llvmpipe/lp_setup.c
index c157323133..71ceafe2b7 100644
--- a/src/gallium/drivers/llvmpipe/lp_setup.c
+++ b/src/gallium/drivers/llvmpipe/lp_setup.c
@@ -907,6 +907,13 @@ lp_setup_set_fragment_sampler_views(struct 
lp_setup_context *setup,
  */
 struct llvmpipe_screen *screen = llvmpipe_screen(res->screen);
 struct sw_winsys *winsys = screen->winsys;
+
+/* unmap the texture which is already mapped */
+if(lp_tex->mapped){
+winsys->displaytarget_unmap(winsys, lp_tex->dt);
+lp_tex->mapped = false;
+}
+
 jit_tex->base = winsys->displaytarget_map(winsys, lp_tex->dt,
  PIPE_TRANSFER_READ);
 jit_tex->row_stride[0] = lp_tex->row_stride[0];
@@ -917,6 +924,8 @@ lp_setup_set_fragment_sampler_views(struct lp_setup_context 
*setup,
 jit_tex->depth = res->depth0;
 jit_tex->first_level = jit_tex->last_level = 0;
 assert(jit_tex->base);
+
+lp_tex->mapped = true;
  }
   }
   else {
diff --git a/src/gallium/drivers/llvmpipe/lp_texture.h 
b/src/gallium/drivers/llvmpipe/lp_texture.h
index 3d315bb9a7..9e39d31eb3 100644
--- a/src/gallium/drivers/llvmpipe/lp_texture.h
+++ b/src/gallium/drivers/llvmpipe/lp_texture.h
@@ -75,6 +75,8 @@ struct llvmpipe_resource
 */
struct sw_displaytarget *dt;
 
+   boolean mapped; /** status of displaytarget, map or unmap */
+
/**
 * Malloc'ed data for regular textures, or a mapping to dt above.
 */
-- 
2.14.1

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

Re: [Mesa-dev] [PATCH] llvmpipe : Fixed an issue where the display target texture was mapped multiple times.

2018-04-19 Thread Roland Scheidegger 
Am 19.04.2018 um 08:04 schrieb Seongchan Jeong:
> The lp_setup_set_fragment_sampler_views function can be called
> when the texture module is enabled. However, mapping can be
> performed several times for one display target texture, but
> unmapping does not proceed. So some logic have been added to
> unmap the display target texture to prevent additional mappings
> when the texture is already mapped.
> ---
>  src/gallium/drivers/llvmpipe/lp_setup.c   | 9 +
>  src/gallium/drivers/llvmpipe/lp_texture.h | 2 ++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c 
> b/src/gallium/drivers/llvmpipe/lp_setup.c
> index c157323133..71ceafe2b7 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> @@ -907,6 +907,13 @@ lp_setup_set_fragment_sampler_views(struct 
> lp_setup_context *setup,
>   */
>  struct llvmpipe_screen *screen = llvmpipe_screen(res->screen);
>  struct sw_winsys *winsys = screen->winsys;
> +
> +/* unmap the texture which is already mapped */
> +if(lp_tex->mapped){
> +winsys->displaytarget_unmap(winsys, lp_tex->dt);
> +lp_tex->mapped = false;
> +}
> +
>  jit_tex->base = winsys->displaytarget_map(winsys, lp_tex->dt,
>   PIPE_TRANSFER_READ);
>  jit_tex->row_stride[0] = lp_tex->row_stride[0];
> @@ -917,6 +924,8 @@ lp_setup_set_fragment_sampler_views(struct 
> lp_setup_context *setup,
>  jit_tex->depth = res->depth0;
>  jit_tex->first_level = jit_tex->last_level = 0;
>  assert(jit_tex->base);
> +
> +lp_tex->mapped = true;

I am not quite convinced this is the right fix.
Clearly the code right now isn't right, and pretty much relies on the
winsys->displaytarget_map() being a no-op there just giving the mapping
without any side effects.
The problem with this fix is it still would be kept mapped in the end
after sampling (and, it can and probably will be mapped elsewhere too
still).

Do you hit any specific bug with the code as-is?

Roland


>   }
>}
>else {
> diff --git a/src/gallium/drivers/llvmpipe/lp_texture.h 
> b/src/gallium/drivers/llvmpipe/lp_texture.h
> index 3d315bb9a7..9e39d31eb3 100644
> --- a/src/gallium/drivers/llvmpipe/lp_texture.h
> +++ b/src/gallium/drivers/llvmpipe/lp_texture.h
> @@ -75,6 +75,8 @@ struct llvmpipe_resource
>  */
> struct sw_displaytarget *dt;
>  
> +   boolean mapped; /** status of displaytarget, map or unmap */
> +
> /**
>  * Malloc'ed data for regular textures, or a mapping to dt above.
>  */
> 

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

Re: [Mesa-dev] [PATCH] llvmpipe : Fixed an issue where the display target texture was mapped multiple times.

2018-04-19 Thread 정성찬 
Dear Roland Scheidegger

Thank you very much for your time and efforts.

First, I want to talk about the problem that I encountered.
I am currently developing a display server system using the llvmpipe driver
and the kms-dri winsys module. During the compositing process, winsys->
displaytarget_map () will be called continuously for the same resource. So
in the case of kms winsys, mmap () returns MAP_FAILED inside the
kms_sw_displaytarget_map function, and segfault terminates the process.

I also do not think this patch is perfect. As you said, the resource is
still
mapped. But in my opinion, this approach is a good way to solve the
aforementioned critical issues.

What do you think? I look forward to your reply.
Sincerely yours,

Seongchan Jeong.


2018-04-20 11:15 GMT+09:00 Roland Scheidegger :

> Am 19.04.2018 um 08:04 schrieb Seongchan Jeong:
> > The lp_setup_set_fragment_sampler_views function can be called
> > when the texture module is enabled. However, mapping can be
> > performed several times for one display target texture, but
> > unmapping does not proceed. So some logic have been added to
> > unmap the display target texture to prevent additional mappings
> > when the texture is already mapped.
> > ---
> >  src/gallium/drivers/llvmpipe/lp_setup.c   | 9 +
> >  src/gallium/drivers/llvmpipe/lp_texture.h | 2 ++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c
> b/src/gallium/drivers/llvmpipe/lp_setup.c
> > index c157323133..71ceafe2b7 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> > +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> > @@ -907,6 +907,13 @@ lp_setup_set_fragment_sampler_views(struct
> lp_setup_context *setup,
> >   */
> >  struct llvmpipe_screen *screen =
> llvmpipe_screen(res->screen);
> >  struct sw_winsys *winsys = screen->winsys;
> > +
> > +/* unmap the texture which is already mapped */
> > +if(lp_tex->mapped){
> > +winsys->displaytarget_unmap(winsys, lp_tex->dt);
> > +lp_tex->mapped = false;
> > +}
> > +
> >  jit_tex->base = winsys->displaytarget_map(winsys,
> lp_tex->dt,
> >
>  PIPE_TRANSFER_READ);
> >  jit_tex->row_stride[0] = lp_tex->row_stride[0];
> > @@ -917,6 +924,8 @@ lp_setup_set_fragment_sampler_views(struct
> lp_setup_context *setup,
> >  jit_tex->depth = res->depth0;
> >  jit_tex->first_level = jit_tex->last_level = 0;
> >  assert(jit_tex->base);
> > +
> > +lp_tex->mapped = true;
>
> I am not quite convinced this is the right fix.
> Clearly the code right now isn't right, and pretty much relies on the
> winsys->displaytarget_map() being a no-op there just giving the mapping
> without any side effects.
> The problem with this fix is it still would be kept mapped in the end
> after sampling (and, it can and probably will be mapped elsewhere too
> still).
>
> Do you hit any specific bug with the code as-is?
>
> Roland
>
>
> >   }
> >}
> >else {
> > diff --git a/src/gallium/drivers/llvmpipe/lp_texture.h
> b/src/gallium/drivers/llvmpipe/lp_texture.h
> > index 3d315bb9a7..9e39d31eb3 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_texture.h
> > +++ b/src/gallium/drivers/llvmpipe/lp_texture.h
> > @@ -75,6 +75,8 @@ struct llvmpipe_resource
> >  */
> > struct sw_displaytarget *dt;
> >
> > +   boolean mapped; /** status of displaytarget, map or unmap */
> > +
> > /**
> >  * Malloc'ed data for regular textures, or a mapping to dt above.
> >  */
> >
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] llvmpipe : Fixed an issue where the display target texture was mapped multiple times.

2018-04-25 Thread Roland Scheidegger 
Am 20.04.2018 um 08:07 schrieb 정성찬:
> Dear Roland Scheidegger
> 
> Thank you very much for your time and efforts.
> 
> First, I want to talk about the problem that I encountered.
> I am currently developing a display server system using the llvmpipe driver
> and the kms-dri winsys module. During the compositing process, winsys->
> displaytarget_map () will be called continuously for the same resource. So 
> in the case of kms winsys, mmap () returns MAP_FAILED inside the 
> kms_sw_displaytarget_map function, and segfault terminates the process.
Isn't there actually a bug in kms_sw_displaytarget_map() too?
By the looks of it (together with mks_sw_displaytarget_unmap() it tries
to keep a count of mappings and only map when it isn't already mapped
(the unmap will only decrease count if map count is still higher than
1). But the logic can't work since the mapped/rd_mapped fields are never
set.

That said, even if the logic there would work, we'd still have an
inbalance in map count obviously.
I suppose we can't keep it always mapped?
If there's only ever one dt texture could maybe simply always try to
unmap it at the beginning of lp_setup_set_fragment_sampler_views (and I
suppose in theory we should do the same for vertex/geometry sampler view
setup, unless we outright forbid mapping dt textures there).
Then inside the loop, only map it when it's seen for the first time (and
record on the setup context it has been mapped).
(And don't forget to unmap it when the setup context is destroyed too.)

> 
> I also do not think this patch is perfect. As you said, the resource is
> still 
> mapped. But in my opinion, this approach is a good way to solve the 
> aforementioned critical issues.

I think it would be better to fix this for real rather than some half
attempt which still leaves things quite broken.

Roland

> 
> What do you think? I look forward to your reply.
> Sincerely yours,
> 
> Seongchan Jeong.
> 
> 
> 2018-04-20 11:15 GMT+09:00 Roland Scheidegger  >:
> 
> Am 19.04.2018 um 08:04 schrieb Seongchan Jeong:
> > The lp_setup_set_fragment_sampler_views function can be called
> > when the texture module is enabled. However, mapping can be
> > performed several times for one display target texture, but
> > unmapping does not proceed. So some logic have been added to
> > unmap the display target texture to prevent additional mappings
> > when the texture is already mapped.
> > ---
> >  src/gallium/drivers/llvmpipe/lp_setup.c   | 9 +
> >  src/gallium/drivers/llvmpipe/lp_texture.h | 2 ++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c
> b/src/gallium/drivers/llvmpipe/lp_setup.c
> > index c157323133..71ceafe2b7 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> > +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> > @@ -907,6 +907,13 @@ lp_setup_set_fragment_sampler_views(struct
> lp_setup_context *setup,
> >               */
> >              struct llvmpipe_screen *screen =
> llvmpipe_screen(res->screen);
> >              struct sw_winsys *winsys = screen->winsys;
> > +
> > +            /* unmap the texture which is already mapped */
> > +            if(lp_tex->mapped){
> > +                winsys->displaytarget_unmap(winsys, lp_tex->dt);
> > +                lp_tex->mapped = false;
> > +            }
> > +
> >              jit_tex->base = winsys->displaytarget_map(winsys,
> lp_tex->dt,
> >                                                         
>  PIPE_TRANSFER_READ);
> >              jit_tex->row_stride[0] = lp_tex->row_stride[0];
> > @@ -917,6 +924,8 @@ lp_setup_set_fragment_sampler_views(struct
> lp_setup_context *setup,
> >              jit_tex->depth = res->depth0;
> >              jit_tex->first_level = jit_tex->last_level = 0;
> >              assert(jit_tex->base);
> > +
> > +            lp_tex->mapped = true;
> 
> I am not quite convinced this is the right fix.
> Clearly the code right now isn't right, and pretty much relies on the
> winsys->displaytarget_map() being a no-op there just giving the mapping
> without any side effects.
> The problem with this fix is it still would be kept mapped in the end
> after sampling (and, it can and probably will be mapped elsewhere too
> still).
> 
> Do you hit any specific bug with the code as-is?
> 
> Roland
> 
> 
> >           }
> >        }
> >        else {
> > diff --git a/src/gallium/drivers/llvmpipe/lp_texture.h
> b/src/gallium/drivers/llvmpipe/lp_texture.h
> > index 3d315bb9a7..9e39d31eb3 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_texture.h
> > +++ b/src/gallium/drivers/llvmpipe/lp_texture.h
> > @@ -75,6 +75,8 @@ struct llvmpipe_resource
> >      */
> >     struct sw_displaytarget *dt;
> > 
> > +   boolean mappe