| Message ID | 20180703191500.2374-6-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 03, 2018 at 09:14:50PM +0200, Thomas Hellstrom wrote: > From: Sinclair Yeh <syeh@vmware.com> > > vmw_kms_atomic_check_modeset() is currently checking config using the > legacy state, which is updated after a commit has happened. > > This means vmw_kms_atomic_check_modeset() will reject an invalid config > on the next update rather than the current one. > > Fix this by using the new states for config checking > > Signed-off-by: Sinclair Yeh <syeh@vmware.com> > Reviewed-by: Deepak Rawat <drawat@vmware.com> > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 40 +++++++++++++++++++---------- > 1 file changed, 26 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index e7a7a2e73bbf..6b8541f9215d 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -1526,33 +1526,45 @@ static int > vmw_kms_atomic_check_modeset(struct drm_device *dev, > struct drm_atomic_state *state) > { > - struct drm_crtc_state *crtc_state; > + struct drm_crtc_state *new_crtc_state; > + struct drm_plane_state *new_plane_state; > + struct drm_plane *plane; > struct drm_crtc *crtc; > struct vmw_private *dev_priv = vmw_priv(dev); > - int i; > + int i, ret, cpp = 0; > > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > - unsigned long requested_bb_mem = 0; > + ret = drm_atomic_helper_check(dev, state); > > - if (dev_priv->active_display_unit == vmw_du_screen_target) { > - struct drm_plane *plane = crtc->primary; > - struct drm_plane_state *plane_state; > + /* If this is not a STDU, then no more checking is necessary */ > + if (ret || dev_priv->active_display_unit != vmw_du_screen_target) > + return ret; > > - plane_state = drm_atomic_get_new_plane_state(state, plane); > + for_each_new_plane_in_state(state, plane, new_plane_state, i) { > + if (new_plane_state->fb) { > + int current_cpp = new_plane_state->fb->pitches[0] / > + new_plane_state->fb->width; > > - if (plane_state && plane_state->fb) { > - int cpp = plane_state->fb->format->cpp[0]; > + if (cpp == 0) > + cpp = current_cpp; > + else if (current_cpp != cpp) > + return -EINVAL; > + } > + } > > - requested_bb_mem += crtc->mode.hdisplay * cpp * > - crtc->mode.vdisplay; > - } > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { Note that the above too loops only walk over the changed states (by default), not all planes/crtc. You either need to add them yourself (which kinda defeats the point of the per-plane/crtc locks for doing pageflips in parallel). Or you need to track things in a driver private structure (which you'll only grab on modesets) and just update the delta. I didn't check the entire code to see whether this is a real problem for you, just wanted to point this out. -Daniel > + unsigned long requested_bb_mem = 0; > + > + if (drm_atomic_crtc_needs_modeset(new_crtc_state)) { > + requested_bb_mem += new_crtc_state->mode.hdisplay * > + new_crtc_state->mode.vdisplay * > + cpp; > > if (requested_bb_mem > dev_priv->prim_bb_mem) > return -EINVAL; > } > } > > - return drm_atomic_helper_check(dev, state); > + return ret; > } > > static const struct drm_mode_config_funcs vmw_kms_funcs = { > -- > 2.18.0.rc1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 07/04/2018 10:35 AM, Daniel Vetter wrote: > On Tue, Jul 03, 2018 at 09:14:50PM +0200, Thomas Hellstrom wrote: >> From: Sinclair Yeh <syeh@vmware.com> >> >> vmw_kms_atomic_check_modeset() is currently checking config using the >> legacy state, which is updated after a commit has happened. >> >> This means vmw_kms_atomic_check_modeset() will reject an invalid config >> on the next update rather than the current one. >> >> Fix this by using the new states for config checking >> >> Signed-off-by: Sinclair Yeh <syeh@vmware.com> >> Reviewed-by: Deepak Rawat <drawat@vmware.com> >> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> >> --- >> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 40 +++++++++++++++++++---------- >> 1 file changed, 26 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> index e7a7a2e73bbf..6b8541f9215d 100644 >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> @@ -1526,33 +1526,45 @@ static int >> vmw_kms_atomic_check_modeset(struct drm_device *dev, >> struct drm_atomic_state *state) >> { >> - struct drm_crtc_state *crtc_state; >> + struct drm_crtc_state *new_crtc_state; >> + struct drm_plane_state *new_plane_state; >> + struct drm_plane *plane; >> struct drm_crtc *crtc; >> struct vmw_private *dev_priv = vmw_priv(dev); >> - int i; >> + int i, ret, cpp = 0; >> >> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { >> - unsigned long requested_bb_mem = 0; >> + ret = drm_atomic_helper_check(dev, state); >> >> - if (dev_priv->active_display_unit == vmw_du_screen_target) { >> - struct drm_plane *plane = crtc->primary; >> - struct drm_plane_state *plane_state; >> + /* If this is not a STDU, then no more checking is necessary */ >> + if (ret || dev_priv->active_display_unit != vmw_du_screen_target) >> + return ret; >> >> - plane_state = drm_atomic_get_new_plane_state(state, plane); >> + for_each_new_plane_in_state(state, plane, new_plane_state, i) { >> + if (new_plane_state->fb) { >> + int current_cpp = new_plane_state->fb->pitches[0] / >> + new_plane_state->fb->width; >> >> - if (plane_state && plane_state->fb) { >> - int cpp = plane_state->fb->format->cpp[0]; >> + if (cpp == 0) >> + cpp = current_cpp; >> + else if (current_cpp != cpp) >> + return -EINVAL; >> + } >> + } >> >> - requested_bb_mem += crtc->mode.hdisplay * cpp * >> - crtc->mode.vdisplay; >> - } >> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > Note that the above too loops only walk over the changed states (by > default), not all planes/crtc. You either need to add them yourself (which > kinda defeats the point of the per-plane/crtc locks for doing pageflips in > parallel). Or you need to track things in a driver private structure > (which you'll only grab on modesets) and just update the delta. > > I didn't check the entire code to see whether this is a real problem for > you, just wanted to point this out. > -Daniel Thanks for pointing this out. This is actually addressed in a later patch by Deepak which chooses the second alternative. We should perhaps have squashed these patches, but keeping track internally becomes easier this way. /Thomas
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index e7a7a2e73bbf..6b8541f9215d 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -1526,33 +1526,45 @@ static int vmw_kms_atomic_check_modeset(struct drm_device *dev, struct drm_atomic_state *state) { - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *new_crtc_state; + struct drm_plane_state *new_plane_state; + struct drm_plane *plane; struct drm_crtc *crtc; struct vmw_private *dev_priv = vmw_priv(dev); - int i; + int i, ret, cpp = 0; - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - unsigned long requested_bb_mem = 0; + ret = drm_atomic_helper_check(dev, state); - if (dev_priv->active_display_unit == vmw_du_screen_target) { - struct drm_plane *plane = crtc->primary; - struct drm_plane_state *plane_state; + /* If this is not a STDU, then no more checking is necessary */ + if (ret || dev_priv->active_display_unit != vmw_du_screen_target) + return ret; - plane_state = drm_atomic_get_new_plane_state(state, plane); + for_each_new_plane_in_state(state, plane, new_plane_state, i) { + if (new_plane_state->fb) { + int current_cpp = new_plane_state->fb->pitches[0] / + new_plane_state->fb->width; - if (plane_state && plane_state->fb) { - int cpp = plane_state->fb->format->cpp[0]; + if (cpp == 0) + cpp = current_cpp; + else if (current_cpp != cpp) + return -EINVAL; + } + } - requested_bb_mem += crtc->mode.hdisplay * cpp * - crtc->mode.vdisplay; - } + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + unsigned long requested_bb_mem = 0; + + if (drm_atomic_crtc_needs_modeset(new_crtc_state)) { + requested_bb_mem += new_crtc_state->mode.hdisplay * + new_crtc_state->mode.vdisplay * + cpp; if (requested_bb_mem > dev_priv->prim_bb_mem) return -EINVAL; } } - return drm_atomic_helper_check(dev, state); + return ret; } static const struct drm_mode_config_funcs vmw_kms_funcs = {