From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}()
Date: Fri, 22 Nov 2019 20:35:13 +0200 [thread overview]
Message-ID: <20191122183513.GQ1208@intel.com> (raw)
Message-ID: <20191122183513.CjSqMpAEX86yRq29cY8chUhY0fKWX_j94C0PuJTlTYY@z> (raw)
In-Reply-To: <20191119101443.GA30416@phenom.ffwll.local>
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().
>
> 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.
> -Daniel
>
> > + ret = drm_framebuffer_check_src_coords(plane_state->src_x,
> > + plane_state->src_y,
> > + plane_state->src_w,
> > + plane_state->src_h,
> > + fb);
> > + if (ret)
> > + return ret;
> > +
> > + ret = page_flip_check_fbs(fb, plane_state->fb);
> > + if (ret)
> > + return ret;
> > +
> > + if (crtc->funcs->page_flip_target)
> > + return crtc->funcs->page_flip_target(crtc, fb, e, flags,
> > + target_vblank, ctx);
> > + else
> > + return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
> > +}
> > +
> > int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > void *data, struct drm_file *file_priv)
> > {
> > struct drm_mode_crtc_page_flip_target *page_flip = data;
> > struct drm_crtc *crtc;
> > struct drm_plane *plane;
> > - struct drm_framebuffer *fb, *old_fb;
> > + struct drm_framebuffer *fb;
> > struct drm_pending_vblank_event *e = NULL;
> > u32 target_vblank = page_flip->sequence;
> > struct drm_modeset_acquire_ctx ctx;
> > @@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > if (ret)
> > goto out;
> >
> > - if (plane->state)
> > - old_fb = plane->state->fb;
> > + if (drm_drv_uses_atomic_modeset(dev))
> > + ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
> > + target_vblank, &ctx);
> > else
> > - old_fb = plane->fb;
> > -
> > - if (old_fb == NULL) {
> > - /* The framebuffer is currently unbound, presumably
> > - * due to a hotplug event, that userspace has not
> > - * yet discovered.
> > - */
> > - ret = -EBUSY;
> > - goto out;
> > - }
> > -
> > - if (plane->state) {
> > - const struct drm_plane_state *state = plane->state;
> > -
> > - ret = drm_framebuffer_check_src_coords(state->src_x,
> > - state->src_y,
> > - state->src_w,
> > - state->src_h,
> > - fb);
> > - } else {
> > - ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
> > - &crtc->mode, fb);
> > - }
> > - if (ret)
> > - goto out;
> > -
> > - if (old_fb->format != fb->format) {
> > - DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > - plane->old_fb = plane->fb;
> > - if (crtc->funcs->page_flip_target)
> > - ret = crtc->funcs->page_flip_target(crtc, fb, e,
> > - page_flip->flags,
> > - target_vblank,
> > - &ctx);
> > - else
> > - ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
> > - &ctx);
> > - if (ret) {
> > - /* Keep the old fb, don't unref it. */
> > - plane->old_fb = NULL;
> > - } else {
> > - if (!plane->state) {
> > - plane->fb = fb;
> > - drm_framebuffer_get(fb);
> > - }
> > - }
> > + ret = page_flip_internal(crtc, fb, e, page_flip->flags,
> > + target_vblank, &ctx);
> >
> > out:
> > - if (plane->old_fb)
> > - drm_framebuffer_put(plane->old_fb);
> > - plane->old_fb = NULL;
> > -
> > if (ret == -EDEADLK) {
> > ret = drm_modeset_backoff(&ctx);
> > if (!ret)
> > --
> > 2.23.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-11-22 18:35 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-15 19:41 [PATCH 0/7] drm: Random pile of core stuff Ville Syrjala
2019-11-15 19:41 ` Ville Syrjala
2019-11-15 19:41 ` [PATCH 1/7] drm: Move page_flip fb lookup earlier Ville Syrjala
2019-11-15 19:41 ` Ville Syrjala
2019-11-15 19:41 ` [PATCH 2/7] drm: Allocate the page flip event earlier Ville Syrjala
2019-11-15 19:41 ` Ville Syrjala
2019-11-15 19:42 ` [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Ville Syrjala
2019-11-15 19:42 ` [PATCH 3/7] drm: Extract page_flip_{internal,atomic}() Ville Syrjala
2019-11-19 10:14 ` [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Daniel Vetter
2019-11-19 10:14 ` [Intel-gfx] " Daniel Vetter
2019-11-22 18:35 ` Ville Syrjälä [this message]
2019-11-22 18:35 ` Ville Syrjälä
2019-11-25 9:02 ` Daniel Vetter
2019-11-25 9:02 ` [Intel-gfx] " Daniel Vetter
2019-11-25 15:05 ` Ville Syrjälä
2019-11-25 15:05 ` [Intel-gfx] " Ville Syrjälä
2019-11-25 17:24 ` Daniel Vetter
2019-11-25 17:24 ` [Intel-gfx] " Daniel Vetter
2019-11-15 19:42 ` [PATCH 4/7] drm: Simplify the setplane old_fb handling further Ville Syrjala
2019-11-15 19:42 ` Ville Syrjala
2019-11-15 19:42 ` [PATCH 5/7] drm/selftests: Add some selftests for drm_atomic_set_mode_for_crtc() Ville Syrjala
2019-11-15 19:42 ` Ville Syrjala
2019-11-19 10:39 ` Daniel Vetter
2019-11-15 19:42 ` [PATCH 6/7] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc() Ville Syrjala
2019-11-19 10:20 ` Daniel Vetter
2019-11-19 10:20 ` Daniel Vetter
2019-11-15 19:42 ` [PATCH 7/7] drm/atomic: Reduce setplane locking Ville Syrjala
2019-11-19 10:47 ` Daniel Vetter
2019-11-19 10:47 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191122183513.GQ1208@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).