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

Thu Mar 23 16:43:07 UTC 2017

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://cgit.freedesktop.org/~airlied/linux/tree/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c?h=drm-next#n1740)
>
> 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 ?)

Regards
Shashank