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

[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 <sroland at vmware.com
> <mailto:sroland at vmware.com>>:
> 
>     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.
>     >      */
>     >
> 
>