On Saturday, March 12, 2022 7:41 AM, Drew Davenport <ddavenp...@chromium.org>
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.nik...@linux.intel.com>
>> Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
>> Cc: Ankit Nautiyal <ankit.k.nauti...@intel.com>
>> Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
>> Signed-off-by: Lee Shawn C <shawn.c....@intel.com>
>> ---
>> 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
>>
