| Message ID | 1414078170-29469-1-git-send-email-arun.siluvery@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 23, 2014 at 04:29:30PM +0100, Arun Siluvery wrote: > We would want to apply some of the workarounds based on a condition to a > particular platform or Gen but we may not know all possible controlling > parameters in advance hence allow to define open conditions; a WA makes > it to the list only if the condition is true. > > With the appropriate conditions we can combine all of the workarounds > and apply them from a single place irrespective of platform instead of > having them in separate functions. > > For: VIZ-4090 > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> Imo we should just pull the condition out into proper control flow. Hiding it like that in the macro doesn't seem to buy us anything at all, but obfuscates the code. -Daniel > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 35 +++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 497b836..0525a5d 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -736,6 +736,41 @@ static int wa_add(struct drm_i915_private *dev_priv, > > #define WA_WRITE(addr, val) WA_REG(addr, val, 0xffffffff) > > +#define WA_SET_BIT_MASKED_IF(cond, addr, mask) \ > + do { \ > + if (cond) { \ > + WA_SET_BIT_MASKED(addr, mask); \ > + } \ > + } while(0) > + > +#define WA_CLR_BIT_MASKED_IF(cond, addr, mask) \ > + do { \ > + if (cond) { \ > + WA_CLR_BIT_MASKED(addr, mask); \ > + } \ > + } while(0) > + > +#define WA_SET_BIT_IF(cond, addr, mask) \ > + do { \ > + if (cond) { \ > + WA_SET_BIT(addr, mask); \ > + } \ > + } while(0) > + > +#define WA_CLR_BIT_IF(cond, addr, mask) \ > + do { \ > + if (cond) { \ > + WA_CLR_BIT(addr, mask); \ > + } \ > + } while(0) > + > +#define WA_WRITE_IF(cond, addr, val) \ > + do { \ > + if (cond) { \ > + WA_WRITE(addr, val); \ > + } \ > + } while(0) > + > static int bdw_init_workarounds(struct intel_engine_cs *ring) > { > struct drm_device *dev = ring->dev; > -- > 2.1.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 23/10/2014 16:51, Daniel Vetter wrote: > On Thu, Oct 23, 2014 at 04:29:30PM +0100, Arun Siluvery wrote: >> We would want to apply some of the workarounds based on a condition to a >> particular platform or Gen but we may not know all possible controlling >> parameters in advance hence allow to define open conditions; a WA makes >> it to the list only if the condition is true. >> >> With the appropriate conditions we can combine all of the workarounds >> and apply them from a single place irrespective of platform instead of >> having them in separate functions. >> >> For: VIZ-4090 >> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> > > Imo we should just pull the condition out into proper control flow. Hiding > it like that in the macro doesn't seem to buy us anything at all, but > obfuscates the code. No we are not hiding the condition, I thought it would be easier to read it this way, e.g., WA_SET_BIT_MASKED_IF(IS_BDW_GT3(dev), WA_REG, WA_MASK); do you prefer adding if(cond) to each WA? regards Arun > -Daniel > >> --- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 35 +++++++++++++++++++++++++++++++++ >> 1 file changed, 35 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 497b836..0525a5d 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -736,6 +736,41 @@ static int wa_add(struct drm_i915_private *dev_priv, >> >> #define WA_WRITE(addr, val) WA_REG(addr, val, 0xffffffff) >> >> +#define WA_SET_BIT_MASKED_IF(cond, addr, mask) \ >> + do { \ >> + if (cond) { \ >> + WA_SET_BIT_MASKED(addr, mask); \ >> + } \ >> + } while(0) >> + >> +#define WA_CLR_BIT_MASKED_IF(cond, addr, mask) \ >> + do { \ >> + if (cond) { \ >> + WA_CLR_BIT_MASKED(addr, mask); \ >> + } \ >> + } while(0) >> + >> +#define WA_SET_BIT_IF(cond, addr, mask) \ >> + do { \ >> + if (cond) { \ >> + WA_SET_BIT(addr, mask); \ >> + } \ >> + } while(0) >> + >> +#define WA_CLR_BIT_IF(cond, addr, mask) \ >> + do { \ >> + if (cond) { \ >> + WA_CLR_BIT(addr, mask); \ >> + } \ >> + } while(0) >> + >> +#define WA_WRITE_IF(cond, addr, val) \ >> + do { \ >> + if (cond) { \ >> + WA_WRITE(addr, val); \ >> + } \ >> + } while(0) >> + >> static int bdw_init_workarounds(struct intel_engine_cs *ring) >> { >> struct drm_device *dev = ring->dev; >> -- >> 2.1.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Oct 23, 2014 at 6:07 PM, Siluvery, Arun <arun.siluvery@linux.intel.com> wrote: > On 23/10/2014 16:51, Daniel Vetter wrote: >> >> On Thu, Oct 23, 2014 at 04:29:30PM +0100, Arun Siluvery wrote: >>> >>> We would want to apply some of the workarounds based on a condition to a >>> particular platform or Gen but we may not know all possible controlling >>> parameters in advance hence allow to define open conditions; a WA makes >>> it to the list only if the condition is true. >>> >>> With the appropriate conditions we can combine all of the workarounds >>> and apply them from a single place irrespective of platform instead of >>> having them in separate functions. >>> >>> For: VIZ-4090 >>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> >> >> >> Imo we should just pull the condition out into proper control flow. Hiding >> it like that in the macro doesn't seem to buy us anything at all, but >> obfuscates the code. > > > No we are not hiding the condition, I thought it would be easier to read it > this way, e.g., > > WA_SET_BIT_MASKED_IF(IS_BDW_GT3(dev), WA_REG, WA_MASK); > > do you prefer adding if(cond) to each WA? Yes. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 497b836..0525a5d 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -736,6 +736,41 @@ static int wa_add(struct drm_i915_private *dev_priv, #define WA_WRITE(addr, val) WA_REG(addr, val, 0xffffffff) +#define WA_SET_BIT_MASKED_IF(cond, addr, mask) \ + do { \ + if (cond) { \ + WA_SET_BIT_MASKED(addr, mask); \ + } \ + } while(0) + +#define WA_CLR_BIT_MASKED_IF(cond, addr, mask) \ + do { \ + if (cond) { \ + WA_CLR_BIT_MASKED(addr, mask); \ + } \ + } while(0) + +#define WA_SET_BIT_IF(cond, addr, mask) \ + do { \ + if (cond) { \ + WA_SET_BIT(addr, mask); \ + } \ + } while(0) + +#define WA_CLR_BIT_IF(cond, addr, mask) \ + do { \ + if (cond) { \ + WA_CLR_BIT(addr, mask); \ + } \ + } while(0) + +#define WA_WRITE_IF(cond, addr, val) \ + do { \ + if (cond) { \ + WA_WRITE(addr, val); \ + } \ + } while(0) + static int bdw_init_workarounds(struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev;
We would want to apply some of the workarounds based on a condition to a particular platform or Gen but we may not know all possible controlling parameters in advance hence allow to define open conditions; a WA makes it to the list only if the condition is true. With the appropriate conditions we can combine all of the workarounds and apply them from a single place irrespective of platform instead of having them in separate functions. For: VIZ-4090 Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)