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

[Andrzej Hajda]

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.

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