Hi Shashank, On 23-03-2017 16:43, Sharma, Shashank wrote: > Regards > > Shashank > > > On 3/23/2017 6:33 PM, Jose Abreu wrote: >> Hi Shashank, >> >> >> On 23-03-2017 16:08, Sharma, Shashank wrote: >>> Regards >>> >>> Shashank >>> >>> >>> On 3/23/2017 5:57 PM, Jose Abreu wrote: >>>> Hi Ville, >>>> >>>> >>>> On 23-03-2017 15:45, Ville Syrjälä wrote: >>>>> On Thu, Mar 23, 2017 at 03:28:29PM +0000, Jose Abreu wrote: >>>>>> Hi Shashank, >>>>>> >>>>>> >>>>>> On 23-03-2017 15:14, Shashank Sharma wrote: >>>>>>> HDMI 1.4b support the CEA video modes as per range of >>>>>>> CEA-861-D (VIC 1-64). >>>>>>> For any other mode, the VIC filed in AVI infoframes should >>>>>>> be 0. >>>>>>> HDMI 2.0 sinks, support video modes range as per CEA-861-F >>>>>>> spec, which is >>>>>>> extended to (VIC 1-107). >>>>>>> >>>>>>> This patch adds a bool input variable, which indicates if >>>>>>> the connected >>>>>>> sink is a HDMI 2.0 sink or not. This will make sure that we >>>>>>> don't pass a >>>>>>> HDMI 2.0 VIC to a HDMI 1.4 sink. >>>>>>> >>>>>>> This patch touches all drm drivers, who are callers of this >>>>>>> function >>>>>>> drm_hdmi_avi_infoframe_from_display_mode but to make sure >>>>>>> there is >>>>>>> no change in current behavior, is_hdmi2 is kept as false. >>>>>>> >>>>>>> In case of I915 driver, this patch checks the >>>>>>> connector->display_info >>>>>>> to check if the connected display is HDMI 2.0. >>>>>>> >>>>>>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> >>>>>>> Cc: Jose Abreu <jose.abreu@xxxxxxxxxxxx> >>>>>>> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx> >>>>>>> Cc: Alex Deucher <alexander.deucher@xxxxxxx> >>>>>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> >>>>>>> >>>>>>> PS: This patch touches a few lines in few files, which were >>>>>>> already above 80 char, so checkpatch gives 80 char warning >>>>>>> again. >>>>>>> - gpu/drm/omapdrm/omap_encoder.c >>>>>>> - gpu/drm/i915/intel_sdvo.c >>>>>>> >>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> >>>>>>> --- >>>>>>> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +- >>>>>>> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +- >>>>>>> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- >>>>>>> drivers/gpu/drm/bridge/analogix-anx78xx.c | 3 ++- >>>>>>> drivers/gpu/drm/bridge/sii902x.c | 2 +- >>>>>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- >>>>>>> drivers/gpu/drm/drm_edid.c | 12 >>>>>>> +++++++++++- >>>>>>> drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- >>>>>>> drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- >>>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 5 ++++- >>>>>>> drivers/gpu/drm/i915/intel_sdvo.c | 3 ++- >>>>>>> drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- >>>>>>> drivers/gpu/drm/omapdrm/omap_encoder.c | 3 ++- >>>>>>> drivers/gpu/drm/radeon/radeon_audio.c | 2 +- >>>>>>> drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +- >>>>>>> drivers/gpu/drm/sti/sti_hdmi.c | 2 +- >>>>>>> drivers/gpu/drm/tegra/hdmi.c | 2 +- >>>>>>> drivers/gpu/drm/tegra/sor.c | 2 +- >>>>>>> drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- >>>>>>> drivers/gpu/drm/zte/zx_hdmi.c | 2 +- >>>>>>> include/drm/drm_edid.h | 3 ++- >>>>>>> 21 files changed, 38 insertions(+), 21 deletions(-) >>>>>>> >>>>>> [snip] >>>>>> >>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>>>>> index af93f7a..5ff2886 100644 >>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>>>>> @@ -1146,7 +1146,7 @@ static void hdmi_config_AVI(struct >>>>>>> dw_hdmi *hdmi, struct drm_display_mode *mode) >>>>>>> u8 val; >>>>>>> /* Initialise info frame from DRM mode */ >>>>>>> - drm_hdmi_avi_infoframe_from_display_mode(&frame, mode); >>>>>>> + drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, >>>>>>> false); >>>>>>> if (hdmi->hdmi_data.enc_out_format == YCBCR444) >>>>>>> frame.colorspace = HDMI_COLORSPACE_YUV444; >>>>>>> >>>>>> dw-hdmi controller has full support for HDMI 2.0 features. >>>>>> It all >>>>>> depends on the platform it is integrated. >>>>>> >>>>>> I think adding a parameter to >>>>>> drm_hdmi_avi_infoframe_from_display_mode is not the best idea >>>>>> because of this case: A bridge can have support for HDMI 2.0 >>>>>> features but the platform may limit this support. I guess it >>>>>> can >>>>>> happen in other drivers too. >>>>> Your driver is in full control of what gets passed here. So I >>>>> don't see >>>>> why that would be a problem. >>>>> >>>>> Also this doesn't really have anything to do with the >>>>> capabilities of >>>>> the source. All we want to make sure is that we don't send a >>>>> VIC the >>>>> sink will not understand. >>>>> >>>> But the driver is not aware of the platform limitations, its >>>> generic to the controller only. We could add a field in pdata >>>> which tells if platform is HDMI 2.0+ but what about other >>>> bridge >>>> drivers or HDMI drivers? They will have to replicate the same >>>> thing also. >>>> >>>> Best regards, >>>> Jose Miguel Abreu >>> I think the driver would be aware of the platform's >>> capabilities, isn't it ? >>> Else how would it even decide which mode to allow, and which to >>> reject ? >> The DRM core propagates the mode to the chain of configuration >> before reaching the bridge driver also, there is a callback >> supplied by pdata (mode_valid) which can check if the mode is >> valid. (see >> https://urldefense.proofpoint.com/v2/url?u=https-3A__cgit.freedesktop.org_-7Eairlied_linux_tree_drivers_gpu_drm_bridge_synopsys_dw-2Dhdmi.c-3Fh-3Ddrm-2Dnext-23n1740&d=DwID-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zC1cWRy6RDWimQG2o866yYylV7c5h_U4aGWjVtnIHH0&s=DPZA3OgLWOO299hjy9dBDBK66kmUGfZArxKbeHNMKOc&e= >> ) >> >> Best regards, >> Jose Miguel Abreu >> >> >>> Regards >>> Shashank > Please correct me if my understanding is not right, but drivers > call mode_valid() to prune/reject modes which they cant support. > and they call drm_set_infoframe_from_videomode() function, when > they are going ahead to the modeset with a mode. > Why would a driver choose to do a modeset, which it could not > support (shouldn't mode_valid have dropped it in atomic_check() > or may be even edid_parsing time ?) Yes, thats all correct :) I got a little out of topic here, sorry. The thing I don't agree with is the adding of the parameter for the drm_hdmi_avi_infoframe_from_display_mode(), because I think its not enough. I will state why: Picture this scenario: 1) EDID is read from HDMI 2.0 sink 2) The 2.0 modes are probed and as there is no check for these modes (they are in the CEA table) they will be added to mode list 3) The modes are passed to userspace. This is a problem because userspace does not yet understand the new aspect ratios, but lets imagine it understands 4) User selects an HDMI 2.0 mode 5) The drm core checks if mode is valid with driver, the driver can say yes or no. Lets think it says yes because the platform supports HDMI 2.0 modes. 6) Mode is committed but VIC will be set to zero according to the patch Now, according to 2.0 spec VIC shall be set when we are in 2.0 modes. I didn't do much inter-op with HDMI 2.0 receivers but I can picture that some of them may just not display anything because VIC and HDMI_VIC are not set. Though, from my experience, generally the receiver is much more robust than the transmitter. Even so, I think we should make sure that transmitter drivers are fully compliant so that we can guarantee the support for less robust receivers. Best regards, Jose Miguel Abreu > > Regards > Shashank _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
