Re: [RFC v3 5/8] drm/msm: update cursors asynchronously through atomic
On 05/17/2017 05:05 PM, Daniel Vetter wrote: On Wed, May 17, 2017 at 10:56:25AM +0530, Archit Taneja wrote: Hi, On 05/16/2017 08:14 PM, Archit Taneja wrote: On 5/13/2017 12:40 AM, Gustavo Padovan wrote: From: Gustavo PadovanAdd support to async updates of cursors by using the new atomic interface for that. Basically what this commit does is do what mdp5_update_cursor_plane_legacy() did but through atomic. Works well on DB820c (which has a APQ8096 SoC). Tested-by: Archit Taneja Actually, after some more thorough testing, I found one issue, mentioned below. v3: move size checks back to drivers (Ville Syrjälä) v2: move fb setting to core and use new state (Eric Anholt) Cc: Rob Clark Signed-off-by: Gustavo Padovan --- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 151 +- 1 file changed, 63 insertions(+), 88 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index a38c5fe..07106c1 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -33,15 +33,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_rect *src, struct drm_rect *dest); -static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, -struct drm_crtc *crtc, -struct drm_framebuffer *fb, -int crtc_x, int crtc_y, -unsigned int crtc_w, unsigned int crtc_h, -uint32_t src_x, uint32_t src_y, -uint32_t src_w, uint32_t src_h, -struct drm_modeset_acquire_ctx *ctx); - static struct mdp5_kms *get_kms(struct drm_plane *plane) { struct msm_drm_private *priv = plane->dev->dev_private; @@ -257,7 +248,7 @@ static const struct drm_plane_funcs mdp5_plane_funcs = { }; static const struct drm_plane_funcs mdp5_cursor_plane_funcs = { -.update_plane = mdp5_update_cursor_plane_legacy, +.update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, .destroy = mdp5_plane_destroy, .set_property = drm_atomic_helper_plane_set_property, @@ -484,11 +475,73 @@ static void mdp5_plane_atomic_update(struct drm_plane *plane, } } +static int mdp5_plane_atomic_async_check(struct drm_plane *plane, + struct drm_plane_state *state) +{ +struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(state); +struct drm_crtc_state *crtc_state; + +crtc_state = drm_atomic_get_existing_crtc_state(state->state, +state->crtc); I see a kernel splat here (a NULL pointer dereference). The async_check function assumes that there is always going to be a plane_state->crtc available. This doesn't seem to be the case at least in the drm_atomic_helper_disable_plane() path. Moving the check to set legacy_cursor_update after calling __drm_atomic_helper_disable_plane() seems to fix the issue. Do you think it's a legit fix? Yes, plane_state->crtc == NULL is what happens when disabling a plane. I guess simplest would be to just not handle this for the async cursor helpers. Okay. There is actually a check for (crtc != NULL) in drm_atomic_async_check(). The problem with it is that it is referring to the old plane state (i.e plane->state->crtc) instead of the new plane state (i.e, plane_state->crtc). Changing this fixes the issue without the need to touch drm_atomic_helper_disable_plane(). I thought we've had tons of igts to test this ... One more point w.r.t msm driver is that we don't use the default drm_atomic_helper_commit() for our atomic_commit op. So I had to call drm_atomic_helper_async_commit() from our atomic_commit implementation (i.e, in msm_atomic_commit in drivers/gpu/drm/msm/msm_atomic.c) Would be great to fix that - msm predates the nonblocking support in the commit helper, but since this is fixed there's no reason anymore for driver-private commit functions. Or at least there shouldn't be, for almost all drivers. You can stuff all your hw commit logic into atomic_commit_tail. Cool. I'll try to to convert to the commit helper funcs. Thanks, Archit -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 5/8] drm/msm: update cursors asynchronously through atomic
On Wed, May 17, 2017 at 10:56:25AM +0530, Archit Taneja wrote: > Hi, > > On 05/16/2017 08:14 PM, Archit Taneja wrote: > > > > > > On 5/13/2017 12:40 AM, Gustavo Padovan wrote: > > > From: Gustavo Padovan> > > > > > Add support to async updates of cursors by using the new atomic > > > interface for that. Basically what this commit does is do what > > > mdp5_update_cursor_plane_legacy() did but through atomic. > > > > Works well on DB820c (which has a APQ8096 SoC). > > > > Tested-by: Archit Taneja > > Actually, after some more thorough testing, I found one issue, mentioned > below. > > > > > > > > > v3: move size checks back to drivers (Ville Syrjälä) > > > > > > v2: move fb setting to core and use new state (Eric Anholt) > > > > > > Cc: Rob Clark > > > Signed-off-by: Gustavo Padovan > > > --- > > > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 151 > > > +- > > > 1 file changed, 63 insertions(+), 88 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > > index a38c5fe..07106c1 100644 > > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > > @@ -33,15 +33,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, > > > struct drm_crtc *crtc, struct drm_framebuffer *fb, > > > struct drm_rect *src, struct drm_rect *dest); > > > -static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, > > > -struct drm_crtc *crtc, > > > -struct drm_framebuffer *fb, > > > -int crtc_x, int crtc_y, > > > -unsigned int crtc_w, unsigned int crtc_h, > > > -uint32_t src_x, uint32_t src_y, > > > -uint32_t src_w, uint32_t src_h, > > > -struct drm_modeset_acquire_ctx *ctx); > > > - > > > static struct mdp5_kms *get_kms(struct drm_plane *plane) > > > { > > > struct msm_drm_private *priv = plane->dev->dev_private; > > > @@ -257,7 +248,7 @@ static const struct drm_plane_funcs mdp5_plane_funcs > > > = { > > > }; > > > static const struct drm_plane_funcs mdp5_cursor_plane_funcs = { > > > -.update_plane = mdp5_update_cursor_plane_legacy, > > > +.update_plane = drm_atomic_helper_update_plane, > > > .disable_plane = drm_atomic_helper_disable_plane, > > > .destroy = mdp5_plane_destroy, > > > .set_property = drm_atomic_helper_plane_set_property, > > > @@ -484,11 +475,73 @@ static void mdp5_plane_atomic_update(struct > > > drm_plane *plane, > > > } > > > } > > > +static int mdp5_plane_atomic_async_check(struct drm_plane *plane, > > > + struct drm_plane_state *state) > > > +{ > > > +struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(state); > > > +struct drm_crtc_state *crtc_state; > > > + > > > +crtc_state = drm_atomic_get_existing_crtc_state(state->state, > > > +state->crtc); > > I see a kernel splat here (a NULL pointer dereference). The async_check > function assumes that there is always going to be a plane_state->crtc > available. This doesn't seem to be the case at least in the > drm_atomic_helper_disable_plane() path. Moving the check to set > legacy_cursor_update after calling __drm_atomic_helper_disable_plane() > seems to fix the issue. Do you think it's a legit fix? Yes, plane_state->crtc == NULL is what happens when disabling a plane. I guess simplest would be to just not handle this for the async cursor helpers. I thought we've had tons of igts to test this ... > One more point w.r.t msm driver is that we don't use the default > drm_atomic_helper_commit() for our atomic_commit op. So I had to > call drm_atomic_helper_async_commit() from our atomic_commit > implementation > (i.e, in msm_atomic_commit in drivers/gpu/drm/msm/msm_atomic.c) Would be great to fix that - msm predates the nonblocking support in the commit helper, but since this is fixed there's no reason anymore for driver-private commit functions. Or at least there shouldn't be, for almost all drivers. You can stuff all your hw commit logic into atomic_commit_tail. -Daniel -- 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
Re: [RFC v3 5/8] drm/msm: update cursors asynchronously through atomic
Hi, On 05/16/2017 08:14 PM, Archit Taneja wrote: On 5/13/2017 12:40 AM, Gustavo Padovan wrote: From: Gustavo PadovanAdd support to async updates of cursors by using the new atomic interface for that. Basically what this commit does is do what mdp5_update_cursor_plane_legacy() did but through atomic. Works well on DB820c (which has a APQ8096 SoC). Tested-by: Archit Taneja Actually, after some more thorough testing, I found one issue, mentioned below. v3: move size checks back to drivers (Ville Syrjälä) v2: move fb setting to core and use new state (Eric Anholt) Cc: Rob Clark Signed-off-by: Gustavo Padovan --- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 151 +- 1 file changed, 63 insertions(+), 88 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index a38c5fe..07106c1 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -33,15 +33,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_rect *src, struct drm_rect *dest); -static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, -struct drm_crtc *crtc, -struct drm_framebuffer *fb, -int crtc_x, int crtc_y, -unsigned int crtc_w, unsigned int crtc_h, -uint32_t src_x, uint32_t src_y, -uint32_t src_w, uint32_t src_h, -struct drm_modeset_acquire_ctx *ctx); - static struct mdp5_kms *get_kms(struct drm_plane *plane) { struct msm_drm_private *priv = plane->dev->dev_private; @@ -257,7 +248,7 @@ static const struct drm_plane_funcs mdp5_plane_funcs = { }; static const struct drm_plane_funcs mdp5_cursor_plane_funcs = { -.update_plane = mdp5_update_cursor_plane_legacy, +.update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, .destroy = mdp5_plane_destroy, .set_property = drm_atomic_helper_plane_set_property, @@ -484,11 +475,73 @@ static void mdp5_plane_atomic_update(struct drm_plane *plane, } } +static int mdp5_plane_atomic_async_check(struct drm_plane *plane, + struct drm_plane_state *state) +{ +struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(state); +struct drm_crtc_state *crtc_state; + +crtc_state = drm_atomic_get_existing_crtc_state(state->state, +state->crtc); I see a kernel splat here (a NULL pointer dereference). The async_check function assumes that there is always going to be a plane_state->crtc available. This doesn't seem to be the case at least in the drm_atomic_helper_disable_plane() path. Moving the check to set legacy_cursor_update after calling __drm_atomic_helper_disable_plane() seems to fix the issue. Do you think it's a legit fix? One more point w.r.t msm driver is that we don't use the default drm_atomic_helper_commit() for our atomic_commit op. So I had to call drm_atomic_helper_async_commit() from our atomic_commit implementation (i.e, in msm_atomic_commit in drivers/gpu/drm/msm/msm_atomic.c) Thanks, Archit +if (WARN_ON(!crtc_state)) +return -EINVAL; + +if (!crtc_state->active) +return -EINVAL; + +mdp5_state = to_mdp5_plane_state(state); + +/* don't use fast path if we don't have a hwpipe allocated yet */ +if (!mdp5_state->hwpipe) +return -EINVAL; + +/* only allow changing of position(crtc x/y or src x/y) in fast path */ +if (plane->state->crtc != state->crtc || +plane->state->src_w != state->src_w || +plane->state->src_h != state->src_h || +plane->state->crtc_w != state->crtc_w || +plane->state->crtc_h != state->crtc_h || +!plane->state->fb || +plane->state->fb != state->fb) +return -EINVAL; + +return 0; +} + +static void mdp5_plane_atomic_async_update(struct drm_plane *plane, + struct drm_plane_state *new_state) +{ +plane->state->src_x = new_state->src_x; +plane->state->src_y = new_state->src_y; +plane->state->crtc_x = new_state->crtc_x; +plane->state->crtc_y = new_state->crtc_y; + +if (plane_enabled(new_state)) { +struct mdp5_ctl *ctl; +struct mdp5_pipeline *pipeline = +mdp5_crtc_get_pipeline(plane->crtc); +int ret; + +ret = mdp5_plane_mode_set(plane, new_state->crtc, new_state->fb, +_state->src, _state->dst); +WARN_ON(ret < 0); + +ctl = mdp5_crtc_get_ctl(new_state->crtc); + +mdp5_ctl_commit(ctl, pipeline, mdp5_plane_get_flush(plane)); +} + +*to_mdp5_plane_state(plane->state) = +*to_mdp5_plane_state(new_state); +} + static const struct drm_plane_helper_funcs
Re: [RFC v3 5/8] drm/msm: update cursors asynchronously through atomic
On 5/13/2017 12:40 AM, Gustavo Padovan wrote: From: Gustavo PadovanAdd support to async updates of cursors by using the new atomic interface for that. Basically what this commit does is do what mdp5_update_cursor_plane_legacy() did but through atomic. Works well on DB820c (which has a APQ8096 SoC). Tested-by: Archit Taneja v3: move size checks back to drivers (Ville Syrjälä) v2: move fb setting to core and use new state (Eric Anholt) Cc: Rob Clark Signed-off-by: Gustavo Padovan --- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 151 +- 1 file changed, 63 insertions(+), 88 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index a38c5fe..07106c1 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -33,15 +33,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_rect *src, struct drm_rect *dest); -static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, - struct drm_crtc *crtc, - struct drm_framebuffer *fb, - int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h, - struct drm_modeset_acquire_ctx *ctx); - static struct mdp5_kms *get_kms(struct drm_plane *plane) { struct msm_drm_private *priv = plane->dev->dev_private; @@ -257,7 +248,7 @@ static const struct drm_plane_funcs mdp5_plane_funcs = { }; static const struct drm_plane_funcs mdp5_cursor_plane_funcs = { - .update_plane = mdp5_update_cursor_plane_legacy, + .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, .destroy = mdp5_plane_destroy, .set_property = drm_atomic_helper_plane_set_property, @@ -484,11 +475,73 @@ static void mdp5_plane_atomic_update(struct drm_plane *plane, } } +static int mdp5_plane_atomic_async_check(struct drm_plane *plane, +struct drm_plane_state *state) +{ + struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(state); + struct drm_crtc_state *crtc_state; + + crtc_state = drm_atomic_get_existing_crtc_state(state->state, + state->crtc); + if (WARN_ON(!crtc_state)) + return -EINVAL; + + if (!crtc_state->active) + return -EINVAL; + + mdp5_state = to_mdp5_plane_state(state); + + /* don't use fast path if we don't have a hwpipe allocated yet */ + if (!mdp5_state->hwpipe) + return -EINVAL; + + /* only allow changing of position(crtc x/y or src x/y) in fast path */ + if (plane->state->crtc != state->crtc || + plane->state->src_w != state->src_w || + plane->state->src_h != state->src_h || + plane->state->crtc_w != state->crtc_w || + plane->state->crtc_h != state->crtc_h || + !plane->state->fb || + plane->state->fb != state->fb) + return -EINVAL; + + return 0; +} + +static void mdp5_plane_atomic_async_update(struct drm_plane *plane, + struct drm_plane_state *new_state) +{ + plane->state->src_x = new_state->src_x; + plane->state->src_y = new_state->src_y; + plane->state->crtc_x = new_state->crtc_x; + plane->state->crtc_y = new_state->crtc_y; + + if (plane_enabled(new_state)) { + struct mdp5_ctl *ctl; + struct mdp5_pipeline *pipeline = + mdp5_crtc_get_pipeline(plane->crtc); + int ret; + + ret = mdp5_plane_mode_set(plane, new_state->crtc, new_state->fb, + _state->src, _state->dst); + WARN_ON(ret < 0); + + ctl = mdp5_crtc_get_ctl(new_state->crtc); + + mdp5_ctl_commit(ctl, pipeline, mdp5_plane_get_flush(plane)); + } + + *to_mdp5_plane_state(plane->state) = + *to_mdp5_plane_state(new_state); +} + static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = { .prepare_fb = mdp5_plane_prepare_fb, .cleanup_fb = mdp5_plane_cleanup_fb, .atomic_check = mdp5_plane_atomic_check, .atomic_update = mdp5_plane_atomic_update, + .atomic_async_check = mdp5_plane_atomic_async_check, + .atomic_async_update = mdp5_plane_atomic_async_update, }; static void set_scanout_locked(struct mdp5_kms *mdp5_kms, @@ -997,84
[RFC v3 5/8] drm/msm: update cursors asynchronously through atomic
From: Gustavo PadovanAdd support to async updates of cursors by using the new atomic interface for that. Basically what this commit does is do what mdp5_update_cursor_plane_legacy() did but through atomic. v3: move size checks back to drivers (Ville Syrjälä) v2: move fb setting to core and use new state (Eric Anholt) Cc: Rob Clark Signed-off-by: Gustavo Padovan --- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 151 +- 1 file changed, 63 insertions(+), 88 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index a38c5fe..07106c1 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -33,15 +33,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_rect *src, struct drm_rect *dest); -static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, - struct drm_crtc *crtc, - struct drm_framebuffer *fb, - int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h, - struct drm_modeset_acquire_ctx *ctx); - static struct mdp5_kms *get_kms(struct drm_plane *plane) { struct msm_drm_private *priv = plane->dev->dev_private; @@ -257,7 +248,7 @@ static const struct drm_plane_funcs mdp5_plane_funcs = { }; static const struct drm_plane_funcs mdp5_cursor_plane_funcs = { - .update_plane = mdp5_update_cursor_plane_legacy, + .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, .destroy = mdp5_plane_destroy, .set_property = drm_atomic_helper_plane_set_property, @@ -484,11 +475,73 @@ static void mdp5_plane_atomic_update(struct drm_plane *plane, } } +static int mdp5_plane_atomic_async_check(struct drm_plane *plane, +struct drm_plane_state *state) +{ + struct mdp5_plane_state *mdp5_state = to_mdp5_plane_state(state); + struct drm_crtc_state *crtc_state; + + crtc_state = drm_atomic_get_existing_crtc_state(state->state, + state->crtc); + if (WARN_ON(!crtc_state)) + return -EINVAL; + + if (!crtc_state->active) + return -EINVAL; + + mdp5_state = to_mdp5_plane_state(state); + + /* don't use fast path if we don't have a hwpipe allocated yet */ + if (!mdp5_state->hwpipe) + return -EINVAL; + + /* only allow changing of position(crtc x/y or src x/y) in fast path */ + if (plane->state->crtc != state->crtc || + plane->state->src_w != state->src_w || + plane->state->src_h != state->src_h || + plane->state->crtc_w != state->crtc_w || + plane->state->crtc_h != state->crtc_h || + !plane->state->fb || + plane->state->fb != state->fb) + return -EINVAL; + + return 0; +} + +static void mdp5_plane_atomic_async_update(struct drm_plane *plane, + struct drm_plane_state *new_state) +{ + plane->state->src_x = new_state->src_x; + plane->state->src_y = new_state->src_y; + plane->state->crtc_x = new_state->crtc_x; + plane->state->crtc_y = new_state->crtc_y; + + if (plane_enabled(new_state)) { + struct mdp5_ctl *ctl; + struct mdp5_pipeline *pipeline = + mdp5_crtc_get_pipeline(plane->crtc); + int ret; + + ret = mdp5_plane_mode_set(plane, new_state->crtc, new_state->fb, + _state->src, _state->dst); + WARN_ON(ret < 0); + + ctl = mdp5_crtc_get_ctl(new_state->crtc); + + mdp5_ctl_commit(ctl, pipeline, mdp5_plane_get_flush(plane)); + } + + *to_mdp5_plane_state(plane->state) = + *to_mdp5_plane_state(new_state); +} + static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = { .prepare_fb = mdp5_plane_prepare_fb, .cleanup_fb = mdp5_plane_cleanup_fb, .atomic_check = mdp5_plane_atomic_check, .atomic_update = mdp5_plane_atomic_update, + .atomic_async_check = mdp5_plane_atomic_async_check, + .atomic_async_update = mdp5_plane_atomic_async_update, }; static void set_scanout_locked(struct mdp5_kms *mdp5_kms, @@ -997,84 +1050,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, return ret; } -static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, -