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@xxxxxxxxxxxx> wrote:
> On Wed, Jan 15, 2025 at 6:50 AM Zack Rusin <zack.rusin@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
Thanks!
z
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
