Re: [PATCH v2 06/14] drm/dp-mst: Use dc and drm helpers to compute timeslots

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

 



On Wed, 2019-08-21 at 12:27 +0000, Kazlauskas, Nicholas wrote:
> On 8/20/19 4:43 PM, Lyude Paul wrote:
> > Some nitpicks below
> > 
> > On Tue, 2019-08-20 at 15:11 -0400, David Francis wrote:
> > > We were using drm helpers to convert a timing into its
> > > bandwidth, its bandwidth into pbn, and its pbn into timeslots
> > > 
> > > These helpers
> > > -Did not take DSC timings into account
> > > -Used the link rate and lane count of the link's aux device,
> > >   which are not the same as the link's current cap
> > > -Did not take FEC into account (FEC reduces the PBN per timeslot)
> > > 
> > > For converting timing into PBN, add a new function
> > > drm_dp_calc_pbn_mode_dsc that handles the DSC case
> > > 
> > > For converting PBN into time slots, amdgpu doesn't use the
> > > 'correct' atomic method (drm_dp_atomic_find_vcpi_slots), so
> > > don't add a new helper to cover our approach. Use the same
> > > means of calculating pbn per time slot as the DSC code.
> > > 
> > > v2: Add drm helper for clock to pbn conversion
> > > 
> > > Cc: Jerry Zuo <Jerry.Zuo@xxxxxxx>
> > > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> > > Signed-off-by: David Francis <David.Francis@xxxxxxx>
> 
> I would like ot see Lyude's nitpicks addressed but also having this 
> patch split into one that just adds the helper and another that uses it 
> in amdgpu.

Sgtm!

> 
> Nicholas Kazlauskas
> 
> > > ---
> > >   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 18 +++++---
> > >   drivers/gpu/drm/drm_dp_mst_topology.c         | 41 +++++++++++++++++++
> > >   include/drm/drm_dp_mst_helper.h               |  2 +-
> > >   3 files changed, 54 insertions(+), 7 deletions(-)
> > > 
> > > 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 5f2c315b18ba..dfa99e0d6e64 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
> > > @@ -189,8 +189,8 @@ bool
> > > dm_helpers_dp_mst_write_payload_allocation_table(
> > >   	int slots = 0;
> > >   	bool ret;
> > >   	int clock;
> > > -	int bpp = 0;
> > >   	int pbn = 0;
> > > +	int pbn_per_timeslot, bpp = 0;
> > >   
> > >   	aconnector = (struct amdgpu_dm_connector *)stream-
> > > >dm_stream_context;
> > >   
> > > @@ -208,7 +208,6 @@ bool
> > > dm_helpers_dp_mst_write_payload_allocation_table(
> > >   		clock = stream->timing.pix_clk_100hz / 10;
> > >   
> > >   		switch (stream->timing.display_color_depth) {
> > > -
> > >   		case COLOR_DEPTH_666:
> > >   			bpp = 6;
> > >   			break;
> > > @@ -234,11 +233,18 @@ bool
> > > dm_helpers_dp_mst_write_payload_allocation_table(
> > >   
> > >   		bpp = bpp * 3;
> > >   
> > > -		/* TODO need to know link rate */
> > > -
> > > -		pbn = drm_dp_calc_pbn_mode(clock, bpp);
> > > +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> > > +		if (stream->timing.flags.DSC)
> > > +			pbn = drm_dp_calc_pbn_mode_dsc(clock,
> > > +					stream-
> > > > timing.dsc_cfg.bits_per_pixel);
> > > +		else
> > > +#endif
> > > +			pbn = drm_dp_calc_pbn_mode(clock, bpp);
> > >   
> > > -		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
> > > +		/* 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);
> > > +		slots = DIV_ROUND_UP(pbn, pbn_per_timeslot);
> > >   		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn,
> > > slots);
> > >   
> > >   		if (!ret)
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 398e7314ea8b..d789b7af7dbf 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -3588,6 +3588,47 @@ static int test_calc_pbn_mode(void)
> > >   	return 0;
> > >   }
> > >   
> > > +/**
> > > + * drm_dp_calc_pbn_mode_dsc() - Calculate the PBN for a mode with DSC
> > > enabled.
> > > + * @clock: dot clock for the mode
> > > + * @dsc_bpp: dsc bits per pixel x16 (e.g. dsc_bpp = 136 is 8.5 bpp)
> > > + *
> > > + * This uses the formula in the spec to calculate the PBN value for a
> > > mode,
> > > + * given that the mode is using DSC
> > 
> > You should use the proper kdoc formatting for documenting return values
> > (not
> > all of the DP MST code does this currently, but that's a bug I haven't
> > taken
> > the time to fix :):
> > 
> > /*
> >   * foo_bar() - foo the bar
> >   *
> >   * foos the bar, guaranteed
> >   * Returns:
> >   * Some magic number
> >   */
> > 
> > > + */
> > > +int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp)
> > > +{
> > > +	u64 kbps;
> > > +	s64 peak_kbps;
> > > +	u32 numerator;
> > > +	u32 denominator;
> > > +
> > > +	kbps = clock * dsc_bpp;
> > > +
> > > +	/*
> > > +	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> > > +	 * The unit of 54/64Mbytes/sec is an arbitrary unit chosen based on
> > > +	 * common multiplier to render an integer PBN for all link rate/lane
> > > +	 * counts combinations
> > > +	 * calculate
> > > +	 * peak_kbps *= (1/16) bppx16 to bpp
> > > +	 * peak_kbps *= (1006/1000)
> > > +	 * peak_kbps *= (64/54)
> > > +	 * peak_kbps *= 8    convert to bytes
> > > +	 *
> > > +	 * Divide numerator and denominator by 16 to avoid overflow
> > > +	 */
> > > +
> > > +	numerator = 64 * 1006 / 16;
> > > +	denominator = 54 * 8 * 1000 * 1000;
> > > +
> > > +	kbps *= numerator;
> > > +	peak_kbps = drm_fixp_from_fraction(kbps, denominator);
> > > +
> > > +	return drm_fixp2int_ceil(peak_kbps);
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_calc_pbn_mode_dsc);
> > > +
> > >   /* we want to kick the TX after we've ack the up/down IRQs. */
> > >   static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
> > >   {
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index 2ba6253ea6d3..ddb518f2157a 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -611,7 +611,7 @@ struct edid *drm_dp_mst_get_edid(struct
> > > drm_connector
> > > *connector, struct drm_dp_
> > >   
> > >   
> > >   int drm_dp_calc_pbn_mode(int clock, int bpp);
> > > -
> > > +int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp);
> > >   
> > >   bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > >   			      struct drm_dp_mst_port *port, int pbn,
> > > int
> > > slots);
-- 
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