* [PATCH] ui/gtk: set the area of the scanout texture correctly @ 2023-06-21 21:31 Dongwon Kim 2023-06-26 11:56 ` Marc-André Lureau 0 siblings, 1 reply; 5+ messages in thread From: Dongwon Kim @ 2023-06-21 21:31 UTC (permalink / raw) To: qemu-devel Cc: Dongwon Kim, Gerd Hoffmann, Marc-André Lureau, Vivek Kasireddy x and y offsets and width and height of the scanout texture is not correctly configured in case guest scanout frame is dmabuf. Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> --- ui/gtk-egl.c | 3 ++- ui/gtk-gl-area.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index 19130041bc..e99e3b0d8c 100644 --- a/ui/gtk-egl.c +++ b/ui/gtk-egl.c @@ -257,7 +257,8 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl, gd_egl_scanout_texture(dcl, dmabuf->texture, dmabuf->y0_top, dmabuf->width, dmabuf->height, - 0, 0, dmabuf->width, dmabuf->height); + dmabuf->x, dmabuf->y, dmabuf->scanout_width, + dmabuf->scanout_height); if (dmabuf->allow_fences) { vc->gfx.guest_fb.dmabuf = dmabuf; diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c index c384a1516b..1605818bd1 100644 --- a/ui/gtk-gl-area.c +++ b/ui/gtk-gl-area.c @@ -299,7 +299,8 @@ void gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl, gd_gl_area_scanout_texture(dcl, dmabuf->texture, dmabuf->y0_top, dmabuf->width, dmabuf->height, - 0, 0, dmabuf->width, dmabuf->height); + dmabuf->x, dmabuf->y, dmabuf->scanout_width, + dmabuf->scanout_height); if (dmabuf->allow_fences) { vc->gfx.guest_fb.dmabuf = dmabuf; -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ui/gtk: set the area of the scanout texture correctly 2023-06-21 21:31 [PATCH] ui/gtk: set the area of the scanout texture correctly Dongwon Kim @ 2023-06-26 11:56 ` Marc-André Lureau 2023-06-26 19:49 ` Kim, Dongwon 0 siblings, 1 reply; 5+ messages in thread From: Marc-André Lureau @ 2023-06-26 11:56 UTC (permalink / raw) To: Dongwon Kim; +Cc: qemu-devel, Gerd Hoffmann, Vivek Kasireddy [-- Attachment #1: Type: text/plain, Size: 2542 bytes --] Hi On Wed, Jun 21, 2023 at 11:53 PM Dongwon Kim <dongwon.kim@intel.com> wrote: > x and y offsets and width and height of the scanout texture > is not correctly configured in case guest scanout frame is > dmabuf. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > I find this a bit confusing, and I don't know how to actually test it. The only place where scanout_{width, height} are set is virtio_gpu_create_dmabuf() and there, they have the same values as width and height. it's too easy to get confused with the values imho. I find the terminology we use for ScanoutTexture much clearer. It uses backing_{width, height} instead, which indicates quite clearly that the usual x/y/w/h are for the sub-region to be shown. I think we should have a preliminary commit that renames scanout_{width, height}. Please give some help/hints on how to actually test this code too. Thanks! --- > ui/gtk-egl.c | 3 ++- > ui/gtk-gl-area.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c > index 19130041bc..e99e3b0d8c 100644 > --- a/ui/gtk-egl.c > +++ b/ui/gtk-egl.c > @@ -257,7 +257,8 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl, > > gd_egl_scanout_texture(dcl, dmabuf->texture, > dmabuf->y0_top, dmabuf->width, dmabuf->height, > - 0, 0, dmabuf->width, dmabuf->height); > + dmabuf->x, dmabuf->y, dmabuf->scanout_width, > + dmabuf->scanout_height); > > if (dmabuf->allow_fences) { > vc->gfx.guest_fb.dmabuf = dmabuf; > diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c > index c384a1516b..1605818bd1 100644 > --- a/ui/gtk-gl-area.c > +++ b/ui/gtk-gl-area.c > @@ -299,7 +299,8 @@ void gd_gl_area_scanout_dmabuf(DisplayChangeListener > *dcl, > > gd_gl_area_scanout_texture(dcl, dmabuf->texture, > dmabuf->y0_top, dmabuf->width, > dmabuf->height, > - 0, 0, dmabuf->width, dmabuf->height); > + dmabuf->x, dmabuf->y, > dmabuf->scanout_width, > + dmabuf->scanout_height); > > if (dmabuf->allow_fences) { > vc->gfx.guest_fb.dmabuf = dmabuf; > -- > 2.34.1 > > > -- Marc-André Lureau [-- Attachment #2: Type: text/html, Size: 3834 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ui/gtk: set the area of the scanout texture correctly 2023-06-26 11:56 ` Marc-André Lureau @ 2023-06-26 19:49 ` Kim, Dongwon 2023-07-04 16:07 ` Marc-André Lureau 0 siblings, 1 reply; 5+ messages in thread From: Kim, Dongwon @ 2023-06-26 19:49 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, Gerd Hoffmann, Vivek Kasireddy Hi Marc-André Lureau, On 6/26/2023 4:56 AM, Marc-André Lureau wrote: > Hi > > On Wed, Jun 21, 2023 at 11:53 PM Dongwon Kim <dongwon.kim@intel.com> > wrote: > > x and y offsets and width and height of the scanout texture > is not correctly configured in case guest scanout frame is > dmabuf. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > > > I find this a bit confusing, and I don't know how to actually test it. > > The only place where scanout_{width, height} are set is > virtio_gpu_create_dmabuf() and there, they have the same values as > width and height. it's too easy to get confused with the values imho. Yes, scanout_width/height are same as width/height as far as there is only one guest display exist. But they will be different in case there multiple displays on the guest side, configured in extended mode (when the guest is running Xorg). In this case, blob for the guest display is same for scanout 1 and 2 but each scanout will have different offset and scanout_width/scanout_height to reference a sub region in the same blob(dmabuf). I added x/y/scanout_width/scanout_height with a previous commit: commit e86a93f55463c088aa0b5260e915ffbf9f86c62b Author: Dongwon Kim <dongwon.kim@intel.com> Date: Wed Nov 3 23:51:52 2021 -0700 virtio-gpu: splitting one extended mode guest fb into n-scanouts > I find the terminology we use for ScanoutTexture much clearer. It uses > backing_{width, height} instead, which indicates quite clearly that > the usual x/y/w/h are for the sub-region to be shown. yeah agreed. Then dmabuf->width/height should be changed to dmabuf->backing_width/height and dmabuf->width/height will be replacing dmabuf->scanout_width/scanout_height. I guess this is what you meant, right? > I think we should have a preliminary commit that renames > scanout_{width, height}. > > Please give some help/hints on how to actually test this code too. So this patch is just to make things look consistent in the code level. Having offset (0,0) in this function call for all different scanouts didn't look right to me. This code change won't make anything done differently though. So no test is applicable. > > Thanks! > > > --- > ui/gtk-egl.c | 3 ++- > ui/gtk-gl-area.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c > index 19130041bc..e99e3b0d8c 100644 > --- a/ui/gtk-egl.c > +++ b/ui/gtk-egl.c > @@ -257,7 +257,8 @@ void > gd_egl_scanout_dmabuf(DisplayChangeListener *dcl, > > gd_egl_scanout_texture(dcl, dmabuf->texture, > dmabuf->y0_top, dmabuf->width, > dmabuf->height, > - 0, 0, dmabuf->width, dmabuf->height); > + dmabuf->x, dmabuf->y, > dmabuf->scanout_width, > + dmabuf->scanout_height); > > if (dmabuf->allow_fences) { > vc->gfx.guest_fb.dmabuf = dmabuf; > diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c > index c384a1516b..1605818bd1 100644 > --- a/ui/gtk-gl-area.c > +++ b/ui/gtk-gl-area.c > @@ -299,7 +299,8 @@ void > gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl, > > gd_gl_area_scanout_texture(dcl, dmabuf->texture, > dmabuf->y0_top, dmabuf->width, > dmabuf->height, > - 0, 0, dmabuf->width, dmabuf->height); > + dmabuf->x, dmabuf->y, > dmabuf->scanout_width, > + dmabuf->scanout_height); > > if (dmabuf->allow_fences) { > vc->gfx.guest_fb.dmabuf = dmabuf; > -- > 2.34.1 > > > > > -- > Marc-André Lureau ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ui/gtk: set the area of the scanout texture correctly 2023-06-26 19:49 ` Kim, Dongwon @ 2023-07-04 16:07 ` Marc-André Lureau 2023-07-05 23:17 ` Kim, Dongwon 0 siblings, 1 reply; 5+ messages in thread From: Marc-André Lureau @ 2023-07-04 16:07 UTC (permalink / raw) To: Kim, Dongwon; +Cc: qemu-devel, Gerd Hoffmann, Vivek Kasireddy [-- Attachment #1: Type: text/plain, Size: 4418 bytes --] Hi On Mon, Jun 26, 2023 at 9:49 PM Kim, Dongwon <dongwon.kim@intel.com> wrote: > Hi Marc-André Lureau, > > On 6/26/2023 4:56 AM, Marc-André Lureau wrote: > > Hi > > > > On Wed, Jun 21, 2023 at 11:53 PM Dongwon Kim <dongwon.kim@intel.com> > > wrote: > > > > x and y offsets and width and height of the scanout texture > > is not correctly configured in case guest scanout frame is > > dmabuf. > > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > > > > > > I find this a bit confusing, and I don't know how to actually test it. > > > > The only place where scanout_{width, height} are set is > > virtio_gpu_create_dmabuf() and there, they have the same values as > > width and height. it's too easy to get confused with the values imho. > > Yes, scanout_width/height are same as width/height as far as there is > only one guest display exist. But they will be different in case there > multiple displays on the guest side, configured in extended mode (when > the guest is running Xorg). > > In this case, blob for the guest display is same for scanout 1 and 2 but > each scanout will have different offset and scanout_width/scanout_height > to reference a sub region in the same blob(dmabuf). > > I added x/y/scanout_width/scanout_height with a previous commit: > > commit e86a93f55463c088aa0b5260e915ffbf9f86c62b > Author: Dongwon Kim <dongwon.kim@intel.com> > Date: Wed Nov 3 23:51:52 2021 -0700 > > virtio-gpu: splitting one extended mode guest fb into n-scanouts > > > I find the terminology we use for ScanoutTexture much clearer. It uses > > backing_{width, height} instead, which indicates quite clearly that > > the usual x/y/w/h are for the sub-region to be shown. > yeah agreed. Then dmabuf->width/height should be changed to > dmabuf->backing_width/height and dmabuf->width/height will be replacing > dmabuf->scanout_width/scanout_height. I guess this is what you meant, > right? > right, can you send a new patch? thanks > > I think we should have a preliminary commit that renames > > scanout_{width, height}. > > > > Please give some help/hints on how to actually test this code too. > > So this patch is just to make things look consistent in the code level. > Having offset (0,0) in this function call for all different scanouts > didn't look right to me. This code change won't make anything done > differently though. So no test is applicable. > > > > > Thanks! > > > > > > --- > > ui/gtk-egl.c | 3 ++- > > ui/gtk-gl-area.c | 3 ++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c > > index 19130041bc..e99e3b0d8c 100644 > > --- a/ui/gtk-egl.c > > +++ b/ui/gtk-egl.c > > @@ -257,7 +257,8 @@ void > > gd_egl_scanout_dmabuf(DisplayChangeListener *dcl, > > > > gd_egl_scanout_texture(dcl, dmabuf->texture, > > dmabuf->y0_top, dmabuf->width, > > dmabuf->height, > > - 0, 0, dmabuf->width, dmabuf->height); > > + dmabuf->x, dmabuf->y, > > dmabuf->scanout_width, > > + dmabuf->scanout_height); > > > > if (dmabuf->allow_fences) { > > vc->gfx.guest_fb.dmabuf = dmabuf; > > diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c > > index c384a1516b..1605818bd1 100644 > > --- a/ui/gtk-gl-area.c > > +++ b/ui/gtk-gl-area.c > > @@ -299,7 +299,8 @@ void > > gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl, > > > > gd_gl_area_scanout_texture(dcl, dmabuf->texture, > > dmabuf->y0_top, dmabuf->width, > > dmabuf->height, > > - 0, 0, dmabuf->width, dmabuf->height); > > + dmabuf->x, dmabuf->y, > > dmabuf->scanout_width, > > + dmabuf->scanout_height); > > > > if (dmabuf->allow_fences) { > > vc->gfx.guest_fb.dmabuf = dmabuf; > > -- > > 2.34.1 > > > > > > > > > > -- > > Marc-André Lureau > -- Marc-André Lureau [-- Attachment #2: Type: text/html, Size: 6254 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ui/gtk: set the area of the scanout texture correctly 2023-07-04 16:07 ` Marc-André Lureau @ 2023-07-05 23:17 ` Kim, Dongwon 0 siblings, 0 replies; 5+ messages in thread From: Kim, Dongwon @ 2023-07-05 23:17 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, Gerd Hoffmann, Vivek Kasireddy On 7/4/2023 9:07 AM, Marc-André Lureau wrote: > Hi > > On Mon, Jun 26, 2023 at 9:49 PM Kim, Dongwon <dongwon.kim@intel.com> > wrote: > > Hi Marc-André Lureau, > > On 6/26/2023 4:56 AM, Marc-André Lureau wrote: > > Hi > > > > On Wed, Jun 21, 2023 at 11:53 PM Dongwon Kim > <dongwon.kim@intel.com> > > wrote: > > > > x and y offsets and width and height of the scanout texture > > is not correctly configured in case guest scanout frame is > > dmabuf. > > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > > > > > > I find this a bit confusing, and I don't know how to actually > test it. > > > > The only place where scanout_{width, height} are set is > > virtio_gpu_create_dmabuf() and there, they have the same values as > > width and height. it's too easy to get confused with the values > imho. > > Yes, scanout_width/height are same as width/height as far as there is > only one guest display exist. But they will be different in case > there > multiple displays on the guest side, configured in extended mode > (when > the guest is running Xorg). > > In this case, blob for the guest display is same for scanout 1 and > 2 but > each scanout will have different offset and > scanout_width/scanout_height > to reference a sub region in the same blob(dmabuf). > > I added x/y/scanout_width/scanout_height with a previous commit: > > commit e86a93f55463c088aa0b5260e915ffbf9f86c62b > Author: Dongwon Kim <dongwon.kim@intel.com> > Date: Wed Nov 3 23:51:52 2021 -0700 > > virtio-gpu: splitting one extended mode guest fb into n-scanouts > > > I find the terminology we use for ScanoutTexture much clearer. > It uses > > backing_{width, height} instead, which indicates quite clearly that > > the usual x/y/w/h are for the sub-region to be shown. > yeah agreed. Then dmabuf->width/height should be changed to > dmabuf->backing_width/height and dmabuf->width/height will be > replacing > dmabuf->scanout_width/scanout_height. I guess this is what you > meant, right? > > > right, can you send a new patch? > thanks https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg01081.html Thanks! > > > I think we should have a preliminary commit that renames > > scanout_{width, height}. > > > > Please give some help/hints on how to actually test this code too. > > So this patch is just to make things look consistent in the code > level. > Having offset (0,0) in this function call for all different scanouts > didn't look right to me. This code change won't make anything done > differently though. So no test is applicable. > > > > > Thanks! > > > > > > --- > > ui/gtk-egl.c | 3 ++- > > ui/gtk-gl-area.c | 3 ++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c > > index 19130041bc..e99e3b0d8c 100644 > > --- a/ui/gtk-egl.c > > +++ b/ui/gtk-egl.c > > @@ -257,7 +257,8 @@ void > > gd_egl_scanout_dmabuf(DisplayChangeListener *dcl, > > > > gd_egl_scanout_texture(dcl, dmabuf->texture, > > dmabuf->y0_top, dmabuf->width, > > dmabuf->height, > > - 0, 0, dmabuf->width, > dmabuf->height); > > + dmabuf->x, dmabuf->y, > > dmabuf->scanout_width, > > + dmabuf->scanout_height); > > > > if (dmabuf->allow_fences) { > > vc->gfx.guest_fb.dmabuf = dmabuf; > > diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c > > index c384a1516b..1605818bd1 100644 > > --- a/ui/gtk-gl-area.c > > +++ b/ui/gtk-gl-area.c > > @@ -299,7 +299,8 @@ void > > gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl, > > > > gd_gl_area_scanout_texture(dcl, dmabuf->texture, > > dmabuf->y0_top, dmabuf->width, > > dmabuf->height, > > - 0, 0, dmabuf->width, > dmabuf->height); > > + dmabuf->x, dmabuf->y, > > dmabuf->scanout_width, > > + dmabuf->scanout_height); > > > > if (dmabuf->allow_fences) { > > vc->gfx.guest_fb.dmabuf = dmabuf; > > -- > > 2.34.1 > > > > > > > > > > -- > > Marc-André Lureau > > > > -- > Marc-André Lureau ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-05 23:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-21 21:31 [PATCH] ui/gtk: set the area of the scanout texture correctly Dongwon Kim 2023-06-26 11:56 ` Marc-André Lureau 2023-06-26 19:49 ` Kim, Dongwon 2023-07-04 16:07 ` Marc-André Lureau 2023-07-05 23:17 ` Kim, Dongwon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).