On Tue, 2019-10-08 at 12:24 -0400, 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! Referring to this response, jfyi: https://lists.freedesktop.org/archives/amd-gfx/2019-October/040683.html (sorry again for replying before reading 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@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
