| Message ID | 20200417152244.77623-1-michael.j.ruhl@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Refactor dma mask usage to a common helper | expand |
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@intel.com> > cc: Brian Welty <brian.welty@intel.com> > cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > 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.] > /** > * 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? -Chris
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(). Assume we pull that code into i915.ko, we will one day. -Chris
>-----Original Message----- >From: Chris Wilson <chris@chris-wilson.co.uk> >Sent: Friday, April 17, 2020 11:42 AM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [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@intel.com> >> cc: Brian Welty <brian.welty@intel.com> >> cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> 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. 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.
>-----Original Message----- >From: Chris Wilson <chris@chris-wilson.co.uk> >Sent: Friday, April 17, 2020 11:43 AM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [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(). > >Assume we pull that code into i915.ko, we will one day. The gen5 stuff has the same dma_mask_size that I added to the i915 config structures. I thought about pulling it out of the gen 5 area, but it appears that that can be built as a separate driver, and if I pulled it out, the ability for it to be set in that path would be lost. M >-Chris
Quoting Ruhl, Michael J (2020-04-17 17:05:07) > >-----Original Message----- > >From: Chris Wilson <chris@chris-wilson.co.uk> > >Sent: Friday, April 17, 2020 11:42 AM > >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- > >gfx@lists.freedesktop.org > >Subject: Re: [Intel-gfx] [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@intel.com> > >> cc: Brian Welty <brian.welty@intel.com> > >> cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> --- > >> 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 > 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.
Quoting Ruhl, Michael J (2020-04-17 17:07:24) > >-----Original Message----- > >From: Chris Wilson <chris@chris-wilson.co.uk> > >Sent: Friday, April 17, 2020 11:43 AM > >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- > >gfx@lists.freedesktop.org > >Subject: Re: [Intel-gfx] [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(). > > > >Assume we pull that code into i915.ko, we will one day. > > The gen5 stuff has the same dma_mask_size that I added to the > i915 config structures. > > I thought about pulling it out of the gen 5 area, but it appears that that > can be built as a separate driver, and if I pulled it out, the ability for it to > be set in that path would be lost. It's not viable standalone anymore. It's over a decade dead by this point, we can drop the old AGP interface and then the sole user is i915.ko. Even if we did not, you can't use the AGP interface at the same time as i915.ko so we can just orphan char/agp, and use our own code. -Chris
>-----Original Message----- >From: Chris Wilson <chris@chris-wilson.co.uk> >Sent: Friday, April 17, 2020 12:17 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [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@chris-wilson.co.uk> >> >Sent: Friday, April 17, 2020 11:42 AM >> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >> >gfx@lists.freedesktop.org >> >Subject: Re: [Intel-gfx] [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@intel.com> >> >> cc: Brian Welty <brian.welty@intel.com> >> >> cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> --- >> >> 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.
>-----Original Message----- >From: Chris Wilson <chris@chris-wilson.co.uk> >Sent: Friday, April 17, 2020 12:20 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH] drm/i915: Refactor dma mask usage to a >common helper > >Quoting Ruhl, Michael J (2020-04-17 17:07:24) >> >-----Original Message----- >> >From: Chris Wilson <chris@chris-wilson.co.uk> >> >Sent: Friday, April 17, 2020 11:43 AM >> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >> >gfx@lists.freedesktop.org >> >Subject: Re: [Intel-gfx] [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(). >> > >> >Assume we pull that code into i915.ko, we will one day. >> >> The gen5 stuff has the same dma_mask_size that I added to the >> i915 config structures. >> >> I thought about pulling it out of the gen 5 area, but it appears that that >> can be built as a separate driver, and if I pulled it out, the ability for it to >> be set in that path would be lost. > >It's not viable standalone anymore. It's over a decade dead by this >point, we can drop the old AGP interface and then the sole user is >i915.ko. Even if we did not, you can't use the AGP interface at the same >time as i915.ko so we can just orphan char/agp, and use our own code. Ok, so can I do something like this in char/agp/intel-gtt.c: #if IS_ENABLED(CONFIG_AGP_INTEL) 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)); #endif This would fix my flow issue from the previous comments. Mike >-Chris
>-----Original Message----- >From: Ruhl, Michael J >Sent: Friday, April 17, 2020 1:21 PM >To: 'Chris Wilson' <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org >Subject: RE: [Intel-gfx] [PATCH] drm/i915: Refactor dma mask usage to a >common helper > >>-----Original Message----- >>From: Chris Wilson <chris@chris-wilson.co.uk> >>Sent: Friday, April 17, 2020 12:20 PM >>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>gfx@lists.freedesktop.org >>Subject: Re: [Intel-gfx] [PATCH] drm/i915: Refactor dma mask usage to a >>common helper >> >>Quoting Ruhl, Michael J (2020-04-17 17:07:24) >>> >-----Original Message----- >>> >From: Chris Wilson <chris@chris-wilson.co.uk> >>> >Sent: Friday, April 17, 2020 11:43 AM >>> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>> >gfx@lists.freedesktop.org >>> >Subject: Re: [Intel-gfx] [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(). >>> > >>> >Assume we pull that code into i915.ko, we will one day. >>> >>> The gen5 stuff has the same dma_mask_size that I added to the >>> i915 config structures. >>> >>> I thought about pulling it out of the gen 5 area, but it appears that that >>> can be built as a separate driver, and if I pulled it out, the ability for it to >>> be set in that path would be lost. >> >>It's not viable standalone anymore. It's over a decade dead by this >>point, we can drop the old AGP interface and then the sole user is >>i915.ko. Even if we did not, you can't use the AGP interface at the same >>time as i915.ko so we can just orphan char/agp, and use our own code. > >Ok, so can I do something like this in char/agp/intel-gtt.c: > >#if IS_ENABLED(CONFIG_AGP_INTEL) > 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)); >#endif Hmmm, will still need to add an "if (bridge)" around it as well. M > >This would fix my flow issue from the previous comments. > >Mike > >>-Chris
Quoting Ruhl, Michael J (2020-04-17 18:14:24) > 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? If this is the straw the breaks the camel's back to pull in the gmch code under gt/, let me add a pile of hay! Honestly, I would set the dma masks as we think they should be here and if they are being overridden in char/agp, so be it. When we do pull the code out, it will remain correct; and overall with the refactoring you are doing the driver will be much more consistent across all gen. -Chris
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); +} + /** * 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, \ 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, \ @@ -365,6 +372,7 @@ static const struct intel_device_info gm45_info = { .has_coherent_ggtt = true, \ /* ilk does support rc6, but we do not implement [power] contexts */ \ .has_rc6 = 0, \ + .dma_mask_size = 36, \ I9XX_PIPE_OFFSETS, \ I9XX_CURSOR_OFFSETS, \ ILK_COLORS, \ @@ -395,6 +403,7 @@ static const struct intel_device_info ilk_m_info = { .has_rc6 = 1, \ .has_rc6p = 1, \ .has_rps = true, \ + .dma_mask_size = 40, \ .ppgtt_type = INTEL_PPGTT_ALIASING, \ .ppgtt_size = 31, \ I9XX_PIPE_OFFSETS, \ @@ -445,6 +454,7 @@ static const struct intel_device_info snb_m_gt2_info = { .has_rc6 = 1, \ .has_rc6p = 1, \ .has_rps = true, \ + .dma_mask_size = 40, \ .ppgtt_type = INTEL_PPGTT_ALIASING, \ .ppgtt_size = 31, \ IVB_PIPE_OFFSETS, \ @@ -504,6 +514,7 @@ static const struct intel_device_info vlv_info = { .has_rps = true, .display.has_gmch = 1, .display.has_hotplug = 1, + .dma_mask_size = 40, .ppgtt_type = INTEL_PPGTT_ALIASING, .ppgtt_size = 31, .has_snoop = true, @@ -554,6 +565,7 @@ static const struct intel_device_info hsw_gt3_info = { G75_FEATURES, \ GEN(8), \ .has_logical_ring_contexts = 1, \ + .dma_mask_size = 39, \ .ppgtt_type = INTEL_PPGTT_FULL, \ .ppgtt_size = 48, \ .has_64bit_reloc = 1, \ @@ -602,6 +614,7 @@ static const struct intel_device_info chv_info = { .has_rps = true, .has_logical_ring_contexts = 1, .display.has_gmch = 1, + .dma_mask_size = 39, .ppgtt_type = INTEL_PPGTT_ALIASING, .ppgtt_size = 32, .has_reset_engine = 1, @@ -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 --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index db8496b4c38d..91bb7891c70c 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -98,6 +98,7 @@ void intel_device_info_print_static(const struct intel_device_info *info, drm_printf(p, "platform: %s\n", intel_platform_name(info->platform)); drm_printf(p, "ppgtt-size: %d\n", info->ppgtt_size); drm_printf(p, "ppgtt-type: %d\n", info->ppgtt_type); + drm_printf(p, "dma_mask_size: %u\n", info->dma_mask_size); #define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name)); DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); 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 */ + enum intel_ppgtt_type ppgtt_type; unsigned int ppgtt_size; /* log2, e.g. 31/32/48 bits */
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@intel.com> cc: Brian Welty <brian.welty@intel.com> cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- 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(-)