Messages in this thread | | | Date | Mon, 31 Jan 2022 07:34:51 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH 1/3] i915/gvt: Introduce the mmio_table.c to support VFIO new mdev API |
| |
> ifeq ($(CONFIG_DRM_I915_GVT),y) > -i915-y += intel_gvt.o > +i915-y += intel_gvt.o gvt/mmio_table.o
With the split from my series in mind that builds all of the gvt/ subdirectory into a separate module I'd be tempted to places this new file into the main i915 directory as e.g. intel_gvt_mmio_table.c, given that it will have to be built into the main i915 module.
> -static void init_device_info(struct intel_gvt *gvt)
I'd keep this function as-is, as the newly added caller isn't actually needed (I'll comment on this more on the next patch).
> -/* Describe per-platform limitations. */ > -struct intel_gvt_device_info { > - u32 max_support_vgpus; > - u32 cfg_space_size; > - u32 mmio_size; > - u32 mmio_bar; > - unsigned long msi_cap_offset; > - u32 gtt_start_offset; > - u32 gtt_entry_size; > - u32 gtt_entry_size_shift; > - int gmadr_bytes_in_cmd; > - u32 max_surface_size; > -};
.. and with that there should be no need to move this declaration as well.
> -struct gvt_mmio_block { > +struct intel_gvt_mmio_block {
Any good reason for this rename? It just seems to create a lot of churn without muchof a reason.
> +static int do_mmio(u32 offset, u16 flags, u32 size, u32 addr_mask, > + u32 ro_mask, u32 device, > + struct intel_gvt_mmio_table_iter *iter)
Nit: I'd pass the iter first to these kinds of callbacks. Also the do_ name (including for the method name in the struct itself) looks odd. I'd rather use a _cb or _fn postfix.
> + for (i = start; i < end; i += 4) { > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->offset = i; > + p = intel_gvt_find_mmio_info(gvt, info->offset); > + if (p) { > + WARN(1, "dup mmio definition offset %x\n", > + info->offset); > + kfree(info); > + > + /* We return -EEXIST here to make GVT-g load fail. > + * So duplicated MMIO can be found as soon as > + * possible. > + */ > + return -EEXIST; > + }
I'd allocate the new structure only after the lookup to simplify this a bit.
> + > + info->ro_mask = ro_mask;
The r/o mask is only used here, so why not move it into the local declarations in handlers.c instead of the table built into i915.ko?
> + info->device = device; > + info->read = intel_vgpu_default_mmio_read; > + info->write = intel_vgpu_default_mmio_write;
Given that we always initialize ->read and ->write here, setup_mmio_handler can be simplified a bit and only needs to override the handlers if actually specified in the table.
> +static int init_mmio_info(struct intel_gvt *gvt) > +{ > + struct intel_gvt_mmio_table_iter iter; > + > + iter.i915 = gvt->gt->i915; > + iter.data = gvt; > + iter.do_mmio = do_mmio; > + iter.do_mmio_block = do_mmio_block;
Nit: This and the other caller could initialize the iter structure statically:
struct intel_gvt_mmio_table_iter iter = { .i915 = gvt->gt->i915, .data = gvt, .mmio_cb = intel_gvt_setup_mmio_cb, .mmio_block_cb = intel_gvt_setup_mmio_block_cb, };
> + block = find_mmio_block(gvt, VGT_PVINFO_PAGE); > + block->read = pvinfo_mmio_read; > + block->write = pvinfo_mmio_write;
Check for NULL here?
> } else if (IS_SKYLAKE(i915) || > - IS_KABYLAKE(i915) || > - IS_COFFEELAKE(i915) || > - IS_COMETLAKE(i915)) { > - ret = init_bdw_mmio_info(gvt); > + IS_KABYLAKE(i915) || > + IS_COFFEELAKE(i915) || > + IS_COMETLAKE(i915)) { > + ret = init_bdw_mmio_handlers(gvt);
Why the spurious reformatting?
> +/** > + * intel_gvt_get_device_type - return the device flag of a GVT device > + * @i915: drm i915 private data > + * > + * This function will return the device flag of a GVT device. > + */ > +unsigned long intel_gvt_get_device_type(struct drm_i915_private *i915) > +{ > + if (IS_BROADWELL(i915)) > + return D_BDW; > + else if (IS_SKYLAKE(i915)) > + return D_SKL; > + else if (IS_KABYLAKE(i915)) > + return D_KBL; > + else if (IS_BROXTON(i915)) > + return D_BXT; > + else if (IS_COFFEELAKE(i915) || IS_COMETLAKE(i915)) > + return D_CFL; > + > + return 0; > +}
I'd move this into intel_gvt.c, next to is_supported_device which also lists all the supported platforms. Preferably as a prep patch that does the move and change of argument before the main MMIO table patch.
> +++ b/drivers/gpu/drm/i915/gvt/mmio_table.h
Do we really need this new header vs just using intel_gvt.h?
| |