[5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

Commit Message

Jike Song Sept. 19, 2014, 6:47 p.m. UTC

From: Yu Zhang <[e-mail]>

Display switch logic is added to notify the vgt mediator that
current vgpu have a valid surface to show. It does so by writing
the display_ready field in PV INFO page, and then will be handled
in vgt mediator. This is useful to avoid trickiness when the VM's
framebuffer is being accessed in the middle of VM modesetting,
e.g. compositing the framebuffer in the host side

Signed-off-by: Yu Zhang <[e-mail]>
Signed-off-by: Jike Song <[e-mail]>
Signed-off-by: Zhiyuan Lv <[e-mail]>
---
 drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
 drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 3 files changed, 25 insertions(+)

Comments

Chris Wilson Sept. 19, 2014, 8:14 a.m. UTC | #1

On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> From: Yu Zhang <[e-mail]>
> 
> Display switch logic is added to notify the vgt mediator that
> current vgpu have a valid surface to show. It does so by writing
> the display_ready field in PV INFO page, and then will be handled
> in vgt mediator. This is useful to avoid trickiness when the VM's
> framebuffer is being accessed in the middle of VM modesetting,
> e.g. compositing the framebuffer in the host side
> 
> Signed-off-by: Yu Zhang <[e-mail]>
> Signed-off-by: Jike Song <[e-mail]>
> Signed-off-by: Zhiyuan Lv <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index bb4ad52..d9462e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	} else {
>  		/* Start out suspended in ums mode. */
>  		dev_priv->ums.mm_suspended = 1;
> +		if (intel_vgpu_active(dev)) {
> +			/*
> +			 * Notify a valid surface after modesetting,
> +			 * when running inside a VM.
> +			 */
> +			I915_WRITE(vgt_info_off(display_ready),
> +				VGT_DRV_DISPLAY_READY);

That's interesting. As i915.ko is definitely not ready at this point.

> +		}
>  	}
>  
>  	i915_setup_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a70f12e..38d606c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6673,5 +6673,12 @@ enum punit_power_well {
>  #define vgt_info_off(x) \
>  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>  
> +/*
> + * The information set by the guest gfx driver, through the display_ready field
> + */
> +enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> +};
>  
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b78f00a..3af9259 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			intel_modeset_check_state(set->crtc->dev);
>  	}
>  
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);

Should there not be some coordination before we start the modeset?
-Chris

Wang, Zhi A Sept. 19, 2014, 11:37 a.m. UTC | #2

Hi Chris:
    Thanks for the comment. See my comments below.

-----Original Message-----
From: Intel-gfx [mailto:[e-mail]] On Behalf Of Chris Wilson
Sent: Friday, September 19, 2014 4:15 PM
To: Song, Jike
Cc: Vetter, Daniel; [e-mail]
Subject: Re: [Intel-gfx] [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> From: Yu Zhang <[e-mail]>
> 
> Display switch logic is added to notify the vgt mediator that current 
> vgpu have a valid surface to show. It does so by writing the 
> display_ready field in PV INFO page, and then will be handled in vgt 
> mediator. This is useful to avoid trickiness when the VM's framebuffer 
> is being accessed in the middle of VM modesetting, e.g. compositing 
> the framebuffer in the host side
> 
> Signed-off-by: Yu Zhang <[e-mail]>
> Signed-off-by: Jike Song <[e-mail]>
> Signed-off-by: Zhiyuan Lv <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c 
> b/drivers/gpu/drm/i915/i915_dma.c index bb4ad52..d9462e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	} else {
>  		/* Start out suspended in ums mode. */
>  		dev_priv->ums.mm_suspended = 1;
> +		if (intel_vgpu_active(dev)) {
> +			/*
> +			 * Notify a valid surface after modesetting,
> +			 * when running inside a VM.
> +			 */
> +			I915_WRITE(vgt_info_off(display_ready),
> +				VGT_DRV_DISPLAY_READY);

That's interesting. As i915.ko is definitely not ready at this point.

Zhi: Will remove this piece in V2, as we have already had a similar logic in intel_crtc_set_config.

> +		}
>  	}
>  
>  	i915_setup_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> b/drivers/gpu/drm/i915/i915_reg.h index a70f12e..38d606c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6673,5 +6673,12 @@ enum punit_power_well {  #define 
> vgt_info_off(x) \
>  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>  
> +/*
> + * The information set by the guest gfx driver, through the 
> +display_ready field  */ enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> +};
>  
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b78f00a..3af9259 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			intel_modeset_check_state(set->crtc->dev);
>  	}
>  
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);

Should there not be some coordination before we start the modeset?

Zhi: In the design of GVT-g, there will be only one VM can occupy the display monitor at one time,
which is called "Foreground VM". Switching foreground VM is implemented by switching only a part
of display registers e.g. DSPCNTR/DSPSURF/DISPSTRIDE/DSPLINOFF/DSPTILEOFF. Other display 
mode set registers are virtualized to VM. So we may not care about the beginning of a modeset
sequence here.

But we really expect to know when a VM can be switched to foreground, which is, VM has
already configured those registers to valid values. If we switch a VM without valid values in those
registers to foreground e.g. during VM booting time, I'm afraid that's not what we expect...

-Chris

--
Chris Wilson, Intel Open Source Technology Centre _______________________________________________
Intel-gfx mailing list
[e-mail]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 19, 2014, 4:09 p.m. UTC | #3

On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> From: Yu Zhang <[e-mail]>
> 
> Display switch logic is added to notify the vgt mediator that
> current vgpu have a valid surface to show. It does so by writing
> the display_ready field in PV INFO page, and then will be handled
> in vgt mediator. This is useful to avoid trickiness when the VM's
> framebuffer is being accessed in the middle of VM modesetting,
> e.g. compositing the framebuffer in the host side
> 
> Signed-off-by: Yu Zhang <[e-mail]>
> Signed-off-by: Jike Song <[e-mail]>
> Signed-off-by: Zhiyuan Lv <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index bb4ad52..d9462e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	} else {
>  		/* Start out suspended in ums mode. */
>  		dev_priv->ums.mm_suspended = 1;
> +		if (intel_vgpu_active(dev)) {
> +			/*
> +			 * Notify a valid surface after modesetting,
> +			 * when running inside a VM.
> +			 */
> +			I915_WRITE(vgt_info_off(display_ready),
> +				VGT_DRV_DISPLAY_READY);
> +		}
>  	}
>  
>  	i915_setup_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a70f12e..38d606c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6673,5 +6673,12 @@ enum punit_power_well {
>  #define vgt_info_off(x) \
>  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>  
> +/*
> + * The information set by the guest gfx driver, through the display_ready field
> + */
> +enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> +};
>  
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b78f00a..3af9259 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			intel_modeset_check_state(set->crtc->dev);
>  	}
>  
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> +	}

Since very shortly we now have the frontbuffer tracking support code.
Should we move it there? See intel_frontbuffer_flush&flip.
-Daniel

> +
>  	if (ret) {
>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
>  			      set->crtc->base.id, ret);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zhiyuan Lv Sept. 29, 2014, 6:31 a.m. UTC | #4

Hi Daniel,

On Fri, Sep 19, 2014 at 06:09:37PM +0200, Daniel Vetter wrote:
> On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> > From: Yu Zhang <[e-mail]>
> > 
> > Display switch logic is added to notify the vgt mediator that
> > current vgpu have a valid surface to show. It does so by writing
> > the display_ready field in PV INFO page, and then will be handled
> > in vgt mediator. This is useful to avoid trickiness when the VM's
> > framebuffer is being accessed in the middle of VM modesetting,
> > e.g. compositing the framebuffer in the host side
> > 
> > Signed-off-by: Yu Zhang <[e-mail]>
> > Signed-off-by: Jike Song <[e-mail]>
> > Signed-off-by: Zhiyuan Lv <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
> >  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
> >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index bb4ad52..d9462e1 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	} else {
> >  		/* Start out suspended in ums mode. */
> >  		dev_priv->ums.mm_suspended = 1;
> > +		if (intel_vgpu_active(dev)) {
> > +			/*
> > +			 * Notify a valid surface after modesetting,
> > +			 * when running inside a VM.
> > +			 */
> > +			I915_WRITE(vgt_info_off(display_ready),
> > +				VGT_DRV_DISPLAY_READY);
> > +		}
> >  	}
> >  
> >  	i915_setup_sysfs(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a70f12e..38d606c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6673,5 +6673,12 @@ enum punit_power_well {
> >  #define vgt_info_off(x) \
> >  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> >  
> > +/*
> > + * The information set by the guest gfx driver, through the display_ready field
> > + */
> > +enum vgt_display_status {
> > +	VGT_DRV_DISPLAY_NOT_READY = 0,
> > +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> > +};
> >  
> >  #endif /* _I915_REG_H_ */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b78f00a..3af9259 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  			intel_modeset_check_state(set->crtc->dev);
> >  	}
> >  
> > +	if (intel_vgpu_active(dev)) {
> > +		/*
> > +		 * Notify a valid surface after modesetting,
> > +		 * when running inside a VM.
> > +		 */
> > +		struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> > +	}
> 
> Since very shortly we now have the frontbuffer tracking support code.
> Should we move it there? See intel_frontbuffer_flush&flip.

Thank you very much for the comments and sorry for the delayed reply!

For virtual machines we only need such notification once for guest system
booting, would that be too heavy to add code into the flush&flip function,
consider that they are to be called many times?

Going forward, we want to use the same "display_ready" for i915 running in host
machines so that we can capture display status changes. Would that be good to
keep the code in "intel_crtc_set_config"? Appreciate your inputs. Thanks!

Regards,
-Zhiyuan

> -Daniel
> 
> > +
> >  	if (ret) {
> >  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> >  			      set->crtc->base.id, ret);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 29, 2014, 12:30 p.m. UTC | #5

On Mon, Sep 29, 2014 at 02:31:17PM +0800, Zhiyuan Lv wrote:
> Hi Daniel,
> 
> On Fri, Sep 19, 2014 at 06:09:37PM +0200, Daniel Vetter wrote:
> > On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> > > From: Yu Zhang <[e-mail]>
> > > 
> > > Display switch logic is added to notify the vgt mediator that
> > > current vgpu have a valid surface to show. It does so by writing
> > > the display_ready field in PV INFO page, and then will be handled
> > > in vgt mediator. This is useful to avoid trickiness when the VM's
> > > framebuffer is being accessed in the middle of VM modesetting,
> > > e.g. compositing the framebuffer in the host side
> > > 
> > > Signed-off-by: Yu Zhang <[e-mail]>
> > > Signed-off-by: Jike Song <[e-mail]>
> > > Signed-off-by: Zhiyuan Lv <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
> > >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
> > >  3 files changed, 25 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index bb4ad52..d9462e1 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > >  	} else {
> > >  		/* Start out suspended in ums mode. */
> > >  		dev_priv->ums.mm_suspended = 1;
> > > +		if (intel_vgpu_active(dev)) {
> > > +			/*
> > > +			 * Notify a valid surface after modesetting,
> > > +			 * when running inside a VM.
> > > +			 */
> > > +			I915_WRITE(vgt_info_off(display_ready),
> > > +				VGT_DRV_DISPLAY_READY);
> > > +		}
> > >  	}
> > >  
> > >  	i915_setup_sysfs(dev);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index a70f12e..38d606c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6673,5 +6673,12 @@ enum punit_power_well {
> > >  #define vgt_info_off(x) \
> > >  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> > >  
> > > +/*
> > > + * The information set by the guest gfx driver, through the display_ready field
> > > + */
> > > +enum vgt_display_status {
> > > +	VGT_DRV_DISPLAY_NOT_READY = 0,
> > > +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> > > +};
> > >  
> > >  #endif /* _I915_REG_H_ */
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index b78f00a..3af9259 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> > >  			intel_modeset_check_state(set->crtc->dev);
> > >  	}
> > >  
> > > +	if (intel_vgpu_active(dev)) {
> > > +		/*
> > > +		 * Notify a valid surface after modesetting,
> > > +		 * when running inside a VM.
> > > +		 */
> > > +		struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> > > +	}
> > 
> > Since very shortly we now have the frontbuffer tracking support code.
> > Should we move it there? See intel_frontbuffer_flush&flip.
> 
> Thank you very much for the comments and sorry for the delayed reply!
> 
> For virtual machines we only need such notification once for guest system
> booting, would that be too heavy to add code into the flush&flip function,
> consider that they are to be called many times?
> 
> Going forward, we want to use the same "display_ready" for i915 running in host
> machines so that we can capture display status changes. Would that be good to
> keep the code in "intel_crtc_set_config"? Appreciate your inputs. Thanks!

I guess the question is what exactly you want to signal to the hyporvisor
with this. I guess I need to dig a bit into the sourcecode for the
hyperviros part, and you need to make a really clear specification of when
guests should call this and what the hyporvisor must to in reaction of
this.

I don't think we'll have any issues with a bit of overhead in the
frontbuffer flip/flush functions, they're not called too often. Aside:
There's no nice kerneldoc for this stuff:

http://people.freedesktop.org/~danvet/drm/drmI915.html#idp54709056

Cheers, Daniel

> 
> Regards,
> -Zhiyuan
> 
> > -Daniel
> > 
> > > +
> > >  	if (ret) {
> > >  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> > >  			      set->crtc->base.id, ret);
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > [phone] - http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zhiyuan Lv Sept. 30, 2014, 10:25 a.m. UTC | #6

Hi Daniel,

On Mon, Sep 29, 2014 at 02:30:09PM +0200, Daniel Vetter wrote:
> On Mon, Sep 29, 2014 at 02:31:17PM +0800, Zhiyuan Lv wrote:
> > Hi Daniel,
> > 
> > On Fri, Sep 19, 2014 at 06:09:37PM +0200, Daniel Vetter wrote:
> > > On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> > > > From: Yu Zhang <[e-mail]>
> > > > 
> > > > Display switch logic is added to notify the vgt mediator that
> > > > current vgpu have a valid surface to show. It does so by writing
> > > > the display_ready field in PV INFO page, and then will be handled
> > > > in vgt mediator. This is useful to avoid trickiness when the VM's
> > > > framebuffer is being accessed in the middle of VM modesetting,
> > > > e.g. compositing the framebuffer in the host side
> > > > 
> > > > Signed-off-by: Yu Zhang <[e-mail]>
> > > > Signed-off-by: Jike Song <[e-mail]>
> > > > Signed-off-by: Zhiyuan Lv <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
> > > >  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
> > > >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
> > > >  3 files changed, 25 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > index bb4ad52..d9462e1 100644
> > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > > >  	} else {
> > > >  		/* Start out suspended in ums mode. */
> > > >  		dev_priv->ums.mm_suspended = 1;
> > > > +		if (intel_vgpu_active(dev)) {
> > > > +			/*
> > > > +			 * Notify a valid surface after modesetting,
> > > > +			 * when running inside a VM.
> > > > +			 */
> > > > +			I915_WRITE(vgt_info_off(display_ready),
> > > > +				VGT_DRV_DISPLAY_READY);
> > > > +		}
> > > >  	}
> > > >  
> > > >  	i915_setup_sysfs(dev);
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index a70f12e..38d606c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6673,5 +6673,12 @@ enum punit_power_well {
> > > >  #define vgt_info_off(x) \
> > > >  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> > > >  
> > > > +/*
> > > > + * The information set by the guest gfx driver, through the display_ready field
> > > > + */
> > > > +enum vgt_display_status {
> > > > +	VGT_DRV_DISPLAY_NOT_READY = 0,
> > > > +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> > > > +};
> > > >  
> > > >  #endif /* _I915_REG_H_ */
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index b78f00a..3af9259 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> > > >  			intel_modeset_check_state(set->crtc->dev);
> > > >  	}
> > > >  
> > > > +	if (intel_vgpu_active(dev)) {
> > > > +		/*
> > > > +		 * Notify a valid surface after modesetting,
> > > > +		 * when running inside a VM.
> > > > +		 */
> > > > +		struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +
> > > > +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> > > > +	}
> > > 
> > > Since very shortly we now have the frontbuffer tracking support code.
> > > Should we move it there? See intel_frontbuffer_flush&flip.
> > 
> > Thank you very much for the comments and sorry for the delayed reply!
> > 
> > For virtual machines we only need such notification once for guest system
> > booting, would that be too heavy to add code into the flush&flip function,
> > consider that they are to be called many times?
> > 
> > Going forward, we want to use the same "display_ready" for i915 running in host
> > machines so that we can capture display status changes. Would that be good to
> > keep the code in "intel_crtc_set_config"? Appreciate your inputs. Thanks!
> 
> I guess the question is what exactly you want to signal to the hyporvisor
> with this. I guess I need to dig a bit into the sourcecode for the
> hyperviros part, and you need to make a really clear specification of when
> guests should call this and what the hyporvisor must to in reaction of
> this.

In this patchset for i915 driver running inside a virtual machine, the code
here is to signal vgt that guest framebuffer address in plane surface register
is ready to be written into physical registers. vgt's reaction is to make
policy change. After that point, VM's write to surface MMIO will go into
physical register directly. Here we do not need notifications for every guest
display surface MMIO write, but just the time we can do the policy change.

The usage of this is for guest system booting. At the very beginning, we block
the guest surface MMIO writes so that the monitor still shows up host
contents, then we rely on this notification to show up guest VM's screen.

We have another change for i915 driver to notify vgt the modesetting. But that
is for i915 running as a host driver. Sorry that it may be confusing to mix
two things together. This can be discussed later.

> 
> I don't think we'll have any issues with a bit of overhead in the
> frontbuffer flip/flush functions, they're not called too often. Aside:
> There's no nice kerneldoc for this stuff:
> 
> http://people.freedesktop.org/~danvet/drm/drmI915.html#idp54709056
>

Thanks for the info! The doc is helpful for me to understand the new feature.
Since intel_frontbuffer_flush() will be called every time there is page flip
request, that may not be what we want: just one time notification to do the
policy change. Thanks!

Regards,
-Zhiyuan

> Cheers, Daniel
> 
> > 
> > Regards,
> > -Zhiyuan
> > 
> > > -Daniel
> > > 
> > > > +
> > > >  	if (ret) {
> > > >  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> > > >  			      set->crtc->base.id, ret);
> > > > -- 
> > > > 1.9.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > [e-mail]
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > [phone] - http://blog.ffwll.ch
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 30, 2014, 10:56 a.m. UTC | #7

On Tue, Sep 30, 2014 at 06:25:26PM +0800, Zhiyuan Lv wrote:
> On Mon, Sep 29, 2014 at 02:30:09PM +0200, Daniel Vetter wrote:
> > I guess the question is what exactly you want to signal to the hyporvisor
> > with this. I guess I need to dig a bit into the sourcecode for the
> > hyperviros part, and you need to make a really clear specification of when
> > guests should call this and what the hyporvisor must to in reaction of
> > this.
> 
> In this patchset for i915 driver running inside a virtual machine, the code
> here is to signal vgt that guest framebuffer address in plane surface register
> is ready to be written into physical registers. vgt's reaction is to make
> policy change. After that point, VM's write to surface MMIO will go into
> physical register directly. Here we do not need notifications for every guest
> display surface MMIO write, but just the time we can do the policy change.
> 
> The usage of this is for guest system booting. At the very beginning, we block
> the guest surface MMIO writes so that the monitor still shows up host
> contents, then we rely on this notification to show up guest VM's screen.

This sounds more like we need one call at the end of the i915 driver load
sequence to signal to the hypervisor that everything is now down and a
proper driver is in charge of the virtual machine gfx.

> We have another change for i915 driver to notify vgt the modesetting. But that
> is for i915 running as a host driver. Sorry that it may be confusing to mix
> two things together. This can be discussed later.

Yeah, better split that out and move it to the host-side vgt integration
series.

> > I don't think we'll have any issues with a bit of overhead in the
> > frontbuffer flip/flush functions, they're not called too often. Aside:
> > There's no nice kerneldoc for this stuff:
> > 
> > http://people.freedesktop.org/~danvet/drm/drmI915.html#idp54709056
> >
> 
> Thanks for the info! The doc is helpful for me to understand the new feature.
> Since intel_frontbuffer_flush() will be called every time there is page flip
> request, that may not be what we want: just one time notification to do the
> policy change. Thanks!

Always nice when my documentation work proves useful ;-)

Cheers, Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index bb4ad52..d9462e1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1790,6 +1790,14 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	} else {
 		/* Start out suspended in ums mode. */
 		dev_priv->ums.mm_suspended = 1;
+		if (intel_vgpu_active(dev)) {
+			/*
+			 * Notify a valid surface after modesetting,
+			 * when running inside a VM.
+			 */
+			I915_WRITE(vgt_info_off(display_ready),
+				VGT_DRV_DISPLAY_READY);
+		}
 	}
 
 	i915_setup_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a70f12e..38d606c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6673,5 +6673,12 @@  enum punit_power_well {
 #define vgt_info_off(x) \
 	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
 
+/*
+ * The information set by the guest gfx driver, through the display_ready field
+ */
+enum vgt_display_status {
+	VGT_DRV_DISPLAY_NOT_READY = 0,
+	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
+};
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b78f00a..3af9259 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11642,6 +11642,16 @@  static int intel_crtc_set_config(struct drm_mode_set *set)
 			intel_modeset_check_state(set->crtc->dev);
 	}
 
+	if (intel_vgpu_active(dev)) {
+		/*
+		 * Notify a valid surface after modesetting,
+		 * when running inside a VM.
+		 */
+		struct drm_i915_private *dev_priv = dev->dev_private;
+
+		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
+	}
+
 	if (ret) {
 		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
 			      set->crtc->base.id, ret);