From: Daniel Vetter <daniel@ffwll.ch> To: "Ville Syrjälä" <ville.syrjala@linux.intel.com> Cc: intel-gfx <intel-gfx@lists.freedesktop.org>, dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Date: Mon, 25 Nov 2019 18:24:54 +0100 [thread overview] Message-ID: <CAKMK7uFr-gd6jKgW49EkJ4mR2RK7PSdMd9gdYuDz=Qm_hJJYqQ@mail.gmail.com> (raw) In-Reply-To: <20191125150545.GV1208@intel.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: "Ville Syrjälä" <ville.syrjala@linux.intel.com> Cc: intel-gfx <intel-gfx@lists.freedesktop.org>, dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH 3/7] drm: Extract page_flip_{internal, atomic}() Date: Mon, 25 Nov 2019 18:24:54 +0100 [thread overview] Message-ID: <CAKMK7uFr-gd6jKgW49EkJ4mR2RK7PSdMd9gdYuDz=Qm_hJJYqQ@mail.gmail.com> (raw) Message-ID: <20191125172454.dW2mzxO_Ns04TbNpj7xDBynF5e2QXs0EjFA2ssyOmeE@z> (raw) In-Reply-To: <20191125150545.GV1208@intel.com> 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 _______________________________________________ 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-25 17:24 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ä 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 [this message] 2019-11-25 17:24 ` 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='CAKMK7uFr-gd6jKgW49EkJ4mR2RK7PSdMd9gdYuDz=Qm_hJJYqQ@mail.gmail.com' \ --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).