> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Monday, September 13, 2021 3:00 PM
> To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v2 4/6] drm/edid: parse the DisplayID v2.0 VESA vendor block
> for MSO
>
> On Mon, 13 Sep 2021, "Shankar, Uma" <uma.shankar@xxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> >> Of Jani Nikula
> >> Sent: Tuesday, August 31, 2021 7:48 PM
> >> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx;
> >> Nikula, Jani <jani.nikula@xxxxxxxxx>
> >> Subject: [PATCH v2 4/6] drm/edid: parse the DisplayID v2.0 VESA
> >> vendor block for MSO
> >>
> >> The VESA Organization Vendor-Specific Data Block, defined in VESA
> >> DisplayID Standard v2.0, specifies the eDP Multi-SST Operation (MSO)
> >> stream count and segment pixel overlap.
> >>
> >> DisplayID v1.3 has Appendix B: DisplayID as an EDID Extension,
> >> describing how DisplayID sections may be embedded in EDID extension
> >> blocks. DisplayID v2.0 does not have such a section, perhaps implying
> >> that DisplayID v2.0 data should not be included in EDID extensions,
> >> but rather in a "pure" DisplayID structure at its own DDC address pair A4h/A5h, as
> described in VESA E-DDC Standard v1.3 chapter 3.
> >>
> >> However, in practice, displays out in the field have embedded
> >> DisplayID
> >> v2.0 data blocks in EDID extensions, including, in particular, some
> >> eDP MSO displays, where a pure DisplayID structure is not available at all.
> >>
> >> Parse the MSO data from the DisplayID data block. Do it as part of
> >> drm_add_display_info(), extending it to parse also DisplayID data to
> >> avoid requiring extra calls to update the information.
> >>
> >> v2: Check for VESA OUI (Ville)
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/drm_edid.c | 72
> >> +++++++++++++++++++++++++++++++++++++
> >> include/drm/drm_connector.h | 12 +++++++
> >> include/drm/drm_displayid.h | 13
> >> +++++++
> >> 3 files changed, 97 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 92974b1478bc..c45c225267ca 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -28,6 +28,7 @@
> >> * DEALINGS IN THE SOFTWARE.
> >> */
> >>
> >> +#include <linux/bitfield.h>
> >> #include <linux/hdmi.h>
> >> #include <linux/i2c.h>
> >> #include <linux/kernel.h>
> >> @@ -5145,6 +5146,71 @@ void drm_get_monitor_range(struct
> >> drm_connector *connector,
> >> info->monitor_range.max_vfreq); }
> >>
> >> +static void drm_parse_vesa_mso_data(struct drm_connector *connector,
> >> + const struct displayid_block *block) {
> >> + struct displayid_vesa_vendor_specific_block *vesa =
> >> + (struct displayid_vesa_vendor_specific_block *)block;
> >> + struct drm_display_info *info = &connector->display_info;
> >> +
> >> + if (block->num_bytes < 3) {
> >> + drm_dbg_kms(connector->dev, "Unexpected vendor block size
> >> %u\n",
> >> + block->num_bytes);
> >> + return;
> >> + }
> >> +
> >> + if (oui(vesa->oui[0], vesa->oui[1], vesa->oui[2]) != VESA_IEEE_OUI)
> >> + return;
> >> +
> >> + if (sizeof(*vesa) != sizeof(*block) + block->num_bytes) {
> >> + drm_dbg_kms(connector->dev, "Unexpected VESA vendor block
> >> size\n");
> >> + return;
> >> + }
> >> +
> >> + switch (FIELD_GET(DISPLAYID_VESA_MSO_MODE, vesa->mso)) {
> >> + default:
> >> + drm_dbg_kms(connector->dev, "Reserved MSO mode value\n");
> >> + fallthrough;
> >> + case 0:
> >> + info->mso_stream_count = 0;
> >> + break;
> >> + case 1:
> >> + info->mso_stream_count = 2; /* 2 or 4 links */
> >> + break;
> >> + case 2:
> >> + info->mso_stream_count = 4; /* 4 links */
> >> + break;
> >> + }
> >> +
> >> + if (!info->mso_stream_count) {
> >> + info->mso_pixel_overlap = 0;
> >> + return;
> >> + }
> >> +
> >> + info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP,
> >> vesa->mso);
> >> + if (info->mso_pixel_overlap > 8) {
> >> + drm_dbg_kms(connector->dev, "Reserved MSO pixel overlap value
> >> %u\n",
> >> + info->mso_pixel_overlap);
> >> + info->mso_pixel_overlap = 8;
> >
> > Going beyond 8 is not right from a vendor perspective as it goes into reserved
> region.
> > Should we not just set to 0 or how we decide that we fixed overlap at
> > 8. It seems an undefined operation and it may vary from sink to sink.
>
> I don't know if there's a right choice here. I don't mind setting it to
> 0 if you prefer that.
I feel it will be wrong on a vendor's behalf if he programs anything above 8. But not sure what
should driver do in such case, it's an undefined behavior. If he intentionally programs it as say 20,
fixing at 8 will still lead to some artifacts. 0 only helps in case where this field is corrupted and has
garbage data.
No strong objection, I will leave to your discretion Jani.
With this, all else looks good to me.
Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
Regards,
Uma Shankar
> BR,
> Jani.
>
>
> >
> > Regards,
> > Uma Shankar
> >
> >> + }
> >> +
> >> + drm_dbg_kms(connector->dev, "MSO stream count %u, pixel overlap %u\n",
> >> + info->mso_stream_count, info->mso_pixel_overlap); }
> >> +
> >> +static void drm_update_mso(struct drm_connector *connector, const
> >> +struct edid *edid) {
> >> + const struct displayid_block *block;
> >> + struct displayid_iter iter;
> >> +
> >> + displayid_iter_edid_begin(edid, &iter);
> >> + displayid_iter_for_each(block, &iter) {
> >> + if (block->tag == DATA_BLOCK_2_VENDOR_SPECIFIC)
> >> + drm_parse_vesa_mso_data(connector, block);
> >> + }
> >> + displayid_iter_end(&iter);
> >> +}
> >> +
> >> /* A connector has no EDID information, so we've got no EDID to
> >> compute quirks from. Reset
> >> * all of the values which would have been set from EDID
> >> */
> >> @@ -5168,6 +5234,9 @@ drm_reset_display_info(struct drm_connector
> >> *connector)
> >>
> >> info->non_desktop = 0;
> >> memset(&info->monitor_range, 0, sizeof(info->monitor_range));
> >> +
> >> + info->mso_stream_count = 0;
> >> + info->mso_pixel_overlap = 0;
> >> }
> >>
> >> u32 drm_add_display_info(struct drm_connector *connector, const
> >> struct edid
> >> *edid) @@ -5246,6 +5315,9 @@ u32 drm_add_display_info(struct
> >> drm_connector *connector, const struct edid *edi
> >> info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
> >> if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
> >> info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
> >> +
> >> + drm_update_mso(connector, edid);
> >> +
> >> return quirks;
> >> }
> >>
> >> diff --git a/include/drm/drm_connector.h
> >> b/include/drm/drm_connector.h index 79fa34e5ccdb..379746d3266f 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -590,6 +590,18 @@ struct drm_display_info {
> >> * @monitor_range: Frequency range supported by monitor range descriptor
> >> */
> >> struct drm_monitor_range_info monitor_range;
> >> +
> >> + /**
> >> + * @mso_stream_count: eDP Multi-SST Operation (MSO) stream count from
> >> + * the DisplayID VESA vendor block. 0 for conventional Single-Stream
> >> + * Transport (SST), or 2 or 4 MSO streams.
> >> + */
> >> + u8 mso_stream_count;
> >> +
> >> + /**
> >> + * @mso_pixel_overlap: eDP MSO segment pixel overlap, 0-8 pixels.
> >> + */
> >> + u8 mso_pixel_overlap;
> >> };
> >>
> >> int drm_display_info_set_bus_formats(struct drm_display_info *info,
> >> diff --git a/include/drm/drm_displayid.h
> >> b/include/drm/drm_displayid.h index
> >> 79771091771a..7ffbd9f7bfc7 100644
> >> --- a/include/drm/drm_displayid.h
> >> +++ b/include/drm/drm_displayid.h
> >> @@ -23,9 +23,12 @@
> >> #define DRM_DISPLAYID_H
> >>
> >> #include <linux/types.h>
> >> +#include <linux/bits.h>
> >>
> >> struct edid;
> >>
> >> +#define VESA_IEEE_OUI 0x3a0292
> >> +
> >> /* DisplayID Structure versions */
> >> #define DISPLAY_ID_STRUCTURE_VER_12 0x12
> >> #define DISPLAY_ID_STRUCTURE_VER_20 0x20
> >> @@ -126,6 +129,16 @@ struct displayid_detailed_timing_block {
> >> struct displayid_detailed_timings_1 timings[]; };
> >>
> >> +#define DISPLAYID_VESA_MSO_OVERLAP GENMASK(3, 0)
> >> +#define DISPLAYID_VESA_MSO_MODE GENMASK(6, 5)
> >> +
> >> +struct displayid_vesa_vendor_specific_block {
> >> + struct displayid_block base;
> >> + u8 oui[3];
> >> + u8 data_structure_type;
> >> + u8 mso;
> >> +} __packed;
> >> +
> >> /* DisplayID iteration */
> >> struct displayid_iter {
> >> const struct edid *edid;
> >> --
> >> 2.30.2
> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center
