* [bug report] drm/vmwgfx: Cursor update fixes
@ 2018-03-27 9:05 Dan Carpenter
2018-03-27 9:16 ` Daniel Vetter
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2018-03-27 9:05 UTC (permalink / raw)
To: thellstrom; +Cc: dri-devel
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.
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] drm/vmwgfx: Cursor update fixes
2018-03-27 9:05 [bug report] drm/vmwgfx: Cursor update fixes Dan Carpenter
@ 2018-03-27 9:16 ` Daniel Vetter
2018-03-27 12:16 ` Thomas Hellstrom
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Vetter @ 2018-03-27 9:16 UTC (permalink / raw)
To: Dan Carpenter; +Cc: thellstrom, dri-devel
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] drm/vmwgfx: Cursor update fixes
2018-03-27 9:16 ` Daniel Vetter
@ 2018-03-27 12:16 ` Thomas Hellstrom
0 siblings, 0 replies; 3+ messages in thread
From: Thomas Hellstrom @ 2018-03-27 12:16 UTC (permalink / raw)
To: Daniel Vetter, Dan Carpenter; +Cc: dri-devel
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@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=
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-03-27 12:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-27 9:05 [bug report] drm/vmwgfx: Cursor update fixes Dan Carpenter
2018-03-27 9:16 ` Daniel Vetter
2018-03-27 12:16 ` Thomas Hellstrom
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.