Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 25, 2019 at 4:05 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Mon, Nov 25, 2019 at 10:02:38AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Yank out the code for the plane->fb/old_fb/crtc handling from
> > > > > the page flip path into page_flip_internal(), and provide a
> > > > > simpler variant for atomic drivers.
> > > > >
> > > > > We'll also move the fb vs. src viewport checks into the new
> > > > > functions as they are slightly different between the two paths.
> > > > > If the atomic .page_flip() implementations are guaranteed
> > > > > to call drm_atomic_plane_check() we could even drop the check
> > > > > entirely from page_flip_atomic(). For now toss in a FIXME.
> > > > >
> > > > > v2: Bit more polish in page_flip_internal()
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++-------------
> > > > >  1 file changed, 102 insertions(+), 57 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > index 38878da5b704..6052475a20a5 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
> > > > >         return drm_mode_cursor_common(dev, req, file_priv);
> > > > >  }
> > > > >
> > > > > +static int page_flip_check_fbs(const struct drm_framebuffer *fb,
> > > > > +                              const struct drm_framebuffer *old_fb)
> > > > > +{
> > > > > +       /* The framebuffer is currently unbound, presumably
> > > > > +        * due to a hotplug event, that userspace has not
> > > > > +        * yet discovered.
> > > > > +        */
> > > > > +       if (!old_fb)
> > > > > +               return -EBUSY;
> > > > > +
> > > > > +       if (old_fb->format != fb->format) {
> > > > > +               DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > > +               return -EINVAL;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int page_flip_internal(struct drm_crtc *crtc,
> > > > > +                             struct drm_framebuffer *fb,
> > > > > +                             struct drm_pending_vblank_event *e,
> > > > > +                             u32 flags,
> > > > > +                             u32 target_vblank,
> > > > > +                             struct drm_modeset_acquire_ctx *ctx)
> > > > > +{
> > > > > +       struct drm_plane *plane = crtc->primary;
> > > > > +       int ret;
> > > > > +
> > > > > +       WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> > > > > +
> > > > > +       ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > > > > +                                     &crtc->mode, fb);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       ret = page_flip_check_fbs(fb, plane->fb);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       plane->old_fb = plane->fb;
> > > > > +       if (crtc->funcs->page_flip_target)
> > > > > +               ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > > > > +                                                   target_vblank, ctx);
> > > > > +       else
> > > > > +               ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > > > > +       if (ret) {
> > > > > +               plane->old_fb = NULL;
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       plane->fb = fb;
> > > > > +       drm_framebuffer_get(plane->fb);
> > > > > +
> > > > > +       drm_framebuffer_put(plane->old_fb);
> > > > > +       plane->old_fb = NULL;
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int page_flip_atomic(struct drm_crtc *crtc,
> > > > > +                           struct drm_framebuffer *fb,
> > > > > +                           struct drm_pending_vblank_event *e,
> > > > > +                           u32 flags,
> > > > > +                           u32 target_vblank,
> > > > > +                           struct drm_modeset_acquire_ctx *ctx)
> > > > > +{
> > > > > +       struct drm_plane *plane = crtc->primary;
> > > > > +       struct drm_plane_state *plane_state = plane->state;
> > > > > +       int ret;
> > > > > +
> > > > > +       WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
> > > > > +
> > > > > +       /*
> > > > > +        * FIXME: Can we assume all drivers end up calling
> > > > > +        * drm_atomic_plane_check() in their page flip paths?
> > > > > +        * If so we could remove this.
> > > > > +        */
> > > >
> > > > This is definitely wrong, core has to check these. Currently no driver
> > > > checks this at all. I think you're thinking of
> > > > drm_atomic_helper_check_plane_state().
> > >
> > > No, I'm thinking of drm_atomic_plane_check(). That one already does the
> > > "does the src rect fit into the fb" check. It gets called from
> > > drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some
> > > drivers might take some short circuit path past that for page_flip().
> >
> > tbh if you're worried about this, I'm tempted to rip out the option for
> > atomic drivers to overwrite their page_flip implementation, and force the
> > default function onto everyone (drm_atomic_helper_page_flip_target and
> > friends would need to be moved to drm_plane.c for that ofc).
> >
> > We seem to have 0 need for hacks for old userspace in this area, which is
> > great. So the option to overwrite could only lead to trouble.
> >
> > > > That aside I'm kinda not getting the point of the shuffling/cleanup in the
> > > > first 4 patches here, so will skip them.
> > >
> > > The point was to get that legacy plane->fb/old_fb stuff totally
> > > out from the atomic codepath. Right now it's polluting the
> > > supposedly higher level code shared by both atomic and legacy
> > > drivers.
> >
> > Hm ... I think if you sell it like that, and maybe even go one further and
> > inline page_flip for all atomic drivers, I can be sold on this. As-is I
> > just didn't really get why you're reworking all this.
>
> Hmm. Inlining doesn't look like 100% trivial code shufling due to
> .page_flip_target. We'd need some other mechanism for the driver to
> indicate support for that.

Ah right, I forgot about that one.

> OTOH I'm not sure we even support that stuff on any atomic driver.
> We have the helper but no one actually uses it. And as ususal I'm
> totally unusre what amdgpu is doing since I keep forgetting which
> parts of it are atomic and which are not.

amdgpu/DC does. I guess the simplest soulution would be to add
mode_config->has_flip_target. It's all wired through atomic states,
just not yet exposed as a property. Until that's done we need a
separate flag.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx