[Intel-gfx] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions

Wed Dec 5 15:06:28 UTC 2018

On Wed, Dec 05, 2018 at 08:40:34AM +0100, Andrzej Hajda wrote:
> On 04.12.2018 20:02, Ville Syrjälä wrote:
> > On Tue, Dec 04, 2018 at 08:03:53AM +0100, Andrzej Hajda wrote:
> >> On 03.12.2018 22:48, Ville Syrjälä wrote:
> >>> On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
> >>>> Quite late, hopefully not too late.
> >>>>
> >>>>
> >>>> On 21.11.2018 12:51, Ville Syrjälä wrote:
> >>>>> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> >>>>>>>  		return;
> >>>>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> index a6e8f4591e63..0cc293a6ac24 100644
> >>>>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 *ctx,
> >>>>>>>  	int ret;
> >>>>>>>  
> >>>>>>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi,
> >>>>>>> -						       mode,
> >>>>>>> -						       true);
> >>>>>>> +						       NULL, mode);
> >>>>>>>  	if (ctx->use_packed_pixel)
> >>>>>>>  		frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >>>>>>>  
> >>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> index 64c3cf027518..88b720b63126 100644
> >>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> @@ -1344,7 +1344,8 @@ 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, false);
> >>>>>>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame,
> >>>>>>> +						 &hdmi->connector, mode);
> >>>>>>>  
> >>>>>>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >>>>>>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>>>>> index b506e3622b08..501ac05ba7da 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>>>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector *connector,
> >>>>>>>  }
> >>>>>>>  EXPORT_SYMBOL(drm_set_preferred_mode);
> >>>>>>>  
> >>>>>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
> >>>>>> You're usually known for adding const all around, why not const pointer
> >>>>>> here and in all the other drm_* functions that call this?
> >>>>> My current approach is to constify states/fbs/etc. but not so much
> >>>>> crtcs/connectors/etc. Too much const can sometimes get in the way
> >>>>> of things requiring that you remove the const later. But I guess
> >>>>> in this case the const shouldn't really get in the way of anything
> >>>>> because these are pretty much supposed to be pure functions.
> >>>>>
> >>>>>>> +{
> >>>>>>> +	/*
> >>>>>>> +	 * FIXME: sil-sii8620 doesn't have a connector around when
> >>>>>>> +	 * we need one, so we have to be prepared for a NULL connector.
> >>>>>>> +	 */
> >>>>>>> +	if (!connector)
> >>>>>>> +		return false;
> >>>>>> This actually changes the is_hdmi2_sink value for sil-sii8620.
> >>>>> Hmm. No idea why they would have set that to true when everyone else is
> >>>>> passing false. 
> >>>> Because false does not work :) More precisely MHLv3 (used in Sii8620)
> >>>> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
> >>>>
> >>>> Unfortunately I have no access to MHL specs, but my experiments and
> >>>> vendor drivers strongly suggests it is done this way.
> >>>>
> >>>> This is important in case of 4K modes which are handled differently by
> >>>> HDMI 1.4 and HDMI2.0.
> >>> HDMI 2.0 handles 4k just like 1.4 handled it when you use one of
> >>> the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we
> >>> switch over to the HDMI 2.0 specific signalling.
> >>
> >> The difference is in infoframes:
> >>
> >> HDMI 1.4 sets AVI infoframe VIC to 0, and sends HDMI_VIC in VSI.
> >>
> >> HDMI 2.0 sets AVI infoframe to non zero VICs introduced by
> >> HDMI2.0/CEA-861-F, VSI can be omitted if I remember correctly, unless 3d
> >> is in use.
> > Like I said, The HDMI 1.4 method is used even with HDMI 2.0 sinks unless
> > some feature gets used which can't be signalled via the HDMI 1.4 vendor
> > specific infoframe.
> 
> 
> Do you mean that 4K VICs 95, 94, 93, 98 defined in CEA-861-F are not
> used at all for non-3d video in HDMI 2.0?
> 
> Chapter 10.1 of HDMI2.0 spec says clearly:
> 
> > When transmitting any additional Video Format for which a VIC value
> > has been defined in
> > CEA-861-F tables 1, 2, and 3, an HDMI Source shall set the VIC field
> > to the Video Code for
> > that format.
> 
> 
> It contradicts your statement, or am I missing something?

IIRC the spec said you can only use either one or the other.

Section 10.2 doesn't explicity say that the AVI VIC must be zero when
HDMI_VIC is non-zero, but it does more or less imply it. And looks
like appendix E outright forbids having both as non-zero.

> 
> 
> >
> >>
> >> So setting VICs to non-zero in case of HDMI1.4 sinks and 4k modes seems
> >> risky.
> > That is not what I was proposing.
> 
> 
> Maybe I have misread your statement:
> 
> On 21.11.2018 12:51, Ville Syrjälä wrote:
> 
> > That said, I was actually thinking of removing this hdmi2 vs. not
> > stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> > "just in case", but we already have a similar issue with earlier
> > cea-861 revisions and haven't seen any bugs where an older monitor
> > would get confused by a VIC not yet defined when the monitor was
> > produced.
> 
> 
> I do not know what it could mean other that allowing sending VICs
> unknown to sink?

Yes it might send unknown VICs to sinks. But that happens anyway becasue
we can't tell which VICs the sink supports. We can only try to deduce
the spec version indirectly which is not very robust. And we only do that
for CTA-861-F, earlier revisions of the spec we don't try to deduce at all.
And alternative might be to send VICs only for those modes that the sink
actually declared in the EDID, but that opens up the question of whether
we should only do that for modes that have an SVD, or whether we should
do it for modes we get by other means (eg. detailed timing descriptors).

What I wasn't suggesting was to send AVI VIC != 0 + HDMI_VIC != 0
because that is not legal.

> 
> Maybe better would be just to wait for the patch to avoid misunderstandings.
> 
> 
> Regards
> 
> Andrzej
> 
> 
> >
> >>
> >> Regards
> >>
> >> Andrzej
> >>
> >>
> >>>> The pipeline looks like (in parenthesis HDMI version on the stream):
> >>>>
> >>>> exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV
> >>>>
> >>>>
> >>>>> I guess I can change this to true to not change it. IIRC
> >>>>> that was the only driver that didn't have a connector around.
> >>>>>
> >>>>> That said, I was actually thinking of removing this hdmi2 vs. not
> >>>>> stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> >>>>> "just in case", but we already have a similar issue with earlier
> >>>>> cea-861 revisions and haven't seen any bugs where an older monitor
> >>>>> would get confused by a VIC not yet defined when the monitor was
> >>>>> produced.
> >>>>>
> >>>> Are you sure this is a good idea? As I said earlier 4K modes are present
> >>>> in HDMI 1.4 and 2.0 but they are handled differently, so this is not
> >>>> only matter of unknown VIC in AVIF.
> >>>>
> >>>>
> >>>> Regards
> >>>>
> >>>> Andrzej
> 

-- 
Ville Syrjälä
Intel