Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
On Tue, Dec 05, 2023 at 02:50:57PM +0200, Jani Nikula wrote: > On Tue, 05 Dec 2023, "Shankar, Uma" wrote: > >> Yeah, writing 0 is done with an intention to disable it but that’s not the > >> way to > >> have this option disabled. Infact there is no reason to write to it for > >> DP1.4+ if sink > >> is compliant. > > > > The change looks ok and aligns with spec, its > > Reviewed-by: Uma Shankar > > Ville, any further objections or concerns? Okay to merge? I guess a small comment on why we're avoiding the BW_SET in there might be good idea, lest we forget and try to reunify the two branches by accident. But otherwise seems fine. -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
On Tue, 05 Dec 2023, "Shankar, Uma" wrote: >> Yeah, writing 0 is done with an intention to disable it but that’s not the >> way to >> have this option disabled. Infact there is no reason to write to it for >> DP1.4+ if sink >> is compliant. > > The change looks ok and aligns with spec, its > Reviewed-by: Uma Shankar Ville, any further objections or concerns? Okay to merge? BR, Jani. -- Jani Nikula, Intel
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
> -Original Message-
> From: Shankar, Uma
> Sent: Monday, December 4, 2023 10:23 PM
> To: Nikula, Jani ; Ville Syrjälä
>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET
> when using rate select
>
>
>
> > -Original Message-
> > From: Nikula, Jani
> > Sent: Monday, December 4, 2023 10:18 PM
> > To: Shankar, Uma ; Ville Syrjälä
> >
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH] drm/i915/edp: don't write to
> > DP_LINK_BW_SET when using rate select
> >
> > On Mon, 04 Dec 2023, "Shankar, Uma" wrote:
> > >> -Original Message-
> > >> From: Intel-gfx On Behalf
> > >> Of Jani Nikula
> > >> Sent: Monday, December 4, 2023 3:28 PM
> > >> To: Ville Syrjälä
> > >> Cc: intel-gfx@lists.freedesktop.org
> > >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to
> > >> DP_LINK_BW_SET when using rate select
> > >>
> > >> On Fri, 01 Dec 2023, Ville Syrjälä wrote:
> > >> > On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
> > >> >> The eDP 1.5 spec adds a clarification for eDP 1.4x:
> > >> >>
> > >> >> > For eDP v1.4x, if the Source device chooses the Main-Link rate
> > >> >> > by way of DPCD 00100h, the Sink device shall ignore DPCD
> > >> >> > 00115h[2:0].
> > >> >>
> > >> >> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
> > >> >> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause
> > >> >> the panel to ignore the rate set method.
> > >> >
> > >> > What a terrible way to specify this :( This means the device must
> > >> > hav some internal state to keep track of whethe BW_SET was ever
> > >> > written.
> > >>
> > >> Indeed.
> > >>
> > >> Additionally, eDP 1.5 specifies LINK_CONFIGURATION_STATUS (DPCD
> > >> 0020Ch) which exposes the internal state as link rate set status,
> > >> and whether that status is valid or not.
> > >>
> > >> Overall the spec looks like that's just for status, but the
> > >> register is annotated Write/Read so who knows.
> > >>
> > >> >
> > >> >>
> > >> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
> > >> >> Tested-by: Animesh Manna
> > >> >> Signed-off-by: Jani Nikula
> > >> >> ---
> > >> >> .../drm/i915/display/intel_dp_link_training.c | 23
> > >> >> +++
> > >> >> 1 file changed, 13 insertions(+), 10 deletions(-)
> > >> >>
> > >> >> diff --git
> > >> >> a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> >> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> >> index dbc1b66c8ee4..6336a39030a4 100644
> > >> >> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> >> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> >> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct
> > >> >> intel_dp
> > >> *intel_dp,
> > >> >>const struct intel_crtc_state *crtc_state,
> > >> >>u8 link_bw, u8 rate_select) {
> > >> >> - u8 link_config[2];
> > >> >> + u8 lane_count = crtc_state->lane_count;
> > >> >>
> > >> >> - /* Write the link configuration data */
> > >> >> - link_config[0] = link_bw;
> > >> >> - link_config[1] = crtc_state->lane_count;
> > >> >>if (crtc_state->enhanced_framing)
> > >> >> - link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> > >> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET,
> > >> >> link_config, 2);
> > >> >> + lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> > >> >> +
> > >> >> + if (link_bw) {
> > >> >> + /* eDP 1.3 and earlier link bw set method. */
> > >> >> + u8 link_config[] = { link_bw, lane_count };
> > >> >>
> > >> >> - /* eDP 1.4 rate select method. */
>
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
> -Original Message-
> From: Nikula, Jani
> Sent: Monday, December 4, 2023 10:18 PM
> To: Shankar, Uma ; Ville Syrjälä
>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET
> when using rate select
>
> On Mon, 04 Dec 2023, "Shankar, Uma" wrote:
> >> -Original Message-
> >> From: Intel-gfx On Behalf
> >> Of Jani Nikula
> >> Sent: Monday, December 4, 2023 3:28 PM
> >> To: Ville Syrjälä
> >> Cc: intel-gfx@lists.freedesktop.org
> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to
> >> DP_LINK_BW_SET when using rate select
> >>
> >> On Fri, 01 Dec 2023, Ville Syrjälä wrote:
> >> > On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
> >> >> The eDP 1.5 spec adds a clarification for eDP 1.4x:
> >> >>
> >> >> > For eDP v1.4x, if the Source device chooses the Main-Link rate
> >> >> > by way of DPCD 00100h, the Sink device shall ignore DPCD 00115h[2:0].
> >> >>
> >> >> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
> >> >> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause the
> >> >> panel to ignore the rate set method.
> >> >
> >> > What a terrible way to specify this :( This means the device must
> >> > hav some internal state to keep track of whethe BW_SET was ever written.
> >>
> >> Indeed.
> >>
> >> Additionally, eDP 1.5 specifies LINK_CONFIGURATION_STATUS (DPCD
> >> 0020Ch) which exposes the internal state as link rate set status, and
> >> whether that status is valid or not.
> >>
> >> Overall the spec looks like that's just for status, but the register
> >> is annotated Write/Read so who knows.
> >>
> >> >
> >> >>
> >> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
> >> >> Tested-by: Animesh Manna
> >> >> Signed-off-by: Jani Nikula
> >> >> ---
> >> >> .../drm/i915/display/intel_dp_link_training.c | 23
> >> >> +++
> >> >> 1 file changed, 13 insertions(+), 10 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> >> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> >> index dbc1b66c8ee4..6336a39030a4 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> >> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct intel_dp
> >> *intel_dp,
> >> >>const struct intel_crtc_state *crtc_state,
> >> >>u8 link_bw, u8 rate_select) {
> >> >> - u8 link_config[2];
> >> >> + u8 lane_count = crtc_state->lane_count;
> >> >>
> >> >> - /* Write the link configuration data */
> >> >> - link_config[0] = link_bw;
> >> >> - link_config[1] = crtc_state->lane_count;
> >> >>if (crtc_state->enhanced_framing)
> >> >> - link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> >> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config, 2);
> >> >> + lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> >> >> +
> >> >> + if (link_bw) {
> >> >> + /* eDP 1.3 and earlier link bw set method. */
> >> >> + u8 link_config[] = { link_bw, lane_count };
> >> >>
> >> >> - /* eDP 1.4 rate select method. */
> >> >> - if (!link_bw)
> >> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_RATE_SET,
> >> >> -_select, 1);
> >> >> + drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET,
> >> link_config,
> >> >> +ARRAY_SIZE(link_config));
> >> >> + } else {
> >> >> + /* eDP 1.4 rate select method. */
> >> >> + drm_dp_dpcd_writeb(_dp->aux, DP_LANE_COUNT_SET,
> >> lane_count);
> >> >> + drm_dp_dpcd_writeb(_dp->aux, DP_LINK_RATE_SET,
> >> rate_select);
> >> >
> >> > Doesn't look there's anything in the spec that specifies when the
> >> > device is supposed to
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
On Mon, 04 Dec 2023, "Shankar, Uma" wrote:
>> -Original Message-
>> From: Intel-gfx On Behalf Of Jani
>> Nikula
>> Sent: Monday, December 4, 2023 3:28 PM
>> To: Ville Syrjälä
>> Cc: intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET
>> when using rate select
>>
>> On Fri, 01 Dec 2023, Ville Syrjälä wrote:
>> > On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
>> >> The eDP 1.5 spec adds a clarification for eDP 1.4x:
>> >>
>> >> > For eDP v1.4x, if the Source device chooses the Main-Link rate by
>> >> > way of DPCD 00100h, the Sink device shall ignore DPCD 00115h[2:0].
>> >>
>> >> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
>> >> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause the
>> >> panel to ignore the rate set method.
>> >
>> > What a terrible way to specify this :( This means the device must hav
>> > some internal state to keep track of whethe BW_SET was ever written.
>>
>> Indeed.
>>
>> Additionally, eDP 1.5 specifies LINK_CONFIGURATION_STATUS (DPCD 0020Ch)
>> which exposes the internal state as link rate set status, and whether that
>> status is
>> valid or not.
>>
>> Overall the spec looks like that's just for status, but the register is
>> annotated
>> Write/Read so who knows.
>>
>> >
>> >>
>> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
>> >> Tested-by: Animesh Manna
>> >> Signed-off-by: Jani Nikula
>> >> ---
>> >> .../drm/i915/display/intel_dp_link_training.c | 23
>> >> +++
>> >> 1 file changed, 13 insertions(+), 10 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> >> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> >> index dbc1b66c8ee4..6336a39030a4 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> >> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct intel_dp
>> *intel_dp,
>> >>const struct intel_crtc_state *crtc_state,
>> >>u8 link_bw, u8 rate_select)
>> >> {
>> >> - u8 link_config[2];
>> >> + u8 lane_count = crtc_state->lane_count;
>> >>
>> >> - /* Write the link configuration data */
>> >> - link_config[0] = link_bw;
>> >> - link_config[1] = crtc_state->lane_count;
>> >>if (crtc_state->enhanced_framing)
>> >> - link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> >> + lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> >> +
>> >> + if (link_bw) {
>> >> + /* eDP 1.3 and earlier link bw set method. */
>> >> + u8 link_config[] = { link_bw, lane_count };
>> >>
>> >> - /* eDP 1.4 rate select method. */
>> >> - if (!link_bw)
>> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_RATE_SET,
>> >> -_select, 1);
>> >> + drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET,
>> link_config,
>> >> +ARRAY_SIZE(link_config));
>> >> + } else {
>> >> + /* eDP 1.4 rate select method. */
>> >> + drm_dp_dpcd_writeb(_dp->aux, DP_LANE_COUNT_SET,
>> lane_count);
>> >> + drm_dp_dpcd_writeb(_dp->aux, DP_LINK_RATE_SET,
>> rate_select);
>> >
>> > Doesn't look there's anything in the spec that specifies when the
>> > device is supposed to reset its internal state to stop ignoring
>> DP_LINK_RATE_SET.
>> > Do we know when this panel does it? When VDD is removed?
>>
>> No idea. Animesh?
>>
>> I think it's just crazy writing 0 to explicitly disable DP_LINK_BW_SET
>> renders
>> DP_LINK_RATE_SET unusable. Pretty sure we've seen panels where this works as
>> you'd expect.
>>
>> And the above depends on pre-os using the same logic as us for choosing
>> DP_LINK_RATE_SET. GOP seems to do that. But if it or some other pre-os used
>> DP_LINK_BW_SET, we'd fail. With some other panels, writing the 0 might
>> recover
>> from tha
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
> -Original Message-
> From: Intel-gfx On Behalf Of Jani
> Nikula
> Sent: Monday, December 4, 2023 3:28 PM
> To: Ville Syrjälä
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET
> when using rate select
>
> On Fri, 01 Dec 2023, Ville Syrjälä wrote:
> > On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
> >> The eDP 1.5 spec adds a clarification for eDP 1.4x:
> >>
> >> > For eDP v1.4x, if the Source device chooses the Main-Link rate by
> >> > way of DPCD 00100h, the Sink device shall ignore DPCD 00115h[2:0].
> >>
> >> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
> >> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause the
> >> panel to ignore the rate set method.
> >
> > What a terrible way to specify this :( This means the device must hav
> > some internal state to keep track of whethe BW_SET was ever written.
>
> Indeed.
>
> Additionally, eDP 1.5 specifies LINK_CONFIGURATION_STATUS (DPCD 0020Ch)
> which exposes the internal state as link rate set status, and whether that
> status is
> valid or not.
>
> Overall the spec looks like that's just for status, but the register is
> annotated
> Write/Read so who knows.
>
> >
> >>
> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
> >> Tested-by: Animesh Manna
> >> Signed-off-by: Jani Nikula
> >> ---
> >> .../drm/i915/display/intel_dp_link_training.c | 23
> >> +++
> >> 1 file changed, 13 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> index dbc1b66c8ee4..6336a39030a4 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct intel_dp
> *intel_dp,
> >>const struct intel_crtc_state *crtc_state,
> >>u8 link_bw, u8 rate_select)
> >> {
> >> - u8 link_config[2];
> >> + u8 lane_count = crtc_state->lane_count;
> >>
> >> - /* Write the link configuration data */
> >> - link_config[0] = link_bw;
> >> - link_config[1] = crtc_state->lane_count;
> >>if (crtc_state->enhanced_framing)
> >> - link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config, 2);
> >> + lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> >> +
> >> + if (link_bw) {
> >> + /* eDP 1.3 and earlier link bw set method. */
> >> + u8 link_config[] = { link_bw, lane_count };
> >>
> >> - /* eDP 1.4 rate select method. */
> >> - if (!link_bw)
> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_RATE_SET,
> >> -_select, 1);
> >> + drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET,
> link_config,
> >> +ARRAY_SIZE(link_config));
> >> + } else {
> >> + /* eDP 1.4 rate select method. */
> >> + drm_dp_dpcd_writeb(_dp->aux, DP_LANE_COUNT_SET,
> lane_count);
> >> + drm_dp_dpcd_writeb(_dp->aux, DP_LINK_RATE_SET,
> rate_select);
> >
> > Doesn't look there's anything in the spec that specifies when the
> > device is supposed to reset its internal state to stop ignoring
> DP_LINK_RATE_SET.
> > Do we know when this panel does it? When VDD is removed?
>
> No idea. Animesh?
>
> I think it's just crazy writing 0 to explicitly disable DP_LINK_BW_SET renders
> DP_LINK_RATE_SET unusable. Pretty sure we've seen panels where this works as
> you'd expect.
>
> And the above depends on pre-os using the same logic as us for choosing
> DP_LINK_RATE_SET. GOP seems to do that. But if it or some other pre-os used
> DP_LINK_BW_SET, we'd fail. With some other panels, writing the 0 might recover
> from that.
The spec does leave it a bit open on this one:
115h: LINK_RATE_SET and TX_GTC_CAPABILITY
• DPCD 1h = 00h/DPCD 02201h = 00h – Source device shall use this field to
choose
the link rate, and the Sink device shall ignore DPCD 00100h
• DPCD 1h/DPCD 02201h = Valid link rate – Source device may optionally
choose
a link rate associated with HBR3, HBR2,HBR, –or– RBR by writing to DPCD 00100h
So the 2nd point here does mentions that sinks can optionally use value of
00100h
if 2201h is not 00. So programming a value to this DPCD is not right unless we
program
the right value (not 0).
I feel safe way would be be to go with LINK_BW_SET for DP1.3 and for DP1.4+
always use
LINK_RATE_SET and have it mutually exclusive.
Some TCONs would have ignored and we got lucky but we can't leave it ambiguous,
we will be compliant
to spec if we don't write 0x100. So let's go with this change.
Regards,
Uma Shankar
> No r-b, so do you have any better ideas?
>
>
> BR,
> Jani.
>
>
> >
> >> + }
> >> }
> >>
> >> /*
> >> --
> >> 2.39.2
>
> --
> Jani Nikula, Intel
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
> -Original Message-
> From: Nikula, Jani
> Sent: Monday, December 4, 2023 3:28 PM
> To: Ville Syrjälä
> Cc: intel-gfx@lists.freedesktop.org; Manna, Animesh
>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to
> DP_LINK_BW_SET when using rate select
>
> On Fri, 01 Dec 2023, Ville Syrjälä wrote:
> > On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
> >> The eDP 1.5 spec adds a clarification for eDP 1.4x:
> >>
> >> > For eDP v1.4x, if the Source device chooses the Main-Link rate by
> >> > way of DPCD 00100h, the Sink device shall ignore DPCD 00115h[2:0].
> >>
> >> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
> >> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause the
> >> panel to ignore the rate set method.
> >
> > What a terrible way to specify this :( This means the device must hav
> > some internal state to keep track of whethe BW_SET was ever written.
>
> Indeed.
>
> Additionally, eDP 1.5 specifies LINK_CONFIGURATION_STATUS (DPCD
> 0020Ch) which exposes the internal state as link rate set status, and whether
> that status is valid or not.
>
> Overall the spec looks like that's just for status, but the register is
> annotated
> Write/Read so who knows.
>
> >
> >>
> >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
> >> Tested-by: Animesh Manna
> >> Signed-off-by: Jani Nikula
> >> ---
> >> .../drm/i915/display/intel_dp_link_training.c | 23
> >> +++
> >> 1 file changed, 13 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> index dbc1b66c8ee4..6336a39030a4 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct intel_dp
> *intel_dp,
> >>const struct intel_crtc_state *crtc_state,
> >>u8 link_bw, u8 rate_select)
> >> {
> >> - u8 link_config[2];
> >> + u8 lane_count = crtc_state->lane_count;
> >>
> >> - /* Write the link configuration data */
> >> - link_config[0] = link_bw;
> >> - link_config[1] = crtc_state->lane_count;
> >>if (crtc_state->enhanced_framing)
> >> - link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config,
> 2);
> >> + lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> >> +
> >> + if (link_bw) {
> >> + /* eDP 1.3 and earlier link bw set method. */
> >> + u8 link_config[] = { link_bw, lane_count };
> >>
> >> - /* eDP 1.4 rate select method. */
> >> - if (!link_bw)
> >> - drm_dp_dpcd_write(_dp->aux, DP_LINK_RATE_SET,
> >> -_select, 1);
> >> + drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET,
> link_config,
> >> +ARRAY_SIZE(link_config));
> >> + } else {
> >> + /* eDP 1.4 rate select method. */
> >> + drm_dp_dpcd_writeb(_dp->aux,
> DP_LANE_COUNT_SET, lane_count);
> >> + drm_dp_dpcd_writeb(_dp->aux, DP_LINK_RATE_SET,
> rate_select);
> >
> > Doesn't look there's anything in the spec that specifies when the
> > device is supposed to reset its internal state to stop ignoring
> DP_LINK_RATE_SET.
> > Do we know when this panel does it? When VDD is removed?
>
> No idea. Animesh?
Hi Jani/Ville,
Tried below experiment and sharing my observation below:
Forcefully changed the value of dpcd 0x100 (LINK_BW_SET) to random value (0x99)
in edp_init_connector and later while VDD is on during modeset sequence I can
see it is not holding its value rather got reset to default value. This will
confirm when VDD is removed panel reset its internal state.
Regards,
Animesh
>
> I think it's just crazy writing 0 to explicitly disable DP_LINK_BW_SET renders
> DP_LINK_RATE_SET unusable. Pretty sure we've seen panels where this
> works as you'd expect.
>
> And the above depends on pre-os using the same logic as us for choosing
> DP_LINK_RATE_SET. GOP seems to do that. But if it or some other pre-os
> used DP_LINK_BW_SET, we'd fail. With some other panels, writing the 0
> might recover from that.
>
> No r-b, so do you have any better ideas?
>
>
> BR,
> Jani.
>
>
> >
> >> + }
> >> }
> >>
> >> /*
> >> --
> >> 2.39.2
>
> --
> Jani Nikula, Intel
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
On Fri, 01 Dec 2023, Ville Syrjälä wrote:
> On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
>> The eDP 1.5 spec adds a clarification for eDP 1.4x:
>>
>> > For eDP v1.4x, if the Source device chooses the Main-Link rate by way
>> > of DPCD 00100h, the Sink device shall ignore DPCD 00115h[2:0].
>>
>> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
>> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause the panel
>> to ignore the rate set method.
>
> What a terrible way to specify this :( This means the device must
> hav some internal state to keep track of whethe BW_SET was ever
> written.
Indeed.
Additionally, eDP 1.5 specifies LINK_CONFIGURATION_STATUS (DPCD 0020Ch)
which exposes the internal state as link rate set status, and whether
that status is valid or not.
Overall the spec looks like that's just for status, but the register is
annotated Write/Read so who knows.
>
>>
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
>> Tested-by: Animesh Manna
>> Signed-off-by: Jani Nikula
>> ---
>> .../drm/i915/display/intel_dp_link_training.c | 23 +++
>> 1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> index dbc1b66c8ee4..6336a39030a4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
>> const struct intel_crtc_state *crtc_state,
>> u8 link_bw, u8 rate_select)
>> {
>> -u8 link_config[2];
>> +u8 lane_count = crtc_state->lane_count;
>>
>> -/* Write the link configuration data */
>> -link_config[0] = link_bw;
>> -link_config[1] = crtc_state->lane_count;
>> if (crtc_state->enhanced_framing)
>> -link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> -drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> +lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> +
>> +if (link_bw) {
>> +/* eDP 1.3 and earlier link bw set method. */
>> +u8 link_config[] = { link_bw, lane_count };
>>
>> -/* eDP 1.4 rate select method. */
>> -if (!link_bw)
>> -drm_dp_dpcd_write(_dp->aux, DP_LINK_RATE_SET,
>> - _select, 1);
>> +drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config,
>> + ARRAY_SIZE(link_config));
>> +} else {
>> +/* eDP 1.4 rate select method. */
>> +drm_dp_dpcd_writeb(_dp->aux, DP_LANE_COUNT_SET,
>> lane_count);
>> +drm_dp_dpcd_writeb(_dp->aux, DP_LINK_RATE_SET,
>> rate_select);
>
> Doesn't look there's anything in the spec that specifies when the device
> is supposed to reset its internal state to stop ignoring DP_LINK_RATE_SET.
> Do we know when this panel does it? When VDD is removed?
No idea. Animesh?
I think it's just crazy writing 0 to explicitly disable DP_LINK_BW_SET
renders DP_LINK_RATE_SET unusable. Pretty sure we've seen panels where
this works as you'd expect.
And the above depends on pre-os using the same logic as us for choosing
DP_LINK_RATE_SET. GOP seems to do that. But if it or some other pre-os
used DP_LINK_BW_SET, we'd fail. With some other panels, writing the 0
might recover from that.
No r-b, so do you have any better ideas?
BR,
Jani.
>
>> +}
>> }
>>
>> /*
>> --
>> 2.39.2
--
Jani Nikula, Intel
Re: [Intel-gfx] [PATCH] drm/i915/edp: don't write to DP_LINK_BW_SET when using rate select
On Fri, Dec 01, 2023 at 03:41:41PM +0200, Jani Nikula wrote:
> The eDP 1.5 spec adds a clarification for eDP 1.4x:
>
> > For eDP v1.4x, if the Source device chooses the Main-Link rate by way
> > of DPCD 00100h, the Sink device shall ignore DPCD 00115h[2:0].
>
> We write 0 to DP_LINK_BW_SET (DPCD 100h) even when using
> DP_LINK_RATE_SET (DPCD 114h). Stop doing that, as it can cause the panel
> to ignore the rate set method.
What a terrible way to specify this :( This means the device must
hav some internal state to keep track of whethe BW_SET was ever
written.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9081
> Tested-by: Animesh Manna
> Signed-off-by: Jani Nikula
> ---
> .../drm/i915/display/intel_dp_link_training.c | 23 +++
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index dbc1b66c8ee4..6336a39030a4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -650,19 +650,22 @@ intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
> const struct intel_crtc_state *crtc_state,
> u8 link_bw, u8 rate_select)
> {
> - u8 link_config[2];
> + u8 lane_count = crtc_state->lane_count;
>
> - /* Write the link configuration data */
> - link_config[0] = link_bw;
> - link_config[1] = crtc_state->lane_count;
> if (crtc_state->enhanced_framing)
> - link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> - drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config, 2);
> + lane_count |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> +
> + if (link_bw) {
> + /* eDP 1.3 and earlier link bw set method. */
> + u8 link_config[] = { link_bw, lane_count };
>
> - /* eDP 1.4 rate select method. */
> - if (!link_bw)
> - drm_dp_dpcd_write(_dp->aux, DP_LINK_RATE_SET,
> - _select, 1);
> + drm_dp_dpcd_write(_dp->aux, DP_LINK_BW_SET, link_config,
> + ARRAY_SIZE(link_config));
> + } else {
> + /* eDP 1.4 rate select method. */
> + drm_dp_dpcd_writeb(_dp->aux, DP_LANE_COUNT_SET,
> lane_count);
> + drm_dp_dpcd_writeb(_dp->aux, DP_LINK_RATE_SET,
> rate_select);
Doesn't look there's anything in the spec that specifies when the device
is supposed to reset its internal state to stop ignoring DP_LINK_RATE_SET.
Do we know when this panel does it? When VDD is removed?
> + }
> }
>
> /*
> --
> 2.39.2
--
Ville Syrjälä
Intel
