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

[Zhiyuan Lv <zhiyuan.lv@intel.com>]

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 <yu.c.zhang@intel.com>
> > > > 
> > > > 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 <yu.c.zhang@intel.com>
> > > > Signed-off-by: Jike Song <jike.song@intel.com>
> > > > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > > > ---
> > > >  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
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx