Quoting Michael J. Ruhl (2020-04-17 19:51:34)
> DMA_MASK bit values are different for different generations.
>
> This will become more difficult to manage over time with the open
> coded usage of different versions of the device.
>
> Fix by:
> disallow setting of dma mask in AGP path (< GEN(5) for i915,
> add dma_mask_size to the device info configuration,
> updating open code call sequence to the latest interface,
> refactoring into a common function for setting the dma segment
> and mask info
>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
> cc: Brian Welty <brian.welty@xxxxxxxxx>
> cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>
> ---
> v1: removed i915 depenancy from agp path for dma mask
> Consolidated segment size and work arounds to the helper
> ---
> drivers/char/agp/intel-gtt.c | 17 +++--
> drivers/gpu/drm/i915/gt/intel_ggtt.c | 15 ----
> drivers/gpu/drm/i915/i915_drv.c | 94 +++++++++++++++---------
> drivers/gpu/drm/i915/i915_pci.c | 14 ++++
> drivers/gpu/drm/i915/intel_device_info.c | 1 +
> drivers/gpu/drm/i915/intel_device_info.h | 2 +
> 6 files changed, 87 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 3d42fc4290bc..4b34a5195c65 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -1407,13 +1407,16 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
>
> dev_info(&bridge_pdev->dev, "Intel %s Chipset\n", intel_gtt_chipsets[i].name);
>
> - mask = intel_private.driver->dma_mask_size;
> - if (pci_set_dma_mask(intel_private.pcidev, DMA_BIT_MASK(mask)))
> - dev_err(&intel_private.pcidev->dev,
> - "set gfx device dma mask %d-bit failed!\n", mask);
> - else
> - pci_set_consistent_dma_mask(intel_private.pcidev,
> - DMA_BIT_MASK(mask));
> + if (bridge) {
> + mask = intel_private.driver->dma_mask_size;
> + if (pci_set_dma_mask(intel_private.pcidev, DMA_BIT_MASK(mask)))
> + dev_err(&intel_private.pcidev->dev,
> + "set gfx device dma mask %d-bit failed!\n",
> + mask);
> + else
> + pci_set_consistent_dma_mask(intel_private.pcidev,
> + DMA_BIT_MASK(mask));
> + }
This could even go under IS_ENABLED(CONFIG_AGP_INTEL)
> +/**
> + * i915_set_dma_info - set all relevant PCI dma info as configured for the
> + * platform
> + * @i915: valid i915 instance
> + *
> + * Set the dma max segment size, device and coherent masks. The dma mask set
> + * needs to occur before i915_ggtt_probe_hw.
> + *
> + * A couple of platforms have special needs. Address them as well.
> + *
> + */
> +static int i915_set_dma_info(struct drm_i915_private *i915)
> +{
> + struct pci_dev *pdev = i915->drm.pdev;
> + unsigned int mask_size = INTEL_INFO(i915)->dma_mask_size;
> + int ret;
> +
> + GEM_BUG_ON(!mask_size);
> +
> + /*
> + * We don't have a max segment size, so set it to the max so sg's
> + * debugging layer doesn't complain
> + */
> + dma_set_max_seg_size(&pdev->dev, UINT_MAX);
> +
> + ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(mask_size));
> + if (ret)
> + goto mask_err;
> +
> + /* overlay on gen2 is broken and can't address above 1G */
> + if (IS_GEN(i915, 2))
> + mask_size = 30;
> +
> + /*
> + * 965GM sometimes incorrectly writes to hardware status page (HWS)
> + * using 32bit addressing, overwriting memory if HWS is located
> + * above 4GB.
> + *
> + * The documentation also mentions an issue with undefined
> + * behaviour if any general state is accessed within a page above 4GB,
> + * which also needs to be handled carefully.
> + */
> + if (IS_I965G(i915) || IS_I965GM(i915))
> + mask_size = 32;
> +
> + ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(mask_size));
> + if (ret)
> + goto mask_err;
This has captured all the old w/a knowledge in one spot, and we don't
have the dma-mask spread over multiple files/locations.
> +
> + return 0;
> +
> +mask_err:
> + drm_err(&i915->drm, "Can't set DMA mask/consistent mask (%d)\n", ret);
> + return ret;
And on later error we are fine not to cleanup the dma-mask, as
pci_device takes care of that for us.
> +}
> @@ -685,6 +698,7 @@ static const struct intel_device_info skl_gt4_info = {
> .has_logical_ring_contexts = 1, \
> .has_logical_ring_preemption = 1, \
> .has_gt_uc = 1, \
> + .dma_mask_size = 39, \
> .ppgtt_type = INTEL_PPGTT_FULL, \
> .ppgtt_size = 48, \
> .has_reset_engine = 1, \
Diff making a hash of this file again, but they all looked correct.
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index cce6a72c5ebc..69c9257c6c6a 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -158,6 +158,8 @@ struct intel_device_info {
>
> enum intel_platform platform;
>
> + unsigned int dma_mask_size; /* available DMA address bits */
One day we should pack this struct and see how much .data we can save.
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
