[bug report] drm/vmwgfx: Cursor update fixes

[Daniel Vetter]

On Tue, Mar 27, 2018 at 12:05:38PM +0300, Dan Carpenter wrote:
> Hello Thomas Hellstrom,
> 
> The patch 25db875401c8: "drm/vmwgfx: Cursor update fixes" from Jan
> 16, 2018, leads to the following static checker warning:
> 
> 	drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:513 vmw_du_cursor_plane_atomic_check()
> 	info: return a literal instead of 'ret'
> 
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>    493  int vmw_du_cursor_plane_atomic_check(struct drm_plane *plane,
>    494                                       struct drm_plane_state *new_state)
>    495  {
>    496          int ret = 0;
>    497          struct vmw_surface *surface = NULL;
>    498          struct drm_framebuffer *fb = new_state->fb;
>    499  
>    500          struct drm_rect src = drm_plane_state_src(new_state);
>    501          struct drm_rect dest = drm_plane_state_dest(new_state);
>    502  
>    503          /* Turning off */
>    504          if (!fb)
>    505                  return ret;
>                         ^^^^^^^^^^
> This would be more clear if it were "return 0;"
> 
>    506  
>    507          ret = drm_plane_helper_check_update(plane, new_state->crtc, fb,
>    508                                              &src, &dest,
>    509                                              DRM_MODE_ROTATE_0,
>    510                                              DRM_PLANE_HELPER_NO_SCALING,
>    511                                              DRM_PLANE_HELPER_NO_SCALING,
>    512                                              true, true, &new_state->visible);
>    513          if (!ret)
>    514                  return ret;
> 
> Same here.  With the current code, it almost looks like that the if
> statement is reversed.  As a newbie to this subsystem but with a bit of
> kernel experience, it would take me a while to figure out if the if
> statement is deliberate or a bug.

It is a bug, drm_plane_helper_check_update returns negative errno's on
failure. Also, this code should use drm_atomic_helper_check_plane_state,
not the legacy wrapper, since vmwgfx is an atomic driver (but maybe that's
fixed in -next already).
-Daniel
> 
>    515  
>    516          /* A lot of the code assumes this */
>    517          if (new_state->crtc_w != 64 || new_state->crtc_h != 64) {
>    518                  DRM_ERROR("Invalid cursor dimensions (%d, %d)\n",
>    519                            new_state->crtc_w, new_state->crtc_h);
>    520                  ret = -EINVAL;
>    521          }
>    522  
>    523          if (!vmw_framebuffer_to_vfb(fb)->dmabuf)
>    524                  surface = vmw_framebuffer_to_vfbs(fb)->surface;
>    525  
>    526          if (surface && !surface->snooper.image) {
>    527                  DRM_ERROR("surface not suitable for cursor\n");
>    528                  ret = -EINVAL;
>    529          }
>    530  
>    531          return ret;
>    532  }
> 
> regards,
> dan carpenter
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch