| Message ID | 20180913200140.14073-5-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: drm/i915: GTT remapping for display | expand |
Quoting Ville Syrjala (2018-09-13 21:01:39) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > With gtt remapping plugged in we can simply raise the stride > limit on gen4+. Let's just arbitraily pick 256 KiB as the limit. > > No remapping CCS because the virtual address of each page actually > matters due to the new hash mode > (WaCompressedResourceDisplayNewHashMode:skl,kbl etc.), and no remapping > on gen2/3 due to lack of fence on the remapped vma. > > v2: Rebase due to is_ccs_modifier() > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 93b0de594c5d..346572cf734a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2506,6 +2506,19 @@ static > u32 intel_fb_max_stride(struct drm_i915_private *dev_priv, > u32 pixel_format, u64 modifier) > { > + /* > + * Arbitrary limit for gen4+. We can deal with any page > + * aligned stride via GTT remapping. Gen2/3 need a fence > + * for tiled scanout which the remapped vma won't have, > + * so we don't allow remapping on those platforms. > + * > + * Also the new hash mode we use for CCS isn't compatible > + * with remapping as the virtual address of the pages > + * affects the compressed data. > + */ > + if (INTEL_GEN(dev_priv) >= 4 && !is_ccs_modifier(modifier)) > + return 256*1024; If arbitrary, why 256k? Will we not go beyond 8 bytes per pixel in the future? If you used U32_MAX then we will just reject v.large strides on other grounds (too large for GTT, stride/size overflow). Hmm, or is the 256k to keep the overflow checks simpler? -Chris
On Thu, Sep 13, 2018 at 09:27:25PM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2018-09-13 21:01:39) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > With gtt remapping plugged in we can simply raise the stride > > limit on gen4+. Let's just arbitraily pick 256 KiB as the limit. > > > > No remapping CCS because the virtual address of each page actually > > matters due to the new hash mode > > (WaCompressedResourceDisplayNewHashMode:skl,kbl etc.), and no remapping > > on gen2/3 due to lack of fence on the remapped vma. > > > > v2: Rebase due to is_ccs_modifier() > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 93b0de594c5d..346572cf734a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2506,6 +2506,19 @@ static > > u32 intel_fb_max_stride(struct drm_i915_private *dev_priv, > > u32 pixel_format, u64 modifier) > > { > > + /* > > + * Arbitrary limit for gen4+. We can deal with any page > > + * aligned stride via GTT remapping. Gen2/3 need a fence > > + * for tiled scanout which the remapped vma won't have, > > + * so we don't allow remapping on those platforms. > > + * > > + * Also the new hash mode we use for CCS isn't compatible > > + * with remapping as the virtual address of the pages > > + * affects the compressed data. > > + */ > > + if (INTEL_GEN(dev_priv) >= 4 && !is_ccs_modifier(modifier)) > > + return 256*1024; > > If arbitrary, why 256k? Will we not go beyond 8 bytes per pixel in the > future? I'm not aware of 32bpc formats being a thing for the display. 16bpc is a thing for sure and that was on factor I used to pick the 256 KiB. Another one was the max fence stride we support is 256KiB. While that's not necessarily important these days I figured at least it's a limit we already have elsewhere so should hopefully avoid some unknown overflows :) > If you used U32_MAX then we will just reject v.large strides on > other grounds (too large for GTT, stride/size overflow). > > Hmm, or is the 256k to keep the overflow checks simpler? I was trying to keep it reasonably low to avoid introducing more overflow possibilities. But I think I will have to review more of the existing code against those even with the 256KiB limit. Didn't really think through the tile/aligned offset calculations yet to see if they are in danger now.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 93b0de594c5d..346572cf734a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2506,6 +2506,19 @@ static u32 intel_fb_max_stride(struct drm_i915_private *dev_priv, u32 pixel_format, u64 modifier) { + /* + * Arbitrary limit for gen4+. We can deal with any page + * aligned stride via GTT remapping. Gen2/3 need a fence + * for tiled scanout which the remapped vma won't have, + * so we don't allow remapping on those platforms. + * + * Also the new hash mode we use for CCS isn't compatible + * with remapping as the virtual address of the pages + * affects the compressed data. + */ + if (INTEL_GEN(dev_priv) >= 4 && !is_ccs_modifier(modifier)) + return 256*1024; + return intel_plane_fb_max_stride(dev_priv, pixel_format, modifier); }