| Message ID | 20191115194204.22244-4-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Random pile of core stuff | expand |
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
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
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. -Daniel > > > -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
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. 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.
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
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. + */ + 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)