Re: [PATCH 13/14] drm/amd/display: Recalculate VCPI slots for new DSC connectors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2019-10-08 at 21:26 +0000, Mikita Lipski wrote:
> 
> 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?

This would probably be a better fit for drm_dp_mst_topology_mgr's atomic state
(take a look at the atomic VCPI helpers), specifically in struct
drm_dp_mst_vcpi (maybe we should rename it to struct drm_dp_mst_port_bw while
we're at it) and store DSC enablement information there. Then we can write a
helper to go through all of the enabled ports on the topology and add the
affected ones into the atomic state and set ->mode_changed on all of them.
> 
> 
> > 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
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux