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 <daniel.vetter@xxxxxxxx>
> > ---
> > 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.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->flags);
> > if (ret) {
> > if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > @@ -4540,7 +4547,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > kfree(e);
> > }
> > /* Keep the old fb, don't unref it. */
> > - old_fb = NULL;
> > + crtc->primary->old_fb = NULL;
> > } else {
> > /*
> > * Warn if the driver hasn't properly updated the crtc->fb
> > @@ -4556,8 +4563,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > out:
> > if (fb)
> > drm_framebuffer_unreference(fb);
> > - if (old_fb)
> > - drm_framebuffer_unreference(old_fb);
> > + if (crtc->primary->old_fb)
> > + drm_framebuffer_unreference(crtc->primary->old_fb);
> > + crtc->primary->old_fb = NULL;
> > drm_modeset_unlock_crtc(crtc);
> >
> > return ret;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index b0e30c5526ce..5fffb5c53ba6 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -341,10 +341,6 @@ struct drm_crtc {
> > int cursor_x;
> > int cursor_y;
> >
> > - /* Temporary tracking of the old fb while a modeset is ongoing. Used
> > - * by drm_mode_set_config_internal to implement correct refcounting. */
> > - struct drm_framebuffer *old_fb;
> > -
> > bool enabled;
> >
> > /* Requested mode from modesetting. */
> > @@ -622,6 +618,10 @@ struct drm_plane {
> > struct drm_crtc *crtc;
> > struct drm_framebuffer *fb;
> >
> > + /* Temporary tracking of the old fb while a modeset is ongoing. Used
> > + * by drm_mode_set_config_internal to implement correct refcounting. */
>
> Might want to update the wording of this comment slightly since it isn't
> just for drm_mode_set_config_internal (or modesets) anymore.
Good idea, will augment.
-Daniel
>
>
>
> Matt
>
> > + struct drm_framebuffer *old_fb;
> > +
> > const struct drm_plane_funcs *funcs;
> >
> > struct drm_object_properties properties;
> > --
> > 2.0.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
