Re: [Intel-gfx] [PATCH 2/4] drm/i915: Simplify intel_cx0_program_phy_lane() with loop

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 31 Jul 2023, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2023-07-31 08:04:12-03:00)
>>On Tue, 25 Jul 2023, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>> It is possible to generalize the "disable" value for the transmitters to
>>> be a bit mask based on the port width and the port reversal boolean,
>>> with a small exception for DP-alt mode with "x1" port width.
>>>
>>> Simplify the code by using such a mask and a for-loop instead of using
>>> switch-case statements.
>>>
>>> BSpec: 64539
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 79 +++++---------------
>>>  1 file changed, 20 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> index b903ceb0b56a..f10ebdfd696a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>>> @@ -2604,7 +2604,8 @@ static void intel_cx0_program_phy_lane(struct drm_i915_private *i915,
>>>                                         struct intel_encoder *encoder, int lane_count,
>>>                                         bool lane_reversal)
>>>  {
>>> -        u8 l0t1, l0t2, l1t1, l1t2;
>>> +        int i;
>>> +        u8 disables;
>>>          bool dp_alt_mode = intel_tc_port_in_dp_alt_mode(enc_to_dig_port(encoder));
>>>          enum port port = encoder->port;
>>>  
>>> @@ -2614,66 +2615,26 @@ static void intel_cx0_program_phy_lane(struct drm_i915_private *i915,
>>>                                C10_VDR_CTRL_MSGBUS_ACCESS,
>>>                                MB_WRITE_COMMITTED);
>>>  
>>> -        /* TODO: DP-alt MFD case where only one PHY lane should be programmed. */
>>> -        l0t1 = intel_cx0_read(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(1, 2));
>>> -        l0t2 = intel_cx0_read(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(2, 2));
>>> -        l1t1 = intel_cx0_read(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(1, 2));
>>> -        l1t2 = intel_cx0_read(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(2, 2));
>>> -
>>> -        l0t1 |= CONTROL2_DISABLE_SINGLE_TX;
>>> -        l0t2 |= CONTROL2_DISABLE_SINGLE_TX;
>>> -        l1t1 |= CONTROL2_DISABLE_SINGLE_TX;
>>> -        l1t2 |= CONTROL2_DISABLE_SINGLE_TX;
>>> -
>>> -        if (lane_reversal) {
>>> -                switch (lane_count) {
>>> -                case 4:
>>> -                        l0t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        fallthrough;
>>> -                case 3:
>>> -                        l0t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        fallthrough;
>>> -                case 2:
>>> -                        l1t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        fallthrough;
>>> -                case 1:
>>> -                        l1t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        break;
>>> -                default:
>>> -                        MISSING_CASE(lane_count);
>>> -                }
>>> -        } else {
>>> -                switch (lane_count) {
>>> -                case 4:
>>> -                        l1t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        fallthrough;
>>> -                case 3:
>>> -                        l1t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        fallthrough;
>>> -                case 2:
>>> -                        l0t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        l0t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        break;
>>> -                case 1:
>>> -                        if (dp_alt_mode)
>>> -                                l0t2 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        else
>>> -                                l0t1 &= ~CONTROL2_DISABLE_SINGLE_TX;
>>> -                        break;
>>> -                default:
>>> -                        MISSING_CASE(lane_count);
>>> -                }
>>> +        if (lane_reversal)
>>> +                disables = REG_GENMASK8(3, 0) >> lane_count;
>>> +        else
>>> +                disables = REG_GENMASK8(3, 0) << lane_count;
>>> +
>>> +        if (dp_alt_mode && lane_count == 1) {
>>> +                disables &= ~REG_GENMASK8(1, 0);
>>> +                disables |= REG_FIELD_PREP8(REG_GENMASK8(1, 0), 0x1);
>>>          }
>>>  
>>> -        /* disable MLs */
>>> -        intel_cx0_write(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(1, 2),
>>> -                        l0t1, MB_WRITE_COMMITTED);
>>> -        intel_cx0_write(i915, port, INTEL_CX0_LANE0, PHY_CX0_TX_CONTROL(2, 2),
>>> -                        l0t2, MB_WRITE_COMMITTED);
>>> -        intel_cx0_write(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(1, 2),
>>> -                        l1t1, MB_WRITE_COMMITTED);
>>> -        intel_cx0_write(i915, port, INTEL_CX0_LANE1, PHY_CX0_TX_CONTROL(2, 2),
>>> -                        l1t2, MB_WRITE_COMMITTED);
>>> +        /* TODO: DP-alt MFD case where only one PHY lane should be programmed. */
>>> +        for (i = 0; i < 4; i++) {
>>> +                int tx = i % 2 + 1;
>>> +                u8 lane_mask = i / 2 == 0 ? INTEL_CX0_LANE0 : INTEL_CX0_LANE1;
>>
>>I'm just catching up on mails and quickly eyeballing stuff, but
>>
>>        i / 2 == 0
>>
>>looks suspect.
>
> i / 2 == 0 should give us the correct selection of lane_mask: the first two
> iterations are for the first PHY lane and the last two are for the last PHY
> lane.

I think the most obvious way to express that is i < 2.

BR,
Jani.

>
> --
> Gustavo Sousa
>
>>
>>BR,
>>Jani.
>>
>>> +
>>> +                intel_cx0_rmw(i915, port, lane_mask, PHY_CX0_TX_CONTROL(tx, 2),
>>> +                              CONTROL2_DISABLE_SINGLE_TX,
>>> +                              disables & BIT(i) ? CONTROL2_DISABLE_SINGLE_TX : 0,
>>> +                              MB_WRITE_COMMITTED);
>>> +        }
>>>  
>>>          if (intel_is_c10phy(i915, intel_port_to_phy(i915, port)))
>>>                  intel_cx0_rmw(i915, port, INTEL_CX0_BOTH_LANES,
>>
>>-- 
>>Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center