Hi Ville, On 23-03-2017 17:42, Ville Syrjälä wrote: > On Thu, Mar 23, 2017 at 05:11:44PM +0000, Jose Abreu wrote: >> 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 > That's only because you haven't updated your driver to pass 'true' > as needed. > > The other option would be to pass the connector instead of a boolean, > but according to Shashank that would required a lot of plumbing changes > because many drivers don't have the connector immediately available > where they call these functions. And plumbing the connector through > probably requires intimate knowledge of each driver, and thus is not > something we would be confortable doing. Hence I think the bool is the > best short term solution, letting each driver author figure out how to > get at the right connector and pass on the required information. > > I don't really understand what you're even suggesting as an alternative. > We need to know what the sink supports, and if your driver is architected > in some crazy way where you don't even have that information where it's > needed, I don't think there's anything anyone can do. > I submitted a RFC yesterday where there was an alternative [1][2][3][4][5][6]: Don't expose 2.0 modes to drivers and to userspace unless asked to. The infoframe part is a different topic though. We could add a helper in the DRM EDID which checks if sink is 2.0 and then, in conjunction with the flag I introduced in the RFC patch 4/5 we would know that driver supports 2.0 and sink is 2.0, then we would eliminate the bool. What do you think? Best regards, Jose Miguel Abreu [1] https://lists.freedesktop.org/archives/dri-devel/2017-March/136596.html [2] https://patchwork.kernel.org/patch/9640215/ [3] https://patchwork.kernel.org/patch/9640219/ [4] https://patchwork.kernel.org/patch/9640207/ [5] https://patchwork.kernel.org/patch/9640217/ [6] https://patchwork.kernel.org/patch/9640205/ _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
