>-----Original Message----- >From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >Sent: Friday, April 17, 2020 12:17 PM >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; intel- >gfx@xxxxxxxxxxxxxxxxxxxxx >Subject: Re: [PATCH] drm/i915: Refactor dma mask usage to a >common helper > >Quoting Ruhl, Michael J (2020-04-17 17:05:07) >> >-----Original Message----- >> >From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> >Sent: Friday, April 17, 2020 11:42 AM >> >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; intel- >> >gfx@xxxxxxxxxxxxxxxxxxxxx >> >Subject: Re: [PATCH] drm/i915: Refactor dma mask usage to a >> >common helper >> > >> >Quoting Michael J. Ruhl (2020-04-17 16:22:44) >> >> 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: >> >> adding 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_mask >> >> >> >> Note: GEN(5) and down is also set in intel_gmch_probe(). >> >> >> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx> >> >> cc: Brian Welty <brian.welty@xxxxxxxxx> >> >> cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> >> >> --- >> >> drivers/gpu/drm/i915/gt/intel_ggtt.c | 15 -------------- >> >> drivers/gpu/drm/i915/i915_drv.c | 25 >++++++++++++++++++++++++ >> >> 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 ++ >> >> 5 files changed, 42 insertions(+), 15 deletions(-) >> >> >> >> 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..3c5654b5d321 100644 >> >> --- a/drivers/gpu/drm/i915/i915_drv.c >> >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> >> @@ -566,6 +566,29 @@ static void intel_sanitize_options(struct >> >drm_i915_private *dev_priv) >> >> intel_gvt_sanitize_options(dev_priv); >> >> } >> >> >> >> +/** >> >> + * i915_set_dma_mask - set the dma mask as configured for the >platform >> >> + * @i915: valid i915 instance >> >> + * >> >> + * Set the dma device and coherent masks. This needs to occur before >> >> + * i915_ggtt_probe_hw. >> >> + * >> >> + * NOTE: for devices < GEN(6), the dma_mask will also be set in >> >> + * intel_gmch_probe() (i.e. it will be set a second time). >> >> + */ >> >> +static void i915_set_dma_mask(struct drm_i915_private *i915) >> >> +{ >> >> + struct pci_dev *pdev = i915->drm.pdev; >> >> + unsigned int mask_size = INTEL_INFO(i915)->dma_mask_size; >> >> + int err; >> >> + >> >> + GEM_BUG_ON(!mask_size); >> >> + >> >> + err = dma_set_mask_and_coherent(&pdev->dev, >> >DMA_BIT_MASK(mask_size)); >> >> + if (err) >> >> + DRM_ERROR("Can't set DMA mask/consistent mask (%d)\n", >err); >> > >> >Which device again? How serious of an error is it exactly since it is >> >ignored? Since it is being ignored, what's the impact to the user? >> >Should they take any action, or can they ignore it? [If they can ignore >> >it as well, this is barely anything to take note of.] >> >> I am keeping the "original" code path intact. The original locations spit >> out an error, and then ignore it. > >You changed from drm_err() though :-p Yeah, not sure why I did that.... >> A quick sampling of drivers shows that most drivers exit on this failure. >> >> I am not sure why the i915 does not. I am willing to change it. 😊 > >If we cannot constrain the mask to be a subset of the device >capabilities we probably should bail. I'm happy if you do propagate the >error. I will update. >> >> >> /** >> >> * i915_driver_hw_probe - setup state requiring device access >> >> * @dev_priv: device private >> >> @@ -613,6 +636,8 @@ static int i915_driver_hw_probe(struct >> >drm_i915_private *dev_priv) >> >> >> >> i915_perf_init(dev_priv); >> >> >> >> + i915_set_dma_mask(dev_priv); >> >> + >> >> ret = i915_ggtt_probe_hw(dev_priv); >> >> if (ret) >> >> goto err_perf; >> >> diff --git a/drivers/gpu/drm/i915/i915_pci.c >> >b/drivers/gpu/drm/i915/i915_pci.c >> >> index 66738f2c4f28..2fc25ec12c3d 100644 >> >> --- a/drivers/gpu/drm/i915/i915_pci.c >> >> +++ b/drivers/gpu/drm/i915/i915_pci.c >> >> @@ -171,6 +171,7 @@ >> >> .engine_mask = BIT(RCS0), \ >> >> .has_snoop = true, \ >> >> .has_coherent_ggtt = false, \ >> >> + .dma_mask_size = 32, \ >> >> I9XX_PIPE_OFFSETS, \ >> >> I9XX_CURSOR_OFFSETS, \ >> >> I9XX_COLORS, \ >> >> @@ -190,6 +191,7 @@ >> >> .engine_mask = BIT(RCS0), \ >> >> .has_snoop = true, \ >> >> .has_coherent_ggtt = false, \ >> >> + .dma_mask_size = 32, \ >> >> I845_PIPE_OFFSETS, \ >> >> I845_CURSOR_OFFSETS, \ >> >> I9XX_COLORS, \ >> >> @@ -226,6 +228,7 @@ static const struct intel_device_info i865g_info = { >> >> .engine_mask = BIT(RCS0), \ >> >> .has_snoop = true, \ >> >> .has_coherent_ggtt = true, \ >> >> + .dma_mask_size = 32, \ >> > >> >gen2 is set to 30b? >> > >> >> I9XX_PIPE_OFFSETS, \ >> >> I9XX_CURSOR_OFFSETS, \ >> >> I9XX_COLORS, \ >> >> @@ -286,6 +289,7 @@ static const struct intel_device_info g33_info = { >> >> PLATFORM(INTEL_G33), >> >> .display.has_hotplug = 1, >> >> .display.has_overlay = 1, >> >> + .dma_mask_size = 36, >> >> }; >> >> >> >> static const struct intel_device_info pnv_g_info = { >> >> @@ -293,6 +297,7 @@ static const struct intel_device_info pnv_g_info = >{ >> >> PLATFORM(INTEL_PINEVIEW), >> >> .display.has_hotplug = 1, >> >> .display.has_overlay = 1, >> >> + .dma_mask_size = 36, >> >> }; >> >> >> >> static const struct intel_device_info pnv_m_info = { >> >> @@ -301,6 +306,7 @@ static const struct intel_device_info pnv_m_info = >{ >> >> .is_mobile = 1, >> >> .display.has_hotplug = 1, >> >> .display.has_overlay = 1, >> >> + .dma_mask_size = 36, >> >> }; >> >> >> >> #define GEN4_FEATURES \ >> >> @@ -313,6 +319,7 @@ static const struct intel_device_info pnv_m_info = >{ >> >> .engine_mask = BIT(RCS0), \ >> >> .has_snoop = true, \ >> >> .has_coherent_ggtt = true, \ >> >> + .dma_mask_size = 36, \ >> >> I9XX_PIPE_OFFSETS, \ >> >> I9XX_CURSOR_OFFSETS, \ >> >> I965_COLORS, \ >> > >> >I thought we had restricted i965g/i965gm to 32b. >> > >> >Hmm. dma_set_coherent_mask. >> > >> >Not much point tablifying one without the other? >> >> Since these were the only two, adding this to the table seemed like overkill. >> >> Gen2 (restricted to 30 bits), i965g and i965gm have the coherent mask set >after >> the i915_ggtt_enable_hw occurs (in i915_drv.c) >> >> I can move this code into the common location for setting the dma mask, >but I >> was not clear on the ramifications of this. Do you know if there is a reason >for >> the location of this workaround? Ahh, I just remembered why I did not put the work arounds in the new helper. I discovered that the intel_gmch_probe() code is in the char/agp directory, so the path is: i915 set dma_mask < gen5 set dma_mask i915 < gen 5 work arounds If I put the workarounds in the set dma mask, I lose the workaround. Hmmm, intel_gmch_probe() has "gpu_pdev" to indicate that it is being inited from i915. I could use this to skip the dma mask setting for this path. Is that reasonable? Mike >Tbh, I was surprised the pci_set_dma_mask() was being so late in ggtt >probe :) > >But the only reason why the dma_set_coherent_mask() is being done much >earlier is because that was historically where we were first handed the >pci device and so where we configure the pci_dev (as opposed to thinking >of operating on the drm_i915_device). > >There should not be a problem pulling them into the same location, and >returning an error in case of failure. And if we do that we might as >well do something like: > >i915_set_dma_mask { > int dma_mask_size = INTEL_INFO(i915)->dma_mask_size; > > err = dma_set_mask(&i915->drm.dev, >DMA_BIT_MASK(dma_mask_size)); > if (err) > goto err; > > /* w/a overlay... */ > if (IS_GEN2) > dma_mask_size = 30; > > /* w/a lots of fun restrictions */ > if (IS_I965G...) > dma_mask_size = 32; > > err = dma_set_coherent_mask(&i915->drm.dev, >DMA_BIT_MASK(dma_mask_size)); > if (err) > goto err; > > return 0; > >err: > drm_err(&i915->drm, "...."); > return err; >} > >? >-Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx