Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/bridge: lt9611uxc: race condition in EDID SRAM polling causes torn reads - [Medium] drm/bridge: lt9611uxc: strict EDID header check breaks repairable displays --
commit 6559cd85aa7ae1a652dd056a93747b35998d927d Author: Ravi Agola <[email protected]> drm/bridge: lt9611uxc: support displays with up to 4 EDID blocks This commit adds support for fetching up to 4 EDID blocks in the LT9611UXC bridge driver by re-triggering access for blocks beyond the first two. It introduces a polling mechanism for blocks 0 and 2, checking for a valid EDID header match to verify read success. It also increases the EDID wait time to handle slower platform interrupts. > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > index b3bb7f2cebb3b..a74077a65f62b 100644 > --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c [ ... ] > @@ -394,20 +422,48 @@ static int lt9611uxc_get_edid_block(void *data, u8 > *buf, unsigned int block, siz > if (block >= EDID_NUM_BLOCKS) > return -EINVAL; > > - lt9611uxc_lock(lt9611uxc); > + if (block == 0 || block == 2) { > > - regmap_write(lt9611uxc->regmap, 0xb00b, 0x10); > + lt9611uxc_lock(lt9611uxc); > > - regmap_write(lt9611uxc->regmap, 0xb00a, block * EDID_BLOCK_SIZE); > + edid_ready_flag = (block == 0) ? BIT(0) : BIT(1); > > - ret = regmap_noinc_read(lt9611uxc->regmap, 0xb0b0, buf, len); > - if (ret) > - dev_err(lt9611uxc->dev, "edid read failed: %d\n", ret); > + /* > + * Set the EDID ready flag so that lt9611uxc can fetch correct > EDID block > + */ > + regmap_write(lt9611uxc->regmap, 0xb028, edid_ready_flag); > > - lt9611uxc_unlock(lt9611uxc); > + lt9611uxc_unlock(lt9611uxc); > > - return 0; > -}; > + do { > + msleep(100); > + ret = lt9611uxc_read_edid_block(lt9611uxc, block, buf, > len); > + if (ret) > + break; > + /* > + * Compare first 8 bytes of EDID header for block 0 and > block 2 > + * to confirm EDID read successfully > + */ > + header_matched = (memcmp(edid_header, buf, 8) == 0); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Medium] Will this strict exact match check inadvertently drop blocks with repairable header corruptions? If the display provides a corrupted header that the DRM core is natively designed to fix via edid_header_fix(), it seems this strict memcmp will never match and the polling loop will exhaust its retries, returning -ETIMEDOUT. Does this break initialization for displays with non-compliant but repairable EDID headers? > + edid_valid = (block == 0 && header_matched) || > + (block == 2 && !header_matched); > + if (edid_valid) > + break; ^^^^^ [Severity: High] Does this condition break the wait loop too early, ignoring that the rest of the block is still being written by the MCU? When the DRM core requests block 2, the bridge driver writes to 0xb028 to trigger the MCU fetch, and then polls the SRAM. The SRAM initially contains block 0 (which has a valid header). As soon as the MCU overwrites the first byte of SRAM with block 2 data, the !header_matched condition evaluates to true. Could this cause the driver to immediately stop polling and read the rest of the block, which might still contain stale data from block 0? > + > + } while (retry_cnt-- > 0); > + > + if (!ret && !edid_valid) > + ret = -ETIMEDOUT; > + } else { > + ret = lt9611uxc_read_edid_block(lt9611uxc, block, buf, len); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Blocks 1 and 3 are read immediately here without any synchronization. Does this guarantee reading torn data if the firmware is still fetching them over I2C? > + > + lt9611uxc_lock(lt9611uxc); > + regmap_write(lt9611uxc->regmap, 0xb028, 0x00); > + lt9611uxc_unlock(lt9611uxc); > + } > + > + return ret; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260922-lt9611usc_edid34_misc_next-v4-1-47d531582...@oss.qualcomm.com?part=1
