On Saturday, March 12, 2022 7:41 AM, Drew Davenport <ddavenport@xxxxxxxxxxxx> wrote:
>On Fri, Mar 11, 2022 at 09:22:14AM +0800, Lee Shawn C wrote:
>> drm_find_cea_extension() always look for a top level CEA block. Pass
>> ext_index from caller then this function to search next available CEA
>> ext block from a specific EDID block pointer.
>>
>> v2: save proper extension block index if CTA data information
>> was found in DispalyID block.
>>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>> Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_edid.c | 43
>> +++++++++++++++++++-------------------
>> 1 file changed, 21 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 561f53831e29..e267d31d5c87 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3353,16 +3353,14 @@ const u8 *drm_find_edid_extension(const struct edid *edid,
>> return edid_ext;
>> }
>>
>> -static const u8 *drm_find_cea_extension(const struct edid *edid)
>> +static const u8 *drm_find_cea_extension(const struct edid *edid, int
>> +*ext_index)
>> {
>> const struct displayid_block *block;
>> struct displayid_iter iter;
>> const u8 *cea;
>> - int ext_index = 0;
>>
>> - /* Look for a top level CEA extension block */
>> - /* FIXME: make callers iterate through multiple CEA ext blocks? */
>> - cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
>> + /* Look for a CEA extension block from ext_index */
>> + cea = drm_find_edid_extension(edid, CEA_EXT, ext_index);
>> if (cea)
>> return cea;
>>
>> @@ -3370,6 +3368,7 @@ static const u8 *drm_find_cea_extension(const struct edid *edid)
>> displayid_iter_edid_begin(edid, &iter);
>> displayid_iter_for_each(block, &iter) {
>> if (block->tag == DATA_BLOCK_CTA) {
>> + *ext_index = iter.ext_index;
>This could still end up in an infinite loop in patch 2 in the case that there is no CEA_EXT block in the edid, but there is a CEA block in the DisplayId block.
>
>Repeating my review comment from elsewhere, consider the case:
>- If there are no cea extension blocks in the EDID,
> drm_find_edid_extension returns NULL
>- drm_find_cea_extension will then return the first DisplayId block
> with tag DATA_BLOCK_CTA
>
>If the version of the cea data from DisplayId block is less than 3, the loop will restart and call drm_find_cea_extension the same way, returning the same DisplayID block every time.
>
>Setting *ext_index inside the display_iter_for_each block doesn't change this, since we're not checking it.
>
>But I don't think we want to use the same *ext_index both to pass into drm_find_edid_extension and for tracking the next DisplayId block to check.
>This might end up in similar infinite loops or skipping DisplayId blocks.
>
>Maybe you'll need to pass in two indexes to drm_find_cea_extension, one which is passed to drm_find_edid_extension, and the other to keep track of the next DisplayId block to check.
As you mentioned, this situation would cause infinite loop. We may need two different parameters to store CEA and DisplayID block index.
Best regards,
Shawn
>> cea = (const u8 *)block;
>> break;
>> }
>> @@ -3643,10 +3642,10 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
>> struct drm_device *dev = connector->dev;
>> struct drm_display_mode *mode, *tmp;
>> LIST_HEAD(list);
>> - int modes = 0;
>> + int modes = 0, ext_index = 0;
>>
>> /* Don't add CEA modes if the CEA extension block is missing */
>> - if (!drm_find_cea_extension(edid))
>> + if (!drm_find_cea_extension(edid, &ext_index))
>> return 0;
>>
>> /*
>> @@ -4321,11 +4320,11 @@ static void drm_parse_y420cmdb_bitmap(struct
>> drm_connector *connector, static int add_cea_modes(struct
>> drm_connector *connector, struct edid *edid) {
>> - const u8 *cea = drm_find_cea_extension(edid);
>> - const u8 *db, *hdmi = NULL, *video = NULL;
>> + const u8 *cea, *db, *hdmi = NULL, *video = NULL;
>> u8 dbl, hdmi_len, video_len = 0;
>> - int modes = 0;
>> + int modes = 0, ext_index = 0;
>>
>> + cea = drm_find_cea_extension(edid, &ext_index);
>> if (cea && cea_revision(cea) >= 3) {
>> int i, start, end;
>>
>> @@ -4562,7 +4561,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>> uint8_t *eld = connector->eld;
>> const u8 *cea;
>> const u8 *db;
>> - int total_sad_count = 0;
>> + int total_sad_count = 0, ext_index = 0;
>> int mnl;
>> int dbl;
>>
>> @@ -4571,7 +4570,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>> if (!edid)
>> return;
>>
>> - cea = drm_find_cea_extension(edid);
>> + cea = drm_find_cea_extension(edid, &ext_index);
>> if (!cea) {
>> DRM_DEBUG_KMS("ELD: no CEA Extension found\n");
>> return;
>> @@ -4655,11 +4654,11 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>> */
>> int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads) {
>> - int count = 0;
>> + int count = 0, ext_index = 0;
>> int i, start, end, dbl;
>> const u8 *cea;
>>
>> - cea = drm_find_cea_extension(edid);
>> + cea = drm_find_cea_extension(edid, &ext_index);
>> if (!cea) {
>> DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
>> return 0;
>> @@ -4717,11 +4716,11 @@ EXPORT_SYMBOL(drm_edid_to_sad);
>> */
>> int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb) {
>> - int count = 0;
>> + int count = 0, ext_index = 0;
>> int i, start, end, dbl;
>> const u8 *cea;
>>
>> - cea = drm_find_cea_extension(edid);
>> + cea = drm_find_cea_extension(edid, &ext_index);
>> if (!cea) {
>> DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
>> return 0;
>> @@ -4814,9 +4813,9 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
>> {
>> const u8 *edid_ext;
>> int i;
>> - int start_offset, end_offset;
>> + int start_offset, end_offset, ext_index = 0;
>>
>> - edid_ext = drm_find_cea_extension(edid);
>> + edid_ext = drm_find_cea_extension(edid, &ext_index);
>> if (!edid_ext)
>> return false;
>>
>> @@ -4853,9 +4852,9 @@ bool drm_detect_monitor_audio(struct edid *edid)
>> const u8 *edid_ext;
>> int i, j;
>> bool has_audio = false;
>> - int start_offset, end_offset;
>> + int start_offset, end_offset, ext_index = 0;
>>
>> - edid_ext = drm_find_cea_extension(edid);
>> + edid_ext = drm_find_cea_extension(edid, &ext_index);
>> if (!edid_ext)
>> goto end;
>>
>> @@ -5177,9 +5176,9 @@ static void drm_parse_cea_ext(struct
>> drm_connector *connector, {
>> struct drm_display_info *info = &connector->display_info;
>> const u8 *edid_ext;
>> - int i, start, end;
>> + int i, start, end, ext_index = 0;
>>
>> - edid_ext = drm_find_cea_extension(edid);
>> + edid_ext = drm_find_cea_extension(edid, &ext_index);
>> if (!edid_ext)
>> return;
>>
>> --
>> 2.17.1
>>
