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

[Andrzej Hajda]

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.


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


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