Hi Ville, On 23-03-2017 18:14, Ville Syrjälä wrote: > On Thu, Mar 23, 2017 at 05:53:34PM +0000, Jose Abreu wrote: >> 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. > I don't think that's a good plan. We already hit a massive snag with > the aspect ratio uapi changes. So we should aim for something that > requires no userspace changes. For this stuff I see no reason to > bother userspace with these details. A mode is just a mode, whether > it was specified in HDMI 2.0 or DMT or CVT is quite irrelevant. I agree that we should aim for no userspace changes but sometimes they are necessary. Anyway, I was forgetting that the new aspect ratios will not be exported, so I guess we are safe for now. > >> 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, > But that needs the connector, which some drivers don't have ready > at hand. And we already have that information pre-parsed in the > display_info so we pretty much already have what you suggest. > The only thing is that we require the driver to get that information > since the infoframe code doesn't know how to do that for every > driver. Right, so, no connector :/ I can check if dw-hdmi is in this condition and send a patch to adjust the flag. Best regards, Jose Miguel Abreu > >> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_dri-2Ddevel_2017-2DMarch_136596.html&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=7cX3HPDl6xffu7Zfoz30XBwhOyvIZ1RzTTb1z35ziV0&e= >> [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640215_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=ZHn9oCaMsLmPoaHbthOZXA8x8ixy2jik_3ZZRrjNtOs&e= >> [3] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640219_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=Xk-h91zq4XWDElWy9unrTER7BgryuUyKBb6dOusgAVw&e= >> [4] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640207_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=ByJ1jqHxbvkcZp64T1FFO9C_MljhDpJzVQgPIywiQ74&e= >> [5] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640217_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=giBMu5vbko6VWhq-Ob90DDBhEcpRqFLeSgZmeNnOEX8&e= >> [6] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9640205_&d=DwIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=IHo0Vkqnru9M6CloOKR-DrbSSmwnEHmThqrw9onMZ2o&s=cAN-hG4zuHeWTJSYg1ID2hCprnYuKeZDDtxIKsy81j8&e= _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
