Re: [PATCH 06/14] drm/amd/display: Use dc helpers to compute timeslot distribution

["Francis, David" <David.Francis@amd.com>]

On 8/19/19 11:50 AM, David Francis wrote:
>> We were using drm helpers to convert a timing into its
>> bandwidth, its bandwidth into pbn, and its pbn into timeslots
>>
>> These helpers
>> -Did not take DSC timings into account
>> -Used the link rate and lane count of the link's aux device,
>>   which are not the same as the link's current cap
>> -Did not take FEC into account (FEC reduces the PBN per timeslot)
>>
>> Use the DC helpers (dc_bandwidth_in_kbps_from_timing,
>> dc_link_bandwidth_kbps) instead
>>
>> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
>> Signed-off-by: David Francis <David.Francis@amd.com>
>
>Wouldn't this be a good candidate for shared DRM helpers or to modify
>the existing ones to support this usecase?
>
>Seems like this would be shared across drivers.
>
>Nicholas Kazlauskas
>

These functions use information which is not stored in drm structs but tracked in
a driver-specific way
 - Whether or not FEC is enabled
 - Whether or not DSC is enabled, and with what config
 - The current link setting
This could be shifted into drm helpers, but it would require functions with signatures like
drm_dp_calc_pbn_mode(clock, bpp, dsc_bpp)
and
drm_dp_find_vcpi_slots(mst_mgr, pbn, fec_enabled, lane_count, link_rate)
and each driver would need to convert their own structs into those fields
before calling these helpers.

Is that worth it?

>> ---
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 41 ++++---------------
>>   1 file changed, 8 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index 5f2c315b18ba..b32c0790399a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>>        int slots = 0;
>>        bool ret;
>>        int clock;
>> -     int bpp = 0;
>>        int pbn = 0;
>> +     int pbn_per_timeslot;
>>
>>        aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
>>
>> @@ -205,40 +205,15 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>>        mst_port = aconnector->port;
>>
>>        if (enable) {
>> -             clock = stream->timing.pix_clk_100hz / 10;
>> -
>> -             switch (stream->timing.display_color_depth) {
>> -
>> -             case COLOR_DEPTH_666:
>> -                     bpp = 6;
>> -                     break;
>> -             case COLOR_DEPTH_888:
>> -                     bpp = 8;
>> -                     break;
>> -             case COLOR_DEPTH_101010:
>> -                     bpp = 10;
>> -                     break;
>> -             case COLOR_DEPTH_121212:
>> -                     bpp = 12;
>> -                     break;
>> -             case COLOR_DEPTH_141414:
>> -                     bpp = 14;
>> -                     break;
>> -             case COLOR_DEPTH_161616:
>> -                     bpp = 16;
>> -                     break;
>> -             default:
>> -                     ASSERT(bpp != 0);
>> -                     break;
>> -             }
>> -
>> -             bpp = bpp * 3;
>> -
>> -             /* TODO need to know link rate */
>> +             clock = dc_bandwidth_in_kbps_from_timing(&stream->timing);
>>
>> -             pbn = drm_dp_calc_pbn_mode(clock, bpp);
>> +             /* dc_bandwidth_in_kbps_from_timing already takes bpp into account */
>> +             pbn = drm_dp_calc_pbn_mode(clock, 1);
>>
>> -             slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
>> +             /* Convert kilobits per second / 64 (for 64 timeslots) to pbn (54/64 megabytes per second) */
>> +             pbn_per_timeslot = dc_link_bandwidth_kbps(
>> +                             stream->link, dc_link_get_link_cap(stream->link)) / (8 * 1000 * 54);
>> +             slots = DIV_ROUND_UP(pbn, pbn_per_timeslot);
>>                ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>>
>>                if (!ret)

________________________________________
From: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
Sent: August 19, 2019 3:21 PM
To: Francis, David; dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
Cc: Zuo, Jerry
Subject: Re: [PATCH 06/14] drm/amd/display: Use dc helpers to compute timeslot distribution

On 8/19/19 11:50 AM, David Francis wrote:
> We were using drm helpers to convert a timing into its
> bandwidth, its bandwidth into pbn, and its pbn into timeslots
>
> These helpers
> -Did not take DSC timings into account
> -Used the link rate and lane count of the link's aux device,
>   which are not the same as the link's current cap
> -Did not take FEC into account (FEC reduces the PBN per timeslot)
>
> Use the DC helpers (dc_bandwidth_in_kbps_from_timing,
> dc_link_bandwidth_kbps) instead
>
> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
> Signed-off-by: David Francis <David.Francis@amd.com>

Wouldn't this be a good candidate for shared DRM helpers or to modify
the existing ones to support this usecase?

Seems like this would be shared across drivers.

Nicholas Kazlauskas

> ---
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 41 ++++---------------
>   1 file changed, 8 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 5f2c315b18ba..b32c0790399a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -189,8 +189,8 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>       int slots = 0;
>       bool ret;
>       int clock;
> -     int bpp = 0;
>       int pbn = 0;
> +     int pbn_per_timeslot;
>
>       aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
>
> @@ -205,40 +205,15 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>       mst_port = aconnector->port;
>
>       if (enable) {
> -             clock = stream->timing.pix_clk_100hz / 10;
> -
> -             switch (stream->timing.display_color_depth) {
> -
> -             case COLOR_DEPTH_666:
> -                     bpp = 6;
> -                     break;
> -             case COLOR_DEPTH_888:
> -                     bpp = 8;
> -                     break;
> -             case COLOR_DEPTH_101010:
> -                     bpp = 10;
> -                     break;
> -             case COLOR_DEPTH_121212:
> -                     bpp = 12;
> -                     break;
> -             case COLOR_DEPTH_141414:
> -                     bpp = 14;
> -                     break;
> -             case COLOR_DEPTH_161616:
> -                     bpp = 16;
> -                     break;
> -             default:
> -                     ASSERT(bpp != 0);
> -                     break;
> -             }
> -
> -             bpp = bpp * 3;
> -
> -             /* TODO need to know link rate */
> +             clock = dc_bandwidth_in_kbps_from_timing(&stream->timing);
>
> -             pbn = drm_dp_calc_pbn_mode(clock, bpp);
> +             /* dc_bandwidth_in_kbps_from_timing already takes bpp into account */
> +             pbn = drm_dp_calc_pbn_mode(clock, 1);
>
> -             slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
> +             /* Convert kilobits per second / 64 (for 64 timeslots) to pbn (54/64 megabytes per second) */
> +             pbn_per_timeslot = dc_link_bandwidth_kbps(
> +                             stream->link, dc_link_get_link_cap(stream->link)) / (8 * 1000 * 54);
> +             slots = DIV_ROUND_UP(pbn, pbn_per_timeslot);
>               ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>
>               if (!ret)
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel