Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7451 bytes --]

On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > Hi Thierry, 
> > > 
> > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > https://patchwork.kernel.org/patch/9452259/ 
> > 
> > I think there had been pushback before about caching capabilities from
> > EDID, so from that point of view my patch is more inline with existing
> > EDID parsing API.
> 
> Hm, where was that pushback? We do have a bit a mess between explicitly
> parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.

You did object to a very similar patch some time ago that did a similar
thing for DPCD stuff. And also Villa had commented on an earlier patch
from Jose about concerns of bloating core structures:

	https://patchwork.freedesktop.org/patch/104806/

> I think long-term stuffing it into drm_display_info is probably better,
> since then we only have 1 interaction point between the probe code and the
> atomic_check code. That should be useful for eventually fixing the lack of
> locking between the two, if I ever get around to that ;-)

I don't really have objections to caching the results of parsing, it's
what I had proposed and what seemed most natural back when I was working
on the DPCD helpers. But if we now agree that this is the preferred way
to do things, then we should at least agree that it applies to all areas
for the sake of consistency.

Also, it might be worth looking into improving the structures, and maybe
adding new ones to order things more conveniently or at least group them
in some logical way. In my opinion some of our data structures are
becoming somewhat... unwieldy.

> > Other than that the patches are mostly equivalent, except yours parses
> > more information than just the SCDC bits.
> 
> So merge patch 1 from your series + Shashank's parsing patch? Everyone
> agrees and can you pls cross-r-b stamp so I can apply it all?

I think I spotted a mistake in Shashank's parsing patch. Let me take
another look.

If we can agree on a common way forward on how to deal with this kind of
static data, I have no objections to caching data for the duration of a
hotplug period.

Thierry

> > > -----Original Message-----
> > > From: Thierry Reding [mailto:thierry.reding@gmail.com] 
> > > Sent: Saturday, December 3, 2016 12:54 AM
> > > To: dri-devel@lists.freedesktop.org
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; Sharma, Shashank <shashank.sharma@intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; Jose Abreu <joabreu@synopsys.com>
> > > Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection
> > > 
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Sinks compliant with the HDMI 2.0 specification may support SCDC, a mechanism for the source and the sink to communicate. Sinks advertise support for this feature in the HDMI Forum Vendor Specific Data Block as defined in the HDMI 2.0 specification, section 10.4 "Status and Control Data Channel". Implement a small helper that find the HF-VSDB and parses it to check if the sink supports SCDC.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > Changes in v2:
> > > - rename internal function to cea_db_is_hdmi_forum_vsdb() for more
> > >   clarity (Ville Syrjälä)
> > > 
> > >  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_edid.h     |  1 +
> > >  include/linux/hdmi.h       |  1 +
> > >  3 files changed, 51 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 336be31ff3de..369961597ee5 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> > >  	return hdmi_id == HDMI_IEEE_OUI;
> > >  }
> > >  
> > > +static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
> > > +	unsigned int oui;
> > > +
> > > +	if (cea_db_tag(db) != VENDOR_BLOCK)
> > > +		return false;
> > > +
> > > +	if (cea_db_payload_len(db) < 7)
> > > +		return false;
> > > +
> > > +	oui = db[3] << 16 | db[2] << 8 | db[1];
> > > +
> > > +	return oui == HDMI_FORUM_IEEE_OUI;
> > > +}
> > > +
> > >  #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)
> > >  
> > > @@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> > >  
> > >  /**
> > > + * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
> > > + * @edid: sink EDID information
> > > + *
> > > + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> > > + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> > > + * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
> > > + *
> > > + * Returns:
> > > + * True if the sink supports SCDC, false otherwise.
> > > + */
> > > +bool drm_detect_hdmi_scdc(struct edid *edid) {
> > > +	unsigned int start, end, i;
> > > +	const u8 *cea;
> > > +
> > > +	cea = drm_find_cea_extension(edid);
> > > +	if (!cea)
> > > +		return false;
> > > +
> > > +	if (cea_db_offsets(cea, &start, &end))
> > > +		return false;
> > > +
> > > +	for_each_cea_db(cea, i, start, end) {
> > > +		if (cea_db_is_hdmi_forum_vsdb(&cea[i])) {
> > > +			if (cea[i + 6] & 0x80)
> > > +				return true;
> > > +		}
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +EXPORT_SYMBOL(drm_detect_hdmi_scdc);
> > > +
> > > +/**
> > >   * drm_detect_monitor_audio - check monitor audio capability
> > >   * @edid: EDID block to scan
> > >   *
> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 38eabf65f19d..7ea7e90846d8 100644
> > > --- a/include/drm/drm_edid.h
> > > +++ b/include/drm/drm_edid.h
> > > @@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
> > >  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);  bool drm_detect_hdmi_monitor(struct edid *edid);
> > > +bool drm_detect_hdmi_scdc(struct edid *edid);
> > >  bool drm_detect_monitor_audio(struct edid *edid);  bool drm_rgb_quant_range_selectable(struct edid *edid);  int drm_add_modes_noedid(struct drm_connector *connector, diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index edbb4fc674ed..d271ff23984f 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_FORUM_IEEE_OUI 0xc45dd8
> > >  #define HDMI_INFOFRAME_HEADER_SIZE  4
> > >  #define HDMI_AVI_INFOFRAME_SIZE    13
> > >  #define HDMI_SPD_INFOFRAME_SIZE    25
> > > --
> > > 2.10.2
> > > 
> 
> 
> 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel