[Intel-gfx] [PATCH v4 2/2] drm: Add HDMI 2.0 VIC support for AVI info-frames
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Mar 23 18:14:18 UTC 2017
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.
> 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.
> 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/
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list