Re: [Intel-gfx] [PATCH 4/8] drm: Move ->old_fb from crtc to plane
On Tue, Jul 29, 2014 at 04:46:11PM -0700, Matt Roper wrote:
> On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:
> > Atomic implemenations for legacy ioctls must be able to drop locks.
> > Which doesn't cause havoc since we only do that while constructing
> > the new state, so no driver or hardware state change has happened.
> >
> > The only troubling bit is the fb refcounting the core does - if
> > someone else has snuck in then it might potentially unref an
> > outdated framebuffer. To fix that move the old_fb temporary storage
> > into struct drm_plane for all ioctls, so that the atomic helpers can
> > update it.
> >
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/drm_crtc.c | 40
> > include/drm/drm_crtc.h | 8
> > 2 files changed, 28 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index c09374038f9a..bacf565449d5 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
> > */
> > void drm_plane_force_disable(struct drm_plane *plane)
> > {
> > - struct drm_framebuffer *old_fb = plane->fb;
> > int ret;
> >
> > - if (!old_fb)
> > + if (!plane->fb)
> > return;
> >
> > + plane->old_fb = plane->fb;
> > ret = plane->funcs->disable_plane(plane);
> > if (ret) {
> > DRM_ERROR("failed to disable plane with busy fb\n");
> > + plane->old_fb = NULL;
> > return;
> > }
> > /* disconnect the plane from the fb and crtc: */
> > - __drm_framebuffer_unreference(old_fb);
> > + __drm_framebuffer_unreference(plane->old_fb);
> > + plane->old_fb = NULL;
> > plane->fb = NULL;
> > plane->crtc = NULL;
> > }
> > @@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
> > uint32_t src_w, uint32_t src_h)
> > {
> > struct drm_device *dev = plane->dev;
> > - struct drm_framebuffer *old_fb = NULL;
> > + struct drm_framebuffer *old_fb;
>
> I think there may be cases where old_fb gets unref'd without ever being
> set if we drop the NULL assignment. E.g., if the possible_crtcs test or
> the format test fail, we jump down to out and then test the value +
> unref which could be garbage.
Oops, totally missed that. And somehow also missed the gcc warning about
unitialized usage of old_fb - that one was the reason why I've dropped the
initializer. Looks like I've failed.
> Would it be simpler to just drm_modeset_lock_all() unconditionally at
> the start of the function and then just unlock after the unrefs at the
> end of the function so that we don't need a local old_fb?
Yeah considered that and since you're suggesting this too I'll do it.
Trying hard to not grab locks for the error case is fairly pointless
optimization.
>
> > int ret = 0;
> > unsigned int fb_width, fb_height;
> > int i;
> > @@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane
> > *plane,
> > /* No fb means shut it down */
> > if (!fb) {
> > drm_modeset_lock_all(dev);
> > - old_fb = plane->fb;
> > + plane->old_fb = plane->fb;
> > ret = plane->funcs->disable_plane(plane);
> > if (!ret) {
> > plane->crtc = NULL;
> > plane->fb = NULL;
> > } else {
> > - old_fb = NULL;
> > + plane->old_fb = NULL;
> > }
> > + old_fb = plane->old_fb;
> > + plane->old_fb = NULL;
> > drm_modeset_unlock_all(dev);
> > goto out;
> > }
> > @@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
> > }
> >
> > drm_modeset_lock_all(dev);
> > - old_fb = plane->fb;
> > + plane->old_fb = plane->fb;
> > ret = plane->funcs->update_plane(plane, crtc, fb,
> > crtc_x, crtc_y, crtc_w, crtc_h,
> > src_x, src_y, src_w, src_h);
> > @@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
> > plane->fb = fb;
> > fb = NULL;
> > } else {
> > - old_fb = NULL;
> > + plane->old_fb = NULL;
> > }
> > + old_fb = plane->old_fb;
> > + plane->old_fb = NULL;
> > drm_modeset_unlock_all(dev);
> >
> > out:
> > @@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set
> > *set)
> > * crtcs. Atomic modeset will have saner semantics ...
> > */
> > list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
> > - tmp->old_fb = tmp->primary->fb;
> > + tmp->primary->old_fb = tmp->primary->fb;
> >
> > fb = set->fb;
> >
> > @@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set
> > *set)
> > list_for_each_entry(tmp, &crtc->dev->mode_config.c
Re: [Intel-gfx] [PATCH 4/8] drm: Move ->old_fb from crtc to plane
On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:
> Atomic implemenations for legacy ioctls must be able to drop locks.
> Which doesn't cause havoc since we only do that while constructing
> the new state, so no driver or hardware state change has happened.
>
> The only troubling bit is the fb refcounting the core does - if
> someone else has snuck in then it might potentially unref an
> outdated framebuffer. To fix that move the old_fb temporary storage
> into struct drm_plane for all ioctls, so that the atomic helpers can
> update it.
>
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_crtc.c | 40
> include/drm/drm_crtc.h | 8
> 2 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index c09374038f9a..bacf565449d5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
> */
> void drm_plane_force_disable(struct drm_plane *plane)
> {
> - struct drm_framebuffer *old_fb = plane->fb;
> int ret;
>
> - if (!old_fb)
> + if (!plane->fb)
> return;
>
> + plane->old_fb = plane->fb;
> ret = plane->funcs->disable_plane(plane);
> if (ret) {
> DRM_ERROR("failed to disable plane with busy fb\n");
> + plane->old_fb = NULL;
> return;
> }
> /* disconnect the plane from the fb and crtc: */
> - __drm_framebuffer_unreference(old_fb);
> + __drm_framebuffer_unreference(plane->old_fb);
> + plane->old_fb = NULL;
> plane->fb = NULL;
> plane->crtc = NULL;
> }
> @@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
>uint32_t src_w, uint32_t src_h)
> {
> struct drm_device *dev = plane->dev;
> - struct drm_framebuffer *old_fb = NULL;
> + struct drm_framebuffer *old_fb;
I think there may be cases where old_fb gets unref'd without ever being
set if we drop the NULL assignment. E.g., if the possible_crtcs test or
the format test fail, we jump down to out and then test the value +
unref which could be garbage.
Would it be simpler to just drm_modeset_lock_all() unconditionally at
the start of the function and then just unlock after the unrefs at the
end of the function so that we don't need a local old_fb?
> int ret = 0;
> unsigned int fb_width, fb_height;
> int i;
> @@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
> /* No fb means shut it down */
> if (!fb) {
> drm_modeset_lock_all(dev);
> - old_fb = plane->fb;
> + plane->old_fb = plane->fb;
> ret = plane->funcs->disable_plane(plane);
> if (!ret) {
> plane->crtc = NULL;
> plane->fb = NULL;
> } else {
> - old_fb = NULL;
> + plane->old_fb = NULL;
> }
> + old_fb = plane->old_fb;
> + plane->old_fb = NULL;
> drm_modeset_unlock_all(dev);
> goto out;
> }
> @@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
> }
>
> drm_modeset_lock_all(dev);
> - old_fb = plane->fb;
> + plane->old_fb = plane->fb;
> ret = plane->funcs->update_plane(plane, crtc, fb,
>crtc_x, crtc_y, crtc_w, crtc_h,
>src_x, src_y, src_w, src_h);
> @@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
> plane->fb = fb;
> fb = NULL;
> } else {
> - old_fb = NULL;
> + plane->old_fb = NULL;
> }
> + old_fb = plane->old_fb;
> + plane->old_fb = NULL;
> drm_modeset_unlock_all(dev);
>
> out:
> @@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set
> *set)
>* crtcs. Atomic modeset will have saner semantics ...
>*/
> list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
> - tmp->old_fb = tmp->primary->fb;
> + tmp->primary->old_fb = tmp->primary->fb;
>
> fb = set->fb;
>
> @@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set
> *set)
> list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
> if (tmp->primary->fb)
> drm_framebuffer_reference(tmp->primary->fb);
> - if (tmp->old_fb)
> - drm_framebuffer_unreference(tmp->old_fb);
> + if (tmp->primary->old_fb)
> + drm_framebuffer_unreference(tmp->primary->old_fb);
> + tmp->primary->old_fb = NULL;
> }
>
> return ret;
> @@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> {
>
Re: [Intel-gfx] [PATCH 4/8] drm: Move ->old_fb from crtc to plane
On 30 July 2014 07:32, Daniel Vetter wrote: > Atomic implemenations for legacy ioctls must be able to drop locks. > Which doesn't cause havoc since we only do that while constructing > the new state, so no driver or hardware state change has happened. > > The only troubling bit is the fb refcounting the core does - if > someone else has snuck in then it might potentially unref an > outdated framebuffer. To fix that move the old_fb temporary storage > into struct drm_plane for all ioctls, so that the atomic helpers can > update it. > > Signed-off-by: Daniel Vetter Seems to make sense to me. Reviewed-by: Dave Airlie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/8] drm: Move ->old_fb from crtc to plane
Atomic implemenations for legacy ioctls must be able to drop locks.
Which doesn't cause havoc since we only do that while constructing
the new state, so no driver or hardware state change has happened.
The only troubling bit is the fb refcounting the core does - if
someone else has snuck in then it might potentially unref an
outdated framebuffer. To fix that move the old_fb temporary storage
into struct drm_plane for all ioctls, so that the atomic helpers can
update it.
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_crtc.c | 40
include/drm/drm_crtc.h | 8
2 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c09374038f9a..bacf565449d5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
*/
void drm_plane_force_disable(struct drm_plane *plane)
{
- struct drm_framebuffer *old_fb = plane->fb;
int ret;
- if (!old_fb)
+ if (!plane->fb)
return;
+ plane->old_fb = plane->fb;
ret = plane->funcs->disable_plane(plane);
if (ret) {
DRM_ERROR("failed to disable plane with busy fb\n");
+ plane->old_fb = NULL;
return;
}
/* disconnect the plane from the fb and crtc: */
- __drm_framebuffer_unreference(old_fb);
+ __drm_framebuffer_unreference(plane->old_fb);
+ plane->old_fb = NULL;
plane->fb = NULL;
plane->crtc = NULL;
}
@@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
uint32_t src_w, uint32_t src_h)
{
struct drm_device *dev = plane->dev;
- struct drm_framebuffer *old_fb = NULL;
+ struct drm_framebuffer *old_fb;
int ret = 0;
unsigned int fb_width, fb_height;
int i;
@@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
/* No fb means shut it down */
if (!fb) {
drm_modeset_lock_all(dev);
- old_fb = plane->fb;
+ plane->old_fb = plane->fb;
ret = plane->funcs->disable_plane(plane);
if (!ret) {
plane->crtc = NULL;
plane->fb = NULL;
} else {
- old_fb = NULL;
+ plane->old_fb = NULL;
}
+ old_fb = plane->old_fb;
+ plane->old_fb = NULL;
drm_modeset_unlock_all(dev);
goto out;
}
@@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
}
drm_modeset_lock_all(dev);
- old_fb = plane->fb;
+ plane->old_fb = plane->fb;
ret = plane->funcs->update_plane(plane, crtc, fb,
crtc_x, crtc_y, crtc_w, crtc_h,
src_x, src_y, src_w, src_h);
@@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
plane->fb = fb;
fb = NULL;
} else {
- old_fb = NULL;
+ plane->old_fb = NULL;
}
+ old_fb = plane->old_fb;
+ plane->old_fb = NULL;
drm_modeset_unlock_all(dev);
out:
@@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
* crtcs. Atomic modeset will have saner semantics ...
*/
list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
- tmp->old_fb = tmp->primary->fb;
+ tmp->primary->old_fb = tmp->primary->fb;
fb = set->fb;
@@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
if (tmp->primary->fb)
drm_framebuffer_reference(tmp->primary->fb);
- if (tmp->old_fb)
- drm_framebuffer_unreference(tmp->old_fb);
+ if (tmp->primary->old_fb)
+ drm_framebuffer_unreference(tmp->primary->old_fb);
+ tmp->primary->old_fb = NULL;
}
return ret;
@@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
{
struct drm_mode_crtc_page_flip *page_flip = data;
struct drm_crtc *crtc;
- struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+ struct drm_framebuffer *fb = NULL;
struct drm_pending_vblank_event *e = NULL;
unsigned long flags;
int ret = -EINVAL;
@@ -4530,7 +4537,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
(void (*) (struct drm_pending_event *)) kfree;
}
- old_fb = crtc->primary->fb;
+ crtc->primary->old_fb = crtc->primary->fb;
ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->
