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 <div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 7, 2018 at 7:14 AM, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.veli...@gmail.com"; target="_blank">emil.l.veli...@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 6 March 2018 at 22:43, Lepton Wu <<a href="mailto:lep...@chromium.org";>lep...@chromium.org</a>> wrote:<br> > If user calls map twice for kms_sw_displaytarget, the first mapped<br> > buffer could get leaked. Instead of calling mmap every time, just<br> > reuse previous mapping. Since user could map same displaytarget with<br> > different flags, we have to keep two different pointers, one for rw<br> > mapping and one for ro mapping.<br> ><br> > Change-Id: I65308f0ff2640bd57b2577c6a3469<wbr>540c9722859<br> > Signed-off-by: Lepton Wu <<a href="mailto:lep...@chromium.org";>lep...@chromium.org</a>><br> > ---<br> > .../winsys/sw/kms-dri/kms_dri_<wbr>sw_winsys.c | 26 ++++++++++++++-----<br> > 1 file changed, 19 insertions(+), 7 deletions(-)<br> ><br> > diff --git a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br> > index 22e1c936ac5..30343222470 100644<br> > --- a/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br> > +++ b/src/gallium/winsys/sw/kms-<wbr>dri/kms_dri_sw_winsys.c<br> > @@ -70,6 +70,7 @@ struct kms_sw_displaytarget<br> ><br> > uint32_t handle;<br> > void *mapped;<br> > + void *ro_mapped;<br> ><br> > int ref_count;<br> > struct list_head link;<br> > @@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(<wbr>struct sw_winsys *ws,<br> > if (kms_sw_dt->ref_count > 0)<br> > return;<br> ><br> > + if (kms_sw_dt->ro_mapped)<br> > + munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);<br> > + if (kms_sw_dt->mapped)<br> > + munmap(kms_sw_dt->mapped, kms_sw_dt->size);<br> > +<br> </div></div>I'm not 100% sure about this. There's a reason why dt_unmap exists.<br> <br> Skimming through the existing code [1] - there's a handful of cases<br> that indicate the leaks you're hitting.<br> I'd look into addressing those properly because as-is this looks alike<br> a duck-taping the problem.<br> <br> With the hunk gone the patch is<br> Reviewed-by: Emil Velikov <<a href="mailto:emil.veli...@collabora.com";>emil.veli...@collabora.com</a>><br> <br> -Emil<br> <br> [1] $ git grep -20 -w displaytarget_[a-z]*map<br> </blockquote></div><br></div> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
