| Message ID | 20191227235147.32366-2-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Add support for non-power-of-2 FB plane alignment | expand |
Quoting Imre Deak (2019-12-27 23:51:46) > Currently the start address of a UV plane in a semiplanar YUV FB is tile > size (4kB) aligned. I noticed, that enforcing only this alignment leads > oddly to random memory corruptions on TGL while scanning out Y-tiled > FBs. This issue can be easily reproduced with a UV plane that is not > aligned to the plane's tile row size. > > Some experiments showed the correct alignment to be tile row size > indeed. This also makes sense, since the de-tiling fence created for the One would expect the implicit fence to follow the fence detiling HW logic, which does not require tile-row alignment, just 4096 alignment (since gen4). That suggests this logic is peculiar to the display engine. > object - with its own stride and so "left" and "right" edge - applies to > all the planes in the FB, so each tile row of all planes should be tile > row aligned. > > In fact BSpec requires this alignment since SKL. On SKL we may enforce > this due to the AUX plane x,y coords check, but on ICL and TGL we don't. > For now enforce this only on TGL; I can follow up with any necessary > change for ICL after more tests. Considering the severity of the error, I strongly suggest we fix all suspected machines and Cc:stable. > BSpec requires a stricter alignment for linear UV planes too (kind of a > tile row alignment), but it's unclear whether that's really needed > (couldn't be explained with the de-tiling fence as above) and enforcing > that could break existing user space; so avoid that too for now until > more tests. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > +static unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, > + int color_plane) > +{ > + unsigned int tile_width, tile_height; > + > + intel_tile_dims(fb, color_plane, &tile_width, &tile_height); > + > + return fb->pitches[color_plane] * tile_height; Ok, that is tile_row_size. -Chris
On Mon, Dec 30, 2019 at 10:53:21AM +0000, Chris Wilson wrote: > Quoting Imre Deak (2019-12-27 23:51:46) > > Currently the start address of a UV plane in a semiplanar YUV FB is tile > > size (4kB) aligned. I noticed, that enforcing only this alignment leads > > oddly to random memory corruptions on TGL while scanning out Y-tiled > > FBs. This issue can be easily reproduced with a UV plane that is not > > aligned to the plane's tile row size. > > > > Some experiments showed the correct alignment to be tile row size > > indeed. This also makes sense, since the de-tiling fence created for the > > One would expect the implicit fence to follow the fence detiling HW > logic, which does not require tile-row alignment, just 4096 alignment > (since gen4). That suggests this logic is peculiar to the display engine. Not sure what's an implicit fence, I only considered the fence we add (which is added at offset 0 for the whole object via the set_tiling IOCTL). But I suppose you mean the display would use its own fence even if we don't add one (since an explicit fence for display is not needed for a while now). Not sure what's the exact reason for the spec's tile-row alignment restriction for UV surface addresses, maybe it's required both by an implicit or explicit fence. Note that I haven't seen the memory corruption if I didn't add the explicit fence for the object, but I haven't checked if there are other problems even then, once I noticed the spec alignment requirement. To me what's logical as a rule is to not wrap around the right edge of the fence (implicit or explicit) by programming a display surface base address and an x offset. That would also mean that the stride of FB surfaces (for planar YUV surfaces all surface stride's being the same) match the stride of an implicit or explicit fence. Not sure if we enforce all these. > > object - with its own stride and so "left" and "right" edge - applies to > > all the planes in the FB, so each tile row of all planes should be tile > > row aligned. > > > > In fact BSpec requires this alignment since SKL. On SKL we may enforce > > this due to the AUX plane x,y coords check, but on ICL and TGL we don't. > > For now enforce this only on TGL; I can follow up with any necessary > > change for ICL after more tests. > > Considering the severity of the error, I strongly suggest we fix all > suspected machines and Cc:stable. Ok, will add Cc now to this one, but would follow up with other machines after more empirical proofs. > > BSpec requires a stricter alignment for linear UV planes too (kind of a > > tile row alignment), but it's unclear whether that's really needed > > (couldn't be explained with the de-tiling fence as above) and enforcing > > that could break existing user space; so avoid that too for now until > > more tests. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > +static unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, > > + int color_plane) > > +{ > > + unsigned int tile_width, tile_height; > > + > > + intel_tile_dims(fb, color_plane, &tile_width, &tile_height); > > + > > + return fb->pitches[color_plane] * tile_height; > > Ok, that is tile_row_size. > -Chris
Quoting Imre Deak (2019-12-30 19:20:36) > On Mon, Dec 30, 2019 at 10:53:21AM +0000, Chris Wilson wrote: > > Quoting Imre Deak (2019-12-27 23:51:46) > > > Currently the start address of a UV plane in a semiplanar YUV FB is tile > > > size (4kB) aligned. I noticed, that enforcing only this alignment leads > > > oddly to random memory corruptions on TGL while scanning out Y-tiled > > > FBs. This issue can be easily reproduced with a UV plane that is not > > > aligned to the plane's tile row size. > > > > > > Some experiments showed the correct alignment to be tile row size > > > indeed. This also makes sense, since the de-tiling fence created for the > > > > One would expect the implicit fence to follow the fence detiling HW > > logic, which does not require tile-row alignment, just 4096 alignment > > (since gen4). That suggests this logic is peculiar to the display engine. > > Not sure what's an implicit fence, I only considered the fence we add > (which is added at offset 0 for the whole object via the set_tiling > IOCTL). But I suppose you mean the display would use its own fence even > if we don't add one (since an explicit fence for display is not needed > for a while now). Not sure what's the exact reason for the spec's > tile-row alignment restriction for UV surface addresses, maybe it's > required both by an implicit or explicit fence. Note that I haven't seen > the memory corruption if I didn't add the explicit fence for the object, > but I haven't checked if there are other problems even then, once I > noticed the spec alignment requirement. > > To me what's logical as a rule is to not wrap around the right edge of > the fence (implicit or explicit) by programming a display surface base > address and an x offset. That would also mean that the stride of FB > surfaces (for planar YUV surfaces all surface stride's being the same) > match the stride of an implicit or explicit fence. Not sure if we > enforce all these. > > > > object - with its own stride and so "left" and "right" edge - applies to > > > all the planes in the FB, so each tile row of all planes should be tile > > > row aligned. > > > > > > In fact BSpec requires this alignment since SKL. On SKL we may enforce > > > this due to the AUX plane x,y coords check, but on ICL and TGL we don't. > > > For now enforce this only on TGL; I can follow up with any necessary > > > change for ICL after more tests. > > > > Considering the severity of the error, I strongly suggest we fix all > > suspected machines and Cc:stable. > > Ok, will add Cc now to this one, but would follow up with other machines > after more empirical proofs. Personally, I would do it in one patch. The downside of imposing a stricter alignment is increased fragmentation (and perhaps the occasional bit of rebinding), which should be of little concern. If you can figure out the meaning of the rule, please do add it to the changelog and comments, Acked-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index d8970198c77e..de382bba9a91 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1995,6 +1995,13 @@ intel_format_info_is_yuv_semiplanar(const struct drm_format_info *info, info->num_planes == (is_ccs_modifier(modifier) ? 4 : 2); } +static bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, + int color_plane) +{ + return intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier) && + color_plane == 1; +} + static unsigned int intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane) { @@ -2069,6 +2076,16 @@ static void intel_tile_dims(const struct drm_framebuffer *fb, int color_plane, *tile_height = intel_tile_height(fb, color_plane); } +static unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, + int color_plane) +{ + unsigned int tile_width, tile_height; + + intel_tile_dims(fb, color_plane, &tile_width, &tile_height); + + return fb->pitches[color_plane] * tile_height; +} + unsigned int intel_fb_align_height(const struct drm_framebuffer *fb, int color_plane, unsigned int height) @@ -2143,7 +2160,8 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, struct drm_i915_private *dev_priv = to_i915(fb->dev); /* AUX_DIST needs only 4K alignment */ - if (is_aux_plane(fb, color_plane)) + if ((INTEL_GEN(dev_priv) < 12 && is_aux_plane(fb, color_plane)) || + is_ccs_plane(fb, color_plane)) return 4096; switch (fb->modifier) { @@ -2158,6 +2176,10 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb, case I915_FORMAT_MOD_Y_TILED_CCS: case I915_FORMAT_MOD_Yf_TILED_CCS: case I915_FORMAT_MOD_Y_TILED: + if (INTEL_GEN(dev_priv) >= 12 && + is_semiplanar_uv_plane(fb, color_plane)) + return intel_tile_row_size(fb, color_plane); + /* Fall-through */ case I915_FORMAT_MOD_Yf_TILED: return 1 * 1024 * 1024; default: @@ -2504,9 +2526,17 @@ static int intel_fb_offset_to_xy(int *x, int *y, { struct drm_i915_private *dev_priv = to_i915(fb->dev); unsigned int height; + u32 alignment; + + if (INTEL_GEN(dev_priv) >= 12 && + is_semiplanar_uv_plane(fb, color_plane)) + alignment = intel_tile_row_size(fb, color_plane); + else if (fb->modifier != DRM_FORMAT_MOD_LINEAR) + alignment = intel_tile_size(dev_priv); + else + alignment = 0; - if (fb->modifier != DRM_FORMAT_MOD_LINEAR && - fb->offsets[color_plane] % intel_tile_size(dev_priv)) { + if (alignment != 0 && fb->offsets[color_plane] % alignment) { DRM_DEBUG_KMS("Misaligned offset 0x%08x for color plane %d\n", fb->offsets[color_plane], color_plane); return -EINVAL;
Currently the start address of a UV plane in a semiplanar YUV FB is tile size (4kB) aligned. I noticed, that enforcing only this alignment leads oddly to random memory corruptions on TGL while scanning out Y-tiled FBs. This issue can be easily reproduced with a UV plane that is not aligned to the plane's tile row size. Some experiments showed the correct alignment to be tile row size indeed. This also makes sense, since the de-tiling fence created for the object - with its own stride and so "left" and "right" edge - applies to all the planes in the FB, so each tile row of all planes should be tile row aligned. In fact BSpec requires this alignment since SKL. On SKL we may enforce this due to the AUX plane x,y coords check, but on ICL and TGL we don't. For now enforce this only on TGL; I can follow up with any necessary change for ICL after more tests. BSpec requires a stricter alignment for linear UV planes too (kind of a tile row alignment), but it's unclear whether that's really needed (couldn't be explained with the de-tiling fence as above) and enforcing that could break existing user space; so avoid that too for now until more tests. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 36 ++++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-)