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

From: Andrzej Hajda <a.hajda@samsung.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: amd-gfx@lists.freedesktop.org, nouveau@lists.freedesktop.org,
	intel-gfx@lists.freedesktop.org,
	"Seung-Woo Kim" <sw0312.kim@samsung.com>,
	"Russell King" <linux@armlinux.org.uk>,
	dri-devel@lists.freedesktop.org,
	"Vincent Abriou" <vincent.abriou@st.com>,
	linux-tegra@vger.kernel.org,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Tomi Valkeinen" <tomi.valkeinen@ti.com>,
	linux-arm-msm@vger.kernel.org,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Shawn Guo" <shawnguo@kernel.org>,
	"Ben Skeggs" <bskeggs@redhat.com>,
	freedreno@lists.freedesktop.org,
	"Christian König" <christian.koenig@amd.com>,
	"Laurent Pinchart" <Laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
Date: Wed, 5 Dec 2018 08:40:34 +0100	[thread overview]
Message-ID: <239ce5d3-3959-7926-7c0e-26997ec4e5ee@samsung.com> (raw)
In-Reply-To: <20181204190234.GL9144@intel.com>

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?

>
>>
>> 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?

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx