... 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!
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
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@amd.com wrote:
From: Mikita Lipski mikita.lipski@amd.com
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@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: Lyude Paul lyude@redhat.com Signed-off-by: Mikita Lipski mikita.lipski@amd.com
.../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);