Re: [PATCH] drm: edid: HDMI 2.0 HF-VSDB block parsing

From: Jose Abreu
Date: Fri Aug 12 2016 - 12:42:57 EST

Next message: Kirill A. Shutemov: "[PATCH] Add few more configurations to test ext4 with huge pages"
Previous message: Lorenzo Pieralisi: "Re: [PATCH V8 2/8] ACPI: Add new IORT functions to support MSI domain handling"
In reply to: Ville Syrjälä: "Re: [PATCH] drm: edid: HDMI 2.0 HF-VSDB block parsing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Ville,

On 12-08-2016 14:46, Ville Syrjälä wrote:
> On Wed, Aug 10, 2016 at 04:29:21PM +0100, Jose Abreu wrote:
>> Adds parsing for HDMI 2.0 'HDMI Forum Vendor
>> Specific Data Block'. This block is present in
>> some HDMI 2.0 EDID's and gives information about
>> scrambling support, SCDC, 3D Views, and others.
>>
>> Parsed parameters are stored in drm_connector
>> structure.
>>
>> Signed-off-by: Jose Abreu <joabreu@xxxxxxxxxxxx>
>> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Thierry not in cc? Is this based on his SCDC work [1] or did we have
> multiple people implementing the same thing, partially at least?
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2015-September/090929.html

Thanks for pointing this. I am quite new to DRM and I was not
aware of the existence of that patch set. I also implemented SCDC
in a very similar way. Will cc him next time.

>
>> ---
>> drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>> include/drm/drm_crtc.h | 12 ++++++++++++
>> include/drm/drm_edid.h | 5 +++++
>> include/linux/hdmi.h | 1 +
>> 4 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 7df26d4..bcacf11 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3160,6 +3160,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>> return hdmi_id == HDMI_IEEE_OUI;
>> }
>>
>> +static bool cea_db_is_hdmi_hf_vsdb(const u8 *db)
> IIRC I asked Thierry to spell out the hdmi forum to avoid having this
> look like alphabet soup. But now that I've read the spec, I do see it
> being referred as hdmi-hf there as well, so I suppose it's fine.
>
>> +{
>> + int hdmi_id;
>> +
>> + if (cea_db_tag(db) != VENDOR_BLOCK)
>> + return false;
>> +
>> + if (cea_db_payload_len(db) < 7)
>> + return false;
>> +
>> + hdmi_id = db[1] | (db[2] << 8) | (db[3] << 16);
>> +
>> + return hdmi_id == HDMI_IEEE_OUI_HF;
>> +}
>> +
>> #define for_each_cea_db(cea, i, start, end) \
>> for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>>
>> @@ -3287,6 +3302,37 @@ parse_hdmi_vsdb(struct drm_connector *connector, const u8 *db)
>> }
>>
>> static void
>> +parse_hdmi_hf_vsdb(struct drm_connector *connector, const u8 *db)
>> +{
>> + u8 len = cea_db_payload_len(db);
>> +
>> + if (len < 7)
>> + return;
>> +
>> + if (db[4] != 1)
>> + return; /* invalid version */
>> +
>> + connector->max_tmds_char = db[5] * 5;
> We already have the max_tmds_clock. Also I was just trying move some of
> the junk out from the connector into display_info [2].
>
> [2] https://lists.freedesktop.org/archives/dri-devel/2016-August/114634.html

This is max value for rates above 340Mcsc and can be used to
limit the 4:2:0 modes. The value of this field is zero when TMDS
character rates above 340Mcsc are not supported, so the source
must check then the max_tmds_clock in this condition and
max_tmds_char otherwise. I think it is a little bit confusing though.

I agree with your patches, specially patch 1 and 2 which reset
the field values. I was planning to send a new version with this
fix because I noticed that when switching from a 2.0 sink to a
1.4 the variables are not updated causing, for example,
scrambling to still be enabled when it can't be.

>
>> + connector->scdc_present = db[6] & (1 << 7);
>> + connector->rr_capable = db[6] & (1 << 6);
>> + connector->flags_3d = db[6] & 0x7;
>> + connector->supports_scramble = connector->scdc_present &&
>> + (db[6] & (1 << 3));
> Do we have actual users for all these? I don't like adding stuff into
> core structures just for the purposes of immediately printing it out.
>
> For the stereo flags, I guess someone would have to figure put how they
> relate to the 3d flags in the other vsdb, and how to convert to drm mode
> flags.

SCDC and RR can be used by SCDC helpers (when available) to check
respectively if sink supports SCDC and if sink supports SCDC read
requests. Scramble is being used by me now in bridge dw-hdmi to
run HDMI 2.0 compliance tests. Still I haven't send the patches
for this.

Relating 3D I agree with you. I also think it would be better a
little help on how to properly encode this.

>
>> +
>> + DRM_DEBUG_KMS("HDMI v2: max TMDS char %d, "
>> + "scdc %s, "
>> + "rr %s, "
>> + "3D flags 0x%x, "
>> + "scramble %s\n",
>> + connector->max_tmds_char,
>> + connector->scdc_present ? "available" : "not available",
>> + connector->rr_capable ? "capable" : "not capable",
>> + connector->flags_3d,
>> + connector->supports_scramble ?
>> + "supported" : "not supported");
>> +}
>> +
>> +static void
>> monitor_name(struct detailed_timing *t, void *data)
>> {
>> if (t->data.other_data.type == EDID_DETAIL_MONITOR_NAME)
>> @@ -3403,6 +3449,9 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>> /* HDMI Vendor-Specific Data Block */
>> if (cea_db_is_hdmi_vsdb(db))
>> parse_hdmi_vsdb(connector, db);
>> + /* HDMI Forum Vendor-Specific Data Block */
>> + else if (cea_db_is_hdmi_hf_vsdb(db))
>> + parse_hdmi_hf_vsdb(connector, db);
> Wasn't there some priority scheme between these two? Or was it just
> about the infoframes? I'd have to dig through the spec to find out.

I think the priority scheme is on 4:2:0 Video Data Block. By spec
it says that the video modes present in this block must be less
preferred than the regular video modes. This order can be
modified using a Video Format Preference Data Block.

>
>> break;
>> default:
>> break;
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index b618b50..eca57d2 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1279,6 +1279,11 @@ struct drm_encoder {
>> * @audio_latency: audio latency info from ELD, if found
>> * @null_edid_counter: track sinks that give us all zeros for the EDID
>> * @bad_edid_counter: track sinks that give us an EDID with invalid checksum
>> + * @max_tmds_char: indicates the maximum TMDS Character Rate supported
>> + * @scdc_present: when set the sink supports SCDC functionality
>> + * @rr_capable: when set the sink is capable of initiating an SCDC read request
>> + * @supports_scramble: when set the sink supports less than 340Mcsc scrambling
>> + * @flags_3d: 3D view(s) supported by the sink, see drm_edid.h (DRM_EDID_3D_*)
>> * @edid_corrupt: indicates whether the last read EDID was corrupt
>> * @debugfs_entry: debugfs directory for this connector
>> * @state: current atomic state for this connector
>> @@ -1377,6 +1382,13 @@ struct drm_connector {
>> int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
>> unsigned bad_edid_counter;
>>
>> + /* EDID bits HDMI 2.0 */
>> + int max_tmds_char; /* in Mcsc */
>> + bool scdc_present;
>> + bool rr_capable;
>> + bool supports_scramble;
>> + int flags_3d;
>> +
>> /* Flag for raw EDID header corruption - used in Displayport
>> * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
>> */
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 919933d..35d39f0 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -266,6 +266,11 @@ struct detailed_timing {
>>
>> #define DRM_ELD_CEA_SAD(mnl, sad) (20 + (mnl) + 3 * (sad))
>>
>> +/* HDMI 2.0 */
>> +#define DRM_EDID_3D_INDEPENDENT_VIEW (1 << 2)
>> +#define DRM_EDID_3D_DUAL_VIEW (1 << 1)
>> +#define DRM_EDID_3D_OSD_DISPARITY (1 << 0)
>> +
>> struct edid {
>> u8 header[8];
>> /* Vendor & product info */
>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
>> index e974420..6e23409 100644
>> --- a/include/linux/hdmi.h
>> +++ b/include/linux/hdmi.h
>> @@ -35,6 +35,7 @@ enum hdmi_infoframe_type {
>> };
>>
>> #define HDMI_IEEE_OUI 0x000c03
>> +#define HDMI_IEEE_OUI_HF 0xc45dd8
>> #define HDMI_INFOFRAME_HEADER_SIZE 4
>> #define HDMI_AVI_INFOFRAME_SIZE 13
>> #define HDMI_SPD_INFOFRAME_SIZE 25
>> --
>> 2.1.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

There are still fields left in the HF-VSDB that I am not
processing. These fields are related to color depth in 4:2:0
modes. I am planning on integrating 4:2:0 modes in bridge dw-hdmi
but first I will need patches from Shashank Sharma to be accepted[1].

[1] https://patchwork.kernel.org/patch/9271403/

Best regards,
Jose Miguel Abreu

Next message: Kirill A. Shutemov: "[PATCH] Add few more configurations to test ext4 with huge pages"
Previous message: Lorenzo Pieralisi: "Re: [PATCH V8 2/8] ACPI: Add new IORT functions to support MSI domain handling"
In reply to: Ville Syrjälä: "Re: [PATCH] drm: edid: HDMI 2.0 HF-VSDB block parsing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]