virtio-gpu: update done only on the scanout associated with rect

[PATCH] virtio-gpu: update done only on the scanout associated with rect

Posted by Dongwon Kim 2 years, 11 months ago

It only needs to update the scanouts containing the rect area
coming with the resource-flush request from the guest.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 hw/display/virtio-gpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 529b5246b2..165ecafd7a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -514,6 +514,9 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
         for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
             scanout = &g->parent_obj.scanout[i];
             if (scanout->resource_id == res->resource_id &&
+                rf.r.x >= scanout->x && rf.r.y >= scanout->y &&
+                rf.r.x + rf.r.width <= scanout->x + scanout->width &&
+                rf.r.y + rf.r.height <= scanout->y + scanout->height &&
                 console_has_gl(scanout->con)) {
                 dpy_gl_update(scanout->con, 0, 0, scanout->width,
                               scanout->height);
-- 
2.30.2

Re: [PATCH] virtio-gpu: update done only on the scanout associated with rect

Posted by Marc-André Lureau 2 years, 11 months ago

Hi

On Fri, May 6, 2022 at 1:46 AM Dongwon Kim <dongwon.kim@intel.com> wrote:

> It only needs to update the scanouts containing the rect area
> coming with the resource-flush request from the guest.
>
>
Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  hw/display/virtio-gpu.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 529b5246b2..165ecafd7a 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -514,6 +514,9 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>          for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
>              scanout = &g->parent_obj.scanout[i];
>              if (scanout->resource_id == res->resource_id &&
> +                rf.r.x >= scanout->x && rf.r.y >= scanout->y &&
> +                rf.r.x + rf.r.width <= scanout->x + scanout->width &&
> +                rf.r.y + rf.r.height <= scanout->y + scanout->height &&
>


That doesn't seem to handle intersections/overlapping, I think it should.


>                  console_has_gl(scanout->con)) {
>                  dpy_gl_update(scanout->con, 0, 0, scanout->width,
>                                scanout->height);
> --
> 2.30.2
>
>
>

-- 
Marc-André Lureau

Re: [PATCH] virtio-gpu: update done only on the scanout associated with rect

Posted by Dongwon Kim 2 years, 11 months ago

On Fri, May 06, 2022 at 11:53:22AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, May 6, 2022 at 1:46 AM Dongwon Kim <dongwon.kim@intel.com> wrote:
> 
> > It only needs to update the scanouts containing the rect area
> > coming with the resource-flush request from the guest.
> >
> >
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> > ---
> >  hw/display/virtio-gpu.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 529b5246b2..165ecafd7a 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -514,6 +514,9 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> >          for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> >              scanout = &g->parent_obj.scanout[i];
> >              if (scanout->resource_id == res->resource_id &&
> > +                rf.r.x >= scanout->x && rf.r.y >= scanout->y &&
> > +                rf.r.x + rf.r.width <= scanout->x + scanout->width &&
> > +                rf.r.y + rf.r.height <= scanout->y + scanout->height &&
> >
> 
> 
> That doesn't seem to handle intersections/overlapping, I think it should.

so set-scanouts and resource flushes are issued per scanout(CRTC/plane
from guest's point of view). In case of intersections/overlapping, there
will be two resource flushes (in case there are two scanouts) and each
resource flush will take care of updating the scanout that covers
partial damaged area.

The problem with the original code is that even if there is a resource
flush request for a single scanout, it does update on both (or more) as
well, which is unnecessary burden.

Thanks

> 
> 
> >                  console_has_gl(scanout->con)) {
> >                  dpy_gl_update(scanout->con, 0, 0, scanout->width,
> >                                scanout->height);
> > --
> > 2.30.2
> >
> >
> >
> 
> -- 
> Marc-André Lureau

Re: [PATCH] virtio-gpu: update done only on the scanout associated with rect

Posted by Gerd Hoffmann 2 years, 8 months ago

On Fri, May 06, 2022 at 10:09:30AM -0700, Dongwon Kim wrote:
> On Fri, May 06, 2022 at 11:53:22AM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Fri, May 6, 2022 at 1:46 AM Dongwon Kim <dongwon.kim@intel.com> wrote:
> > 
> > > It only needs to update the scanouts containing the rect area
> > > coming with the resource-flush request from the guest.
> > >
> > >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> > > ---
> > >  hw/display/virtio-gpu.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > > index 529b5246b2..165ecafd7a 100644
> > > --- a/hw/display/virtio-gpu.c
> > > +++ b/hw/display/virtio-gpu.c
> > > @@ -514,6 +514,9 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> > >          for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> > >              scanout = &g->parent_obj.scanout[i];
> > >              if (scanout->resource_id == res->resource_id &&
> > > +                rf.r.x >= scanout->x && rf.r.y >= scanout->y &&
> > > +                rf.r.x + rf.r.width <= scanout->x + scanout->width &&
> > > +                rf.r.y + rf.r.height <= scanout->y + scanout->height &&
> > >
> > 
> > 
> > That doesn't seem to handle intersections/overlapping, I think it should.
> 
> so set-scanouts and resource flushes are issued per scanout(CRTC/plane
> from guest's point of view). In case of intersections/overlapping, there
> will be two resource flushes (in case there are two scanouts) and each
> resource flush will take care of updating the scanout that covers
> partial damaged area.

Even though the linux kernel driver sends two flushes, one for each
scanout, it is perfectly valid send a single flush for the complete
resource.

So checking whenever the rectangle is completely within the scanout is
not correct.  When the scanout is covered partly you must update too.
Only when the rectangle is completely outside the scanout it is valid to
skip it.

take care,
  Gerd

Re: [PATCH] virtio-gpu: update done only on the scanout associated with rect

Posted by Dongwon Kim 2 years, 8 months ago

On Tue, Jul 19, 2022 at 01:15:26PM +0200, Gerd Hoffmann wrote:
> On Fri, May 06, 2022 at 10:09:30AM -0700, Dongwon Kim wrote:
> > On Fri, May 06, 2022 at 11:53:22AM +0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > On Fri, May 6, 2022 at 1:46 AM Dongwon Kim <dongwon.kim@intel.com> wrote:
> > > 
> > > > It only needs to update the scanouts containing the rect area
> > > > coming with the resource-flush request from the guest.
> > > >
> > > >
> > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> > > > ---
> > > >  hw/display/virtio-gpu.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > > > index 529b5246b2..165ecafd7a 100644
> > > > --- a/hw/display/virtio-gpu.c
> > > > +++ b/hw/display/virtio-gpu.c
> > > > @@ -514,6 +514,9 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> > > >          for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> > > >              scanout = &g->parent_obj.scanout[i];
> > > >              if (scanout->resource_id == res->resource_id &&
> > > > +                rf.r.x >= scanout->x && rf.r.y >= scanout->y &&
> > > > +                rf.r.x + rf.r.width <= scanout->x + scanout->width &&
> > > > +                rf.r.y + rf.r.height <= scanout->y + scanout->height &&
> > > >
> > > 
> > > 
> > > That doesn't seem to handle intersections/overlapping, I think it should.
> > 
> > so set-scanouts and resource flushes are issued per scanout(CRTC/plane
> > from guest's point of view). In case of intersections/overlapping, there
> > will be two resource flushes (in case there are two scanouts) and each
> > resource flush will take care of updating the scanout that covers
> > partial damaged area.
> 
> Even though the linux kernel driver sends two flushes, one for each
> scanout, it is perfectly valid send a single flush for the complete
> resource.
> 
> So checking whenever the rectangle is completely within the scanout is
> not correct.  When the scanout is covered partly you must update too.
> Only when the rectangle is completely outside the scanout it is valid to
> skip it.

Gerd,

I got your point. I will take a look into it.

> 
> take care,
>   Gerd
>