Re: [PATCH v4 2/2] drm: Add HDMI 2.0 VIC support for AVI info-frames

[Jose Abreu <Jose.Abreu@synopsys.com>]

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@linux.intel.com>
>>>>>>>>> Cc: Jose Abreu <jose.abreu@synopsys.com>
>>>>>>>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>>>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>>>>> Cc: Daniel Vetter <daniel.vetter@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@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. 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx