Re: [Intel-gfx] [PATCH v9 2/3] drm/i915: Enable guest i915 full ppgtt functionality

[Zhenyu Wang]

On 2017.08.11 12:46:48 +0300, Joonas Lahtinen wrote:
> 
> > @@ -141,14 +141,19 @@ int intel_sanitize_enable_ppgtt(struct 
> > drm_i915_private *dev_priv,
> >  
> >     has_aliasing_ppgtt = dev_priv->info.has_aliasing_ppgtt;
> >     has_full_ppgtt = dev_priv->info.has_full_ppgtt;
> > -   has_full_48bit_ppgtt = dev_priv->info.has_full_48bit_ppgtt;
> 
>   Leave above line.
> 
> >  
> >     if (intel_vgpu_active(dev_priv)) {
> > -   /* emulation is too hard */
> > -   has_full_ppgtt = false;
> > -   has_full_48bit_ppgtt = false;
> 
>   has_full_ppgtt = false;
>   has_full_48bit_ppgtt =
>   intel_vgpu_has_full_48bit_ppgtt(dev_priv);
> 
> > +   has_full_ppgtt = intel_vgpu_has_full_ppgtt(dev_priv);
> > +   /* GVT-g has no support for 32bit ppgtt */
> > +   if (enable_ppgtt == 2 && has_full_ppgtt) {
> > +   DRM_DEBUG_DRIVER("Force 48bit ppgtt for vGPU\n");
> > +   enable_ppgtt = 3;
> 
> Lets not do an upgrade for the user if they explicitly requested lower
> level. I'd just leave the option to be and it'll get downgraded to
> aliasing ppgtt.
>

If user specified enable_ppgtt=2, it will get enable_ppgtt=2 instead of
aliasing ppgtt, so we try to guard against that for current 48bit ppgtt
support on vgpu. Well either upgrade to full 48bit or downgrade to aliasing.

> > +   }
> >     }
> >  
> > +   has_full_48bit_ppgtt = has_full_ppgtt &&
> > +   dev_priv->info.has_full_48bit_ppgtt;
> > +
> 
> This can then be dropped too.
> 
> I'll send a patch to disconnect the has_full_ppgtt and
> has_full_48bit_ppgtt flags.
>

That'll be good. As we have validated this full ppgtt on vgpu for
quite some time now and plan to push for 4.14. Are you able to
align with that? So I might send first 4.14 pull without this series,
then add them later for dinf.


> 
> >     if (!has_aliasing_ppgtt)
> >     return 0;
> >  
> > @@ -49,12 +49,18 @@ enum vgt_g2v_type {
> > >   VGT_G2V_MAX,
> >  };
> >  
> > +/*
> > + * VGT capabilities type
> > + */
> > +#define VGT_CAPS_FULL_PPGTT_48BIT  BIT(2)
> 
> VGT_CAPS_FULL_48BIT_PPGTT for consistency
> 
> > @@ -75,10 +75,17 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv)
> >     return;
> >     }
> >  
> > +   dev_priv->vgpu.caps = __raw_i915_read32(dev_priv, vgtif_reg(vgt_caps));
> > +
> >     dev_priv->vgpu.active = true;
> >     DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
> >  }
> >  
> > +bool intel_vgpu_has_full_ppgtt(struct drm_i915_private *dev_priv)
> 
> This should really be has_full_48bit_ppgtt() too.
> 
> Regards, Joonas
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v9 2/3] drm/i915: Enable guest i915 full ppgtt functionality

[Joonas Lahtinen]

On to, 2017-08-10 at 07:41 +0800, Tina Zhang wrote:
> Enable the guest i915 full ppgtt functionality when host can provide this
> capability. vgt_caps is introduced to guest i915 driver to get the vgpu
> capabilities from the device model. VGT_CPAS_FULL_PPGTT is one of the
> capabilities type to let guest i915 dirver know that the guest i915 full
> ppgtt is supported by device model.
> 
> Notice that the minor version of pvinfo isn't bumped because of this
> vgt_caps introduction, due to older guest would be broken by simply
> increasing the pvinfo version. Although the pvinfo minor version doesn't
> increase, the compatibility won't be blocked. The compatibility is ensured
> by checking the value of caps field in pvinfo. Zero means no full ppgtt
> support and BIT(2) means this feature is provided.
> 
> Changes since v1:
> - Use u32 instead of uint32_t (Joonas)
> - Move VGT_CAPS_FULL_PPGTT introduction to this patch and use #define
>   instead of enum (Joonas)
> - Rewrite the vgpu full ppgtt capability checking logic. (Joonas)
> - Some coding style refine. (Joonas)
> 
> Changes since v2:
> - Divide the whole patch set into two separate patch series, with one
>   patch in i915 side to check guest i915 full ppgtt capability and enable
>   it when this capability is supported by the device model, and the other
>   one in gvt side which fixs the blocking issue and enables the device
>   model to provide the capability to guest. And this patch focuses on guest
>   i915 side. (Joonas)
> - Change the title from "introduce vgt_caps to pvinfo" to
>   "Enable guest i915 full ppgtt functionality". (Tina)
> 
> Change since v3:
> - Add some comments about pvinfo caps and version. (Joonas)
> 
> Change since v4:
> - Tested by Tina Zhang.
> 
> Change since v5:
> - Add limitation about supporting 32bit full ppgtt.
> 
> Change since v6:
> - Change the fallback to 48bit full ppgtt if i915.ppgtt_enable=2. (Zhenyu)
> 
> Change in v9:
> - Remove the fixme comment due to no plan for 32bit full ppgtt
>   support. (Zhenyu)
> - Reorder the patch-set to fix compiling issue with git-bisect. (Zhenyu)
> - Add print log when forcing guest 48bit full ppgtt. (Zhenyu)
> 
> Reviewed-by: Joonas Lahtinen  in v2
> Signed-off-by: Tina Zhang 



> @@ -141,14 +141,19 @@ int intel_sanitize_enable_ppgtt(struct drm_i915_private 
> *dev_priv,
>  
>   has_aliasing_ppgtt = dev_priv->info.has_aliasing_ppgtt;
>   has_full_ppgtt = dev_priv->info.has_full_ppgtt;
> - has_full_48bit_ppgtt = dev_priv->info.has_full_48bit_ppgtt;

Leave above line.

>  
>   if (intel_vgpu_active(dev_priv)) {
> - /* emulation is too hard */
> - has_full_ppgtt = false;
> - has_full_48bit_ppgtt = false;

has_full_ppgtt = false;
has_full_48bit_ppgtt =
intel_vgpu_has_full_48bit_ppgtt(dev_priv);

> + has_full_ppgtt = intel_vgpu_has_full_ppgtt(dev_priv);
> + /* GVT-g has no support for 32bit ppgtt */
> + if (enable_ppgtt == 2 && has_full_ppgtt) {
> + DRM_DEBUG_DRIVER("Force 48bit ppgtt for vGPU\n");
> + enable_ppgtt = 3;

Lets not do an upgrade for the user if they explicitly requested lower
level. I'd just leave the option to be and it'll get downgraded to
aliasing ppgtt.

> + }
>   }
>  
> + has_full_48bit_ppgtt = has_full_ppgtt &&
> + dev_priv->info.has_full_48bit_ppgtt;
> +

This can then be dropped too.

I'll send a patch to disconnect the has_full_ppgtt and
has_full_48bit_ppgtt flags.


>   if (!has_aliasing_ppgtt)
>   return 0;
>  
> @@ -49,12 +49,18 @@ enum vgt_g2v_type {
> >     VGT_G2V_MAX,
>  };
>  
> +/*
> + * VGT capabilities type
> + */
> +#define VGT_CAPS_FULL_PPGTT_48BITBIT(2)

VGT_CAPS_FULL_48BIT_PPGTT for consistency

> @@ -75,10 +75,17 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv)
>   return;
>   }
>  
> + dev_priv->vgpu.caps = __raw_i915_read32(dev_priv, vgtif_reg(vgt_caps));
> +
>   dev_priv->vgpu.active = true;
>   DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>  }
>  
> +bool intel_vgpu_has_full_ppgtt(struct drm_i915_private *dev_priv)

This should really be has_full_48bit_ppgtt() too.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx