| Message ID | 1391785992-28063-1-git-send-email-vijay.a.purushothaman@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 07, 2014 at 08:43:12PM +0530, Vijay Purushothaman wrote: > B-spec says the FIFO total size is 512. So fix this to 512. > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index cc3ea04..fb73031 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3395,7 +3395,7 @@ > #define I915_FIFO_LINE_SIZE 64 > #define I830_FIFO_LINE_SIZE 32 > > -#define VALLEYVIEW_FIFO_SIZE 255 > +#define VALLEYVIEW_FIFO_SIZE 511 > #define G4X_FIFO_SIZE 127 > #define I965_FIFO_SIZE 512 > #define I945_FIFO_SIZE 127 Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Not that we actually use the value anywhere at the moment. As a side note the FIFO sizing for gmch platforms seems to be a place where the documentation is rather poor. It kind of looks like there are off by one errors in the text, and yet when I was playing around with this stuff on gen2/gen4 machines it kind of looks like the hardware has the same off by one issues too. IIRC my conlusion was that the last cacheline in the FIFO can't actually be used. So specifying 511 matches with my conclusion. One other thing I did notice now that I look at our g4x/vlv watermark code. We seem to assume the watermarks for g4x/vlv work the same way as pch platforms. Ie. you specify the minimum level of data left in the FIFO before it needs to start fetching more. But the documentation suggests that it's the other way around, where you specify the max amount of free space allowed in the FIFO before more data needs to be fetched. We use the latter logic for gen2-gen4. I wonder if the spec is wrong, or if your code is wrong. I guess I just need to verify it on real hardware at some point...
On 2/7/2014 9:28 PM, Ville Syrjälä wrote: > On Fri, Feb 07, 2014 at 08:43:12PM +0530, Vijay Purushothaman wrote: >> B-spec says the FIFO total size is 512. So fix this to 512. >> >> Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index cc3ea04..fb73031 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -3395,7 +3395,7 @@ >> #define I915_FIFO_LINE_SIZE 64 >> #define I830_FIFO_LINE_SIZE 32 >> >> -#define VALLEYVIEW_FIFO_SIZE 255 >> +#define VALLEYVIEW_FIFO_SIZE 511 >> #define G4X_FIFO_SIZE 127 >> #define I965_FIFO_SIZE 512 >> #define I945_FIFO_SIZE 127 > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks for the review. > > Not that we actually use the value anywhere at the moment. This value is used when the display controller is configured in Max FIFO mode. This is working fine in the android tree. At this moment i am rewriting some logic related to this Max FIFO, memory arbiter credits and drain latency handling for the sprite planes. I should have the patches ready over the week end, will test it on monday once i get to office. > > As a side note the FIFO sizing for gmch platforms seems to be a place > where the documentation is rather poor. It kind of looks like there > are off by one errors in the text, and yet when I was playing around > with this stuff on gen2/gen4 machines it kind of looks like the > hardware has the same off by one issues too. IIRC my conlusion was > that the last cacheline in the FIFO can't actually be used. So > specifying 511 matches with my conclusion. I agree.. I was thrown off by this oddity as well and it took some time for me to understand this completely. The display block in Baytrail is a mix and match of features from gen4 & gen5 (Cantiga, Crestline and Ironlake). No wonder this chip has the same h/w issues.. > > One other thing I did notice now that I look at our g4x/vlv watermark > code. We seem to assume the watermarks for g4x/vlv work the same way > as pch platforms. Ie. you specify the minimum level of data left in the > FIFO before it needs to start fetching more. But the documentation > suggests that it's the other way around, where you specify the max > amount of free space allowed in the FIFO before more data needs to be > fetched. We use the latter logic for gen2-gen4. I wonder if the spec > is wrong, or if your code is wrong. I guess I just need to verify it > on real hardware at some point... > I think this is implemented correctly in the android kernel.. On a high level the split is something like 32 KB FIFO per pipe - 16 KB for primary plane and 8 KB for each sprite. When we are in single display mode we can configure the entire 64KB FIFO for the same pipe. There is another trick to enable trickle feed.. With all these tricks i am seeing good memory self refresh numbers - almost on par with the theoretical target. I should be able to post the patch series on monday once i do some sanity testing.. Thanks, Vijay
On Fri, Feb 07, 2014 at 05:58:16PM +0200, Ville Syrjälä wrote: > On Fri, Feb 07, 2014 at 08:43:12PM +0530, Vijay Purushothaman wrote: > > B-spec says the FIFO total size is 512. So fix this to 512. > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index cc3ea04..fb73031 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3395,7 +3395,7 @@ > > #define I915_FIFO_LINE_SIZE 64 > > #define I830_FIFO_LINE_SIZE 32 > > > > -#define VALLEYVIEW_FIFO_SIZE 255 > > +#define VALLEYVIEW_FIFO_SIZE 511 > > #define G4X_FIFO_SIZE 127 > > #define I965_FIFO_SIZE 512 > > #define I945_FIFO_SIZE 127 > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Queued for -next, thanks for the patch. -Daniel
On Fri, Feb 07, 2014 at 09:56:29PM +0530, Vijay Purushothaman wrote: > On 2/7/2014 9:28 PM, Ville Syrjälä wrote: > > On Fri, Feb 07, 2014 at 08:43:12PM +0530, Vijay Purushothaman wrote: > >> B-spec says the FIFO total size is 512. So fix this to 512. > >> > >> Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_reg.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index cc3ea04..fb73031 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -3395,7 +3395,7 @@ > >> #define I915_FIFO_LINE_SIZE 64 > >> #define I830_FIFO_LINE_SIZE 32 > >> > >> -#define VALLEYVIEW_FIFO_SIZE 255 > >> +#define VALLEYVIEW_FIFO_SIZE 511 > >> #define G4X_FIFO_SIZE 127 > >> #define I965_FIFO_SIZE 512 > >> #define I945_FIFO_SIZE 127 > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Thanks for the review. > > > > > Not that we actually use the value anywhere at the moment. > > This value is used when the display controller is configured in Max FIFO > mode. This is working fine in the android tree. At this moment i am > rewriting some logic related to this Max FIFO, memory arbiter credits > and drain latency handling for the sprite planes. I should have the > patches ready over the week end, will test it on monday once i get to > office. Oh right, we do use it in g4x_compute_wm0() which would mean this patch is actually wrong when not in maxfifo mode. We'd need to dynamically select the correct value. When not in maxfifo read the FIFO size from the DSPARB, and when in maxfifo use the 511 value. > > > > As a side note the FIFO sizing for gmch platforms seems to be a place > > where the documentation is rather poor. It kind of looks like there > > are off by one errors in the text, and yet when I was playing around > > with this stuff on gen2/gen4 machines it kind of looks like the > > hardware has the same off by one issues too. IIRC my conlusion was > > that the last cacheline in the FIFO can't actually be used. So > > specifying 511 matches with my conclusion. > > I agree.. I was thrown off by this oddity as well and it took some time > for me to understand this completely. The display block in Baytrail is a > mix and match of features from gen4 & gen5 (Cantiga, Crestline and > Ironlake). No wonder this chip has the same h/w issues.. > > > > > One other thing I did notice now that I look at our g4x/vlv watermark > > code. We seem to assume the watermarks for g4x/vlv work the same way > > as pch platforms. Ie. you specify the minimum level of data left in the > > FIFO before it needs to start fetching more. But the documentation > > suggests that it's the other way around, where you specify the max > > amount of free space allowed in the FIFO before more data needs to be > > fetched. We use the latter logic for gen2-gen4. I wonder if the spec > > is wrong, or if your code is wrong. I guess I just need to verify it > > on real hardware at some point... > > > I think this is implemented correctly in the android kernel.. On a high > level the split is something like 32 KB FIFO per pipe - 16 KB for > primary plane and 8 KB for each sprite. When we are in single display > mode we can configure the entire 64KB FIFO for the same pipe. I'm not talking about the split, but rather the actual watermark for any single plane. This is what the spec says: "Number in 64Bs of space in FIFO above which the Display A Stream will generate requests to Memory" That to me sounds like it wants the opposite value than what we program in. But I think I need to verify this at some point on some g4x machine. > There is > another trick to enable trickle feed.. With all these tricks i am seeing > good memory self refresh numbers - almost on par with the theoretical > target. I should be able to post the patch series on monday once i do > some sanity testing.. Enable trickle feed to get better power savings? Sounds strange. Trickle feed isn't really explained all that well anywhere but based on what I've gathered, my idea has been that it would keep the memory awake almost constantly by trickling in small amounts of data whenever the FIFO has a little bit of room. IIRC I did some experiments which supported that conclusion. I can't recall the specifics anymore but I think with trickle feed enabled, the watermarks were more or less useless. Only when I disabled trickle feed, the watermark level started to have a real effect (ie. set it too low and you get underruns).
On Fri, 2014-02-07 at 19:58 +0100, Daniel Vetter wrote: > On Fri, Feb 07, 2014 at 05:58:16PM +0200, Ville Syrjälä wrote: > > On Fri, Feb 07, 2014 at 08:43:12PM +0530, Vijay Purushothaman wrote: > > > B-spec says the FIFO total size is 512. So fix this to 512. > > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index cc3ea04..fb73031 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -3395,7 +3395,7 @@ > > > #define I915_FIFO_LINE_SIZE 64 > > > #define I830_FIFO_LINE_SIZE 32 > > > > > > -#define VALLEYVIEW_FIFO_SIZE 255 > > > +#define VALLEYVIEW_FIFO_SIZE 511 > > > #define G4X_FIFO_SIZE 127 > > > #define I965_FIFO_SIZE 512 > > > #define I945_FIFO_SIZE 127 > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Queued for -next, thanks for the patch. > -Daniel This breaks DP on my BYT, I get bad flicker with it. Reverting only this one fixes the issue. --Imre
On Wed, 2014-02-12 at 19:24 +0200, Imre Deak wrote: > On Fri, 2014-02-07 at 19:58 +0100, Daniel Vetter wrote: > > On Fri, Feb 07, 2014 at 05:58:16PM +0200, Ville Syrjälä wrote: > > > On Fri, Feb 07, 2014 at 08:43:12PM +0530, Vijay Purushothaman wrote: > > > > B-spec says the FIFO total size is 512. So fix this to 512. > > > > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_reg.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > > index cc3ea04..fb73031 100644 > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > @@ -3395,7 +3395,7 @@ > > > > #define I915_FIFO_LINE_SIZE 64 > > > > #define I830_FIFO_LINE_SIZE 32 > > > > > > > > -#define VALLEYVIEW_FIFO_SIZE 255 > > > > +#define VALLEYVIEW_FIFO_SIZE 511 > > > > #define G4X_FIFO_SIZE 127 > > > > #define I965_FIFO_SIZE 512 > > > > #define I945_FIFO_SIZE 127 > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Queued for -next, thanks for the patch. > > -Daniel > > This breaks DP on my BYT, I get bad flicker with it. Reverting only this > one fixes the issue. Adding Vijay. --Imre
On Wed, Feb 12, 2014 at 6:27 PM, Imre Deak <imre.deak@intel.com> wrote: >> This breaks DP on my BYT, I get bad flicker with it. Reverting only this >> one fixes the issue. > > Adding Vijay. Dropped from dinq for now. -Daniel
> -----Original Message----- > From: Deak, Imre > Sent: Wednesday, February 12, 2014 10:57 PM > To: Purushothaman, Vijay A > Cc: Intel Graphics; Daniel Vetter > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix correct FIFO size for Baytrail > > On Wed, 2014-02-12 at 19:24 +0200, Imre Deak wrote: > > On Fri, 2014-02-07 at 19:58 +0100, Daniel Vetter wrote: > > > On Fri, Feb 07, 2014 at 05:58:16PM +0200, Ville Syrjälä wrote: > > > > On Fri, Feb 07, 2014 at 08:43:12PM +0530, Vijay Purushothaman wrote: > > > > > B-spec says the FIFO total size is 512. So fix this to 512. > > > > > > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_reg.h | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h > > > > > index cc3ea04..fb73031 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > > @@ -3395,7 +3395,7 @@ > > > > > #define I915_FIFO_LINE_SIZE 64 > > > > > #define I830_FIFO_LINE_SIZE 32 > > > > > > > > > > -#define VALLEYVIEW_FIFO_SIZE 255 > > > > > +#define VALLEYVIEW_FIFO_SIZE 511 > > > > > #define G4X_FIFO_SIZE 127 > > > > > #define I965_FIFO_SIZE 512 > > > > > #define I945_FIFO_SIZE 127 > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Queued for -next, thanks for the patch. > > > -Daniel > > > > This breaks DP on my BYT, I get bad flicker with it. Reverting only this > > one fixes the issue. > > Adding Vijay. > > --Imre Oops.. Sorry.. I didn’t expect this to break anything.. Please revert this patch. I will test this change along with other drain latency & PFI credit patches and resubmit this again... Thanks, Vijay
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index cc3ea04..fb73031 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3395,7 +3395,7 @@ #define I915_FIFO_LINE_SIZE 64 #define I830_FIFO_LINE_SIZE 32 -#define VALLEYVIEW_FIFO_SIZE 255 +#define VALLEYVIEW_FIFO_SIZE 511 #define G4X_FIFO_SIZE 127 #define I965_FIFO_SIZE 512 #define I945_FIFO_SIZE 127
B-spec says the FIFO total size is 512. So fix this to 512. Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)