Re: [PATCH v1 2/3] virtio-gpu.c: add resource_destroy class method

[Marc-André Lureau]

Hi

On Fri, Jan 26, 2024 at 10:20 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Fri, 26 Jan 2024 at 17:22, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
> >
> > Hi Manos,
> >
> > On 26/1/24 15:41, Manos Pitsidianakis wrote:
> > > When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
> > > need to do their own bookkeeping (free rutabaga resources that are
> > > associated with the virtio_gpu_simple_resource).
> > >
> > > This commit adds a class method so that virtio-gpu-rutabaga can override
> > > it in the next commit.
> > >
> > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > > ---
> > >   hw/display/virtio-gpu.c        | 19 ++++++++++++++++---
> > >   include/hw/virtio/virtio-gpu.h |  2 ++
> > >   2 files changed, 18 insertions(+), 3 deletions(-)
> >
> >
> > >   static void virtio_gpu_resource_unref(VirtIOGPU *g,
> > > @@ -1488,11 +1491,20 @@ static void 
> > > virtio_gpu_device_unrealize(DeviceState *qdev)
> > >   static void virtio_gpu_reset_bh(void *opaque)
> > >   {
> > >       VirtIOGPU *g = VIRTIO_GPU(opaque);
> > > +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
> > >       struct virtio_gpu_simple_resource *res, *tmp;
> > > +    int32_t result, resource_id;
> > >       int i = 0;
> > >
> > >       QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> > > -        virtio_gpu_resource_destroy(g, res);
> > > +        resource_id = res->resource_id;
> > > +        result = vgc->resource_destroy(g, res);
> > > +        if (result) {
> > > +            error_report("%s: %s resource_destroy"
> > > +                         "for resource_id = %d failed with return value 
> > > = %d;",
> >
> > '%d' is for 'int', for 'int32_t' you need 'PRId32'.
>
> Thanks,
>
> > But why return that type instead of 'int'?
>
> Because devices might use FFI, and other languages use fixed size
> integers. Since rutabaga is the only one doing this right now, I used
> their integer width.
>

Imo introducing an API in QEMU, you should follow the conventions and
use bool/Error. Implementation should adapt with the rutabaga API.

> > > +                         __func__, object_get_typename(OBJECT(g)), 
> > > resource_id,
> > > +                         result);
> > > +        }
> > >       }
> >
> >
> > > diff --git a/include/hw/virtio/virtio-gpu.h 
> > > b/include/hw/virtio/virtio-gpu.h
> > > index 584ba2ed73..5683354236 100644
> > > --- a/include/hw/virtio/virtio-gpu.h
> > > +++ b/include/hw/virtio/virtio-gpu.h
> > > @@ -219,6 +219,8 @@ struct VirtIOGPUClass {
> > >       void (*update_cursor_data)(VirtIOGPU *g,
> > >                                  struct virtio_gpu_scanout *s,
> > >                                  uint32_t resource_id);
> > > +    int32_t (*resource_destroy)(VirtIOGPU *g,
> > > +                                struct virtio_gpu_simple_resource *res);
> > >   };
> > >
> > >   struct VirtIOGPUGL {
> >
>

thanks

-- 
Marc-André Lureau