Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Wed, Dec 05, 2018 at 08:40:34AM +0100, Andrzej Hajda wrote:
> 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(,
> >>> -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(, mode, false);
> >>> + drm_hdmi_avi_infoframe_from_display_mode(,
> >>> + >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
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Wed, Dec 05, 2018 at 09:46:40AM +0100, Andrzej Hajda wrote: > On 05.12.2018 07:32, Laurent Pinchart wrote: > > Hi Ville, > > > > On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote: > >> On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote: > >>> On 03.12.2018 22:38, Ville Syrjälä wrote: > On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > > On 21.11.2018 19:19, Laurent Pinchart wrote: > >> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > >>> From: Ville Syrjälä > >>> > >>> Make life easier for drivers by simply passing the connector > >>> to drm_hdmi_avi_infoframe_from_display_mode() and > >>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > >>> need to worry about is_hdmi2_sink mess. > >> While this is good for display controller drivers, the change isn't > >> great for bridge drivers. Down the road we're looking at moving > >> connector support out of the bridge drivers. Adding an additional > >> dependency to connectors in the bridges will make that more > >> difficult. Ideally bridges should retrieve the information from their > >> sink, regardless of whether it is a connector or another bridge. > > I agree with it, and case of sii8620 shows that there are cases where > > bridge has no direct access to the connector. > It's just a matter of plumbing it through. > >>> What do you mean exactly? > >> void bridge_foo(... > >> + ,struct drm_connector *connector); > >> > > On the other side, since you are passing connector to > > drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > > parameter and rename the function to > > drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > > mode set on the connector differs? > Connectors don't have a mode. > >>> As they are passing video stream they should have it, even if not > >>> directly, for example: > >>> > >>> connector->state->crtc->mode > >> That's not really how atomic works. One shouldn't go digging > >> through the obj->state pointers when we're not holding the > >> relevant locks anymore. The atomic way would be to pass either > >> both crtc state and connector state, or drm_atomic_state + > >> crtc/connector. > > > Usually infoframe contents is generated in modesetting/enable callbacks > so the locks should be in place. Not when doing a nonblocking modeset. > > And the locks should be hold for > drm_hdmi_avi_infoframe_from_display_mode as well - it wouldn't be correct if > > generated infoframe is not relevant to actual video mode. I guess that > if connector->state->crtc->mode > > differs from mode passed to drm_hdmi_avi_infoframe_from_display_mode it > is a sign of possible problem. > > And if they do not differ passing mode as an extra argument is redundant. > > > > Or a bridge state ? With chained bridges the mode can vary along the > > pipeline, > > the CRTC adjusted mode will only cover the link between the CRTC and the > > first > > bridge. It's only a matter of time until we need to store other > > intermediate > > modes in states. I'd rather prepare for that instead of passing the CRTC > > state > > to bridges. > > > Yes, optional bridge states seems reasonable, volunteers needed :) > > > Regards > > Andrzej > > > > > >>> In moment of creating infoframe it should be set properly. > >>> > >> Please see below for an additional comment. > >> > >>> Cc: Alex Deucher > >>> Cc: "Christian König" > >>> Cc: "David (ChunMing) Zhou" > >>> Cc: Archit Taneja > >>> Cc: Andrzej Hajda > >>> Cc: Laurent Pinchart > >>> Cc: Inki Dae > >>> Cc: Joonyoung Shim > >> Cc: Seung-Woo Kim > >>> Cc: Kyungmin Park > >>> Cc: Russell King > >>> Cc: CK Hu > >>> Cc: Philipp Zabel > >>> Cc: Rob Clark > >>> Cc: Ben Skeggs > >>> Cc: Tomi Valkeinen > >>> Cc: Sandy Huang > >>> Cc: "Heiko Stübner" > >>> Cc: Benjamin Gaignard > >>> Cc: Vincent Abriou > >>> Cc: Thierry Reding > >>> Cc: Eric Anholt > >>> Cc: Shawn Guo > >>> Cc: Ilia Mirkin > >>> Cc: amd-...@lists.freedesktop.org > >>> Cc: linux-arm-...@vger.kernel.org > >>> Cc: freedr...@lists.freedesktop.org > >>> Cc: nouv...@lists.freedesktop.org > >>> Cc: linux-te...@vger.kernel.org > >>> Signed-off-by: Ville Syrjälä > >>> --- > >>> > >>> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > >>> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > >>> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > >>> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > >>> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > >>> drivers/gpu/drm/bridge/sii902x.c | 3 ++- > >>> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > >>>
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, Nov 20, 2018 at 06:13:42PM +0200, Ville Syrjala wrote:
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..38c66fbc8276 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -849,7 +849,8 @@ tda998x_write_avi(struct tda998x_priv *priv, struct
> drm_display_mode *mode)
> {
> union hdmi_infoframe frame;
>
> - drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> + drm_hdmi_avi_infoframe_from_display_mode(,
> + >connector, mode);
> frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
>
> tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, );
For this,
Acked-by: Russell King
Thanks.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
Hi Andrzej, On Wednesday, 5 December 2018 10:46:40 EET Andrzej Hajda wrote: > On 05.12.2018 07:32, Laurent Pinchart wrote: > > On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote: > >> On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote: > >>> On 03.12.2018 22:38, Ville Syrjälä wrote: > On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > > On 21.11.2018 19:19, Laurent Pinchart wrote: > >> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > >>> From: Ville Syrjälä > >>> > >>> Make life easier for drivers by simply passing the connector > >>> to drm_hdmi_avi_infoframe_from_display_mode() and > >>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > >>> need to worry about is_hdmi2_sink mess. > >> > >> While this is good for display controller drivers, the change isn't > >> great for bridge drivers. Down the road we're looking at moving > >> connector support out of the bridge drivers. Adding an additional > >> dependency to connectors in the bridges will make that more > >> difficult. Ideally bridges should retrieve the information from their > >> sink, regardless of whether it is a connector or another bridge. > > > > I agree with it, and case of sii8620 shows that there are cases where > > bridge has no direct access to the connector. > > It's just a matter of plumbing it through. > >>> > >>> What do you mean exactly? > >> > >> void bridge_foo(... > >> + ,struct drm_connector *connector); > >> > > On the other side, since you are passing connector to > > drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > > parameter and rename the function to > > drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > > mode set on the connector differs? > > Connectors don't have a mode. > >>> > >>> As they are passing video stream they should have it, even if not > >>> directly, for example: > >>> > >>> connector->state->crtc->mode > >> > >> That's not really how atomic works. One shouldn't go digging > >> through the obj->state pointers when we're not holding the > >> relevant locks anymore. The atomic way would be to pass either > >> both crtc state and connector state, or drm_atomic_state + > >> crtc/connector. > > Usually infoframe contents is generated in modesetting/enable callbacks > so the locks should be in place. > > And the locks should be hold for > drm_hdmi_avi_infoframe_from_display_mode as well - it wouldn't be correct if > > generated infoframe is not relevant to actual video mode. I guess that > if connector->state->crtc->mode > > differs from mode passed to drm_hdmi_avi_infoframe_from_display_mode it > is a sign of possible problem. > > And if they do not differ passing mode as an extra argument is redundant. > > > Or a bridge state ? With chained bridges the mode can vary along the > > pipeline, the CRTC adjusted mode will only cover the link between the > > CRTC and the first bridge. It's only a matter of time until we need to > > store other intermediate modes in states. I'd rather prepare for that > > instead of passing the CRTC state to bridges. > > Yes, optional bridge states seems reasonable, volunteers needed :) I'll give it a go eventually, if nobody beats me to it. The exact timing will depend on many variables so I can't estimate it I'm afraid. All I'm asking is to avoid extending the drm_bridge API in a way that would make introduction of bridge states more complex. If someone needs bridge states, for instance to solve the above issue, then they should be added. [snip] -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On 05.12.2018 07:32, Laurent Pinchart wrote: > Hi Ville, > > On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote: >> On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote: >>> On 03.12.2018 22:38, Ville Syrjälä wrote: On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > On 21.11.2018 19:19, Laurent Pinchart wrote: >> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: >>> From: Ville Syrjälä >>> >>> Make life easier for drivers by simply passing the connector >>> to drm_hdmi_avi_infoframe_from_display_mode() and >>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't >>> need to worry about is_hdmi2_sink mess. >> While this is good for display controller drivers, the change isn't >> great for bridge drivers. Down the road we're looking at moving >> connector support out of the bridge drivers. Adding an additional >> dependency to connectors in the bridges will make that more >> difficult. Ideally bridges should retrieve the information from their >> sink, regardless of whether it is a connector or another bridge. > I agree with it, and case of sii8620 shows that there are cases where > bridge has no direct access to the connector. It's just a matter of plumbing it through. >>> What do you mean exactly? >> void bridge_foo(... >> + ,struct drm_connector *connector); >> > On the other side, since you are passing connector to > drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > parameter and rename the function to > drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > mode set on the connector differs? Connectors don't have a mode. >>> As they are passing video stream they should have it, even if not >>> directly, for example: >>> >>> connector->state->crtc->mode >> That's not really how atomic works. One shouldn't go digging >> through the obj->state pointers when we're not holding the >> relevant locks anymore. The atomic way would be to pass either >> both crtc state and connector state, or drm_atomic_state + >> crtc/connector. Usually infoframe contents is generated in modesetting/enable callbacks so the locks should be in place. And the locks should be hold for drm_hdmi_avi_infoframe_from_display_mode as well - it wouldn't be correct if generated infoframe is not relevant to actual video mode. I guess that if connector->state->crtc->mode differs from mode passed to drm_hdmi_avi_infoframe_from_display_mode it is a sign of possible problem. And if they do not differ passing mode as an extra argument is redundant. > Or a bridge state ? With chained bridges the mode can vary along the > pipeline, > the CRTC adjusted mode will only cover the link between the CRTC and the > first > bridge. It's only a matter of time until we need to store other intermediate > modes in states. I'd rather prepare for that instead of passing the CRTC > state > to bridges. Yes, optional bridge states seems reasonable, volunteers needed :) Regards Andrzej > >>> In moment of creating infoframe it should be set properly. >>> >> Please see below for an additional comment. >> >>> Cc: Alex Deucher >>> Cc: "Christian König" >>> Cc: "David (ChunMing) Zhou" >>> Cc: Archit Taneja >>> Cc: Andrzej Hajda >>> Cc: Laurent Pinchart >>> Cc: Inki Dae >>> Cc: Joonyoung Shim >> Cc: Seung-Woo Kim >>> Cc: Kyungmin Park >>> Cc: Russell King >>> Cc: CK Hu >>> Cc: Philipp Zabel >>> Cc: Rob Clark >>> Cc: Ben Skeggs >>> Cc: Tomi Valkeinen >>> Cc: Sandy Huang >>> Cc: "Heiko Stübner" >>> Cc: Benjamin Gaignard >>> Cc: Vincent Abriou >>> Cc: Thierry Reding >>> Cc: Eric Anholt >>> Cc: Shawn Guo >>> Cc: Ilia Mirkin >>> Cc: amd-...@lists.freedesktop.org >>> Cc: linux-arm-...@vger.kernel.org >>> Cc: freedr...@lists.freedesktop.org >>> Cc: nouv...@lists.freedesktop.org >>> Cc: linux-te...@vger.kernel.org >>> Signed-off-by: Ville Syrjälä >>> --- >>> >>> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- >>> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- >>> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- >>> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- >>> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- >>> drivers/gpu/drm/bridge/sii902x.c | 3 ++- >>> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- >>> drivers/gpu/drm/drm_edid.c| 33 >>> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- >>> drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- >>> drivers/gpu/drm/i915/intel_hdmi.c | 14 +- >>> drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- >>> drivers/gpu/drm/i915/intel_sdvo.c
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
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(,
>>> - 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(, mode, false);
>>> + drm_hdmi_avi_infoframe_from_display_mode(,
>>> +>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
>>
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
Hi Ville, On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote: > On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote: > > On 03.12.2018 22:38, Ville Syrjälä wrote: > >> On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > >>> On 21.11.2018 19:19, Laurent Pinchart wrote: > On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > > From: Ville Syrjälä > > > > Make life easier for drivers by simply passing the connector > > to drm_hdmi_avi_infoframe_from_display_mode() and > > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > > need to worry about is_hdmi2_sink mess. > > While this is good for display controller drivers, the change isn't > great for bridge drivers. Down the road we're looking at moving > connector support out of the bridge drivers. Adding an additional > dependency to connectors in the bridges will make that more > difficult. Ideally bridges should retrieve the information from their > sink, regardless of whether it is a connector or another bridge. > >>> > >>> I agree with it, and case of sii8620 shows that there are cases where > >>> bridge has no direct access to the connector. > >> > >> It's just a matter of plumbing it through. > > > > What do you mean exactly? > > void bridge_foo(... > + ,struct drm_connector *connector); > > >>> On the other side, since you are passing connector to > >>> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > >>> parameter and rename the function to > >>> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > >>> mode set on the connector differs? > >> > >> Connectors don't have a mode. > > > > As they are passing video stream they should have it, even if not > > directly, for example: > > > > connector->state->crtc->mode > > That's not really how atomic works. One shouldn't go digging > through the obj->state pointers when we're not holding the > relevant locks anymore. The atomic way would be to pass either > both crtc state and connector state, or drm_atomic_state + > crtc/connector. Or a bridge state ? With chained bridges the mode can vary along the pipeline, the CRTC adjusted mode will only cover the link between the CRTC and the first bridge. It's only a matter of time until we need to store other intermediate modes in states. I'd rather prepare for that instead of passing the CRTC state to bridges. > > In moment of creating infoframe it should be set properly. > > > Please see below for an additional comment. > > > Cc: Alex Deucher > > Cc: "Christian König" > > Cc: "David (ChunMing) Zhou" > > Cc: Archit Taneja > > Cc: Andrzej Hajda > > Cc: Laurent Pinchart > > Cc: Inki Dae > > Cc: Joonyoung Shim > Cc: Seung-Woo Kim > > Cc: Kyungmin Park > > Cc: Russell King > > Cc: CK Hu > > Cc: Philipp Zabel > > Cc: Rob Clark > > Cc: Ben Skeggs > > Cc: Tomi Valkeinen > > Cc: Sandy Huang > > Cc: "Heiko Stübner" > > Cc: Benjamin Gaignard > > Cc: Vincent Abriou > > Cc: Thierry Reding > > Cc: Eric Anholt > > Cc: Shawn Guo > > Cc: Ilia Mirkin > > Cc: amd-...@lists.freedesktop.org > > Cc: linux-arm-...@vger.kernel.org > > Cc: freedr...@lists.freedesktop.org > > Cc: nouv...@lists.freedesktop.org > > Cc: linux-te...@vger.kernel.org > > Signed-off-by: Ville Syrjälä > > --- > > > > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > > drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > > drivers/gpu/drm/bridge/sii902x.c | 3 ++- > > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > > drivers/gpu/drm/drm_edid.c| 33 > > drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > > drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > > drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > > drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > > drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > > drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > > drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > > drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > > drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > > drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > > drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > > drivers/gpu/drm/tegra/hdmi.c | 3 ++- > > drivers/gpu/drm/tegra/sor.c | 3 ++- > >
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote: > On 03.12.2018 22:38, Ville Syrjälä wrote: > > On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > >> On 21.11.2018 19:19, Laurent Pinchart wrote: > >>> Hi Ville, > >>> > >>> Thank you for the patch. > >>> > >>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > From: Ville Syrjälä > > Make life easier for drivers by simply passing the connector > to drm_hdmi_avi_infoframe_from_display_mode() and > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > need to worry about is_hdmi2_sink mess. > >>> While this is good for display controller drivers, the change isn't great > >>> for > >>> bridge drivers. Down the road we're looking at moving connector support > >>> out of > >>> the bridge drivers. Adding an additional dependency to connectors in the > >>> bridges will make that more difficult. Ideally bridges should retrieve > >>> the > >>> information from their sink, regardless of whether it is a connector or > >>> another bridge. > >> > >> I agree with it, and case of sii8620 shows that there are cases where > >> bridge has no direct access to the connector. > > It's just a matter of plumbing it through. > > > What do you mean exactly? void bridge_foo(... + ,struct drm_connector *connector); > > > > > >> On the other side, since you are passing connector to > >> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > >> parameter and rename the function to > >> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > >> mode set on the connector differs? > > Connectors don't have a mode. > > > As they are passing video stream they should have it, even if not > directly, for example: > > connector->state->crtc->mode That's not really how atomic works. One shouldn't go digging through the obj->state pointers when we're not holding the relevant locks anymore. The atomic way would be to pass either both crtc state and connector state, or drm_atomic_state + crtc/connector. > > In moment of creating infoframe it should be set properly. > > > Regards > > Andrzej > > > > > >> > >> Regards > >> > >> Andrzej > >> > >> > >>> Please see below for an additional comment. > >>> > Cc: Alex Deucher > Cc: "Christian König" > Cc: "David (ChunMing) Zhou" > Cc: Archit Taneja > Cc: Andrzej Hajda > Cc: Laurent Pinchart > Cc: Inki Dae > Cc: Joonyoung Shim > Cc: Seung-Woo Kim > Cc: Kyungmin Park > Cc: Russell King > Cc: CK Hu > Cc: Philipp Zabel > Cc: Rob Clark > Cc: Ben Skeggs > Cc: Tomi Valkeinen > Cc: Sandy Huang > Cc: "Heiko Stübner" > Cc: Benjamin Gaignard > Cc: Vincent Abriou > Cc: Thierry Reding > Cc: Eric Anholt > Cc: Shawn Guo > Cc: Ilia Mirkin > Cc: amd-...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: linux-te...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > drivers/gpu/drm/bridge/sii902x.c | 3 ++- > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > drivers/gpu/drm/drm_edid.c| 33 ++- > drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > drivers/gpu/drm/tegra/hdmi.c | 3 ++- > drivers/gpu/drm/tegra/sor.c | 3 ++- > drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- > drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- > include/drm/drm_edid.h| 8 +++--- > 27 files changed, 94 insertions(+), 66 deletions(-) > >>> For dw-hdmi and omapdrm, > >>> > >>> Reviewed-by: Laurent Pinchart > >>> -- Ville Syrjälä Intel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
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(,
> > - 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(, mode, false);
> > + drm_hdmi_avi_infoframe_from_display_mode(,
> > +>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
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On 03.12.2018 22:38, Ville Syrjälä wrote: > On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: >> On 21.11.2018 19:19, Laurent Pinchart wrote: >>> Hi Ville, >>> >>> Thank you for the patch. >>> >>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: From: Ville Syrjälä Make life easier for drivers by simply passing the connector to drm_hdmi_avi_infoframe_from_display_mode() and drm_hdmi_avi_infoframe_quant_range(). That way drivers don't need to worry about is_hdmi2_sink mess. >>> While this is good for display controller drivers, the change isn't great >>> for >>> bridge drivers. Down the road we're looking at moving connector support out >>> of >>> the bridge drivers. Adding an additional dependency to connectors in the >>> bridges will make that more difficult. Ideally bridges should retrieve the >>> information from their sink, regardless of whether it is a connector or >>> another bridge. >> >> I agree with it, and case of sii8620 shows that there are cases where >> bridge has no direct access to the connector. > It's just a matter of plumbing it through. What do you mean exactly? > >> On the other side, since you are passing connector to >> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode >> parameter and rename the function to >> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and >> mode set on the connector differs? > Connectors don't have a mode. As they are passing video stream they should have it, even if not directly, for example: connector->state->crtc->mode In moment of creating infoframe it should be set properly. Regards Andrzej > >> >> Regards >> >> Andrzej >> >> >>> Please see below for an additional comment. >>> Cc: Alex Deucher Cc: "Christian König" Cc: "David (ChunMing) Zhou" Cc: Archit Taneja Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Inki Dae Cc: Joonyoung Shim Cc: Seung-Woo Kim Cc: Kyungmin Park Cc: Russell King Cc: CK Hu Cc: Philipp Zabel Cc: Rob Clark Cc: Ben Skeggs Cc: Tomi Valkeinen Cc: Sandy Huang Cc: "Heiko Stübner" Cc: Benjamin Gaignard Cc: Vincent Abriou Cc: Thierry Reding Cc: Eric Anholt Cc: Shawn Guo Cc: Ilia Mirkin Cc: amd-...@lists.freedesktop.org Cc: linux-arm-...@vger.kernel.org Cc: freedr...@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Cc: linux-te...@vger.kernel.org Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- drivers/gpu/drm/bridge/sii902x.c | 3 ++- drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- drivers/gpu/drm/drm_edid.c| 33 ++- drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- drivers/gpu/drm/i915/intel_hdmi.c | 14 +- drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- drivers/gpu/drm/i915/intel_sdvo.c | 10 --- drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- drivers/gpu/drm/radeon/radeon_audio.c | 2 +- drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- drivers/gpu/drm/tegra/hdmi.c | 3 ++- drivers/gpu/drm/tegra/sor.c | 3 ++- drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- include/drm/drm_edid.h| 8 +++--- 27 files changed, 94 insertions(+), 66 deletions(-) >>> For dw-hdmi and omapdrm, >>> >>> Reviewed-by: Laurent Pinchart >>> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
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(,
> -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(, mode, false);
> + drm_hdmi_avi_infoframe_from_display_mode(,
> + >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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
Re: [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(,
> >>> -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(, mode, false);
> >>> + drm_hdmi_avi_infoframe_from_display_mode(,
> >>> + >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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > On 21.11.2018 19:19, Laurent Pinchart wrote: > > Hi Ville, > > > > Thank you for the patch. > > > > On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > >> From: Ville Syrjälä > >> > >> Make life easier for drivers by simply passing the connector > >> to drm_hdmi_avi_infoframe_from_display_mode() and > >> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > >> need to worry about is_hdmi2_sink mess. > > While this is good for display controller drivers, the change isn't great > > for > > bridge drivers. Down the road we're looking at moving connector support out > > of > > the bridge drivers. Adding an additional dependency to connectors in the > > bridges will make that more difficult. Ideally bridges should retrieve the > > information from their sink, regardless of whether it is a connector or > > another bridge. > > > I agree with it, and case of sii8620 shows that there are cases where > bridge has no direct access to the connector. It's just a matter of plumbing it through. > > On the other side, since you are passing connector to > drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > parameter and rename the function to > drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > mode set on the connector differs? Connectors don't have a mode. > > > Regards > > Andrzej > > > > > > Please see below for an additional comment. > > > >> Cc: Alex Deucher > >> Cc: "Christian König" > >> Cc: "David (ChunMing) Zhou" > >> Cc: Archit Taneja > >> Cc: Andrzej Hajda > >> Cc: Laurent Pinchart > >> Cc: Inki Dae > >> Cc: Joonyoung Shim > >> Cc: Seung-Woo Kim > >> Cc: Kyungmin Park > >> Cc: Russell King > >> Cc: CK Hu > >> Cc: Philipp Zabel > >> Cc: Rob Clark > >> Cc: Ben Skeggs > >> Cc: Tomi Valkeinen > >> Cc: Sandy Huang > >> Cc: "Heiko Stübner" > >> Cc: Benjamin Gaignard > >> Cc: Vincent Abriou > >> Cc: Thierry Reding > >> Cc: Eric Anholt > >> Cc: Shawn Guo > >> Cc: Ilia Mirkin > >> Cc: amd-...@lists.freedesktop.org > >> Cc: linux-arm-...@vger.kernel.org > >> Cc: freedr...@lists.freedesktop.org > >> Cc: nouv...@lists.freedesktop.org > >> Cc: linux-te...@vger.kernel.org > >> Signed-off-by: Ville Syrjälä > >> --- > >> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > >> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > >> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > >> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > >> drivers/gpu/drm/bridge/sii902x.c | 3 ++- > >> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > >> drivers/gpu/drm/drm_edid.c| 33 ++- > >> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > >> drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > >> drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > >> drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > >> drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > >> drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > >> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > >> drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > >> drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > >> drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > >> drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > >> drivers/gpu/drm/tegra/hdmi.c | 3 ++- > >> drivers/gpu/drm/tegra/sor.c | 3 ++- > >> drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- > >> drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- > >> include/drm/drm_edid.h| 8 +++--- > >> 27 files changed, 94 insertions(+), 66 deletions(-) > > For dw-hdmi and omapdrm, > > > > Reviewed-by: Laurent Pinchart > > -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On 21.11.2018 19:19, Laurent Pinchart wrote: > Hi Ville, > > Thank you for the patch. > > On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: >> From: Ville Syrjälä >> >> Make life easier for drivers by simply passing the connector >> to drm_hdmi_avi_infoframe_from_display_mode() and >> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't >> need to worry about is_hdmi2_sink mess. > While this is good for display controller drivers, the change isn't great for > bridge drivers. Down the road we're looking at moving connector support out > of > the bridge drivers. Adding an additional dependency to connectors in the > bridges will make that more difficult. Ideally bridges should retrieve the > information from their sink, regardless of whether it is a connector or > another bridge. I agree with it, and case of sii8620 shows that there are cases where bridge has no direct access to the connector. On the other side, since you are passing connector to drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode parameter and rename the function to drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and mode set on the connector differs? Regards Andrzej > > Please see below for an additional comment. > >> Cc: Alex Deucher >> Cc: "Christian König" >> Cc: "David (ChunMing) Zhou" >> Cc: Archit Taneja >> Cc: Andrzej Hajda >> Cc: Laurent Pinchart >> Cc: Inki Dae >> Cc: Joonyoung Shim >> Cc: Seung-Woo Kim >> Cc: Kyungmin Park >> Cc: Russell King >> Cc: CK Hu >> Cc: Philipp Zabel >> Cc: Rob Clark >> Cc: Ben Skeggs >> Cc: Tomi Valkeinen >> Cc: Sandy Huang >> Cc: "Heiko Stübner" >> Cc: Benjamin Gaignard >> Cc: Vincent Abriou >> Cc: Thierry Reding >> Cc: Eric Anholt >> Cc: Shawn Guo >> Cc: Ilia Mirkin >> Cc: amd-...@lists.freedesktop.org >> Cc: linux-arm-...@vger.kernel.org >> Cc: freedr...@lists.freedesktop.org >> Cc: nouv...@lists.freedesktop.org >> Cc: linux-te...@vger.kernel.org >> Signed-off-by: Ville Syrjälä >> --- >> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- >> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- >> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- >> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- >> drivers/gpu/drm/bridge/sii902x.c | 3 ++- >> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- >> drivers/gpu/drm/drm_edid.c| 33 ++- >> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- >> drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- >> drivers/gpu/drm/i915/intel_hdmi.c | 14 +- >> drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- >> drivers/gpu/drm/i915/intel_sdvo.c | 10 --- >> drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- >> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- >> drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- >> drivers/gpu/drm/radeon/radeon_audio.c | 2 +- >> drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- >> drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- >> drivers/gpu/drm/tegra/hdmi.c | 3 ++- >> drivers/gpu/drm/tegra/sor.c | 3 ++- >> drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- >> drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- >> include/drm/drm_edid.h| 8 +++--- >> 27 files changed, 94 insertions(+), 66 deletions(-) > For dw-hdmi and omapdrm, > > Reviewed-by: Laurent Pinchart > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
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(,
>>> - 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(, mode, false);
>>> + drm_hdmi_avi_infoframe_from_display_mode(,
>>> +>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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
Hi Ville, Thank you for the patch. On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > From: Ville Syrjälä > > Make life easier for drivers by simply passing the connector > to drm_hdmi_avi_infoframe_from_display_mode() and > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > need to worry about is_hdmi2_sink mess. While this is good for display controller drivers, the change isn't great for bridge drivers. Down the road we're looking at moving connector support out of the bridge drivers. Adding an additional dependency to connectors in the bridges will make that more difficult. Ideally bridges should retrieve the information from their sink, regardless of whether it is a connector or another bridge. Please see below for an additional comment. > Cc: Alex Deucher > Cc: "Christian König" > Cc: "David (ChunMing) Zhou" > Cc: Archit Taneja > Cc: Andrzej Hajda > Cc: Laurent Pinchart > Cc: Inki Dae > Cc: Joonyoung Shim > Cc: Seung-Woo Kim > Cc: Kyungmin Park > Cc: Russell King > Cc: CK Hu > Cc: Philipp Zabel > Cc: Rob Clark > Cc: Ben Skeggs > Cc: Tomi Valkeinen > Cc: Sandy Huang > Cc: "Heiko Stübner" > Cc: Benjamin Gaignard > Cc: Vincent Abriou > Cc: Thierry Reding > Cc: Eric Anholt > Cc: Shawn Guo > Cc: Ilia Mirkin > Cc: amd-...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: linux-te...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > drivers/gpu/drm/bridge/sii902x.c | 3 ++- > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > drivers/gpu/drm/drm_edid.c| 33 ++- > drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > drivers/gpu/drm/tegra/hdmi.c | 3 ++- > drivers/gpu/drm/tegra/sor.c | 3 ++- > drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- > drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- > include/drm/drm_edid.h| 8 +++--- > 27 files changed, 94 insertions(+), 66 deletions(-) For dw-hdmi and omapdrm, Reviewed-by: Laurent Pinchart -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> On Tue, 20 Nov 2018, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Make life easier for drivers by simply passing the connector
> > to drm_hdmi_avi_infoframe_from_display_mode() and
> > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> > need to worry about is_hdmi2_sink mess.
>
> Overall looks about right and nice,
>
> Reviewed-by: Jani Nikula
>
> But please do take that with a grain of salt for everything outside of
> i915 and drm core.
>
> Please also find a few comments inline below.
>
> > Cc: Alex Deucher
> > Cc: "Christian König"
> > Cc: "David (ChunMing) Zhou"
> > Cc: Archit Taneja
> > Cc: Andrzej Hajda
> > Cc: Laurent Pinchart
> > Cc: Inki Dae
> > Cc: Joonyoung Shim
> > Cc: Seung-Woo Kim
> > Cc: Kyungmin Park
> > Cc: Russell King
> > Cc: CK Hu
> > Cc: Philipp Zabel
> > Cc: Rob Clark
> > Cc: Ben Skeggs
> > Cc: Tomi Valkeinen
> > Cc: Sandy Huang
> > Cc: "Heiko Stübner"
> > Cc: Benjamin Gaignard
> > Cc: Vincent Abriou
> > Cc: Thierry Reding
> > Cc: Eric Anholt
> > Cc: Shawn Guo
> > Cc: Ilia Mirkin
> > Cc: amd-...@lists.freedesktop.org
> > Cc: linux-arm-...@vger.kernel.org
> > Cc: freedr...@lists.freedesktop.org
> > Cc: nouv...@lists.freedesktop.org
> > Cc: linux-te...@vger.kernel.org
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++-
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> > drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++--
> > drivers/gpu/drm/bridge/sii902x.c | 3 ++-
> > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +--
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
> > drivers/gpu/drm/drm_edid.c| 33 ++-
> > drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++-
> > drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++-
> > drivers/gpu/drm/i915/intel_hdmi.c | 14 +-
> > drivers/gpu/drm/i915/intel_lspcon.c | 15 ++-
> > drivers/gpu/drm/i915/intel_sdvo.c | 10 ---
> > drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++-
> > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++-
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++--
> > drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++--
> > drivers/gpu/drm/radeon/radeon_audio.c | 2 +-
> > drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++-
> > drivers/gpu/drm/sti/sti_hdmi.c| 3 ++-
> > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++-
> > drivers/gpu/drm/tegra/hdmi.c | 3 ++-
> > drivers/gpu/drm/tegra/sor.c | 3 ++-
> > drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +---
> > drivers/gpu/drm/zte/zx_hdmi.c | 4 ++-
> > include/drm/drm_edid.h| 8 +++---
> > 27 files changed, 94 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > index 4cfecdce29a3..1f0426d2fc2a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > @@ -1682,7 +1682,7 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder
> > *encoder,
> > dce_v10_0_audio_write_sad_regs(encoder);
> > dce_v10_0_audio_write_latency_fields(encoder, mode);
> >
> > - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> > + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> > if (err < 0) {
> > DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> > return;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > index 7c868916d90f..2280b971d758 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > @@ -1724,7 +1724,7 @@ static void dce_v11_0_afmt_setmode(struct drm_encoder
> > *encoder,
> > dce_v11_0_audio_write_sad_regs(encoder);
> > dce_v11_0_audio_write_latency_fields(encoder, mode);
> >
> > - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> > + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> > if (err < 0) {
> > DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> > return;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> > b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> > index 17eaaba36017..db443ec53d3a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> > @@ -1423,6 +1423,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct
> > drm_encoder *encoder,
> > struct amdgpu_device *adev = dev->dev_private;
> > struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
> > struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
> > + struct
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, 20 Nov 2018, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Make life easier for drivers by simply passing the connector
> to drm_hdmi_avi_infoframe_from_display_mode() and
> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> need to worry about is_hdmi2_sink mess.
Overall looks about right and nice,
Reviewed-by: Jani Nikula
But please do take that with a grain of salt for everything outside of
i915 and drm core.
Please also find a few comments inline below.
> Cc: Alex Deucher
> Cc: "Christian König"
> Cc: "David (ChunMing) Zhou"
> Cc: Archit Taneja
> Cc: Andrzej Hajda
> Cc: Laurent Pinchart
> Cc: Inki Dae
> Cc: Joonyoung Shim
> Cc: Seung-Woo Kim
> Cc: Kyungmin Park
> Cc: Russell King
> Cc: CK Hu
> Cc: Philipp Zabel
> Cc: Rob Clark
> Cc: Ben Skeggs
> Cc: Tomi Valkeinen
> Cc: Sandy Huang
> Cc: "Heiko Stübner"
> Cc: Benjamin Gaignard
> Cc: Vincent Abriou
> Cc: Thierry Reding
> Cc: Eric Anholt
> Cc: Shawn Guo
> Cc: Ilia Mirkin
> Cc: amd-...@lists.freedesktop.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +-
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +-
> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++-
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++--
> drivers/gpu/drm/bridge/sii902x.c | 3 ++-
> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +--
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
> drivers/gpu/drm/drm_edid.c| 33 ++-
> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++-
> drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++-
> drivers/gpu/drm/i915/intel_hdmi.c | 14 +-
> drivers/gpu/drm/i915/intel_lspcon.c | 15 ++-
> drivers/gpu/drm/i915/intel_sdvo.c | 10 ---
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++-
> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++--
> drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++--
> drivers/gpu/drm/radeon/radeon_audio.c | 2 +-
> drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++-
> drivers/gpu/drm/sti/sti_hdmi.c| 3 ++-
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++-
> drivers/gpu/drm/tegra/hdmi.c | 3 ++-
> drivers/gpu/drm/tegra/sor.c | 3 ++-
> drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +---
> drivers/gpu/drm/zte/zx_hdmi.c | 4 ++-
> include/drm/drm_edid.h| 8 +++---
> 27 files changed, 94 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 4cfecdce29a3..1f0426d2fc2a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -1682,7 +1682,7 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder
> *encoder,
> dce_v10_0_audio_write_sad_regs(encoder);
> dce_v10_0_audio_write_latency_fields(encoder, mode);
>
> - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> if (err < 0) {
> DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> return;
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index 7c868916d90f..2280b971d758 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -1724,7 +1724,7 @@ static void dce_v11_0_afmt_setmode(struct drm_encoder
> *encoder,
> dce_v11_0_audio_write_sad_regs(encoder);
> dce_v11_0_audio_write_latency_fields(encoder, mode);
>
> - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> if (err < 0) {
> DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> return;
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index 17eaaba36017..db443ec53d3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -1423,6 +1423,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct
> drm_encoder *encoder,
> struct amdgpu_device *adev = dev->dev_private;
> struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
> struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
> + struct drm_connector *connector =
> amdgpu_get_connector_for_encoder(encoder);
> struct hdmi_avi_infoframe frame;
> u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
> uint8_t *payload = buffer + 3;
> @@ -1430,7 +1431,7 @@ static void
Re: [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, Nov 20, 2018 at 06:13:42PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > Make life easier for drivers by simply passing the connector > to drm_hdmi_avi_infoframe_from_display_mode() and > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > need to worry about is_hdmi2_sink mess. > > Cc: Alex Deucher > Cc: "Christian König" > Cc: "David (ChunMing) Zhou" > Cc: Archit Taneja > Cc: Andrzej Hajda > Cc: Laurent Pinchart > Cc: Inki Dae > Cc: Joonyoung Shim > Cc: Seung-Woo Kim > Cc: Kyungmin Park > Cc: Russell King > Cc: CK Hu > Cc: Philipp Zabel > Cc: Rob Clark > Cc: Ben Skeggs > Cc: Tomi Valkeinen > Cc: Sandy Huang > Cc: "Heiko Stübner" > Cc: Benjamin Gaignard > Cc: Vincent Abriou > Cc: Thierry Reding > Cc: Eric Anholt > Cc: Shawn Guo > Cc: Ilia Mirkin > Cc: amd-...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: linux-te...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > drivers/gpu/drm/bridge/sii902x.c | 3 ++- > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > drivers/gpu/drm/drm_edid.c| 33 ++- > drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > drivers/gpu/drm/tegra/hdmi.c | 3 ++- > drivers/gpu/drm/tegra/sor.c | 3 ++- > drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- > drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- > include/drm/drm_edid.h| 8 +++--- > 27 files changed, 94 insertions(+), 66 deletions(-) That's actually a lot nicer: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
