| Message ID | 20151002155641.GA16809@sudip-pc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Oct 2, 2015 at 5:56 PM, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote: > On Thu, Oct 01, 2015 at 07:07:33PM +0200, Patrik Jakobsson wrote: >> On Wed, Sep 30, 2015 at 8:12 AM, Sudip Mukherjee >> <sudipm.mukherjee@gmail.com> wrote: >> > On Tue, Sep 29, 2015 at 03:20:35PM +0200, Patrik Jakobsson wrote: >> >> On Thu, Sep 24, 2015 at 5:57 PM, Sudip Mukherjee >> >> <sudipm.mukherjee@gmail.com> wrote: >> >> > On Wed, Sep 09, 2015 at 06:20:40PM +0530, Sudip Mukherjee wrote: >> >> >> If backing->stolen is true then we were freeing backing by calling >> >> >> psb_gtt_free_range() but we called it again after unlocking the mutex. >> >> >> Lets make it NULL after freeing in psb_gtt_free_range() and check for >> >> >> NULL before calling the function for the second time. >> >> >> >> >> >> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> >> >> >> --- >> >> > Hi Patrik, >> >> > A gentle ping. >> >> > >> >> > regards >> >> > sudip >> >> >> >> Hi, sorry for the late reply. >> >> >> >> Why are we freeing the range twice in the first case? >> > I think, >> > if backing->stolen is true then backing is released using >> > psb_gtt_free_range() but if backing->stolen is false then the gem object >> > is freed but the backing is not yet freed. To free that backing >> > psb_gtt_free_range() has been called second time. My patch tried to fix >> > the possibility of backing->stolen being true and backing being freed 2 >> > times. >> > >> > regards >> > sudip >> >> There are some special handling of the stolen framebuffer that I don't >> remember entirely but the basic concept is that we free the backing >> when we drop the last reference on a gem object. That will trigger a >> psb_gtt_free_range(). So in this case it looks to me that the extra >> free is not needed at all. That's my quick reasoning, feel free to >> prove me wrong :) > > In this case we are allocating backing using psbfb_alloc() and so > backing->stolen is always true. So we can remove the backing->stolen > condition. And if drm_fb_helper_alloc_fbi() fails then we > are jumping to out_err1. So the fitst free will not be needed. Sounds good, could you also rename the labels to what they're doing now. I'm thinking out_release and out_unlock or something you feel is suitable. Thanks Patrik > diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c > index 2eaf1b3..932f07b 100644 > --- a/drivers/gpu/drm/gma500/framebuffer.c > +++ b/drivers/gpu/drm/gma500/framebuffer.c > @@ -466,11 +466,6 @@ static int psbfb_create(struct psb_fbdev *fbdev, > mutex_unlock(&dev->struct_mutex); > return 0; > out_unref: > - if (backing->stolen) > - psb_gtt_free_range(dev, backing); > - else > - drm_gem_object_unreference(&backing->gem); > - > drm_fb_helper_release_fbi(&fbdev->psb_fb_helper); > out_err1: > mutex_unlock(&dev->struct_mutex); > > > If it is ok, I can submit the v2. > > regards > sudip
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 2eaf1b3..932f07b 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -466,11 +466,6 @@ static int psbfb_create(struct psb_fbdev *fbdev, mutex_unlock(&dev->struct_mutex); return 0; out_unref: - if (backing->stolen) - psb_gtt_free_range(dev, backing); - else - drm_gem_object_unreference(&backing->gem); - drm_fb_helper_release_fbi(&fbdev->psb_fb_helper); out_err1: mutex_unlock(&dev->struct_mutex);