Re: [Intel-gfx] [PATCH 1/2] drm/i915/gvt: Move mdev attribute groups into kvmgt module

[Zhenyu Wang <zhenyuw@linux.intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2624 bytes --]

On 2021.04.28 14:21:41 -0300, Jason Gunthorpe wrote:
> On Wed, Apr 28, 2021 at 01:32:43PM +0800, Zhenyu Wang wrote:
> > On 2021.04.27 09:12:35 -0300, Jason Gunthorpe wrote:
> > > On Tue, Apr 27, 2021 at 10:45:06AM +0800, Zhenyu Wang wrote:
> > > > On 2021.04.26 10:39:24 -0300, Jason Gunthorpe wrote:
> > > > > On Mon, Apr 26, 2021 at 05:41:42PM +0800, Zhenyu Wang wrote:
> > > > > > @@ -1667,19 +1773,26 @@ static struct mdev_parent_ops intel_vgpu_ops = {
> > > > > >  
> > > > > >  static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
> > > > > >  {
> > > > > > -	struct attribute_group **kvm_vgpu_type_groups;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = intel_gvt_init_vgpu_type_groups((struct intel_gvt *)gvt);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > >  
> > > > > >  	intel_gvt_ops = ops;
> > > > > > -	if (!intel_gvt_ops->get_gvt_attrs(&kvm_vgpu_type_groups))
> > > > > > -		return -EFAULT;
> > > > > > -	intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;
> > > > > > +	intel_vgpu_ops.supported_type_groups = gvt_vgpu_type_groups;
> > > > > 
> > > > > This patch is an improvement, but this fictional dynamic behavior is
> > > > > still wrong. The supported_type_groups directly flows from the
> > > > > vgpu_types array in vgpu.c and it should not be split up like this
> > > > > 
> > > > > The code copies the rodata vgpu_types into dynamic memory gvt->types
> > > > > then copies that dynamic memory into a dynamic gvt_vgpu_type_groups,
> > > > > which makes very little sense at all.
> > > > 
> > > > vgpu_types is static for we want fixed vgpu mdev type, but actual supported
> > > > types depend on HW resources e.g aperture mem, fence, etc, that's dynamic for
> > > > gvt->types, so gvt_vgpu_type_groups is dynamic from gvt->types.
> > > 
> > > Well, that's worse then, intel_vgpu_ops.supported_type_groups is a
> > > static global, it cannot depend on HW variable calculations.
> > 
> > sorry, maybe I didn't describe this properly, gvt_vgpu_type_groups is
> > static global, but the actual supported types is determined from
> > gvt->types and initialized before mdev device register.
> 
> No, I got it, I saw the code too.
> 
> Think about what happens if there are two GPUs in the system, you get
> two gvt's and with different properties and you are trying to squeeze
> that into a single static global - it doesn't work. 
> 

Well, that's not what this change trys to resolve and on gvt supported platform
that's not an available hardware configuration for IGD. So I think this is fine.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx