| Message ID | 20230915111130.24064-7-ray.huang@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
On 2023/09/15 20:11, Huang Rui wrote: > Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init > feature flags. > We would like to enable the feature with virglrenderer, so add to create > virgl renderer context with flags using context_id when valid. > > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > V4 -> V5: > - Inverted patch 5 and 6 because we should configure > HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) > > hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- > hw/display/virtio-gpu.c | 2 ++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 8bb7a2c21f..312953ec16 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > cc.debug_name); > > - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > - cc.debug_name); > + if (cc.context_init) { > +#ifdef HAVE_VIRGL_CONTEXT_INIT > + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > + cc.context_init, > + cc.nlen, > + cc.debug_name); > + return; > +#endif This should deal with the case when context_init is set while HAVE_VIRGL_CONTEXT_INIT is not defined.
On 2023/09/15 20:11, Huang Rui wrote: > Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init > feature flags. > We would like to enable the feature with virglrenderer, so add to create > virgl renderer context with flags using context_id when valid. > > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > V4 -> V5: > - Inverted patch 5 and 6 because we should configure > HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) > > hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- > hw/display/virtio-gpu.c | 2 ++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 8bb7a2c21f..312953ec16 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > cc.debug_name); > > - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > - cc.debug_name); > + if (cc.context_init) { > +#ifdef HAVE_VIRGL_CONTEXT_INIT > + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > + cc.context_init, > + cc.nlen, > + cc.debug_name); > + return; > +#endif > + } > + > + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); > } > > static void virgl_cmd_context_destroy(VirtIOGPU *g, > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 3e658f1fef..a66cbd9930 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1506,6 +1506,8 @@ static Property virtio_gpu_properties[] = { > DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, > VIRTIO_GPU_FLAG_BLOB_ENABLED, false), > DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), > DEFINE_PROP_END_OF_LIST(), > }; > I think it's more convenient if this feature is enabled by default.
On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: > On 2023/09/15 20:11, Huang Rui wrote: > > Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init > > feature flags. > > We would like to enable the feature with virglrenderer, so add to create > > virgl renderer context with flags using context_id when valid. > > > > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > --- > > > > V4 -> V5: > > - Inverted patch 5 and 6 because we should configure > > HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) > > > > hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- > > hw/display/virtio-gpu.c | 2 ++ > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > > index 8bb7a2c21f..312953ec16 100644 > > --- a/hw/display/virtio-gpu-virgl.c > > +++ b/hw/display/virtio-gpu-virgl.c > > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > > trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > > cc.debug_name); > > > > - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > > - cc.debug_name); > > + if (cc.context_init) { > > +#ifdef HAVE_VIRGL_CONTEXT_INIT > > + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > > + cc.context_init, > > + cc.nlen, > > + cc.debug_name); > > + return; > > +#endif > > This should deal with the case when context_init is set while > HAVE_VIRGL_CONTEXT_INIT is not defined. Actually, I received the comment below before: https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter the case that virgl_renderer_context_create_with_flags is not defined in virglrenderer early version. Should I bring the error message back? Thanks, Ray
On Sat, Sep 16, 2023 at 12:58:31AM +0800, Akihiko Odaki wrote: > On 2023/09/15 20:11, Huang Rui wrote: > > Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init > > feature flags. > > We would like to enable the feature with virglrenderer, so add to create > > virgl renderer context with flags using context_id when valid. > > > > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > --- > > > > V4 -> V5: > > - Inverted patch 5 and 6 because we should configure > > HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) > > > > hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- > > hw/display/virtio-gpu.c | 2 ++ > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > > index 8bb7a2c21f..312953ec16 100644 > > --- a/hw/display/virtio-gpu-virgl.c > > +++ b/hw/display/virtio-gpu-virgl.c > > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > > trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > > cc.debug_name); > > > > - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > > - cc.debug_name); > > + if (cc.context_init) { > > +#ifdef HAVE_VIRGL_CONTEXT_INIT > > + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > > + cc.context_init, > > + cc.nlen, > > + cc.debug_name); > > + return; > > +#endif > > + } > > + > > + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); > > } > > > > static void virgl_cmd_context_destroy(VirtIOGPU *g, > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > index 3e658f1fef..a66cbd9930 100644 > > --- a/hw/display/virtio-gpu.c > > +++ b/hw/display/virtio-gpu.c > > @@ -1506,6 +1506,8 @@ static Property virtio_gpu_properties[] = { > > DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, > > VIRTIO_GPU_FLAG_BLOB_ENABLED, false), > > DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), > > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > I think it's more convenient if this feature is enabled by default. Yes, I will update it in next version. Thanks, Ray
On 2023/09/16 19:32, Huang Rui wrote: > On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: >> On 2023/09/15 20:11, Huang Rui wrote: >>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init >>> feature flags. >>> We would like to enable the feature with virglrenderer, so add to create >>> virgl renderer context with flags using context_id when valid. >>> >>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> --- >>> >>> V4 -> V5: >>> - Inverted patch 5 and 6 because we should configure >>> HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) >>> >>> hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- >>> hw/display/virtio-gpu.c | 2 ++ >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >>> index 8bb7a2c21f..312953ec16 100644 >>> --- a/hw/display/virtio-gpu-virgl.c >>> +++ b/hw/display/virtio-gpu-virgl.c >>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, >>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, >>> cc.debug_name); >>> >>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, >>> - cc.debug_name); >>> + if (cc.context_init) { >>> +#ifdef HAVE_VIRGL_CONTEXT_INIT >>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, >>> + cc.context_init, >>> + cc.nlen, >>> + cc.debug_name); >>> + return; >>> +#endif >> >> This should deal with the case when context_init is set while >> HAVE_VIRGL_CONTEXT_INIT is not defined. > > Actually, I received the comment below before: > > https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ > > At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set > but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter > the case that virgl_renderer_context_create_with_flags is not defined in > virglrenderer early version. Should I bring the error message back? > > Thanks, > Ray I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of reporting an error here. Perhaps it may be easier to add #ifdef around: > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote: > On 2023/09/16 19:32, Huang Rui wrote: > > On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: > >> On 2023/09/15 20:11, Huang Rui wrote: > >>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init > >>> feature flags. > >>> We would like to enable the feature with virglrenderer, so add to create > >>> virgl renderer context with flags using context_id when valid. > >>> > >>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> > >>> Signed-off-by: Huang Rui <ray.huang@amd.com> > >>> --- > >>> > >>> V4 -> V5: > >>> - Inverted patch 5 and 6 because we should configure > >>> HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) > >>> > >>> hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- > >>> hw/display/virtio-gpu.c | 2 ++ > >>> 2 files changed, 13 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > >>> index 8bb7a2c21f..312953ec16 100644 > >>> --- a/hw/display/virtio-gpu-virgl.c > >>> +++ b/hw/display/virtio-gpu-virgl.c > >>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > >>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > >>> cc.debug_name); > >>> > >>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > >>> - cc.debug_name); > >>> + if (cc.context_init) { > >>> +#ifdef HAVE_VIRGL_CONTEXT_INIT > >>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > >>> + cc.context_init, > >>> + cc.nlen, > >>> + cc.debug_name); > >>> + return; > >>> +#endif > >> > >> This should deal with the case when context_init is set while > >> HAVE_VIRGL_CONTEXT_INIT is not defined. > > > > Actually, I received the comment below before: > > > > https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ > > > > At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set > > but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter > > the case that virgl_renderer_context_create_with_flags is not defined in > > virglrenderer early version. Should I bring the error message back? > > > > Thanks, > > Ray > > I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of > reporting an error here. Perhaps it may be easier to add #ifdef around: > > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), How about below changes: --- diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index 8bb7a2c21f..54a3cfe136 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g, trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, cc.debug_name); - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, - cc.debug_name); + if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) { + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, + cc.context_init, + cc.nlen, + cc.debug_name); + return; + } + + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); } static void virgl_cmd_context_destroy(VirtIOGPU *g, diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index be16efbd38..6ff2c8e92d 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = { DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, VIRTIO_GPU_FLAG_BLOB_ENABLED, false), DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), +#ifdef HAVE_VIRGL_CONTEXT_INIT + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true), +#endif DEFINE_PROP_END_OF_LIST(), };
On 2023/09/17 14:45, Huang Rui wrote: > On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote: >> On 2023/09/16 19:32, Huang Rui wrote: >>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: >>>> On 2023/09/15 20:11, Huang Rui wrote: >>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init >>>>> feature flags. >>>>> We would like to enable the feature with virglrenderer, so add to create >>>>> virgl renderer context with flags using context_id when valid. >>>>> >>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> >>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>> --- >>>>> >>>>> V4 -> V5: >>>>> - Inverted patch 5 and 6 because we should configure >>>>> HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) >>>>> >>>>> hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- >>>>> hw/display/virtio-gpu.c | 2 ++ >>>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >>>>> index 8bb7a2c21f..312953ec16 100644 >>>>> --- a/hw/display/virtio-gpu-virgl.c >>>>> +++ b/hw/display/virtio-gpu-virgl.c >>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, >>>>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, >>>>> cc.debug_name); >>>>> >>>>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, >>>>> - cc.debug_name); >>>>> + if (cc.context_init) { >>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT >>>>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, >>>>> + cc.context_init, >>>>> + cc.nlen, >>>>> + cc.debug_name); >>>>> + return; >>>>> +#endif >>>> >>>> This should deal with the case when context_init is set while >>>> HAVE_VIRGL_CONTEXT_INIT is not defined. >>> >>> Actually, I received the comment below before: >>> >>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ >>> >>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set >>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter >>> the case that virgl_renderer_context_create_with_flags is not defined in >>> virglrenderer early version. Should I bring the error message back? >>> >>> Thanks, >>> Ray >> >> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of >> reporting an error here. Perhaps it may be easier to add #ifdef around: >> > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, >> > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), > > How about below changes: > > --- > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 8bb7a2c21f..54a3cfe136 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > cc.debug_name); > > - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > - cc.debug_name); > + if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) { > + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > + cc.context_init, > + cc.nlen, > + cc.debug_name); > + return; > + } > + > + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); > } > > static void virgl_cmd_context_destroy(VirtIOGPU *g, > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index be16efbd38..6ff2c8e92d 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = { > DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, > VIRTIO_GPU_FLAG_BLOB_ENABLED, false), > DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), > +#ifdef HAVE_VIRGL_CONTEXT_INIT > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true), > +#endif > DEFINE_PROP_END_OF_LIST(), > }; > It looks better, but not having #ifdef around virgl_renderer_context_create_with_flags() will result in a link error with old virglrenderer.
On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote: > On 2023/09/17 14:45, Huang Rui wrote: > > On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote: > >> On 2023/09/16 19:32, Huang Rui wrote: > >>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: > >>>> On 2023/09/15 20:11, Huang Rui wrote: > >>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init > >>>>> feature flags. > >>>>> We would like to enable the feature with virglrenderer, so add to create > >>>>> virgl renderer context with flags using context_id when valid. > >>>>> > >>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> > >>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> > >>>>> --- > >>>>> > >>>>> V4 -> V5: > >>>>> - Inverted patch 5 and 6 because we should configure > >>>>> HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) > >>>>> > >>>>> hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- > >>>>> hw/display/virtio-gpu.c | 2 ++ > >>>>> 2 files changed, 13 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > >>>>> index 8bb7a2c21f..312953ec16 100644 > >>>>> --- a/hw/display/virtio-gpu-virgl.c > >>>>> +++ b/hw/display/virtio-gpu-virgl.c > >>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > >>>>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > >>>>> cc.debug_name); > >>>>> > >>>>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > >>>>> - cc.debug_name); > >>>>> + if (cc.context_init) { > >>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT > >>>>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > >>>>> + cc.context_init, > >>>>> + cc.nlen, > >>>>> + cc.debug_name); > >>>>> + return; > >>>>> +#endif > >>>> > >>>> This should deal with the case when context_init is set while > >>>> HAVE_VIRGL_CONTEXT_INIT is not defined. > >>> > >>> Actually, I received the comment below before: > >>> > >>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ > >>> > >>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set > >>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter > >>> the case that virgl_renderer_context_create_with_flags is not defined in > >>> virglrenderer early version. Should I bring the error message back? > >>> > >>> Thanks, > >>> Ray > >> > >> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of > >> reporting an error here. Perhaps it may be easier to add #ifdef around: > >> > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > >> > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), > > > > How about below changes: > > > --- > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > > index 8bb7a2c21f..54a3cfe136 100644 > > --- a/hw/display/virtio-gpu-virgl.c > > +++ b/hw/display/virtio-gpu-virgl.c > > @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > > trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > > cc.debug_name); > > > > - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > > - cc.debug_name); > > + if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) { > > + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > > + cc.context_init, > > + cc.nlen, > > + cc.debug_name); > > + return; > > + } > > + > > + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); > > } > > > > static void virgl_cmd_context_destroy(VirtIOGPU *g, > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > index be16efbd38..6ff2c8e92d 100644 > > --- a/hw/display/virtio-gpu.c > > +++ b/hw/display/virtio-gpu.c > > @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = { > > DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, > > VIRTIO_GPU_FLAG_BLOB_ENABLED, false), > > DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), > > +#ifdef HAVE_VIRGL_CONTEXT_INIT > > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true), > > +#endif > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > It looks better, but not having #ifdef around > virgl_renderer_context_create_with_flags() will result in a link error > with old virglrenderer. Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you have any better idea? Thanks, Ray
On 2023/09/18 14:43, Huang Rui wrote: > On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote: >> On 2023/09/17 14:45, Huang Rui wrote: >>> On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote: >>>> On 2023/09/16 19:32, Huang Rui wrote: >>>>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: >>>>>> On 2023/09/15 20:11, Huang Rui wrote: >>>>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init >>>>>>> feature flags. >>>>>>> We would like to enable the feature with virglrenderer, so add to create >>>>>>> virgl renderer context with flags using context_id when valid. >>>>>>> >>>>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> >>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>>> --- >>>>>>> >>>>>>> V4 -> V5: >>>>>>> - Inverted patch 5 and 6 because we should configure >>>>>>> HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) >>>>>>> >>>>>>> hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- >>>>>>> hw/display/virtio-gpu.c | 2 ++ >>>>>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >>>>>>> index 8bb7a2c21f..312953ec16 100644 >>>>>>> --- a/hw/display/virtio-gpu-virgl.c >>>>>>> +++ b/hw/display/virtio-gpu-virgl.c >>>>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, >>>>>>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, >>>>>>> cc.debug_name); >>>>>>> >>>>>>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, >>>>>>> - cc.debug_name); >>>>>>> + if (cc.context_init) { >>>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT >>>>>>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, >>>>>>> + cc.context_init, >>>>>>> + cc.nlen, >>>>>>> + cc.debug_name); >>>>>>> + return; >>>>>>> +#endif >>>>>> >>>>>> This should deal with the case when context_init is set while >>>>>> HAVE_VIRGL_CONTEXT_INIT is not defined. >>>>> >>>>> Actually, I received the comment below before: >>>>> >>>>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ >>>>> >>>>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set >>>>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter >>>>> the case that virgl_renderer_context_create_with_flags is not defined in >>>>> virglrenderer early version. Should I bring the error message back? >>>>> >>>>> Thanks, >>>>> Ray >>>> >>>> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of >>>> reporting an error here. Perhaps it may be easier to add #ifdef around: >>>> > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, >>>> > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), >>> >>> How about below changes: > >>> --- >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >>> index 8bb7a2c21f..54a3cfe136 100644 >>> --- a/hw/display/virtio-gpu-virgl.c >>> +++ b/hw/display/virtio-gpu-virgl.c >>> @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g, >>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, >>> cc.debug_name); >>> >>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, >>> - cc.debug_name); >>> + if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) { >>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, >>> + cc.context_init, >>> + cc.nlen, >>> + cc.debug_name); >>> + return; >>> + } >>> + >>> + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); >>> } >>> >>> static void virgl_cmd_context_destroy(VirtIOGPU *g, >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c >>> index be16efbd38..6ff2c8e92d 100644 >>> --- a/hw/display/virtio-gpu.c >>> +++ b/hw/display/virtio-gpu.c >>> @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = { >>> DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, >>> VIRTIO_GPU_FLAG_BLOB_ENABLED, false), >>> DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), >>> +#ifdef HAVE_VIRGL_CONTEXT_INIT >>> + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, >>> + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true), >>> +#endif >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >> >> It looks better, but not having #ifdef around >> virgl_renderer_context_create_with_flags() will result in a link error >> with old virglrenderer. > > Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you > have any better idea? Having #ifdef is the right direction.
On Mon, Sep 18, 2023 at 02:07:25PM +0800, Akihiko Odaki wrote: > On 2023/09/18 14:43, Huang Rui wrote: > > On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote: > >> On 2023/09/17 14:45, Huang Rui wrote: > >>> On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote: > >>>> On 2023/09/16 19:32, Huang Rui wrote: > >>>>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: > >>>>>> On 2023/09/15 20:11, Huang Rui wrote: > >>>>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init > >>>>>>> feature flags. > >>>>>>> We would like to enable the feature with virglrenderer, so add to create > >>>>>>> virgl renderer context with flags using context_id when valid. > >>>>>>> > >>>>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> > >>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> > >>>>>>> --- > >>>>>>> > >>>>>>> V4 -> V5: > >>>>>>> - Inverted patch 5 and 6 because we should configure > >>>>>>> HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) > >>>>>>> > >>>>>>> hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- > >>>>>>> hw/display/virtio-gpu.c | 2 ++ > >>>>>>> 2 files changed, 13 insertions(+), 2 deletions(-) > >>>>>>> > >>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > >>>>>>> index 8bb7a2c21f..312953ec16 100644 > >>>>>>> --- a/hw/display/virtio-gpu-virgl.c > >>>>>>> +++ b/hw/display/virtio-gpu-virgl.c > >>>>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > >>>>>>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > >>>>>>> cc.debug_name); > >>>>>>> > >>>>>>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > >>>>>>> - cc.debug_name); > >>>>>>> + if (cc.context_init) { > >>>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT > >>>>>>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > >>>>>>> + cc.context_init, > >>>>>>> + cc.nlen, > >>>>>>> + cc.debug_name); > >>>>>>> + return; > >>>>>>> +#endif > >>>>>> > >>>>>> This should deal with the case when context_init is set while > >>>>>> HAVE_VIRGL_CONTEXT_INIT is not defined. > >>>>> > >>>>> Actually, I received the comment below before: > >>>>> > >>>>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ > >>>>> > >>>>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set > >>>>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter > >>>>> the case that virgl_renderer_context_create_with_flags is not defined in > >>>>> virglrenderer early version. Should I bring the error message back? > >>>>> > >>>>> Thanks, > >>>>> Ray > >>>> > >>>> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of > >>>> reporting an error here. Perhaps it may be easier to add #ifdef around: > >>>> > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > >>>> > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), > >>> > >>> How about below changes: > > >>> --- > >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > >>> index 8bb7a2c21f..54a3cfe136 100644 > >>> --- a/hw/display/virtio-gpu-virgl.c > >>> +++ b/hw/display/virtio-gpu-virgl.c > >>> @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > >>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > >>> cc.debug_name); > >>> > >>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > >>> - cc.debug_name); > >>> + if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) { > >>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > >>> + cc.context_init, > >>> + cc.nlen, > >>> + cc.debug_name); > >>> + return; > >>> + } > >>> + > >>> + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); > >>> } > >>> > >>> static void virgl_cmd_context_destroy(VirtIOGPU *g, > >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > >>> index be16efbd38..6ff2c8e92d 100644 > >>> --- a/hw/display/virtio-gpu.c > >>> +++ b/hw/display/virtio-gpu.c > >>> @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = { > >>> DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, > >>> VIRTIO_GPU_FLAG_BLOB_ENABLED, false), > >>> DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), > >>> +#ifdef HAVE_VIRGL_CONTEXT_INIT > >>> + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, > >>> + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true), > >>> +#endif > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >> > >> It looks better, but not having #ifdef around > >> virgl_renderer_context_create_with_flags() will result in a link error > >> with old virglrenderer. > > > > Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you > > have any better idea? > > Having #ifdef is the right direction. OK, so we can use cc.context_init and make sure context_init function enabled. Please check below: --- diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index 8bb7a2c21f..8363674ebc 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, cc.debug_name); - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, - cc.debug_name); + if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) { +#ifdef HAVE_VIRGL_CONTEXT_INIT + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, + cc.context_init, + cc.nlen, + cc.debug_name); + return; +#endif + } + + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); } static void virgl_cmd_context_destroy(VirtIOGPU *g, diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index be16efbd38..6ff2c8e92d 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = { DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, VIRTIO_GPU_FLAG_BLOB_ENABLED, false), DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), +#ifdef HAVE_VIRGL_CONTEXT_INIT + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true), +#endif DEFINE_PROP_END_OF_LIST(), };
On 2023/09/18 15:20, Huang Rui wrote: > On Mon, Sep 18, 2023 at 02:07:25PM +0800, Akihiko Odaki wrote: >> On 2023/09/18 14:43, Huang Rui wrote: >>> On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote: >>>> On 2023/09/17 14:45, Huang Rui wrote: >>>>> On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote: >>>>>> On 2023/09/16 19:32, Huang Rui wrote: >>>>>>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote: >>>>>>>> On 2023/09/15 20:11, Huang Rui wrote: >>>>>>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init >>>>>>>>> feature flags. >>>>>>>>> We would like to enable the feature with virglrenderer, so add to create >>>>>>>>> virgl renderer context with flags using context_id when valid. >>>>>>>>> >>>>>>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> >>>>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> V4 -> V5: >>>>>>>>> - Inverted patch 5 and 6 because we should configure >>>>>>>>> HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) >>>>>>>>> >>>>>>>>> hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- >>>>>>>>> hw/display/virtio-gpu.c | 2 ++ >>>>>>>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >>>>>>>>> index 8bb7a2c21f..312953ec16 100644 >>>>>>>>> --- a/hw/display/virtio-gpu-virgl.c >>>>>>>>> +++ b/hw/display/virtio-gpu-virgl.c >>>>>>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, >>>>>>>>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, >>>>>>>>> cc.debug_name); >>>>>>>>> >>>>>>>>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, >>>>>>>>> - cc.debug_name); >>>>>>>>> + if (cc.context_init) { >>>>>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT >>>>>>>>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, >>>>>>>>> + cc.context_init, >>>>>>>>> + cc.nlen, >>>>>>>>> + cc.debug_name); >>>>>>>>> + return; >>>>>>>>> +#endif >>>>>>>> >>>>>>>> This should deal with the case when context_init is set while >>>>>>>> HAVE_VIRGL_CONTEXT_INIT is not defined. >>>>>>> >>>>>>> Actually, I received the comment below before: >>>>>>> >>>>>>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/ >>>>>>> >>>>>>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set >>>>>>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter >>>>>>> the case that virgl_renderer_context_create_with_flags is not defined in >>>>>>> virglrenderer early version. Should I bring the error message back? >>>>>>> >>>>>>> Thanks, >>>>>>> Ray >>>>>> >>>>>> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of >>>>>> reporting an error here. Perhaps it may be easier to add #ifdef around: >>>>>> > + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, >>>>>> > + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), >>>>> >>>>> How about below changes: > >>>>> --- >>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >>>>> index 8bb7a2c21f..54a3cfe136 100644 >>>>> --- a/hw/display/virtio-gpu-virgl.c >>>>> +++ b/hw/display/virtio-gpu-virgl.c >>>>> @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g, >>>>> trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, >>>>> cc.debug_name); >>>>> >>>>> - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, >>>>> - cc.debug_name); >>>>> + if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) { >>>>> + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, >>>>> + cc.context_init, >>>>> + cc.nlen, >>>>> + cc.debug_name); >>>>> + return; >>>>> + } >>>>> + >>>>> + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); >>>>> } >>>>> >>>>> static void virgl_cmd_context_destroy(VirtIOGPU *g, >>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c >>>>> index be16efbd38..6ff2c8e92d 100644 >>>>> --- a/hw/display/virtio-gpu.c >>>>> +++ b/hw/display/virtio-gpu.c >>>>> @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = { >>>>> DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, >>>>> VIRTIO_GPU_FLAG_BLOB_ENABLED, false), >>>>> DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), >>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT >>>>> + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, >>>>> + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true), >>>>> +#endif >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> >>>> >>>> It looks better, but not having #ifdef around >>>> virgl_renderer_context_create_with_flags() will result in a link error >>>> with old virglrenderer. >>> >>> Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you >>> have any better idea? >> >> Having #ifdef is the right direction. > > OK, so we can use cc.context_init and make sure context_init function > enabled. Please check below: > > --- > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 8bb7a2c21f..8363674ebc 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, > trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, > cc.debug_name); > > - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, > - cc.debug_name); > + if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) { > +#ifdef HAVE_VIRGL_CONTEXT_INIT > + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, > + cc.context_init, > + cc.nlen, > + cc.debug_name); > + return; > +#endif > + } You can put the if-statement into #ifdef. Otherwise it looks good to me.
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index 8bb7a2c21f..312953ec16 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g, trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id, cc.debug_name); - virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, - cc.debug_name); + if (cc.context_init) { +#ifdef HAVE_VIRGL_CONTEXT_INIT + virgl_renderer_context_create_with_flags(cc.hdr.ctx_id, + cc.context_init, + cc.nlen, + cc.debug_name); + return; +#endif + } + + virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name); } static void virgl_cmd_context_destroy(VirtIOGPU *g, diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 3e658f1fef..a66cbd9930 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1506,6 +1506,8 @@ static Property virtio_gpu_properties[] = { DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags, VIRTIO_GPU_FLAG_BLOB_ENABLED, false), DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0), + DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags, + VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false), DEFINE_PROP_END_OF_LIST(), };
Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init feature flags. We would like to enable the feature with virglrenderer, so add to create virgl renderer context with flags using context_id when valid. Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com> Signed-off-by: Huang Rui <ray.huang@amd.com> --- V4 -> V5: - Inverted patch 5 and 6 because we should configure HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe) hw/display/virtio-gpu-virgl.c | 13 +++++++++++-- hw/display/virtio-gpu.c | 2 ++ 2 files changed, 13 insertions(+), 2 deletions(-)