...
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@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
