[Public]
Hi Imre,
Thanks for your time and suggestions!
I'll adjust it and send out v2 later.
Thanks,
Wayne
> -----Original Message-----
> From: Imre Deak <imre.deak@xxxxxxxxx>
> Sent: Saturday, November 30, 2024 4:24 AM
> To: Lin, Wayne <Wayne.Lin@xxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; lyude@xxxxxxxxxx; Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>
> Subject: Re: [PATCH] drm/dp_mst: Fix drm RAD print
>
> On Wed, Nov 27, 2024 at 05:49:38PM +0800, Wayne Lin wrote:
> > [Why]
> > The RAD of sideband message printed today is incorrect.
> > For RAD stored within MST branch
> > - If MST branch LCT is 1, it's RAD array is untouched and remained as 0.
> > - If MST branch LCT is larger than 1, usd nibble to store the up facing
> > port number in cascaded sequence as illustrated below:
> >
> > u8 RAD[0] = (LCT_2_UFP << 4) | LCT_3_UFP
> > RAD[1] = (LCT_4_UFP << 4) | LCT_5_UFP
> > ...
> >
> > In drm_dp_mst_rad_to_str(), it wrongly to use BIT_MASK(4) to fetch the
> > port number of one nibble.
> >
> > [How]
> > Adjust the code by:
> > - RAD array items are valuable only for LCT >= 1.
> > - Use 0xF as the mask to replace BIT_MASK(4)
> >
> > Fixes: 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
> > selftests")
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Harry Wentland <hwentlan@xxxxxxx>
> > Cc: Lyude Paul <lyude@xxxxxxxxxx>
> > Signed-off-by: Wayne Lin <Wayne.Lin@xxxxxxx>
> > ---
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index f7c6b60629c2..152c60f1e80f 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -179,13 +179,13 @@ static int
> > drm_dp_mst_rad_to_str(const u8 rad[8], u8 lct, char *out, size_t len)
> > {
> > int i;
> > - u8 unpacked_rad[16];
> > + u8 unpacked_rad[16] = {0};
>
> The usual way is arr[16] = {};
>
> >
> > - for (i = 0; i < lct; i++) {
> > + for (i = 1; i < lct; i++) {
> > if (i % 2)
> > - unpacked_rad[i] = rad[i / 2] >> 4;
> > + unpacked_rad[i] = rad[(i - 1) / 2] >> 4;
> > else
> > - unpacked_rad[i] = rad[i / 2] & BIT_MASK(4);
> > + unpacked_rad[i] = rad[(i - 1) / 2] & 0xF;
> > }
>
> So unpacked_rad[0] will be always zero, which represents
> drm_dp_mst_topology_mgr::mst_primary, but not actually stored in
> drm_dp_mst_branch::rad[]. In each element of rad[] the nibbles are ordered the
> most signifcant 4 bits first and the least significant 4 bits second. This could be
> documented at drm_dp_mst_branch::rad[] imo.
>
> The fix looks ok to me:
> Acked-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> drm_dp_get_mst_branch_device() needs the same logic as above extracting one
> element of RAD at a given index, so a helper for this could be added and used in
> both places.
>
> >
> > /* TODO: Eventually add something to printk so we can format the rad
> > --
> > 2.37.3
> >
