Skip to content

vrend: fix incorrect limit sent for max samplers

Ryan Neph requested to merge ryanneph/virglrenderer:rn-fix-sampler-limit into main

Summary

NOTE: this is a fix for ChromeOS-filed bug report: https://g-issues.chromium.org/issues/324405915?pli=1&authuser=0

Prior occasions where this has reared its head:

  • originally landed in:
  • stack corruption was reported and a "Lucky" fix was landed in guest driver, though it reinforced the incorrect understanding further
  • I noticed the misinterpretation and "half"-fixed it in the guest driver, which incidentally undoes the "lucky" fix, although it is more "correct":
  • THIS MR: workaround ALL buggy guest drivers by forcing a semantically correct value to be sent, and renaming the cap on host-side
  • LATER: also rename the cap on the guest-side, and ignore this cap entirely, pretending it never existed. Introduce a new cap to do what was originally intended.

Commit Message

The original support for this driver cap ("max_texture_image_units") conflated the limit for glsl texture "samplers" and the limit for bindable texture "image units".

As a result, the guest driver has ALWAYS incorrectly used the advertised value to inform Galliums internal query for PIPE_SHADER_CAP_MAX_TEXTURE_SAMPLERS rather than the semantically intended behavior of informing a client query for GL_MAX_TEXTURE_IMAGE_UNITS.

This leads to a crash-bug during driver initialization caused by compiler stack protection triggering on stack overflow in virgl_bind_shader_states() on devices for which a native driver query for GL_MAX_TEXTURE_IMAGE_UNITS exceeds Mesa's PIPE_MAX_SAMPLERS (32).

Mali devices are one example where this is true (those return 64 and 128 in many cases).

THE FIX:

Virglrenderer should rename this cap to capture its actual semantic meaning of "max_texture_samplers", matching the way the driver has ALWAYS used it. Then virglrenderer will (with this and later versions) always send PIPE_MAX_SAMPLERS (32) to match that meaning.

Thus this cap becomes truly useless (it already was but now the naming and value actually make sense), but the crashing is solved on all versions of the guest driver.

Later we will implement the ACTUAL cap as a new one as it was intended, gated by a protocol revision bump as it was intended.

Signed-off-by: Ryan Neph ryanneph@google.com

Edited by Ryan Neph

Merge request reports

Loading