[PATCH 1/2] drm/vmwgfx: Refactor cursor handling

[Zack Rusin]

Sorry, I didn't see this before because the email formatting got
really butchered, I only found the underneath comment in it. Please
let me know if there were more.

On Tue, Feb 11, 2025 at 9:33 AM Martin Krastev
<martin.krastev at broadcom.com> wrote:
> On Wed, Jan 15, 2025 at 6:50 AM Zack Rusin <zack.rusin at broadcom.com> wrote:
...
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> > new file mode 100644
> > index 000000000000..05a1ea1f83e9
...
> > +void *vmw_cursor_snooper_create(struct drm_file *file_priv,
> > +                               struct vmw_surface_metadata *metadata)
> > +{
> > +       if (!file_priv->atomic && metadata->scanout &&
> > +           metadata->num_sizes == 1 &&
> > +           metadata->sizes[0].width == VMW_CURSOR_SNOOP_WIDTH &&
> > +           metadata->sizes[0].height == VMW_CURSOR_SNOOP_HEIGHT &&
> > +           metadata->format == VMW_CURSOR_SNOOP_FORMAT) {
> > +               const struct SVGA3dSurfaceDesc *desc =
> > +                       vmw_surface_get_desc(VMW_CURSOR_SNOOP_FORMAT);
> > +               const u32 cursor_size_bytes = VMW_CURSOR_SNOOP_WIDTH *
> > +                                             VMW_CURSOR_SNOOP_HEIGHT *
> > +                                             desc->pitchBytesPerBlock;
> > +               void *image = kzalloc(cursor_size_bytes, GFP_KERNEL);
> > +
> > +               if (!image) {
> > +                       DRM_ERROR("Failed to allocate cursor_image\n");
> > +                       return ERR_PTR(-ENOMEM);
> > +               }
> > +               return image;
> > +       }
> > +       return NULL;
>
> If vmw_cursor_snooper_create() implies an IS_ERR ret-val check by
> caller, as demonstrated in vmw_surface_define_ioctl() below,
> are we sure we want to return a NULL value here and not some ERR_PTR?
> A NULL could surprise some callers down the line.

No, the null is a valid (and in fact ideal) return from this function.
So ideally we don't want the snooper, i.e. it should be NULL. Under
some circumstances (specified by that conditional in
vmw_cursor_snooper_create) we do require a cursor snooper. If we
require it and we're unable to create it that's an error. But if we
don't need it we won't create it and that's not an error.


> LGTM, with one remark -- see the one inlined comment above.
>
> Reviewed-by: Martin Krastev <martin.krastev at broadcom.com>

Thanks!

z
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5427 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250303/23f0c7fc/attachment-0001.bin>