| Message ID | 1482334144-9223-1-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shashank, On 21-12-2016 15:29, Shashank Sharma wrote: [snip] > + > + /** > + * @edid_yuv420_dc_modes: bpc for deep color yuv420 encoding. > + * various sinks can support 10/12/16 bit per channel deep > + * color encoding. edid_yuv420_dc_modes = 0 means sink doesn't > + * support deep color yuv420 encoding. > + */ > + u8 edid_yuv420_dc_modes; > + > + > +#define DRM_HFVSDB_SCDC_SUPPORT (1<<7) > +#define DRM_HFVSDB_SCDC_RR_CAP (1<<6) > +#define DRM_HFVSDB_SCRAMBLING (1<<3) > +#define DRM_HFVSDB_INDEPENDENT_VIEW (1<<2) > +#define DRM_HFVSDB_DUAL_VIEW (1<<1) > +#define DRM_HFVSDB_3D_OSD (1<<0) > + > + /** > + * @scdc_supported: Sink supports SCDC functionality. > + */ > + bool scdc_supported; > + > + /** > + * @scdc_rr_cap: Sink has SCDC read request capability. > + */ > + bool scdc_rr_cap; > + > + /** > + * @scrambling: Sync supports scrambling for <=340 Mcsc TMDS > + * char rates. Above 340 Mcsc rates, scrambling is always reqd. > + */ > + bool scrambling; > + > + /** > + * @independent_view_3d: Sink supports 3d independent view signaling > + * in HF-VSIF. > + */ > + bool independent_view_3d; > + > + /** > + * @dual_view_3d: Sink supports 3d dual view signaling in HF-VSIF. > + */ > + bool dual_view_3d; > + > + /** > + * @osd_disparity_3d: Sink supports 3d osd disparity indication > + * in HF-VSIF. > + */ > + bool osd_disparity_3d; > +}; [snip] I thought we agreed in only adding these fields (edid_yuv420_dc_modes, scdc_supported, scdc_rr_cap, scrambling, independent_view_3d, dual_view_3d, osd_disparity_3d) in patch 2. I think it makes sense because you are only using them after that patch. Though, I would like to hear more comments about this as I am quite new to dri-devel. Best regards, Jose Miguel Abreu
On Thu, Dec 22, 2016 at 10:02:26AM +0000, Jose Abreu wrote: > Hi Shashank, > > > On 21-12-2016 15:29, Shashank Sharma wrote: > > [snip] > > > + > > + /** > > + * @edid_yuv420_dc_modes: bpc for deep color yuv420 encoding. > > + * various sinks can support 10/12/16 bit per channel deep > > + * color encoding. edid_yuv420_dc_modes = 0 means sink doesn't > > + * support deep color yuv420 encoding. > > + */ > > + u8 edid_yuv420_dc_modes; > > + > > + > > +#define DRM_HFVSDB_SCDC_SUPPORT (1<<7) > > +#define DRM_HFVSDB_SCDC_RR_CAP (1<<6) > > +#define DRM_HFVSDB_SCRAMBLING (1<<3) > > +#define DRM_HFVSDB_INDEPENDENT_VIEW (1<<2) > > +#define DRM_HFVSDB_DUAL_VIEW (1<<1) > > +#define DRM_HFVSDB_3D_OSD (1<<0) > > + > > + /** > > + * @scdc_supported: Sink supports SCDC functionality. > > + */ > > + bool scdc_supported; > > + > > + /** > > + * @scdc_rr_cap: Sink has SCDC read request capability. > > + */ > > + bool scdc_rr_cap; > > + > > + /** > > + * @scrambling: Sync supports scrambling for <=340 Mcsc TMDS > > + * char rates. Above 340 Mcsc rates, scrambling is always reqd. > > + */ > > + bool scrambling; > > + > > + /** > > + * @independent_view_3d: Sink supports 3d independent view signaling > > + * in HF-VSIF. > > + */ > > + bool independent_view_3d; > > + > > + /** > > + * @dual_view_3d: Sink supports 3d dual view signaling in HF-VSIF. > > + */ > > + bool dual_view_3d; > > + > > + /** > > + * @osd_disparity_3d: Sink supports 3d osd disparity indication > > + * in HF-VSIF. > > + */ > > + bool osd_disparity_3d; > > +}; > > [snip] > > I thought we agreed in only adding these fields > (edid_yuv420_dc_modes, scdc_supported, scdc_rr_cap, scrambling, > independent_view_3d, dual_view_3d, osd_disparity_3d) in patch 2. > I think it makes sense because you are only using them after that > patch. Though, I would like to hear more comments about this as I > am quite new to dri-devel. Generally you shouldn't add stuff you don't use. In this seires nothing actually uses any of of this stuff, so I don't think we should add any of it. The only piece of information actually used is the max TMDS clock, so parsing that does make sense. But I think that might be buggy as the patch will go ahead and parse both the old and new HDMI data blocks. IIRC the spec did have some words about which one should be used in which case. I don't think I spotted anything matching that in these patches.
Regards Shashank On 12/22/2016 5:26 PM, Ville Syrjälä wrote: > On Thu, Dec 22, 2016 at 10:02:26AM +0000, Jose Abreu wrote: >> Hi Shashank, >> >> >> On 21-12-2016 15:29, Shashank Sharma wrote: >> >> [snip] >> >>> + >>> + /** >>> + * @edid_yuv420_dc_modes: bpc for deep color yuv420 encoding. >>> + * various sinks can support 10/12/16 bit per channel deep >>> + * color encoding. edid_yuv420_dc_modes = 0 means sink doesn't >>> + * support deep color yuv420 encoding. >>> + */ >>> + u8 edid_yuv420_dc_modes; >>> + >>> + >>> +#define DRM_HFVSDB_SCDC_SUPPORT (1<<7) >>> +#define DRM_HFVSDB_SCDC_RR_CAP (1<<6) >>> +#define DRM_HFVSDB_SCRAMBLING (1<<3) >>> +#define DRM_HFVSDB_INDEPENDENT_VIEW (1<<2) >>> +#define DRM_HFVSDB_DUAL_VIEW (1<<1) >>> +#define DRM_HFVSDB_3D_OSD (1<<0) >>> + >>> + /** >>> + * @scdc_supported: Sink supports SCDC functionality. >>> + */ >>> + bool scdc_supported; >>> + >>> + /** >>> + * @scdc_rr_cap: Sink has SCDC read request capability. >>> + */ >>> + bool scdc_rr_cap; >>> + >>> + /** >>> + * @scrambling: Sync supports scrambling for <=340 Mcsc TMDS >>> + * char rates. Above 340 Mcsc rates, scrambling is always reqd. >>> + */ >>> + bool scrambling; >>> + >>> + /** >>> + * @independent_view_3d: Sink supports 3d independent view signaling >>> + * in HF-VSIF. >>> + */ >>> + bool independent_view_3d; >>> + >>> + /** >>> + * @dual_view_3d: Sink supports 3d dual view signaling in HF-VSIF. >>> + */ >>> + bool dual_view_3d; >>> + >>> + /** >>> + * @osd_disparity_3d: Sink supports 3d osd disparity indication >>> + * in HF-VSIF. >>> + */ >>> + bool osd_disparity_3d; >>> +}; >> [snip] >> >> I thought we agreed in only adding these fields >> (edid_yuv420_dc_modes, scdc_supported, scdc_rr_cap, scrambling, >> independent_view_3d, dual_view_3d, osd_disparity_3d) in patch 2. >> I think it makes sense because you are only using them after that >> patch. Though, I would like to hear more comments about this as I >> am quite new to dri-devel. > Generally you shouldn't add stuff you don't use. In this seires nothing > actually uses any of of this stuff, so I don't think we should add > any of it. > > The only piece of information actually used is the max TMDS clock, so > parsing that does make sense. But I think that might be buggy as the > patch will go ahead and parse both the old and new HDMI data blocks. > IIRC the spec did have some words about which one should be used in > which case. I don't think I spotted anything matching that in these > patches. > If I understood the spec right, H14B VSDB block should contain the max_tmds_clock value to be reflected for tmds_clock_rates <340Mhz, But if the sink supports tmds_rates above 340Mhz, it should set this field in hf-vsdb. Also, this field allows sinks to support higher color depths to lower resolutions, than it can support to higher resolutions. In this case, if this byte is hf-vsdb is set, max tmds clock should be picked from this block. Regards Shashank Regards Shashank
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index 8d1cf2d..270ab5d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -180,7 +180,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector) /* Check if bpc is within clock limit. Try to degrade gracefully otherwise */ if ((bpc == 12) && (mode_clock * 3/2 > max_tmds_clock)) { - if ((connector->display_info.edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_30) && + if ((connector->display_info.hdmi_info.edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_30) && (mode_clock * 5/4 <= max_tmds_clock)) bpc = 10; else diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 67d6a73..b552197 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3782,21 +3782,21 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector, if (hdmi[6] & DRM_EDID_HDMI_DC_30) { dc_bpc = 10; - info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30; + info->hdmi_info.edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30; DRM_DEBUG("%s: HDMI sink does deep color 30.\n", connector->name); } if (hdmi[6] & DRM_EDID_HDMI_DC_36) { dc_bpc = 12; - info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36; + info->hdmi_info.edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36; DRM_DEBUG("%s: HDMI sink does deep color 36.\n", connector->name); } if (hdmi[6] & DRM_EDID_HDMI_DC_48) { dc_bpc = 16; - info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48; + info->hdmi_info.edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48; DRM_DEBUG("%s: HDMI sink does deep color 48.\n", connector->name); } diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 27affbd..9edd13b 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -210,7 +210,7 @@ int radeon_get_monitor_bpc(struct drm_connector *connector) /* Check if bpc is within clock limit. Try to degrade gracefully otherwise */ if ((bpc == 12) && (mode_clock * 3/2 > max_tmds_clock)) { - if ((connector->display_info.edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_30) && + if ((connector->display_info.hdmi_info.edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_30) && (mode_clock * 5/4 <= max_tmds_clock)) bpc = 10; else diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 6e352a0..fba2b88 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -90,6 +90,76 @@ enum subpixel_order { }; /** + * struct drm_hdmi_info - runtime data specific to a connected hdmi sink + * + * Describes a given hdmi display (e.g. CRT or flat panel) and its capabilities. + * Mostly refects the advanced features added in HDMI 2.0 specs and the deep + * color support. This is a sub-segment of struct drm_display_info and should be + * used within. + * + * For sinks which provide an EDID this can be filled out by calling + * drm_add_edid_modes(). + */ + +struct drm_hdmi_info { + + /** + * @edid_hdmi_dc_modes: Mask of supported hdmi deep color modes. Even + * more stuff redundant with @bus_formats. + */ + u8 edid_hdmi_dc_modes; + + /** + * @edid_yuv420_dc_modes: bpc for deep color yuv420 encoding. + * various sinks can support 10/12/16 bit per channel deep + * color encoding. edid_yuv420_dc_modes = 0 means sink doesn't + * support deep color yuv420 encoding. + */ + u8 edid_yuv420_dc_modes; + + +#define DRM_HFVSDB_SCDC_SUPPORT (1<<7) +#define DRM_HFVSDB_SCDC_RR_CAP (1<<6) +#define DRM_HFVSDB_SCRAMBLING (1<<3) +#define DRM_HFVSDB_INDEPENDENT_VIEW (1<<2) +#define DRM_HFVSDB_DUAL_VIEW (1<<1) +#define DRM_HFVSDB_3D_OSD (1<<0) + + /** + * @scdc_supported: Sink supports SCDC functionality. + */ + bool scdc_supported; + + /** + * @scdc_rr_cap: Sink has SCDC read request capability. + */ + bool scdc_rr_cap; + + /** + * @scrambling: Sync supports scrambling for <=340 Mcsc TMDS + * char rates. Above 340 Mcsc rates, scrambling is always reqd. + */ + bool scrambling; + + /** + * @independent_view_3d: Sink supports 3d independent view signaling + * in HF-VSIF. + */ + bool independent_view_3d; + + /** + * @dual_view_3d: Sink supports 3d dual view signaling in HF-VSIF. + */ + bool dual_view_3d; + + /** + * @osd_disparity_3d: Sink supports 3d osd disparity indication + * in HF-VSIF. + */ + bool osd_disparity_3d; +}; + +/** * struct drm_display_info - runtime data about the connected sink * * Describes a given display (e.g. CRT or flat panel) and its limitations. For @@ -179,15 +249,14 @@ struct drm_display_info { bool dvi_dual; /** - * @edid_hdmi_dc_modes: Mask of supported hdmi deep color modes. Even - * more stuff redundant with @bus_formats. + * @cea_rev: CEA revision of the HDMI sink. */ - u8 edid_hdmi_dc_modes; + u8 cea_rev; /** - * @cea_rev: CEA revision of the HDMI sink. + * @ drm_hdmi_info: Capabilities of connected HDMI display */ - u8 cea_rev; + struct drm_hdmi_info hdmi_info; }; int drm_display_info_set_bus_formats(struct drm_display_info *info,
This patch creates a new structure drm_hdmi_info (inspired from drm_display_info). Driver will parse HDMI sink's advance capabilities from HF-VSDB and populate this structure. This structure will be kept and used as a sub-class within drm_display_info. We are adding parsing of HF-VSDB In the next patch. V3: Address review comments from Jose - Modify the usage of the structure drm_display_info in other drivers apart from I915 Cc: Thierry Reding <treding@nvidia.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jose Abreu <joabreu@synopsys.com> Suggested-by: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 2 +- drivers/gpu/drm/drm_edid.c | 6 +- drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_connector.h | 79 ++++++++++++++++++++++++-- 4 files changed, 79 insertions(+), 10 deletions(-)