| Message ID | FF3DDC77922A8A4BB08A3BC48A1EA8CB411DA1FF@BGSMSX101.gar.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Other than that the patches are mostly equivalent, except yours parses more information than just the SCDC bits. 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 >
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. 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 ;-) > 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? Thanks, Daniel > > 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
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
On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote: > 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/ Yeah, the same old "adding stuff all over without actual users" story. It's near impossible to judge whether the added things are actually useful (or if the implementation is the best) without seeing how it's going to be used.
On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote: > 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/ DPCD I complained about because somehow we ended up with 2 sets of helpers, one filling a struct and the others returning directly. I objected to the fact that there's 2 (and imo your patch duplicated even more), not that I think one approach is clearly inferior to the other. Demanding that there's some real users is also a valid point. > > 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. We're pretty good at consuming fairly invasive refactorings in drm-misc. So it just boils down to get some agreement on what things should look like (+1 from my side to parsing stuff into structs as a general idea), and then massaging all the existing users of the "wrong" interface using cocci and sed. Unfortunately "just" ;-) > > > 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. Cheers, Daniel
On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote: > On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote: > > 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/ > > DPCD I complained about because somehow we ended up with 2 sets of > helpers, one filling a struct and the others returning directly. I > objected to the fact that there's 2 (and imo your patch duplicated even > more), not that I think one approach is clearly inferior to the other. My recollection is that I had proposed that I do the work of transitioning users of the parsers to the cached information but you had said that it wasn't worth the churn and that we should just go with the existing scheme of passing around the DPCD buffer and extending the parsers as necessary. From that I inferred that the same would be true for EDID and since we already have a couple of helpers that operate on struct edid * and which return features, continuing that scheme was preferred. Anyway, I don't really care either way. Maybe you and Ville can duke it out whether or not we want all of the fields parsed, irrespective of whether or not they will be used. Then I'll go with whatever you decide. > Demanding that there's some real users is also a valid point. > > > > 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. > > We're pretty good at consuming fairly invasive refactorings in drm-misc. > So it just boils down to get some agreement on what things should look > like (+1 from my side to parsing stuff into structs as a general idea), > and then massaging all the existing users of the "wrong" interface using > cocci and sed. > > Unfortunately "just" ;-) I think by now it would be useful to have a nested structure within struct drm_display_info that contains HDMI specific bits. We already have a number of candidates that could be extracted into such a structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(), drm_rgb_quant_range_selectable(), ...). Another possibility would be to subclass struct drm_display_info, as in: struct drm_hdmi_info { struct drm_display_info display; /* HDMI specific information */ ... }; Or yet another would be to create struct drm_hdmi_info as a separate structure and provide a helper which will extract the necessary info from the EDID. Drivers could then store that in driver-private data whereas struct drm_display_info could be reduced to the generic bits that it used to have. Thierry
On Mon, Dec 05, 2016 at 06:11:44PM +0100, Thierry Reding wrote: > On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote: > > On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote: > > > 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/ > > > > DPCD I complained about because somehow we ended up with 2 sets of > > helpers, one filling a struct and the others returning directly. I > > objected to the fact that there's 2 (and imo your patch duplicated even > > more), not that I think one approach is clearly inferior to the other. > > My recollection is that I had proposed that I do the work of > transitioning users of the parsers to the cached information but you had > said that it wasn't worth the churn and that we should just go with the > existing scheme of passing around the DPCD buffer and extending the > parsers as necessary. Hm, I guess it wasn't clear to me that you've offered to do all the conversions. Doing that would be awesome I think (but quite a bit of work), and if we bother with it, parsing into a struct is imo the better idea long-term. > From that I inferred that the same would be true for EDID and since we > already have a couple of helpers that operate on struct edid * and which > return features, continuing that scheme was preferred. > > Anyway, I don't really care either way. Maybe you and Ville can duke it > out whether or not we want all of the fields parsed, irrespective of > whether or not they will be used. Then I'll go with whatever you decide. > > > Demanding that there's some real users is also a valid point. > > > > > > 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. > > > > We're pretty good at consuming fairly invasive refactorings in drm-misc. > > So it just boils down to get some agreement on what things should look > > like (+1 from my side to parsing stuff into structs as a general idea), > > and then massaging all the existing users of the "wrong" interface using > > cocci and sed. > > > > Unfortunately "just" ;-) > > I think by now it would be useful to have a nested structure within > struct drm_display_info that contains HDMI specific bits. We already > have a number of candidates that could be extracted into such a > structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(), > drm_rgb_quant_range_selectable(), ...). > > Another possibility would be to subclass struct drm_display_info, as > in: > > struct drm_hdmi_info { > struct drm_display_info display; > > /* HDMI specific information */ > ... > }; > > Or yet another would be to create struct drm_hdmi_info as a separate > structure and provide a helper which will extract the necessary info > from the EDID. Drivers could then store that in driver-private data > whereas struct drm_display_info could be reduced to the generic bits > that it used to have. I think nested drm_hdmi_info within drm_display_info sounds like a fine idea. -Daniel
On Tue, 06 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Dec 05, 2016 at 06:11:44PM +0100, Thierry Reding wrote: >> On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote: >> > On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote: >> > > 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/ >> > >> > DPCD I complained about because somehow we ended up with 2 sets of >> > helpers, one filling a struct and the others returning directly. I >> > objected to the fact that there's 2 (and imo your patch duplicated even >> > more), not that I think one approach is clearly inferior to the other. >> >> My recollection is that I had proposed that I do the work of >> transitioning users of the parsers to the cached information but you had >> said that it wasn't worth the churn and that we should just go with the >> existing scheme of passing around the DPCD buffer and extending the >> parsers as necessary. > > Hm, I guess it wasn't clear to me that you've offered to do all the > conversions. Doing that would be awesome I think (but quite a bit of > work), and if we bother with it, parsing into a struct is imo the better > idea long-term. I'm concerned about invalidating the data in the structs at the right times. We keep having issues with that whenever we cache stuff. BR, Jani. > >> From that I inferred that the same would be true for EDID and since we >> already have a couple of helpers that operate on struct edid * and which >> return features, continuing that scheme was preferred. >> >> Anyway, I don't really care either way. Maybe you and Ville can duke it >> out whether or not we want all of the fields parsed, irrespective of >> whether or not they will be used. Then I'll go with whatever you decide. >> >> > Demanding that there's some real users is also a valid point. >> > >> > > > 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. >> > >> > We're pretty good at consuming fairly invasive refactorings in drm-misc. >> > So it just boils down to get some agreement on what things should look >> > like (+1 from my side to parsing stuff into structs as a general idea), >> > and then massaging all the existing users of the "wrong" interface using >> > cocci and sed. >> > >> > Unfortunately "just" ;-) >> >> I think by now it would be useful to have a nested structure within >> struct drm_display_info that contains HDMI specific bits. We already >> have a number of candidates that could be extracted into such a >> structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(), >> drm_rgb_quant_range_selectable(), ...). >> >> Another possibility would be to subclass struct drm_display_info, as >> in: >> >> struct drm_hdmi_info { >> struct drm_display_info display; >> >> /* HDMI specific information */ >> ... >> }; >> >> Or yet another would be to create struct drm_hdmi_info as a separate >> structure and provide a helper which will extract the necessary info >> from the EDID. Drivers could then store that in driver-private data >> whereas struct drm_display_info could be reduced to the generic bits >> that it used to have. > > I think nested drm_hdmi_info within drm_display_info sounds like a fine > idea. > -Daniel
Thanks Thierry and Daniel for concluding this discussion, while I was on vacation. Here are the next steps, which I am planning to go for (Please correct me if my understanding is not right) - Shashank to review first of the SCDC patch, and give comments or R-B. - Shashank to work on Thierry's review comments on HF-VSDB parsing patch (including creation of a sub-class nested drm_hdmi_info within drm_display_info) and publish V2 - Thierry/Daniel to review V2 and give comments or R-B Regards Shashank -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, December 6, 2016 1:49 PM To: Thierry Reding <thierry.reding@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch>; Sharma, Shashank <shashank.sharma@intel.com>; Jose Abreu <joabreu@synopsys.com>; dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection On Mon, Dec 05, 2016 at 06:11:44PM +0100, Thierry Reding wrote: > On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote: > > On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote: > > > 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/ > > > > DPCD I complained about because somehow we ended up with 2 sets of > > helpers, one filling a struct and the others returning directly. I > > objected to the fact that there's 2 (and imo your patch duplicated > > even more), not that I think one approach is clearly inferior to the other. > > My recollection is that I had proposed that I do the work of > transitioning users of the parsers to the cached information but you > had said that it wasn't worth the churn and that we should just go > with the existing scheme of passing around the DPCD buffer and > extending the parsers as necessary. Hm, I guess it wasn't clear to me that you've offered to do all the conversions. Doing that would be awesome I think (but quite a bit of work), and if we bother with it, parsing into a struct is imo the better idea long-term. > From that I inferred that the same would be true for EDID and since we > already have a couple of helpers that operate on struct edid * and > which return features, continuing that scheme was preferred. > > Anyway, I don't really care either way. Maybe you and Ville can duke > it out whether or not we want all of the fields parsed, irrespective > of whether or not they will be used. Then I'll go with whatever you decide. > > > Demanding that there's some real users is also a valid point. > > > > > > 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. > > > > We're pretty good at consuming fairly invasive refactorings in drm-misc. > > So it just boils down to get some agreement on what things should > > look like (+1 from my side to parsing stuff into structs as a > > general idea), and then massaging all the existing users of the > > "wrong" interface using cocci and sed. > > > > Unfortunately "just" ;-) > > I think by now it would be useful to have a nested structure within > struct drm_display_info that contains HDMI specific bits. We already > have a number of candidates that could be extracted into such a > structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(), > drm_rgb_quant_range_selectable(), ...). > > Another possibility would be to subclass struct drm_display_info, as > in: > > struct drm_hdmi_info { > struct drm_display_info display; > > /* HDMI specific information */ > ... > }; > > Or yet another would be to create struct drm_hdmi_info as a separate > structure and provide a helper which will extract the necessary info > from the EDID. Drivers could then store that in driver-private data > whereas struct drm_display_info could be reduced to the generic bits > that it used to have. I think nested drm_hdmi_info within drm_display_info sounds like a fine idea. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
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