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