Hi
On Mon, Jun 26, 2023 at 9:49 PM Kim, Dongwon <dongwon....@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....@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 <kra...@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Cc: Vivek Kasireddy <vivek.kasire...@intel.com>
> > Signed-off-by: Dongwon Kim <dongwon....@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....@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
