[Intel-gfx] [PATCH v4 2/2] drm: Add HDMI 2.0 VIC support for AVI info-frames
Jose Abreu
Jose.Abreu at synopsys.com
Thu Mar 23 18:35:32 UTC 2017
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 at linux.intel.com>
>>>>>>>>>>> Cc: Jose Abreu <jose.abreu at synopsys.com>
>>>>>>>>>>> Cc: Andrzej Hajda <a.hajda at samsung.com>
>>>>>>>>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>>>>>>>>>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>>>>>>>>>>>
>>>>>>>>>>> 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 at intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>> 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=
More information about the Intel-gfx
mailing list