Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

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.

> 
> 
> So setting VICs to non-zero in case of HDMI1.4 sinks and 4k modes seems
> risky.

That is not what I was proposing.

> 
> 
> 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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno