Nouveau - Dec 2018 - [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions

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

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

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