| Message ID | 1410807137-8323-1-git-send-email-drake@endlessm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote: > Pageflipping currently causes some inconsistencies that lead to > crashes. Just run an app that causes a CRTC pageflip in a raw X session > and check that it exits cleanly and can be restarted - you'll see > crashes like: > Unable to handle kernel NULL pointer dereference at virtual address 00000334 > PC is at exynos_drm_crtc_plane_commit+0x20/0x40 > LR is at exynos_drm_crtc_plane_commit+0x20/0x40 > [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70) > [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4) > [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8) > [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc) > [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc) > > These crashes happen because drm_plane_force_disable has previously set > plane->crtc to NULL. > > When drm_mode_page_flip_ioctl() is used to flip another framebuffer > onto the primary plane, crtc->primary->fb is correctly updated (this is > a virtual plane created by plane_helper), but plane->fb is not (this > plane is the real one, created by exynos_drm_crtc_create). > > We then come to handle rmfb of the backbuffer, which the "real" primary > plane is incorrectly pointing at. So drm_framebuffer_remove() decides that > the buffer is actually active on a plane and force-disables the plane. > > Ensuring that plane->fb is kept up-to-date solves that issue, but > exposes a reference counting problem. Now we see crashes when rmfb is > called on the front-buffer, because the rmfb code expects to drop 3 > references here, and there are only 2. > > That can be fixed by adopting the reference management found in omapdrm: > Framebuffer references are not taken directly in crtc mode_set context, > but rather in the context of updating the plane, which also covers > flips. Like omapdrm we also unreference the old framebuffer here. > > Signed-off-by: Daniel Drake <drake@endlessm.com> This sounds very much like exynos should switch to universal planes so that the fake primary plane created by the helpers doesn't get in the way. And for chips which already use planes for everything internally this shouldn't be a lot more than a few lines. -Daniel > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 12 ++---------- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 8 ++++++++ > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > index b68e58f..7aa9dee 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, > if (manager->ops->mode_set) > manager->ops->mode_set(manager, &crtc->mode); > > - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h, > - x, y, crtc_w, crtc_h); > - if (ret) > - return ret; > - > - plane->crtc = crtc; > - plane->fb = crtc->primary->fb; > - drm_framebuffer_reference(plane->fb); > - > - return 0; > + return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, > + crtc_w, crtc_h, x, y, crtc_w, crtc_h); > } > > static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y, > diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c > index 8371cbd..df27e35 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c > @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, > overlay->crtc_x, overlay->crtc_y, > overlay->crtc_width, overlay->crtc_height); > > + if (plane->fb) > + drm_framebuffer_unreference(plane->fb); > + > + drm_framebuffer_reference(fb); > + > + plane->fb = fb; > + plane->crtc = crtc; > + > exynos_drm_crtc_plane_mode_set(crtc, overlay); > > return 0; > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 09/16/2014 08:35 AM, Daniel Vetter wrote: > On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote: >> Pageflipping currently causes some inconsistencies that lead to >> crashes. Just run an app that causes a CRTC pageflip in a raw X session >> and check that it exits cleanly and can be restarted - you'll see >> crashes like: >> Unable to handle kernel NULL pointer dereference at virtual address 00000334 >> PC is at exynos_drm_crtc_plane_commit+0x20/0x40 >> LR is at exynos_drm_crtc_plane_commit+0x20/0x40 >> [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70) >> [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4) >> [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8) >> [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc) >> [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc) >> >> These crashes happen because drm_plane_force_disable has previously set >> plane->crtc to NULL. >> >> When drm_mode_page_flip_ioctl() is used to flip another framebuffer >> onto the primary plane, crtc->primary->fb is correctly updated (this is >> a virtual plane created by plane_helper), but plane->fb is not (this >> plane is the real one, created by exynos_drm_crtc_create). >> >> We then come to handle rmfb of the backbuffer, which the "real" primary >> plane is incorrectly pointing at. So drm_framebuffer_remove() decides that >> the buffer is actually active on a plane and force-disables the plane. >> >> Ensuring that plane->fb is kept up-to-date solves that issue, but >> exposes a reference counting problem. Now we see crashes when rmfb is >> called on the front-buffer, because the rmfb code expects to drop 3 >> references here, and there are only 2. >> >> That can be fixed by adopting the reference management found in omapdrm: >> Framebuffer references are not taken directly in crtc mode_set context, >> but rather in the context of updating the plane, which also covers >> flips. Like omapdrm we also unreference the old framebuffer here. >> >> Signed-off-by: Daniel Drake <drake@endlessm.com> > This sounds very much like exynos should switch to universal planes so > that the fake primary plane created by the helpers doesn't get in the way. > And for chips which already use planes for everything internally this > shouldn't be a lot more than a few lines. > -Daniel The patch proposed here of course supersedes my patch fixing fb refcounting. But the best solution is to get rid of virtual plane as Daniel Vetter stated. Daniel (Drake of course :) ) do you want to prepare patch switching to universal planes? Maybe other volunteers? If not I can try to do it, as it seems quite straightforward. Regards Andrzej >> --- >> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 12 ++---------- >> drivers/gpu/drm/exynos/exynos_drm_plane.c | 8 ++++++++ >> 2 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> index b68e58f..7aa9dee 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, >> if (manager->ops->mode_set) >> manager->ops->mode_set(manager, &crtc->mode); >> >> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h, >> - x, y, crtc_w, crtc_h); >> - if (ret) >> - return ret; >> - >> - plane->crtc = crtc; >> - plane->fb = crtc->primary->fb; >> - drm_framebuffer_reference(plane->fb); >> - >> - return 0; >> + return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, >> + crtc_w, crtc_h, x, y, crtc_w, crtc_h); >> } >> >> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y, >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c >> index 8371cbd..df27e35 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c >> @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, >> overlay->crtc_x, overlay->crtc_y, >> overlay->crtc_width, overlay->crtc_height); >> >> + if (plane->fb) >> + drm_framebuffer_unreference(plane->fb); >> + >> + drm_framebuffer_reference(fb); >> + >> + plane->fb = fb; >> + plane->crtc = crtc; >> + >> exynos_drm_crtc_plane_mode_set(crtc, overlay); >> >> return 0; >> -- >> 1.9.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2014? 09? 17? 15:35, Andrzej Hajda wrote: > Hi, > > On 09/16/2014 08:35 AM, Daniel Vetter wrote: >> On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote: >>> Pageflipping currently causes some inconsistencies that lead to >>> crashes. Just run an app that causes a CRTC pageflip in a raw X session >>> and check that it exits cleanly and can be restarted - you'll see >>> crashes like: >>> Unable to handle kernel NULL pointer dereference at virtual address 00000334 >>> PC is at exynos_drm_crtc_plane_commit+0x20/0x40 >>> LR is at exynos_drm_crtc_plane_commit+0x20/0x40 >>> [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70) >>> [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4) >>> [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8) >>> [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc) >>> [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc) >>> >>> These crashes happen because drm_plane_force_disable has previously set >>> plane->crtc to NULL. >>> >>> When drm_mode_page_flip_ioctl() is used to flip another framebuffer >>> onto the primary plane, crtc->primary->fb is correctly updated (this is >>> a virtual plane created by plane_helper), but plane->fb is not (this >>> plane is the real one, created by exynos_drm_crtc_create). >>> >>> We then come to handle rmfb of the backbuffer, which the "real" primary >>> plane is incorrectly pointing at. So drm_framebuffer_remove() decides that >>> the buffer is actually active on a plane and force-disables the plane. >>> >>> Ensuring that plane->fb is kept up-to-date solves that issue, but >>> exposes a reference counting problem. Now we see crashes when rmfb is >>> called on the front-buffer, because the rmfb code expects to drop 3 >>> references here, and there are only 2. >>> >>> That can be fixed by adopting the reference management found in omapdrm: >>> Framebuffer references are not taken directly in crtc mode_set context, >>> but rather in the context of updating the plane, which also covers >>> flips. Like omapdrm we also unreference the old framebuffer here. >>> >>> Signed-off-by: Daniel Drake <drake@endlessm.com> >> This sounds very much like exynos should switch to universal planes so >> that the fake primary plane created by the helpers doesn't get in the way. >> And for chips which already use planes for everything internally this >> shouldn't be a lot more than a few lines. >> -Daniel > > The patch proposed here of course supersedes my patch fixing fb refcounting. > But the best solution is to get rid of virtual plane as Daniel Vetter > stated. > Daniel (Drake of course :) ) do you want to prepare patch switching to > universal planes? > Maybe other volunteers? If not I can try to do it, as it seems quite > straightforward. I think you can do it and you would be a right person to do it. Thanks, Inki Dae > > Regards > Andrzej > >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 12 ++---------- >>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 8 ++++++++ >>> 2 files changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>> index b68e58f..7aa9dee 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>> @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, >>> if (manager->ops->mode_set) >>> manager->ops->mode_set(manager, &crtc->mode); >>> >>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h, >>> - x, y, crtc_w, crtc_h); >>> - if (ret) >>> - return ret; >>> - >>> - plane->crtc = crtc; >>> - plane->fb = crtc->primary->fb; >>> - drm_framebuffer_reference(plane->fb); >>> - >>> - return 0; >>> + return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, >>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h); >>> } >>> >>> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y, >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c >>> index 8371cbd..df27e35 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c >>> @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, >>> overlay->crtc_x, overlay->crtc_y, >>> overlay->crtc_width, overlay->crtc_height); >>> >>> + if (plane->fb) >>> + drm_framebuffer_unreference(plane->fb); >>> + >>> + drm_framebuffer_reference(fb); >>> + >>> + plane->fb = fb; >>> + plane->crtc = crtc; >>> + >>> exynos_drm_crtc_plane_mode_set(crtc, overlay); >>> >>> return 0; >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi, On 09/17/2014 03:49 PM, Inki Dae wrote: > On 2014? 09? 17? 15:35, Andrzej Hajda wrote: >> Hi, >> >> On 09/16/2014 08:35 AM, Daniel Vetter wrote: >>> On Mon, Sep 15, 2014 at 12:52:17PM -0600, Daniel Drake wrote: >>>> Pageflipping currently causes some inconsistencies that lead to >>>> crashes. Just run an app that causes a CRTC pageflip in a raw X session >>>> and check that it exits cleanly and can be restarted - you'll see >>>> crashes like: >>>> Unable to handle kernel NULL pointer dereference at virtual address 00000334 >>>> PC is at exynos_drm_crtc_plane_commit+0x20/0x40 >>>> LR is at exynos_drm_crtc_plane_commit+0x20/0x40 >>>> [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70) >>>> [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4) >>>> [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8) >>>> [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc) >>>> [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc) >>>> >>>> These crashes happen because drm_plane_force_disable has previously set >>>> plane->crtc to NULL. >>>> >>>> When drm_mode_page_flip_ioctl() is used to flip another framebuffer >>>> onto the primary plane, crtc->primary->fb is correctly updated (this is >>>> a virtual plane created by plane_helper), but plane->fb is not (this >>>> plane is the real one, created by exynos_drm_crtc_create). >>>> >>>> We then come to handle rmfb of the backbuffer, which the "real" primary >>>> plane is incorrectly pointing at. So drm_framebuffer_remove() decides that >>>> the buffer is actually active on a plane and force-disables the plane. >>>> >>>> Ensuring that plane->fb is kept up-to-date solves that issue, but >>>> exposes a reference counting problem. Now we see crashes when rmfb is >>>> called on the front-buffer, because the rmfb code expects to drop 3 >>>> references here, and there are only 2. >>>> >>>> That can be fixed by adopting the reference management found in omapdrm: >>>> Framebuffer references are not taken directly in crtc mode_set context, >>>> but rather in the context of updating the plane, which also covers >>>> flips. Like omapdrm we also unreference the old framebuffer here. >>>> >>>> Signed-off-by: Daniel Drake <drake@endlessm.com> >>> This sounds very much like exynos should switch to universal planes so >>> that the fake primary plane created by the helpers doesn't get in the way. >>> And for chips which already use planes for everything internally this >>> shouldn't be a lot more than a few lines. >>> -Daniel >> >> The patch proposed here of course supersedes my patch fixing fb refcounting. >> But the best solution is to get rid of virtual plane as Daniel Vetter >> stated. >> Daniel (Drake of course :) ) do you want to prepare patch switching to >> universal planes? >> Maybe other volunteers? If not I can try to do it, as it seems quite >> straightforward. > > I think you can do it and you would be a right person to do it. > > Thanks, > Inki Dae > >> >> Regards >> Andrzej >> >>>> --- >>>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 12 ++---------- >>>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 8 ++++++++ >>>> 2 files changed, 10 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>>> index b68e58f..7aa9dee 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>>> @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, >>>> if (manager->ops->mode_set) >>>> manager->ops->mode_set(manager, &crtc->mode); >>>> >>>> - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h, >>>> - x, y, crtc_w, crtc_h); >>>> - if (ret) >>>> - return ret; >>>> - >>>> - plane->crtc = crtc; >>>> - plane->fb = crtc->primary->fb; >>>> - drm_framebuffer_reference(plane->fb); It's problem to add this from commit 25c8b5c3048cb6c98d402ca8d4735ccf910f727c. Chip specific drm driver internally doesn't have to care fb reference count if there is no special case. We should have switched to universal plane at that time. Thanks. >>>> - >>>> - return 0; >>>> + return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, >>>> + crtc_w, crtc_h, x, y, crtc_w, crtc_h); >>>> } >>>> >>>> static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y, >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c >>>> index 8371cbd..df27e35 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c >>>> @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, >>>> overlay->crtc_x, overlay->crtc_y, >>>> overlay->crtc_width, overlay->crtc_height); >>>> >>>> + if (plane->fb) >>>> + drm_framebuffer_unreference(plane->fb); >>>> + >>>> + drm_framebuffer_reference(fb); >>>> + >>>> + plane->fb = fb; >>>> + plane->crtc = crtc; >>>> + >>>> exynos_drm_crtc_plane_mode_set(crtc, overlay); >>>> >>>> return 0; >>>> -- >>>> 1.9.1 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > >
On Wed, Sep 17, 2014 at 1:44 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote: > It's problem to add this from commit 25c8b5c3048cb6c98d402ca8d4735ccf910f727c. My patch moves that drm_framebuffer_reference() call to the plane function which is called from crtc_mode_set context (and also called in crtc pageflip path), so there should be no problem here. > Chip specific drm driver internally doesn't have to care fb reference count if > there is no special case. We should have switched to universal plane at that > time. To me it seems like the chip-specific DRM drivers do need to add a reference in the crtc_mode_set and crtc page flip paths otherwise framebuffer removal crashes (expecting to remove 3 references), as noted by my testing and also in commit 25c8b5c304. However, I'll be happy if universal planes means the driver does not have to care about this any more. Andrej, please go ahead if you are interested, I'll be happy to test your results. Thanks Daniel
On Wed, Sep 17, 2014 at 2:19 PM, Daniel Drake <drake@endlessm.com> wrote: >> Chip specific drm driver internally doesn't have to care fb reference count if >> there is no special case. We should have switched to universal plane at that >> time. > > To me it seems like the chip-specific DRM drivers do need to add a > reference in the crtc_mode_set and crtc page flip paths otherwise > framebuffer removal crashes (expecting to remove 3 references), as > noted by my testing and also in commit 25c8b5c304. I think fb refcounting in exynos is just plain busted. If you look at other drivers the only place the refcount framebuffers or backing storage objects is for pageflips to make sure the memory doesn't go away while the hw is still scanning out the old framebuffer. If you refcount anywhere else you either do something really crazy or your driver is broken. > However, I'll be happy if universal planes means the driver does not > have to care about this any more. Andrej, please go ahead if you are > interested, I'll be happy to test your results. universal planes will fix up the mess with 2 drm plane objects (primary plane + exonys internal primary). So should help to untangle this not, but it will not magically fix the refcounting bugs itself. -Daniel
On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > I think fb refcounting in exynos is just plain busted. If you look at > other drivers the only place the refcount framebuffers or backing > storage objects is for pageflips to make sure the memory doesn't go > away while the hw is still scanning out the old framebuffer. If you > refcount anywhere else you either do something really crazy or your > driver is broken. With my patch actually the behaviour is much more similar to omapdrm, which also doesn't quite match your description of "other drivers". See omap_plane.c. There is a fb reference taken for "pinning" in update_pin() which presumably is what you describe - avoid destroying the fb while it is being scanned out. (Maybe exynos should have something equivalent too, but thats a separate issue) However there is *another* fb reference taken in omap_plane_mode_set(). And my patch is modelled to do the same in exynos-drm. I believe this is necessary under the current model. At least, when drm_mode_rmfb() is running for the last user of the active framebuffer, it expects to drop 3 references from the framebuffer before dropping the 4th causes the object to be destroyed, as follows: 1. drm_mode_rmfb explicitly drops a reference - it calls __drm_framebuffer_unregister which then calls __drm_framebuffer_unreference /* Mark fb as reaped, we still have a ref from fpriv->fbs. */ __drm_framebuffer_unregister(dev, fb); 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls drm_mode_set_config_internal() in order to turn off the CRTC, dropping another reference in the process. if (tmp->old_fb) drm_framebuffer_unreference(tmp->old_fb); 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops another reference: /* disconnect the plane from the fb and crtc: */ __drm_framebuffer_unreference(old_fb); 4. drm_framebuffer drops the final reference itself, to cause freeing of the object: drm_framebuffer_unreference(fb); So ordinarily, after a fb is created by drm core (with refcnt at 1), there would have to be 3 references added to it by the time it is the primary fb so that when we do rmfb, it has a refcnt of 4, and gets freed correctly. (The second bug I was seeing with pageflips was that refcnt was 3, which means that the final reference was dropped in (3) above, but __drm_framebuffer_unreference doesn't like that at all - it calls drm_framebuffer_free_bug) Not being overly familiar with DRM internals I tried to go backwards to find out where these 3 references would be created during normal operation. 2 are clear: 1. drm_framebuffer_init() explicitly grabs one: /* Grab the idr reference. */ drm_framebuffer_reference(fb) 2. drm_mode_set_config_internal() takes one: if (tmp->primary->fb) drm_framebuffer_reference(tmp->primary->fb); Where should the 3rd one be created? I don't know, but looking at previous exynos commit 25c8b5c304 and omapdrm, I assumed that the drm driver should take one, both on crtc mode set and crtc page flip. >> However, I'll be happy if universal planes means the driver does not >> have to care about this any more. Andrej, please go ahead if you are >> interested, I'll be happy to test your results. > > universal planes will fix up the mess with 2 drm plane objects > (primary plane + exonys internal primary). So should help to untangle > this not, but it will not magically fix the refcounting bugs itself. So even when we move to universal planes (fixing 1 of the issues), its good that we're having this refcount discussion (which we need to understand to confidently solve the 2nd issue). Thanks for your input! Daniel
Hi, On 09/18/2014 01:41 AM, Daniel Drake wrote: > On Wed, Sep 17, 2014 at 7:45 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> I think fb refcounting in exynos is just plain busted. If you look at >> other drivers the only place the refcount framebuffers or backing >> storage objects is for pageflips to make sure the memory doesn't go >> away while the hw is still scanning out the old framebuffer. If you >> refcount anywhere else you either do something really crazy or your >> driver is broken. > > With my patch actually the behaviour is much more similar to omapdrm, Your patch will occur fb reference count problem when setplane. > which also doesn't quite match your description of "other drivers". > See omap_plane.c. > > There is a fb reference taken for "pinning" in update_pin() which > presumably is what you describe - avoid destroying the fb while it is > being scanned out. (Maybe exynos should have something equivalent too, > but thats a separate issue) > > However there is *another* fb reference taken in > omap_plane_mode_set(). And my patch is modelled to do the same in > exynos-drm. > > I believe this is necessary under the current model. At least, when > drm_mode_rmfb() is running for the last user of the active > framebuffer, it expects to drop 3 references from the framebuffer > before dropping the 4th causes the object to be destroyed, as follows: > > 1. drm_mode_rmfb explicitly drops a reference - it calls > __drm_framebuffer_unregister which then calls > __drm_framebuffer_unreference > /* Mark fb as reaped, we still have a ref from fpriv->fbs. */ > __drm_framebuffer_unregister(dev, fb); > > 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls > drm_mode_set_config_internal() in order to turn off the CRTC, dropping > another reference in the process. > if (tmp->old_fb) > drm_framebuffer_unreference(tmp->old_fb); > > 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops > another reference: > /* disconnect the plane from the fb and crtc: */ > __drm_framebuffer_unreference(old_fb); This call is new path, before universal planes merged, private plane of exynos crtc wasn't included in dev->mode_config.plane_list because private plane wasn't exposed to userspace so this path wasn't called. > > 4. drm_framebuffer drops the final reference itself, to cause freeing > of the object: > drm_framebuffer_unreference(fb); > > > So ordinarily, after a fb is created by drm core (with refcnt at 1), > there would have to be 3 references added to it by the time it is the > primary fb so that when we do rmfb, it has a refcnt of 4, and gets > freed correctly. > (The second bug I was seeing with pageflips was that refcnt was 3, > which means that the final reference was dropped in (3) above, but > __drm_framebuffer_unreference doesn't like that at all - it calls > drm_framebuffer_free_bug) > > Not being overly familiar with DRM internals I tried to go backwards > to find out where these 3 references would be created during normal > operation. 2 are clear: > > 1. drm_framebuffer_init() explicitly grabs one: > /* Grab the idr reference. */ > drm_framebuffer_reference(fb) > > 2. drm_mode_set_config_internal() takes one: > if (tmp->primary->fb) > drm_framebuffer_reference(tmp->primary->fb); > > Where should the 3rd one be created? I don't know, but looking at > previous exynos commit 25c8b5c304 and omapdrm, I assumed that the drm > driver should take one, both on crtc mode set and crtc page flip. So Andrzej added fb reference count increasing in crtc modeset path, but i think we can take away this workaround if remove private plane for exynos crtc. Thanks. > >>> However, I'll be happy if universal planes means the driver does not >>> have to care about this any more. Andrej, please go ahead if you are >>> interested, I'll be happy to test your results. >> >> universal planes will fix up the mess with 2 drm plane objects >> (primary plane + exonys internal primary). So should help to untangle >> this not, but it will not magically fix the refcounting bugs itself. > > So even when we move to universal planes (fixing 1 of the issues), its > good that we're having this refcount discussion (which we need to > understand to confidently solve the 2nd issue). Thanks for your input! > > Daniel >
On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake <drake@endlessm.com> wrote: > However there is *another* fb reference taken in > omap_plane_mode_set(). And my patch is modelled to do the same in > exynos-drm. This is because omapdrm does _everything_ asynchrously, even plain modesets. Unfortunately that async modeset support is broken, so the latest omapdrm patches insert a synchronization point. So picking omap's mode_set logic as a reference because it also does fb refcounting is not a good idea - that code does something crazy and gets it wrong. And really, if you do modeset synchronously the drm core will take care of your refcounting needs. -Daniel
On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake <drake@endlessm.com> wrote: > 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls > drm_mode_set_config_internal() in order to turn off the CRTC, dropping > another reference in the process. > if (tmp->old_fb) > drm_framebuffer_unreference(tmp->old_fb); > > 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops > another reference: > /* disconnect the plane from the fb and crtc: */ > __drm_framebuffer_unreference(old_fb); If 3. here is about the primary plane then this won't happen, since the primary plane pointer&reference has already been cleared in step 2. And even if their would be a bug in here, you _certainly_ should not try to paper over this in your driver, but instead fix up the refcounting done in the drm core. -Daniel
On Thu, Sep 18, 2014 at 12:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Sep 17, 2014 at 6:41 PM, Daniel Drake <drake@endlessm.com> wrote: >> 2. drm_mode_rmfb then calls drm_framebuffer_remove, which calls >> drm_mode_set_config_internal() in order to turn off the CRTC, dropping >> another reference in the process. >> if (tmp->old_fb) >> drm_framebuffer_unreference(tmp->old_fb); >> >> 3. drm_framebuffer_remove calls drm_plane_force_disable() which drops >> another reference: >> /* disconnect the plane from the fb and crtc: */ >> __drm_framebuffer_unreference(old_fb); > > If 3. here is about the primary plane then this won't happen, since > the primary plane pointer&reference has already been cleared in step > 2. I just checked - as Joonyoung suspects, the plane being force disabled in step 3 is the private exynos-drm plane. So thats an issue - but at least now I have a complete understanding of the problem. Sounds like that will also be fixed by moving to universal planes. I'll wait for Andrzej's patch. Thanks! Daniel
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index b68e58f..7aa9dee 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -140,16 +140,8 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, if (manager->ops->mode_set) manager->ops->mode_set(manager, &crtc->mode); - ret = exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, crtc_w, crtc_h, - x, y, crtc_w, crtc_h); - if (ret) - return ret; - - plane->crtc = crtc; - plane->fb = crtc->primary->fb; - drm_framebuffer_reference(plane->fb); - - return 0; + return exynos_plane_mode_set(plane, crtc, crtc->primary->fb, 0, 0, + crtc_w, crtc_h, x, y, crtc_w, crtc_h); } static int exynos_drm_crtc_mode_set_commit(struct drm_crtc *crtc, int x, int y, diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index 8371cbd..df27e35 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -139,6 +139,14 @@ int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, overlay->crtc_x, overlay->crtc_y, overlay->crtc_width, overlay->crtc_height); + if (plane->fb) + drm_framebuffer_unreference(plane->fb); + + drm_framebuffer_reference(fb); + + plane->fb = fb; + plane->crtc = crtc; + exynos_drm_crtc_plane_mode_set(crtc, overlay); return 0;
Pageflipping currently causes some inconsistencies that lead to crashes. Just run an app that causes a CRTC pageflip in a raw X session and check that it exits cleanly and can be restarted - you'll see crashes like: Unable to handle kernel NULL pointer dereference at virtual address 00000334 PC is at exynos_drm_crtc_plane_commit+0x20/0x40 LR is at exynos_drm_crtc_plane_commit+0x20/0x40 [<c03749b4>] (exynos_drm_crtc_plane_commit) from [<c03741bc>] (exynos_drm_crtc_commit+0x44/0x70) [<c03741bc>] (exynos_drm_crtc_commit) from [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2+0xb4/0xc4) [<c03743a0>] (exynos_drm_crtc_mode_set_commit.isra.2) from [<c03744f4>] (exynos_drm_crtc_page_flip+0x140/0x1a8) [<c03744f4>] (exynos_drm_crtc_page_flip) from [<c036b20c>] (drm_mode_page_flip_ioctl+0x224/0x2dc) [<c036b20c>] (drm_mode_page_flip_ioctl) from [<c035c324>] (drm_ioctl+0x338/0x4fc) These crashes happen because drm_plane_force_disable has previously set plane->crtc to NULL. When drm_mode_page_flip_ioctl() is used to flip another framebuffer onto the primary plane, crtc->primary->fb is correctly updated (this is a virtual plane created by plane_helper), but plane->fb is not (this plane is the real one, created by exynos_drm_crtc_create). We then come to handle rmfb of the backbuffer, which the "real" primary plane is incorrectly pointing at. So drm_framebuffer_remove() decides that the buffer is actually active on a plane and force-disables the plane. Ensuring that plane->fb is kept up-to-date solves that issue, but exposes a reference counting problem. Now we see crashes when rmfb is called on the front-buffer, because the rmfb code expects to drop 3 references here, and there are only 2. That can be fixed by adopting the reference management found in omapdrm: Framebuffer references are not taken directly in crtc mode_set context, but rather in the context of updating the plane, which also covers flips. Like omapdrm we also unreference the old framebuffer here. Signed-off-by: Daniel Drake <drake@endlessm.com> --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 12 ++---------- drivers/gpu/drm/exynos/exynos_drm_plane.c | 8 ++++++++ 2 files changed, 10 insertions(+), 10 deletions(-)