[PATCH] drm/i915/gvt: Refine rang checking and info dump

[Zhang, Tina]

> -----Original Message-----
> From: Zhang, Tina
> Sent: Monday, December 18, 2017 3:11 PM
> To: Zhenyu Wang <zhenyuw at linux.intel.com>
> Cc: intel-gvt-dev at lists.freedesktop.org
> Subject: RE: [PATCH] drm/i915/gvt: Refine rang checking and info dump
> 
> 
> 
> > -----Original Message-----
> > From: intel-gvt-dev
> > [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On Behalf Of
> > Zhenyu Wang
> > Sent: Wednesday, December 13, 2017 2:34 PM
> > To: Zhang, Tina <tina.zhang at intel.com>
> > Cc: intel-gvt-dev at lists.freedesktop.org
> > Subject: Re: [PATCH] drm/i915/gvt: Refine rang checking and info dump
> >
> > On 2017.12.13 14:18:07 +0800, Tina Zhang wrote:
> > > The vgpu's GGTT range checking logic in each kind of plane decoder
> > > is dupicated with the one in vgpu_get_plane_info. Besides, during
> > > the guest updating display, the plane's base address could be zero,
> > > which might be a normal case without dumping errors.
> > >
> > > So this patch does the following things:
> > > - Remove the error log when guest framebuffer based address is zero;
> > > - Split the logic in intel_gvt_ggtt_validate_range and let the
> > >   fb_decoder to take care of the base address checking, so that such
> > >   kind of errors can return immediately with dumping logs;
> > > - Fix a typo in dumping base_gpa.
> > >
> > > Signed-off-by: Tina Zhang <tina.zhang at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gvt/dmabuf.c     | 24 +++++++++++++++++++-----
> > >  drivers/gpu/drm/i915/gvt/fb_decoder.c | 33
> > > +++++++++++++++++++++------------
> > >  2 files changed, 40 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> > > b/drivers/gpu/drm/i915/gvt/dmabuf.c
> > > index 2ab584f..9b7af30 100644
> > > --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> > > +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> > > @@ -245,6 +245,8 @@ static int vgpu_get_plane_info(struct drm_device
> > *dev,
> > >  	if (info->size == 0) {
> > >  		gvt_vgpu_err("fb size is zero\n");
> > >  		return -EINVAL;
> > > +	} else if (!vgpu_gmadr_is_valid(vgpu, info->start + info->size - 1)) {
> > > +		return -EFAULT;
> > >  	}
> > >
> > >  	if (info->start & (PAGE_SIZE - 1)) { @@ -257,11 +259,6 @@ static
> > > int vgpu_get_plane_info(struct drm_device *dev,
> > >  		return -EFAULT;
> > >  	}
> > >
> > > -	if (!intel_gvt_ggtt_validate_range(vgpu, info->start, info->size)) {
> > > -		gvt_vgpu_err("invalid gma addr\n");
> > > -		return -EFAULT;
> > > -	}
> > > -
> > >  	return 0;
> > >  }
> > >
> > > @@ -333,6 +330,22 @@ static void update_fb_info(struct
> > vfio_device_gfx_plane_info *gvt_dmabuf,
> > >  	gvt_dmabuf->y_hot = fb_info->y_hot;  }
> > >
> > > +static void dump_fb_info(struct intel_vgpu *vgpu, int type,
> > > +			 struct intel_vgpu_fb_info *fb_info) {
> > > +	gvt_dbg_dpy("vgpu%d: plane_info: type %d, start 0x%llx\n",
> > > +		    vgpu->id, type, fb_info->start);
> > > +	gvt_dbg_dpy("start_gpa 0x%llx, drm_format_mode 0x%llx\n",
> > > +		    fb_info->start_gpa, fb_info->drm_format_mod);
> > > +	gvt_dbg_dpy("drm_format 0x%x, width 0x%x, height 0x%x\n",
> > > +		    fb_info->drm_format, fb_info->width, fb_info->height);
> > > +	gvt_dbg_dpy("stride 0x%x, size 0x%x, x_pos 0x%x, y_pox 0x%x\n",
> > > +		    fb_info->stride, fb_info->size, fb_info->x_pos,
> > > +		    fb_info->y_pos);
> > > +	gvt_dbg_dpy("x_hot 0x%x, y_hot 0x%x\n", fb_info->x_hot,
> > > +		    fb_info->y_hot);
> > > +}
> > > +
> > >  int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args)  {
> > >  	struct drm_device *dev = &vgpu->gvt->dev_priv->drm; @@ -426,6
> > +439,7
> > > @@ int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args)
> > >
> > >  	gvt_dbg_dpy("vgpu%d: %s new dmabuf_obj ref %d, id %d\n", vgpu- id,
> > >  		    __func__, kref_read(&dmabuf_obj->kref), ret);
> > > +	dump_fb_info(vgpu, gfx_plane_info->drm_plane_type, &fb_info);
> > >
> > >  	return 0;
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > > b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > > index 72f4217..00bf9a1 100644
> > > --- a/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > > +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
> > > @@ -239,16 +239,21 @@ int intel_vgpu_decode_primary_plane(struct
> > intel_vgpu *vgpu,
> > >  	plane->hw_format = fmt;
> > >
> > >  	plane->base = vgpu_vreg(vgpu, DSPSURF(pipe)) &
> > I915_GTT_PAGE_MASK;
> > > -	if (!intel_gvt_ggtt_validate_range(vgpu, plane->base, 0)) {
> > > +	if (!plane->base) {
> > > +		/* Might be a normal case, especially during guest
> > > +		 * display updating
> > > +		 */
> > > +		return -ENODEV;
> > > +	} else if (!vgpu_gmadr_is_valid(vgpu, plane->base)) {
> > >  		gvt_vgpu_err("invalid gma address: %lx\n",
> > > -			     (unsigned long)plane->base);
> > > -		return  -EINVAL;
> > > +				     (unsigned long)plane->base);
> > > +		return -EINVAL;
> > >  	}
The invalid plane base is dumping here.
Thanks.

BR,
Tina
> > >
> > >  	plane->base_gpa = intel_vgpu_gma_to_gpa(vgpu->gtt.ggtt_mm,
> > plane->base);
> > >  	if (plane->base_gpa == INTEL_GVT_INVALID_ADDR) {
> > >  		gvt_vgpu_err("invalid gma address: %lx\n",
> > > -				(unsigned long)plane->base);
> > > +				(unsigned long)plane->base_gpa);
> >
> > I think to know the value of plane base has more meaning for failed
> > gpa? Or both?
> Yeah, dumping both is better. Thanks.
> 
> BR,
> Tina
> >
> > --
> > Open Source Technology Center, Intel ltd.
> >
> > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827