On Fri, 11 Mar 2016, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> [ text/plain ]
> On Fri, Mar 11, 2016 at 03:36:48PM +0200, Jani Nikula wrote:
>> On Wed, 09 Mar 2016, ville.syrjala at linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >
>> > SADs may span multiple CEA audio data blocks in the EDID.
>> >
>> > CEA-861-E says:
>> > "The order of the Data Blocks is not constrained. It is also possible
>> > to have more than one of a specific type of data block if necessary to
>> > include all of the descriptors needed to describe the sinkâs
>> > capabilities."
>> >
>> > Each audio data block can carry up to 10 SADs, whereas the ELD SAD limit
>> > is 15 according to HDA 1.0a spec. So we should support at least two data
>> > blocks. And apparently some devices take a more liberal interpretation
>> > and stuff only one SAD per data block even when they would fit into one.
>> >
>> > So let's try to extract all the SADs we can fit into the ELD even when
>> > they span multiple data blocks.
>> >
>> > While at it, toss in a comment to explain the 13 byte monitor name
>> > string limit which confused me at first.
>> >
>> > Cc: Arturo Pérez <artur999555 at gmail.com>
>> > Tested-by: Arturo Pérez <artur999555 at gmail.com>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94197
>> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> > ---
>> > drivers/gpu/drm/drm_edid.c | 17 +++++++++++------
>> > 1 file changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > index fdb1eb014586..414d7f61aa05 100644
>> > --- a/drivers/gpu/drm/drm_edid.c
>> > +++ b/drivers/gpu/drm/drm_edid.c
>> > @@ -3308,7 +3308,7 @@ void drm_edid_to_eld(struct drm_connector
>> > *connector, struct edid *edid)
>> > u8 *cea;
>> > u8 *name;
>> > u8 *db;
>> > - int sad_count = 0;
>> > + int total_sad_count = 0;
>> > int mnl;
>> > int dbl;
>> >
>> > @@ -3322,6 +3322,7 @@ void drm_edid_to_eld(struct drm_connector
>> > *connector, struct edid *edid)
>> >
>> > name = NULL;
>> > drm_for_each_detailed_block((u8 *)edid, monitor_name, &name);
>> > + /* max: 13 bytes EDID, 16 bytes ELD */
>> > for (mnl = 0; name && mnl < 13; mnl++) {
>> > if (name[mnl] == 0x0a)
>> > break;
>> > @@ -3350,11 +3351,15 @@ void drm_edid_to_eld(struct drm_connector
>> > *connector, struct edid *edid)
>> > dbl = cea_db_payload_len(db);
>> >
>> > switch (cea_db_tag(db)) {
>> > + int sad_count;
>> > +
>> > case AUDIO_BLOCK:
>> > /* Audio Data Block, contains SADs */
>> > - sad_count = dbl / 3;
>> > - if (dbl >= 1)
>> > - memcpy(eld + 20 + mnl, &db[1], dbl);
>> > + sad_count = min(dbl / 3, 15 - total_sad_count);
>>
>> What if total_sad_count > 15? At a glance, this seems to self correct by
>> subtracting the excess every second time... ;)
>
> Since sad_count will not exceed 15-total_sad_count, total_sad_count will never
> exceed 15.
Right. I guess it could be spelled out for folks like me. :/
Reviewed-by: Jani Nikula <jani.nikula at intel.com>
If you run out of things to do (that'll be the day), you could follow up
with replacing some of the magic here for offsets etc. with the
DRM_ELD_* defines I added in drm_edid.h.
BR,
Jani.
>
>>
>> BR,
>> Jani.
>>
>> > + if (sad_count >= 1)
>> > + memcpy(eld + 20 + mnl + total_sad_count
>> > * 3,
>> > + &db[1], sad_count * 3);
>> > + total_sad_count += sad_count;
>> > break;
>> > case SPEAKER_BLOCK:
>> > /* Speaker Allocation Data Block */
>> > @@ -3371,13 +3376,13 @@ void drm_edid_to_eld(struct drm_connector
>> > *connector, struct edid *edid)
>> > }
>> > }
>> > }
>> > - eld[5] |= sad_count << 4;
>> > + eld[5] |= total_sad_count << 4;
>> >
>> > eld[DRM_ELD_BASELINE_ELD_LEN] =
>> > DIV_ROUND_UP(drm_eld_calc_baseline_block_size(eld), 4);
>> >
>> > DRM_DEBUG_KMS("ELD size %d, SAD count %d\n",
>> > - drm_eld_size(eld), sad_count);
>> > + drm_eld_size(eld), total_sad_count);
>> > }
>> > EXPORT_SYMBOL(drm_edid_to_eld);
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
