| Message ID | 1453731397-12877-2-git-send-email-tim.gore@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
tim.gore@intel.com writes: > From: Tim Gore <tim.gore@intel.com> > > Add a function that is a place for workarounds that are > GT related but not required per ring. This function is > called on driver load and also after a reset and on > resume, so it is safe for workarounds that get clobbered > in these situations. > > Signed-off-by: Tim Gore <tim.gore@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 7377b67..fe960d5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct i915_address_space *vm, > list_add_tail(&vm->global_link, &dev_priv->vm_list); > } > > +void gtt_write_workarounds(struct drm_device *dev) > +{ static void This can be squashed with 2/3. -Mika > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + /* This function is for gtt related workarounds. This function is > + * called on driver load and after a GPU reset, so you can place > + * workarounds here even if they get overwritten by GPU reset. > + */ > +} > + > int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -2148,6 +2158,8 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > > int i915_ppgtt_init_hw(struct drm_device *dev) > { > + gtt_write_workarounds(dev); > + > /* In the case of execlists, PPGTT is enabled by the context descriptor > * and the PDPs are contained within the context itself. We don't > * need to do anything here. */ > -- > 1.9.1
Tim Gore Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ > -----Original Message----- > From: Mika Kuoppala [mailto:mika.kuoppala@linux.intel.com] > Sent: Monday, January 25, 2016 2:39 PM > To: Gore, Tim; intel-gfx@lists.freedesktop.org > Cc: Gore, Tim; arun.siluvery@linux.intel.com > Subject: Re: [PATCH 1/3] drm/i915: add function for GT related workarounds > > tim.gore@intel.com writes: > > > From: Tim Gore <tim.gore@intel.com> > > > > Add a function that is a place for workarounds that are GT related but > > not required per ring. This function is called on driver load and also > > after a reset and on resume, so it is safe for workarounds that get > > clobbered in these situations. > > > > Signed-off-by: Tim Gore <tim.gore@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index 7377b67..fe960d5 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct > i915_address_space *vm, > > list_add_tail(&vm->global_link, &dev_priv->vm_list); } > > > > +void gtt_write_workarounds(struct drm_device *dev) { > > static void > > This can be squashed with 2/3. > > -Mika > Do you mean all squashed together, into a single patch? Tim > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + /* This function is for gtt related workarounds. This function is > > + * called on driver load and after a GPU reset, so you can place > > + * workarounds here even if they get overwritten by GPU reset. > > + */ > > +} > > + > > int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt > > *ppgtt) { > > struct drm_i915_private *dev_priv = dev->dev_private; @@ -2148,6 > > +2158,8 @@ int i915_ppgtt_init(struct drm_device *dev, struct > > i915_hw_ppgtt *ppgtt) > > > > int i915_ppgtt_init_hw(struct drm_device *dev) { > > + gtt_write_workarounds(dev); > > + > > /* In the case of execlists, PPGTT is enabled by the context > descriptor > > * and the PDPs are contained within the context itself. We don't > > * need to do anything here. */ > > -- > > 1.9.1
On Mon, Jan 25, 2016 at 02:43:06PM +0000, Gore, Tim wrote: > > > Tim Gore > Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ > > > > -----Original Message----- > > From: Mika Kuoppala [mailto:mika.kuoppala@linux.intel.com] > > Sent: Monday, January 25, 2016 2:39 PM > > To: Gore, Tim; intel-gfx@lists.freedesktop.org > > Cc: Gore, Tim; arun.siluvery@linux.intel.com > > Subject: Re: [PATCH 1/3] drm/i915: add function for GT related workarounds > > > > tim.gore@intel.com writes: > > > > > From: Tim Gore <tim.gore@intel.com> > > > > > > Add a function that is a place for workarounds that are GT related but > > > not required per ring. This function is called on driver load and also > > > after a reset and on resume, so it is safe for workarounds that get > > > clobbered in these situations. > > > > > > Signed-off-by: Tim Gore <tim.gore@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > index 7377b67..fe960d5 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > @@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct > > i915_address_space *vm, > > > list_add_tail(&vm->global_link, &dev_priv->vm_list); } > > > > > > +void gtt_write_workarounds(struct drm_device *dev) { > > > > static void > > > > This can be squashed with 2/3. > > > > -Mika > > > Do you mean all squashed together, into a single patch? I would. They are all setting the same register to a nominal value, for the same purpose. u32 val; /* Wa:bar,foo,baz */ val = 0; if (is_bar(dev_priv)) val = 1; else if (is_foo(dev_priv)) val = 2; else if (is_baz(dev_priv)) val = 3; if (val) I915_WRITE(REG, val); Would result in slightly less horrendous code. -Chris
On 25/01/2016 16:17, Chris Wilson wrote: > On Mon, Jan 25, 2016 at 02:43:06PM +0000, Gore, Tim wrote: >> >> >> Tim Gore >> Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ >> >> >>> -----Original Message----- >>> From: Mika Kuoppala [mailto:mika.kuoppala@linux.intel.com] >>> Sent: Monday, January 25, 2016 2:39 PM >>> To: Gore, Tim; intel-gfx@lists.freedesktop.org >>> Cc: Gore, Tim; arun.siluvery@linux.intel.com >>> Subject: Re: [PATCH 1/3] drm/i915: add function for GT related workarounds >>> >>> tim.gore@intel.com writes: >>> >>>> From: Tim Gore <tim.gore@intel.com> >>>> >>>> Add a function that is a place for workarounds that are GT related but >>>> not required per ring. This function is called on driver load and also >>>> after a reset and on resume, so it is safe for workarounds that get >>>> clobbered in these situations. >>>> >>>> Signed-off-by: Tim Gore <tim.gore@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c >>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c >>>> index 7377b67..fe960d5 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >>>> @@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct >>> i915_address_space *vm, >>>> list_add_tail(&vm->global_link, &dev_priv->vm_list); } >>>> >>>> +void gtt_write_workarounds(struct drm_device *dev) { >>> >>> static void >>> >>> This can be squashed with 2/3. >>> >>> -Mika >>> >> Do you mean all squashed together, into a single patch? > > I would. They are all setting the same register to a nominal value, for > the same purpose. Don't we normally split WA into individual patches or is this only for this WA? regards Arun > > u32 val; > > /* Wa:bar,foo,baz */ > val = 0; > if (is_bar(dev_priv)) > val = 1; > else if (is_foo(dev_priv)) > val = 2; > else if (is_baz(dev_priv)) > val = 3; > if (val) > I915_WRITE(REG, val); > > Would result in slightly less horrendous code. > -Chris >
On Mon, Jan 25, 2016 at 04:41:42PM +0000, Arun Siluvery wrote: > On 25/01/2016 16:17, Chris Wilson wrote: > >On Mon, Jan 25, 2016 at 02:43:06PM +0000, Gore, Tim wrote: > >> > >> > >>Tim Gore > >>Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ > >> > >> > >>>-----Original Message----- > >>>From: Mika Kuoppala [mailto:mika.kuoppala@linux.intel.com] > >>>Sent: Monday, January 25, 2016 2:39 PM > >>>To: Gore, Tim; intel-gfx@lists.freedesktop.org > >>>Cc: Gore, Tim; arun.siluvery@linux.intel.com > >>>Subject: Re: [PATCH 1/3] drm/i915: add function for GT related workarounds > >>> > >>>tim.gore@intel.com writes: > >>> > >>>>From: Tim Gore <tim.gore@intel.com> > >>>> > >>>>Add a function that is a place for workarounds that are GT related but > >>>>not required per ring. This function is called on driver load and also > >>>>after a reset and on resume, so it is safe for workarounds that get > >>>>clobbered in these situations. > >>>> > >>>>Signed-off-by: Tim Gore <tim.gore@intel.com> > >>>>--- > >>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++ > >>>> 1 file changed, 12 insertions(+) > >>>> > >>>>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>index 7377b67..fe960d5 100644 > >>>>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>@@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct > >>>i915_address_space *vm, > >>>> list_add_tail(&vm->global_link, &dev_priv->vm_list); } > >>>> > >>>>+void gtt_write_workarounds(struct drm_device *dev) { > >>> > >>>static void > >>> > >>>This can be squashed with 2/3. > >>> > >>>-Mika > >>> > >>Do you mean all squashed together, into a single patch? > > > >I would. They are all setting the same register to a nominal value, for > >the same purpose. > > Don't we normally split WA into individual patches or is this only > for this WA? Is it not the same w/a applied to different generations? You either split it per device, so that a bisect + revert only affects one machine, or not all. Choose your poison. -Chris
On 25/01/2016 17:10, Chris Wilson wrote: > On Mon, Jan 25, 2016 at 04:41:42PM +0000, Arun Siluvery wrote: >> On 25/01/2016 16:17, Chris Wilson wrote: >>> On Mon, Jan 25, 2016 at 02:43:06PM +0000, Gore, Tim wrote: >>>> >>>> >>>> Tim Gore >>>> Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ >>>> >>>> >>>>> -----Original Message----- >>>>> From: Mika Kuoppala [mailto:mika.kuoppala@linux.intel.com] >>>>> Sent: Monday, January 25, 2016 2:39 PM >>>>> To: Gore, Tim; intel-gfx@lists.freedesktop.org >>>>> Cc: Gore, Tim; arun.siluvery@linux.intel.com >>>>> Subject: Re: [PATCH 1/3] drm/i915: add function for GT related workarounds >>>>> >>>>> tim.gore@intel.com writes: >>>>> >>>>>> From: Tim Gore <tim.gore@intel.com> >>>>>> >>>>>> Add a function that is a place for workarounds that are GT related but >>>>>> not required per ring. This function is called on driver load and also >>>>>> after a reset and on resume, so it is safe for workarounds that get >>>>>> clobbered in these situations. >>>>>> >>>>>> Signed-off-by: Tim Gore <tim.gore@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++ >>>>>> 1 file changed, 12 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c >>>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c >>>>>> index 7377b67..fe960d5 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >>>>>> @@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct >>>>> i915_address_space *vm, >>>>>> list_add_tail(&vm->global_link, &dev_priv->vm_list); } >>>>>> >>>>>> +void gtt_write_workarounds(struct drm_device *dev) { >>>>> >>>>> static void >>>>> >>>>> This can be squashed with 2/3. >>>>> >>>>> -Mika >>>>> >>>> Do you mean all squashed together, into a single patch? >>> >>> I would. They are all setting the same register to a nominal value, for >>> the same purpose. >> >> Don't we normally split WA into individual patches or is this only >> for this WA? > > Is it not the same w/a applied to different generations? You either > split it per device, so that a bisect + revert only affects one machine, > or not all. Choose your poison. yes but the value programmed is different for each device. I think as Mika suggested, squashing 1, 2 which covers gen8 and another patch for gen9 is a good split. regards Arun > -Chris >
> -----Original Message----- > From: Arun Siluvery [mailto:arun.siluvery@linux.intel.com] > Sent: Monday, January 25, 2016 6:04 PM > To: Chris Wilson; Gore, Tim; Mika Kuoppala; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915: add function for GT related > workarounds > > On 25/01/2016 17:10, Chris Wilson wrote: > > On Mon, Jan 25, 2016 at 04:41:42PM +0000, Arun Siluvery wrote: > >> On 25/01/2016 16:17, Chris Wilson wrote: > >>> On Mon, Jan 25, 2016 at 02:43:06PM +0000, Gore, Tim wrote: > >>>> > >>>> > >>>> Tim Gore > >>>> Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, > >>>> Swindon SN3 1RJ > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Mika Kuoppala [mailto:mika.kuoppala@linux.intel.com] > >>>>> Sent: Monday, January 25, 2016 2:39 PM > >>>>> To: Gore, Tim; intel-gfx@lists.freedesktop.org > >>>>> Cc: Gore, Tim; arun.siluvery@linux.intel.com > >>>>> Subject: Re: [PATCH 1/3] drm/i915: add function for GT related > >>>>> workarounds > >>>>> > >>>>> tim.gore@intel.com writes: > >>>>> > >>>>>> From: Tim Gore <tim.gore@intel.com> > >>>>>> > >>>>>> Add a function that is a place for workarounds that are GT > >>>>>> related but not required per ring. This function is called on > >>>>>> driver load and also after a reset and on resume, so it is safe > >>>>>> for workarounds that get clobbered in these situations. > >>>>>> > >>>>>> Signed-off-by: Tim Gore <tim.gore@intel.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++ > >>>>>> 1 file changed, 12 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>>> index 7377b67..fe960d5 100644 > >>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>>>>> @@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct > >>>>> i915_address_space *vm, > >>>>>> list_add_tail(&vm->global_link, &dev_priv->vm_list); } > >>>>>> > >>>>>> +void gtt_write_workarounds(struct drm_device *dev) { > >>>>> > >>>>> static void > >>>>> > >>>>> This can be squashed with 2/3. > >>>>> > >>>>> -Mika > >>>>> > >>>> Do you mean all squashed together, into a single patch? > >>> > >>> I would. They are all setting the same register to a nominal value, > >>> for the same purpose. > >> > >> Don't we normally split WA into individual patches or is this only > >> for this WA? > > > > Is it not the same w/a applied to different generations? You either > > split it per device, so that a bisect + revert only affects one > > machine, or not all. Choose your poison. > > yes but the value programmed is different for each device. > > I think as Mika suggested, squashing 1, 2 which covers gen8 and another > patch for gen9 is a good split. > > regards > Arun > I kept the introduction of the new w/a function as a separate patch so that the gen8 and gen9 patches are independent. If you squash 1 and 2, then You can't revert just the gen8 changes because this would break the gen9 stuff. > Tim Gore Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ> > > > -Chris > >
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7377b67..fe960d5 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2132,6 +2132,16 @@ static void i915_address_space_init(struct i915_address_space *vm, list_add_tail(&vm->global_link, &dev_priv->vm_list); } +void gtt_write_workarounds(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + /* This function is for gtt related workarounds. This function is + * called on driver load and after a GPU reset, so you can place + * workarounds here even if they get overwritten by GPU reset. + */ +} + int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -2148,6 +2158,8 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) int i915_ppgtt_init_hw(struct drm_device *dev) { + gtt_write_workarounds(dev); + /* In the case of execlists, PPGTT is enabled by the context descriptor * and the PDPs are contained within the context itself. We don't * need to do anything here. */