[bug report] drm/vmwgfx: Cursor update fixes

[Thomas Hellstrom]

Hi!

Thanks for the comments, I'll take a closer look.

/Thomas



On 03/27/2018 11:16 AM, Daniel Vetter wrote:
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=EmKCi9L_95tppMT-q6ky7JkUBYYdk22nnaoiRUQRZ3c&s=acr_OiqUZeMipWTzfnf3FAppMYvjv-tetZPy2bYXVxg&e=