On 08.10.2019 12:24, Lyude Paul wrote:
> ...
> yikes
> I need to apologize because I was going through my email and I realized you
> _did_ respond to me earlier regarding some of these questions, it just appears
> the reply fell through the cracks and somehow I didn't notice it until just
> now. Sorry about that!
>
No worries, the patches got messy and are easily lost in here. I'll
clean up on the next batch so it will be clearer what's happening
> I'm still not sure I understand why this is a future enhancement? Everything
> we need to implement these helpers should already be here, it's just a matter
> of keeping track who has dsc enabled where in the mst atomic state
As I've mentioned earlier our policy for DSC is to keep it disabled if
not needed, because it is a lossy compression. Beside the fact that we
don't want imperfect image on the screen, enabling DSC would also
increase our power consumption. If we need it - we use the greedy
algorithm inside compute_mst_dsc_configs_for_link to only enable DSC on
streams that can't fit into allowed bandwidth without compression.
I'm not exactly sure how it can be moved as helper to DRM.
We keep track on which stream has DSC enabled by raising DSC flag in
dc_stream_state flags structure.
That it why pbn recalculation was moved to a separate function so only
streams that have DSC flag raised get VCPI recalculated.
I guess if we would have dsc_desired flag on drm_connnector_state and it
would then perform recalculation of PBN and VCPI for the connector.
Would that be something applicable as a DRM helper?
> On Mon, 2019-10-07 at 17:52 -0400, Lyude Paul wrote:
>> Ok, let's stop and slow down for a minute here since I've repeated myself a
>> few times, and I'd like to figure out what's actually happening here and
>> whether I'm not being clear enough with my explanations, or if there's just
>> some misunderstanding here.
>>
>> I'll start of by trying my best to explain my hesistance in accepting these
>> patches. To be clear, MST with DSC is a big thing in terms of impact.
>> There's
>> a lot of things to consider:
>> * What should the default DSC policy for MST be? Should we keep it on by
>> default so that we don't need to trigger a large number of PBN
>> recalculations and enable DSC on a per-case basis, or are there
>> legitimate
>> reasons to keep DSC off by default (such as potential issues with
>> lossiness
>> down the line).
>> * We have other drivers in the tree that are planning on implementing MST
>> DSC
>> quite soon. Chances are they'll be implementing helpers to do this so
>> this
>> can be supported across the DRM tree, and chances are we'll just have to
>> rewrite and remove most of this code in that case once they do.
>> * amdgpu already has a _lot_ of code that doesn't need to, and shouldn't be
>> there. This ranges from various DC specific helpers that are essentially
>> the same as the helpers that we already implement in the rest of DRM, to
>> misuses of various kernel APIs, and a complete lack of any kind of code
>> style (at least the last time that I checked) in or out of DC. This makes
>> the driver more difficult for people who don't work at AMD but are well
>> accustomed to working on the rest of the kernel, e.g. myself and others,
>> and it's a problem that seriously needs to be solved. Adding more
>> redundant
>> DP helpers that will inevitably get removed is just making the problem
>> worse.
>>
>> With all of this being said: I've been asking the same questions about this
>> patch throughout the whole patch series so far (since it got passed off to
>> you
>> from David's internship ending):
>>
>> * Can we keep DSC enabled by default, so that these patches aren't needed?
>> * If we can't, can we move this into common code so that other drivers can
>> use it?
>> The problem is I haven't received any responses to these questions, other
>> then
>> being told by David a month or two ago that it would be expedient to just
>> accept the patches and move on. Honestly, if discussion had been had about
>> the
>> changes I requested, I would have given my r-b a long time ago. In fact-I
>> really want to give it now! There's multiple patches in this series that
>> would
>> be very useful for other things that are being worked on at the moment, such
>> as the changes to drm_dp_dpcd_read/drm_dp_dpcd_write(). But I really think
>> this needs to be discussed first, and I'm worried that just going ahead and
>> giving my R-B before they have been discussed will further encourage rushing
>> changes like this in the future and continually building more tech debt for
>> ourselves.
>>
>> Please note as well: if anything I've asked for is confusing, or you don't
>> understand what I'm asking or looking for I am more then willing to help
>> explain things and help out as best as I can. I understand that a lot of the
>> developers working on DRM at AMD may have more experience in Windows and Mac
>> land and as a result, trying to get used to the way that we do things in the
>> Linux kernel can be a confusing endeavor. I'm more then happy to help out
>> with
>> this wherever I can, all you need to do is ask. Asking a few questions about
>> something you aren't sure you understand can save both of us a lot of time,
>> and avoid having to go through this many patch respins.
>>
>> In the mean time, I'd be willing to look at what patches from this series
>> that
>> have already been reviewed which could be pushed to drm-misc or friends in
>> the
>> mean time to speed things up a bit.
>>
>> On Tue, 2019-10-01 at 12:17 -0400, mikita.lipski@xxxxxxx wrote:
>>> From: Mikita Lipski <mikita.lipski@xxxxxxx>
>>>
>>> Since for DSC MST connector's PBN is claculated differently
>>> due to compression, we have to recalculate both PBN and
>>> VCPI slots for that connector.
>>>
>>> This patch proposes to use similair logic as in
>>> dm_encoder_helper_atomic_chek, but since we do not know which
>>> connectors will have DSC enabled we have to recalculate PBN only
>>> after that's determined, which is done in
>>> compute_mst_dsc_configs_for_state.
>>>
>>> Cc: Jerry Zuo <Jerry.Zuo@xxxxxxx>
>>> Cc: Harry Wentland <harry.wentland@xxxxxxx>
>>> Cc: Lyude Paul <lyude@xxxxxxxxxx>
>>> Signed-off-by: Mikita Lipski <mikita.lipski@xxxxxxx>
>>> ---
>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 64 +++++++++++++++++--
>>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 --
>>> 2 files changed, 59 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 81e30918f9ec..7501ce2233ed 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -4569,6 +4569,27 @@ static void dm_encoder_helper_disable(struct
>>> drm_encoder *encoder)
>>>
>>> }
>>>
>>> +static int convert_dc_color_depth_into_bpc (enum dc_color_depth
>>> display_color_depth)
>>> +{
>>> + switch (display_color_depth) {
>>> + case COLOR_DEPTH_666:
>>> + return 6;
>>> + case COLOR_DEPTH_888:
>>> + return 8;
>>> + case COLOR_DEPTH_101010:
>>> + return 10;
>>> + case COLOR_DEPTH_121212:
>>> + return 12;
>>> + case COLOR_DEPTH_141414:
>>> + return 14;
>>> + case COLOR_DEPTH_161616:
>>> + return 16;
>>> + default:
>>> + break;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>>> struct drm_crtc_state *crtc_state,
>>> struct drm_connector_state
>>> *conn_state)
>>> @@ -4616,6 +4637,36 @@ const struct drm_encoder_helper_funcs
>>> amdgpu_dm_encoder_helper_funcs = {
>>> .atomic_check = dm_encoder_helper_atomic_check
>>> };
>>>
>>> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>>> +static int dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state
>>> *state,
>>> + struct dc_state *dc_state)
>>> +{
>>> + struct dc_stream_state *stream;
>>> + struct amdgpu_dm_connector *aconnector;
>>> + int i, clock = 0, bpp = 0;
>>> +
>>> + for (i = 0; i < dc_state->stream_count; i++) {
>>> + stream = dc_state->streams[i];
>>> + aconnector = (struct amdgpu_dm_connector *)stream-
>>>> dm_stream_context;
>>> +
>>> + if (stream && stream->timing.flags.DSC == 1) {
>>> + bpp = convert_dc_color_depth_into_bpc(stream-
>>>> timing.display_color_depth)* 3;
>>> + clock = stream->timing.pix_clk_100hz / 10;
>>> +
>>> + aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp,
>>> true);
>>> +
>>> + aconnector->vcpi_slots =
>>> drm_dp_atomic_find_vcpi_slots(state,
>>> + &aconnector-
>>>> mst_port->mst_mgr,
>>> + aconnector->port,
>>> + aconnector->pbn);
>>> + if (aconnector->vcpi_slots < 0)
>>> + return aconnector->vcpi_slots;
>>> + }
>>> + }
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> static void dm_drm_plane_reset(struct drm_plane *plane)
>>> {
>>> struct dm_plane_state *amdgpu_state = NULL;
>>> @@ -7629,11 +7680,6 @@ static int amdgpu_dm_atomic_check(struct drm_device
>>> *dev,
>>> if (ret)
>>> goto fail;
>>>
>>> - /* Perform validation of MST topology in the state*/
>>> - ret = drm_dp_mst_atomic_check(state);
>>> - if (ret)
>>> - goto fail;
>>> -
>>> if (state->legacy_cursor_update) {
>>> /*
>>> * This is a fast cursor update coming from the plane update
>>> @@ -7705,6 +7751,10 @@ static int amdgpu_dm_atomic_check(struct drm_device
>>> *dev,
>>> #ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>>> if (!compute_mst_dsc_configs_for_state(dm_state->context))
>>> goto fail;
>>> +
>>> + ret = dm_update_mst_vcpi_slots_for_dsc(state, dm_state-
>>>> context);
>>> + if (ret)
>>> + goto fail;
>>> #endif
>>> if (dc_validate_global_state(dc, dm_state->context, false) !=
>>> DC_OK) {
>>> ret = -EINVAL;
>>> @@ -7734,6 +7784,10 @@ static int amdgpu_dm_atomic_check(struct drm_device
>>> *dev,
>>> dc_retain_state(old_dm_state->context);
>>> }
>>> }
>>> + /* Perform validation of MST topology in the state*/
>>> + ret = drm_dp_mst_atomic_check(state);
>>> + if (ret)
>>> + goto fail;
>>>
>>> /* Store the overall update type for use later in atomic check. */
>>> for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
>>> 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 bd694499e3de..3e44e2da2d0d 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
>>> @@ -201,12 +201,6 @@ bool
>>> dm_helpers_dp_mst_write_payload_allocation_table(
>>> mst_port = aconnector->port;
>>>
>>> if (enable) {
>>> -
>>> - /* 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);
>>> - aconnector->vcpi_slots = DIV_ROUND_UP(aconnector->pbn,
>>> pbn_per_timeslot);
>>> -
>>> ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
>>> aconnector->pbn,
>>> aconnector->vcpi_slots);
--
Thanks,
Mikita Lipski
Software Engineer, AMD
mikita.lipski@xxxxxxx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
