From: Daniel Vetter <daniel@ffwll.ch> To: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Date: Tue, 19 Nov 2019 11:14:43 +0100 [thread overview] Message-ID: <20191119101443.GA30416@phenom.ffwll.local> (raw) In-Reply-To: <20191115194204.22244-4-ville.syrjala@linux.intel.com> 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(). That aside I'm kinda not getting the point of the shuffling/cleanup in the first 4 patches here, so will skip them. -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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Ville Syrjala <ville.syrjala@linux.intel.com> 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: Tue, 19 Nov 2019 11:14:43 +0100 [thread overview] Message-ID: <20191119101443.GA30416@phenom.ffwll.local> (raw) Message-ID: <20191119101443.QTnTdDbWldrq-up0RcZ7wQcEZctTuKC0iUNXzE4TIUY@z> (raw) In-Reply-To: <20191115194204.22244-4-ville.syrjala@linux.intel.com> 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(). That aside I'm kinda not getting the point of the shuffling/cleanup in the first 4 patches here, so will skip them. -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 _______________________________________________ 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-19 10:14 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 ` Daniel Vetter [this message] 2019-11-19 10:14 ` [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Daniel Vetter 2019-11-22 18:35 ` Ville Syrjälä 2019-11-22 18:35 ` [Intel-gfx] " 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=20191119101443.GA30416@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=ville.syrjala@linux.intel.com \ /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: linkBe 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).