[Intel-gfx] [PATCH v1] drm/i915: Refactor setting dma info to a common helper

[Chris Wilson]

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 at intel.com>
> cc: Brian Welty <brian.welty at intel.com>
> cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> 
> ---
> 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 at chris-wilson.co.uk>
-Chris