Re: [Heads up to maintainers] Re: [PATCH v8 1/1] drm/drm_mst: Use Extended Base Receiver Capability DPCD space
On Mon, May 03, 2021 at 11:33:33AM +0300, Jani Nikula wrote:
> On Fri, 30 Apr 2021, Jani Nikula wrote:
> > On Thu, 29 Apr 2021, Lyude Paul wrote:
> >> JFYI Jani and Ben: I will be pushing this patch to drm-misc-next sometime
> >> today if there's no objections
> >
> > Thanks for the heads-up, I think this breaks i915. See my review
> > comments elsewhere in the thread.
>
> Looks like this was merged anyway.
Yeah in general rule of thumb is to let cross-driver stuff soak for a week
(assuming correctly cc'ed and all that already). I think that's the sweet
spot between maintainers who complain that it's too short and others
complaining it's too quick :-)
-Daniel
>
> 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver Capability DPCD space")
>
> I'm not happy how this played out.
>
> 1) You need to Cc relevant people
>
> 2) You need to get the ack before merging changes
>
> 3) You need to give people more than a day to react, with time zones and
> all; I replied as soon as I saw the heads-up, but it was already too
> late
>
> It's broken on i915, and perhaps that could be fixed.
>
> However I also think using DP spec rate codes and calling them "rate" is
> a bad interface, especially when the unit breaks down with DP 2.0 rate
> codes. It's confusing and it's not future proof. Fixing that afterwards
> falls to whoever comes next to pick up the pieces.
>
> I'd rather just see this reverted and redone.
>
>
> BR,
> Jani.
>
>
> >
> > BR,
> > Jani.
> >
> >
> >>
> >> On Wed, 2021-04-28 at 19:43 -0400, Nikola Cornij wrote:
> >>> [why]
> >>> DP 1.4a spec madates that if DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is
> >>> set, Extended Base Receiver Capability DPCD space must be used. Without
> >>> doing that, the three DPCD values that differ will be wrong, leading to
> >>> incorrect or limited functionality. MST link rate, for example, could
> >>> have a lower value. Also, Synaptics quirk wouldn't work out well when
> >>> Extended DPCD was not read, resulting in no DSC for such hubs.
> >>>
> >>> [how]
> >>> Modify MST topology manager to use the values from Extended DPCD where
> >>> applicable.
> >>>
> >>> To prevent regression on the sources that have a lower maximum link rate
> >>> capability than MAX_LINK_RATE from Extended DPCD, have the drivers
> >>> supply maximum lane count and rate at initialization time.
> >>>
> >>> This also reverts 'commit 2dcab875e763 ("Revert drm/dp_mst: Retrieve
> >>> extended DPCD caps for topology manager")', brining the change back to
> >>> the original 'commit ad44c03208e4 ("drm/dp_mst: Retrieve extended DPCD
> >>> caps for topology manager")'.
> >>>
> >>> Signed-off-by: Nikola Cornij
> >>> ---
> >>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++
> >>> .../gpu/drm/amd/display/dc/core/dc_link_dp.c | 18 ++
> >>> drivers/gpu/drm/amd/display/dc/dc_link.h | 2 ++
> >>> drivers/gpu/drm/drm_dp_mst_topology.c | 33 ---
> >>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++-
> >>> drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-
> >>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 7
> >>> include/drm/drm_dp_mst_helper.h | 12 ++-
> >>> 8 files changed, 71 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> >>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> >>> index 997567f6f0ba..b7e01b6fb328 100644
> >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> >>> @@ -429,6 +429,8 @@ void amdgpu_dm_initialize_dp_connector(struct
> >>> amdgpu_display_manager *dm,
> >>> struct amdgpu_dm_connector
> >>> *aconnector,
> >>> int link_index)
> >>> {
> >>> + struct dc_link_settings max_link_enc_cap = {0};
> >>> +
> >>> aconnector->dm_dp_aux.aux.name =
> >>> kasprintf(GFP_KERNEL, "AMDGPU DM aux hw bus %d",
> >>> link_index);
> >>> @@ -443,6 +445,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> >>> amdgpu_display_manager *dm,
> >>> if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
> >>> return;
> >>>
> >>> + dc_link_dp_get_max_link_enc_cap(aconnector->dc_link,
> >>> &max_link_enc_cap);
> >>> aconnector->mst_mgr.cbs = &dm_mst_cbs;
> >>> drm_dp_mst_topology_mgr_init(
> >>> &aconnector->mst_mgr,
> >>> @@ -450,6 +453,8 @@ void amdgpu_dm_initialize_dp_connector(struct
> >>> amdgpu_display_manager *dm,
> >>> &aconnector->dm_dp_aux.aux,
> >>> 16,
> >>> 4,
> >>> + max_link_enc_cap.lane_count,
> >>> + max_link_enc_cap.link_rate,
> >>> aconnector->connector_id);
> >>>
> >>> drm_connector_attach_dp_subconnector_property(&aconnect
Re: [Heads up to maintainers] Re: [PATCH v8 1/1] drm/drm_mst: Use Extended Base Receiver Capability DPCD space
On Mon, 03 May 2021, Jani Nikula wrote:
> On Fri, 30 Apr 2021, Jani Nikula wrote:
>> On Thu, 29 Apr 2021, Lyude Paul wrote:
>>> JFYI Jani and Ben: I will be pushing this patch to drm-misc-next sometime
>>> today if there's no objections
>>
>> Thanks for the heads-up, I think this breaks i915. See my review
>> comments elsewhere in the thread.
>
> Looks like this was merged anyway.
>
> 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver Capability DPCD space")
>
> I'm not happy how this played out.
>
> 1) You need to Cc relevant people
>
> 2) You need to get the ack before merging changes
>
> 3) You need to give people more than a day to react, with time zones and
> all; I replied as soon as I saw the heads-up, but it was already too
> late
>
> It's broken on i915, and perhaps that could be fixed.
>
> However I also think using DP spec rate codes and calling them "rate" is
> a bad interface, especially when the unit breaks down with DP 2.0 rate
> codes. It's confusing and it's not future proof. Fixing that afterwards
> falls to whoever comes next to pick up the pieces.
>
> I'd rather just see this reverted and redone.
Okay, just saw that you'd fixed i915 already. Thanks. Let's roll with
that then.
BR,
Jani.
>
>
> BR,
> Jani.
>
>
>>
>> BR,
>> Jani.
>>
>>
>>>
>>> On Wed, 2021-04-28 at 19:43 -0400, Nikola Cornij wrote:
[why]
DP 1.4a spec madates that if DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is
set, Extended Base Receiver Capability DPCD space must be used. Without
doing that, the three DPCD values that differ will be wrong, leading to
incorrect or limited functionality. MST link rate, for example, could
have a lower value. Also, Synaptics quirk wouldn't work out well when
Extended DPCD was not read, resulting in no DSC for such hubs.
[how]
Modify MST topology manager to use the values from Extended DPCD where
applicable.
To prevent regression on the sources that have a lower maximum link rate
capability than MAX_LINK_RATE from Extended DPCD, have the drivers
supply maximum lane count and rate at initialization time.
This also reverts 'commit 2dcab875e763 ("Revert drm/dp_mst: Retrieve
extended DPCD caps for topology manager")', brining the change back to
the original 'commit ad44c03208e4 ("drm/dp_mst: Retrieve extended DPCD
caps for topology manager")'.
Signed-off-by: Nikola Cornij
---
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++
.../gpu/drm/amd/display/dc/core/dc_link_dp.c | 18 ++
drivers/gpu/drm/amd/display/dc/dc_link.h | 2 ++
drivers/gpu/drm/drm_dp_mst_topology.c | 33 ---
drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-
drivers/gpu/drm/radeon/radeon_dp_mst.c | 7
include/drm/drm_dp_mst_helper.h | 12 ++-
8 files changed, 71 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 997567f6f0ba..b7e01b6fb328 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -429,6 +429,8 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
struct amdgpu_dm_connector
*aconnector,
int link_index)
{
+ struct dc_link_settings max_link_enc_cap = {0};
+
aconnector->dm_dp_aux.aux.name =
kasprintf(GFP_KERNEL, "AMDGPU DM aux hw bus %d",
link_index);
@@ -443,6 +445,7 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
return;
+ dc_link_dp_get_max_link_enc_cap(aconnector->dc_link,
&max_link_enc_cap);
aconnector->mst_mgr.cbs = &dm_mst_cbs;
drm_dp_mst_topology_mgr_init(
&aconnector->mst_mgr,
@@ -450,6 +453,8 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
&aconnector->dm_dp_aux.aux,
16,
4,
+ max_link_enc_cap.lane_count,
+ max_link_enc_cap.link_rate,
aconnector->connector_id);
drm_connector_attach_dp_subconnector_property(&aconnector->base);
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
index 7d2e433c2275..6fe66b7ee53e 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
+++ b/drivers/gpu/drm/amd/display/d
Re: [Heads up to maintainers] Re: [PATCH v8 1/1] drm/drm_mst: Use Extended Base Receiver Capability DPCD space
On Fri, 30 Apr 2021, Jani Nikula wrote:
> On Thu, 29 Apr 2021, Lyude Paul wrote:
>> JFYI Jani and Ben: I will be pushing this patch to drm-misc-next sometime
>> today if there's no objections
>
> Thanks for the heads-up, I think this breaks i915. See my review
> comments elsewhere in the thread.
Looks like this was merged anyway.
98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver Capability DPCD space")
I'm not happy how this played out.
1) You need to Cc relevant people
2) You need to get the ack before merging changes
3) You need to give people more than a day to react, with time zones and
all; I replied as soon as I saw the heads-up, but it was already too
late
It's broken on i915, and perhaps that could be fixed.
However I also think using DP spec rate codes and calling them "rate" is
a bad interface, especially when the unit breaks down with DP 2.0 rate
codes. It's confusing and it's not future proof. Fixing that afterwards
falls to whoever comes next to pick up the pieces.
I'd rather just see this reverted and redone.
BR,
Jani.
>
> BR,
> Jani.
>
>
>>
>> On Wed, 2021-04-28 at 19:43 -0400, Nikola Cornij wrote:
>>> [why]
>>> DP 1.4a spec madates that if DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is
>>> set, Extended Base Receiver Capability DPCD space must be used. Without
>>> doing that, the three DPCD values that differ will be wrong, leading to
>>> incorrect or limited functionality. MST link rate, for example, could
>>> have a lower value. Also, Synaptics quirk wouldn't work out well when
>>> Extended DPCD was not read, resulting in no DSC for such hubs.
>>>
>>> [how]
>>> Modify MST topology manager to use the values from Extended DPCD where
>>> applicable.
>>>
>>> To prevent regression on the sources that have a lower maximum link rate
>>> capability than MAX_LINK_RATE from Extended DPCD, have the drivers
>>> supply maximum lane count and rate at initialization time.
>>>
>>> This also reverts 'commit 2dcab875e763 ("Revert drm/dp_mst: Retrieve
>>> extended DPCD caps for topology manager")', brining the change back to
>>> the original 'commit ad44c03208e4 ("drm/dp_mst: Retrieve extended DPCD
>>> caps for topology manager")'.
>>>
>>> Signed-off-by: Nikola Cornij
>>> ---
>>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++
>>> .../gpu/drm/amd/display/dc/core/dc_link_dp.c | 18 ++
>>> drivers/gpu/drm/amd/display/dc/dc_link.h | 2 ++
>>> drivers/gpu/drm/drm_dp_mst_topology.c | 33 ---
>>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++-
>>> drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-
>>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 7
>>> include/drm/drm_dp_mst_helper.h | 12 ++-
>>> 8 files changed, 71 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>>> index 997567f6f0ba..b7e01b6fb328 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>>> @@ -429,6 +429,8 @@ void amdgpu_dm_initialize_dp_connector(struct
>>> amdgpu_display_manager *dm,
>>> struct amdgpu_dm_connector
>>> *aconnector,
>>> int link_index)
>>> {
>>> + struct dc_link_settings max_link_enc_cap = {0};
>>> +
>>> aconnector->dm_dp_aux.aux.name =
>>> kasprintf(GFP_KERNEL, "AMDGPU DM aux hw bus %d",
>>> link_index);
>>> @@ -443,6 +445,7 @@ void amdgpu_dm_initialize_dp_connector(struct
>>> amdgpu_display_manager *dm,
>>> if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
>>> return;
>>>
>>> + dc_link_dp_get_max_link_enc_cap(aconnector->dc_link,
>>> &max_link_enc_cap);
>>> aconnector->mst_mgr.cbs = &dm_mst_cbs;
>>> drm_dp_mst_topology_mgr_init(
>>> &aconnector->mst_mgr,
>>> @@ -450,6 +453,8 @@ void amdgpu_dm_initialize_dp_connector(struct
>>> amdgpu_display_manager *dm,
>>> &aconnector->dm_dp_aux.aux,
>>> 16,
>>> 4,
>>> + max_link_enc_cap.lane_count,
>>> + max_link_enc_cap.link_rate,
>>> aconnector->connector_id);
>>>
>>> drm_connector_attach_dp_subconnector_property(&aconnector->base);
>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>>> b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>>> index 7d2e433c2275..6fe66b7ee53e 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>>> @@ -1894,6 +1894,24 @@ bool dc_link_dp_sync_lt_end(struct dc_link *link,
>>> bool link_down)
>>> return true;
>>> }
>>>
>>> +bool dc_link_dp_get_max_link_enc_cap(const struct dc_link *link, struct
>>> dc_link_settings *max_link_enc_cap
Re: [Heads up to maintainers] Re: [PATCH v8 1/1] drm/drm_mst: Use Extended Base Receiver Capability DPCD space
On Thu, 29 Apr 2021, Lyude Paul wrote:
> JFYI Jani and Ben: I will be pushing this patch to drm-misc-next sometime
> today if there's no objections
Thanks for the heads-up, I think this breaks i915. See my review
comments elsewhere in the thread.
BR,
Jani.
>
> On Wed, 2021-04-28 at 19:43 -0400, Nikola Cornij wrote:
>> [why]
>> DP 1.4a spec madates that if DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is
>> set, Extended Base Receiver Capability DPCD space must be used. Without
>> doing that, the three DPCD values that differ will be wrong, leading to
>> incorrect or limited functionality. MST link rate, for example, could
>> have a lower value. Also, Synaptics quirk wouldn't work out well when
>> Extended DPCD was not read, resulting in no DSC for such hubs.
>>
>> [how]
>> Modify MST topology manager to use the values from Extended DPCD where
>> applicable.
>>
>> To prevent regression on the sources that have a lower maximum link rate
>> capability than MAX_LINK_RATE from Extended DPCD, have the drivers
>> supply maximum lane count and rate at initialization time.
>>
>> This also reverts 'commit 2dcab875e763 ("Revert drm/dp_mst: Retrieve
>> extended DPCD caps for topology manager")', brining the change back to
>> the original 'commit ad44c03208e4 ("drm/dp_mst: Retrieve extended DPCD
>> caps for topology manager")'.
>>
>> Signed-off-by: Nikola Cornij
>> ---
>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++
>> .../gpu/drm/amd/display/dc/core/dc_link_dp.c | 18 ++
>> drivers/gpu/drm/amd/display/dc/dc_link.h | 2 ++
>> drivers/gpu/drm/drm_dp_mst_topology.c | 33 ---
>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++-
>> drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-
>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 7
>> include/drm/drm_dp_mst_helper.h | 12 ++-
>> 8 files changed, 71 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 997567f6f0ba..b7e01b6fb328 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -429,6 +429,8 @@ void amdgpu_dm_initialize_dp_connector(struct
>> amdgpu_display_manager *dm,
>> struct amdgpu_dm_connector
>> *aconnector,
>> int link_index)
>> {
>> + struct dc_link_settings max_link_enc_cap = {0};
>> +
>> aconnector->dm_dp_aux.aux.name =
>> kasprintf(GFP_KERNEL, "AMDGPU DM aux hw bus %d",
>> link_index);
>> @@ -443,6 +445,7 @@ void amdgpu_dm_initialize_dp_connector(struct
>> amdgpu_display_manager *dm,
>> if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
>> return;
>>
>> + dc_link_dp_get_max_link_enc_cap(aconnector->dc_link,
>> &max_link_enc_cap);
>> aconnector->mst_mgr.cbs = &dm_mst_cbs;
>> drm_dp_mst_topology_mgr_init(
>> &aconnector->mst_mgr,
>> @@ -450,6 +453,8 @@ void amdgpu_dm_initialize_dp_connector(struct
>> amdgpu_display_manager *dm,
>> &aconnector->dm_dp_aux.aux,
>> 16,
>> 4,
>> + max_link_enc_cap.lane_count,
>> + max_link_enc_cap.link_rate,
>> aconnector->connector_id);
>>
>> drm_connector_attach_dp_subconnector_property(&aconnector->base);
>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> index 7d2e433c2275..6fe66b7ee53e 100644
>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> @@ -1894,6 +1894,24 @@ bool dc_link_dp_sync_lt_end(struct dc_link *link,
>> bool link_down)
>> return true;
>> }
>>
>> +bool dc_link_dp_get_max_link_enc_cap(const struct dc_link *link, struct
>> dc_link_settings *max_link_enc_cap)
>> +{
>> + if (!max_link_enc_cap) {
>> + DC_LOG_ERROR("%s: Could not return max link encoder caps",
>> __func__);
>> + return false;
>> + }
>> +
>> + if (link->link_enc->funcs->get_max_link_cap) {
>> + link->link_enc->funcs->get_max_link_cap(link->link_enc,
>> max_link_enc_cap);
>> + return true;
>> + }
>> +
>> + DC_LOG_ERROR("%s: Max link encoder caps unknown", __func__);
>> + max_link_enc_cap->lane_count = 1;
>> + max_link_enc_cap->link_rate = 6;
>> + return false;
>> +}
>> +
>> static struct dc_link_settings get_max_link_cap(struct dc_link *link)
>> {
>> struct dc_link_settings max_link_cap = {0};
>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_link.h
>> b/drivers/gpu/drm/amd/display/dc/dc_link.h
>> index b0013e674864..cb6d0543d839 100644
>> --- a/drivers/gpu/drm/amd/display/dc
