Re: [Intel-gfx] [PATCH v1] drm/i915: Refactor setting dma info to a common helper
Quoting Ruhl, Michael J (2020-04-17 20:31:52)
> >-Original Message-
> >From: Chris Wilson
> >> 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)
>
> I was going to, but then I was going to have to add:
>
> #if IS_ENABLED(CONFIG_AGP_INTEL)
> int mask;
> #endif
>
> as well...so I stopped while I was a ahead. If you would like the #if I will
> add it.
Nope. If it ends up making the code much harder to read, then its not
worth the hassle.
> >> @@ -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.
>
> Hmm, this value could be a u8. However, placement for optimal usage is a
> bit more difficult...
>
> I can update if you would like.
Rainy day task. To make any savings we need to look at all the fields
and pahole. Plus if we do more at once, it feels like we accomplished
something :)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v1] drm/i915: Refactor setting dma info to a common helper
>-Original Message-
>From: Chris Wilson
>Sent: Friday, April 17, 2020 3:05 PM
>To: Ruhl, Michael J ; intel-
>g...@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH v1] drm/i915: Refactor setting dma info to a
>common helper
>
>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
>> cc: Brian Welty
>> cc: Daniele Ceraolo Spurio
>>
>> ---
>> 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)
I was going to, but then I was going to have to add:
#if IS_ENABLED(CONFIG_AGP_INTEL)
int mask;
#endif
as well...so I stopped while I was a ahead. If you would like the #if I will
add it.
>> +/**
>> + * 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.
>&
Re: [Intel-gfx] [PATCH v1] drm/i915: Refactor setting dma info to a common helper
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
> cc: Brian Welty
> cc: Daniele Ceraolo Spurio
>
> ---
> 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 th
[Intel-gfx] [PATCH v1] drm/i915: Refactor setting dma info to a common helper
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
cc: Brian Welty
cc: Daniele Ceraolo Spurio
---
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));
+ }
if (intel_gtt_init() != 0) {
intel_gmch_remove();
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index eebd1190506f..66165b10256e 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -840,7 +840,6 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
struct pci_dev *pdev = i915->drm.pdev;
unsigned int size;
u16 snb_gmch_ctl;
- int err;
/* TODO: We're not aware of mappable constraints on gen8 yet */
if (!IS_DGFX(i915)) {
@@ -848,13 +847,6 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
ggtt->mappable_end = resource_size(&ggtt->gmadr);
}
- err = pci_set_dma_mask(pdev, DMA_BIT_MASK(39));
- if (!err)
- err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(39));
- if (err)
- drm_err(&i915->drm,
- "Can't set DMA mask/consistent mask (%d)\n", err);
-
pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
if (IS_CHERRYVIEW(i915))
size = chv_get_total_gtt_size(snb_gmch_ctl);
@@ -990,7 +982,6 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
struct pci_dev *pdev = i915->drm.pdev;
unsigned int size;
u16 snb_gmch_ctl;
- int err;
ggtt->gmadr = pci_resource(pdev, 2);
ggtt->mappable_end = resource_size(&ggtt->gmadr);
@@ -1005,12 +996,6 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
return -ENXIO;
}
- err = pci_set_dma_mask(pdev, DMA_BIT_MASK(40));
- if (!err)
- err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(40));
- if (err)
- drm_err(&i915->drm,
- "Can't set DMA mask/consistent mask (%d)\n", err);
pci_read_config_word(pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
size = gen6_get_total_gtt_size(snb_gmch_ctl);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 641f5e03b661..21e30b30d9d6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -566,6 +566,62 @@ static void intel_sanitize_options(struct drm_i915_private
*dev_priv)
intel_gvt_sanitize_options(dev_priv);
}
+/**
+ * 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(st
