Hi Laurent, Thank you for the review. On 26/03/2021 02:00, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Wed, Mar 24, 2021 at 09:53:32AM +0100, Hans Verkuil wrote: >> While testing support for large (> 256 bytes) EDIDs on the Renesas >> Koelsch board I noticed that the adv7511 bridge driver only read the >> first two blocks. >> >> The media V4L2 version for the adv7511 (drivers/media/i2c/adv7511-v4l2.c) >> handled this correctly. >> >> Besides a simple bug when setting the segment register (it was set to the >> block number instead of block / 2), the logic of the code was also weird. >> In particular reading the DDC_STATUS is odd: this is unrelated to EDID >> reading. > > Bits 3:0 of DDC_STATUS report the DDC controller state, which can be > used to wait until the DDC controller is idle (it reports, among other > possible states, if an EDID read is in progress). Other options are > possible of course, including waiting for ADV7511_INT0_EDID_READY as > done in adv7511_wait_for_edid(), but I wonder if the !irq case in > adv7511_wait_for_edid() wouldn't be better of busy-looping on the DDC > status instead of running the interrupt handler manually. That's > unrelated to this patch though. > >> The reworked code just waits for any EDID segment reads to finish (this >> does nothing if the a segment is already read), checks if the desired >> segment matches the read segment, and if not, then it requests the new >> segment and waits again for the EDID segment to be read. >> >> Finally it checks if the currently buffered EDID segment contains the >> desired EDID block, and if not it will update the EDID buffer from >> the adv7511. >> >> Tested with my Koelsch board and with EDIDs of 1, 2, 3 and 4 blocks. >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> >> --- >> Testing on the Renesas board also requires these two adv7604 patches >> if you want to test with an HDMI cable between the HDMI input and output: >> >> https://patchwork.linuxtv.org/project/linux-media/patch/00882808-472a-d429-c565-a701da579ead@xxxxxxxxx/ >> https://patchwork.linuxtv.org/project/linux-media/patch/c7093e76-ffb4-b19c-f576-b264f935a3ce@xxxxxxxxx/ >> --- >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index 76555ae64e9c..9e8db1c60167 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -328,6 +328,7 @@ static void adv7511_set_link_config(struct adv7511 *adv7511, >> static void __adv7511_power_on(struct adv7511 *adv7511) >> { >> adv7511->current_edid_segment = -1; >> + adv7511->edid_read = false; >> >> regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, >> ADV7511_POWER_POWER_DOWN, 0); >> @@ -529,29 +530,35 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block, >> struct adv7511 *adv7511 = data; >> struct i2c_msg xfer[2]; >> uint8_t offset; >> + unsigned int cur_segment; >> unsigned int i; >> int ret; >> >> if (len > 128) >> return -EINVAL; >> >> - if (adv7511->current_edid_segment != block / 2) { >> - unsigned int status; >> + /* wait for any EDID segment reads to finish */ >> + adv7511_wait_for_edid(adv7511, 200); > > Why do we need to wait for the EDID read to complete here ? Does the > ADV7511 initiate an EDID read by itself that we need to wait for it to > complete ? Yes. When powered on it will automatically start to read the first segment. > >> >> - ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS, >> - &status); >> + ret = regmap_read(adv7511->regmap, ADV7511_REG_EDID_SEGMENT, &cur_segment); >> + if (ret < 0) >> + return ret; > > Do we need to read this from the device, can't we instead use > current_edid_segment ? We can, provided we take into account that after poweron current_edid_segment is -1, and in that case we must not set ADV7511_REG_EDID_SEGMENT again. I'll make that change. > >> + >> + /* >> + * If the current read segment does not match what we need, then >> + * write the new segment and wait for it to be read. >> + */ >> + if (cur_segment != block / 2) { >> + adv7511->edid_read = false; >> + cur_segment = block / 2; >> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT, >> + cur_segment); >> + ret = adv7511_wait_for_edid(adv7511, 200); >> if (ret < 0) >> return ret; >> + } >> >> - if (status != 2) { >> - adv7511->edid_read = false; >> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT, >> - block); >> - ret = adv7511_wait_for_edid(adv7511, 200); >> - if (ret < 0) >> - return ret; >> - } >> - >> + if (adv7511->current_edid_segment != cur_segment) { > > Do you need to test this condition separately from the previous one ? Yes, again for the initial power on case where segment 0 has been read without ADV7511_REG_EDID_SEGMENT being written by us, and so we still need to read out segment 0 from the adv7511. I'll add some comments in v2. > >> /* Break this apart, hopefully more I2C controllers will >> * support 64 byte transfers than 256 byte transfers >> */ >> @@ -579,7 +586,7 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block, >> offset += 64; >> } >> >> - adv7511->current_edid_segment = block / 2; >> + adv7511->current_edid_segment = cur_segment; >> } >> >> if (block % 2 == 0) > Regards, Hans
