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);
--
Cheers,
Lyude Paul
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
