Re: [PATCH 04/11] drm/amd/display: Handle SMU msg response

["Kazlauskas, Nicholas" <nicholas.kazlauskas@xxxxxxx>]

On 2020-06-26 12:48 p.m., Alex Deucher wrote:
> On Fri, Jun 26, 2020 at 12:19 PM Eryk Brol <eryk.brol@xxxxxxx> wrote:
> > From: Yongqiang Sun <yongqiang.sun@xxxxxxx>
> >
> > [Why]
> > SMU may return error code to driver, but driver only check if response
> > is OK.
> >
> > [How]
> > Check SMU response instead of reg_wait, assert in case of reponse isn't
> > OK.
>
> Will you ever get concurrent calls to these interfaces or do you
> already have a higher level lock to prevent that?  You need to make
> sure you don't have multiple threads using these interfaces at the
> same time or you'll need locking to protect the message, param, and
> response registers.
>
> Alex

IIRC this is a dedicated message port for DCN. I think DC locking 
prevents this from happening on the software side and HW/firmware has a 
different port.

Regards,
Nicholas Kazlauskas

> > Signed-off-by: Yongqiang Sun <yongqiang.sun@xxxxxxx>
> > Reviewed-by: Tony Cheng <Tony.Cheng@xxxxxxx>
> > Acked-by: Eryk Brol <eryk.brol@xxxxxxx>
> > ---
> >   .../dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c  | 39 +++++++++++++++++-
> >   .../dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c   | 40 ++++++++++++++++++-
> >   2 files changed, 75 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
> > index c320b7af7d34..dbc7cde00433 100644
> > --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
> > +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
> > @@ -26,6 +26,7 @@
> >   #include "core_types.h"
> >   #include "clk_mgr_internal.h"
> >   #include "reg_helper.h"
> > +#include <linux/delay.h>
> >
> >   #define MAX_INSTANCE   5
> >   #define MAX_SEGMENT            5
> > @@ -68,10 +69,42 @@ static const struct IP_BASE MP1_BASE  = { { { { 0x00016000, 0, 0, 0, 0 } },
> >   #define VBIOSSMC_MSG_SetDispclkFreq           0x4
> >   #define VBIOSSMC_MSG_SetDprefclkFreq          0x5
> >
> > +#define VBIOSSMC_Status_BUSY                      0x0
> > +#define VBIOSSMC_Result_OK                        0x1
> > +#define VBIOSSMC_Result_Failed                    0xFF
> > +#define VBIOSSMC_Result_UnknownCmd                0xFE
> > +#define VBIOSSMC_Result_CmdRejectedPrereq         0xFD
> > +#define VBIOSSMC_Result_CmdRejectedBusy           0xFC
> > +
> > +/*
> > + * Function to be used instead of REG_WAIT macro because the wait ends when
> > + * the register is NOT EQUAL to zero, and because the translation in msg_if.h
> > + * won't work with REG_WAIT.
> > + */
> > +static uint32_t rv1_smu_wait_for_response(struct clk_mgr_internal *clk_mgr, unsigned int delay_us, unsigned int max_retries)
> > +{
> > +       uint32_t res_val = VBIOSSMC_Status_BUSY;
> > +
> > +       do {
> > +               res_val = REG_READ(MP1_SMN_C2PMSG_91);
> > +               if (res_val != VBIOSSMC_Status_BUSY)
> > +                       break;
> > +
> > +               if (delay_us >= 1000)
> > +                       msleep(delay_us/1000);
> > +               else if (delay_us > 0)
> > +                       udelay(delay_us);
> > +       } while (max_retries--);
> > +
> > +       return res_val;
> > +}
> > +
> >   int rv1_vbios_smu_send_msg_with_param(struct clk_mgr_internal *clk_mgr, unsigned int msg_id, unsigned int param)
> >   {
> > +       uint32_t result;
> > +
> >          /* First clear response register */
> > -       REG_WRITE(MP1_SMN_C2PMSG_91, 0);
> > +       REG_WRITE(MP1_SMN_C2PMSG_91, VBIOSSMC_Status_BUSY);
> >
> >          /* Set the parameter register for the SMU message, unit is Mhz */
> >          REG_WRITE(MP1_SMN_C2PMSG_83, param);
> > @@ -79,7 +112,9 @@ int rv1_vbios_smu_send_msg_with_param(struct clk_mgr_internal *clk_mgr, unsigned
> >          /* Trigger the message transaction by writing the message ID */
> >          REG_WRITE(MP1_SMN_C2PMSG_67, msg_id);
> >
> > -       REG_WAIT(MP1_SMN_C2PMSG_91, CONTENT, 1, 10, 200000);
> > +       result = rv1_smu_wait_for_response(clk_mgr, 10, 1000);
> > +
> > +       ASSERT(result == VBIOSSMC_Result_OK);
> >
> >          /* Actual dispclk set is returned in the parameter register */
> >          return REG_READ(MP1_SMN_C2PMSG_83);
> > diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
> > index 6878aedf1d3e..d2facbb114d3 100644
> > --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
> > +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
> > @@ -26,6 +26,7 @@
> >   #include "core_types.h"
> >   #include "clk_mgr_internal.h"
> >   #include "reg_helper.h"
> > +#include <linux/delay.h>
> >
> >   #include "renoir_ip_offset.h"
> >
> > @@ -53,10 +54,43 @@
> >   #define VBIOSSMC_MSG_EnableTmdp48MHzRefclkPwrDown 0xD
> >   #define VBIOSSMC_MSG_UpdatePmeRestore                    0xE
> >
> > +#define VBIOSSMC_Status_BUSY                      0x0
> > +#define VBIOSSMC_Result_OK                        0x1
> > +#define VBIOSSMC_Result_Failed                    0xFF
> > +#define VBIOSSMC_Result_UnknownCmd                0xFE
> > +#define VBIOSSMC_Result_CmdRejectedPrereq         0xFD
> > +#define VBIOSSMC_Result_CmdRejectedBusy           0xFC
> > +
> > +/*
> > + * Function to be used instead of REG_WAIT macro because the wait ends when
> > + * the register is NOT EQUAL to zero, and because the translation in msg_if.h
> > + * won't work with REG_WAIT.
> > + */
> > +static uint32_t rn_smu_wait_for_response(struct clk_mgr_internal *clk_mgr, unsigned int delay_us, unsigned int max_retries)
> > +{
> > +       uint32_t res_val = VBIOSSMC_Status_BUSY;
> > +
> > +       do {
> > +               res_val = REG_READ(MP1_SMN_C2PMSG_91);
> > +               if (res_val != VBIOSSMC_Status_BUSY)
> > +                       break;
> > +
> > +               if (delay_us >= 1000)
> > +                       msleep(delay_us/1000);
> > +               else if (delay_us > 0)
> > +                       udelay(delay_us);
> > +       } while (max_retries--);
> > +
> > +       return res_val;
> > +}
> > +
> > +
> >   int rn_vbios_smu_send_msg_with_param(struct clk_mgr_internal *clk_mgr, unsigned int msg_id, unsigned int param)
> >   {
> > +       uint32_t result;
> > +
> >          /* First clear response register */
> > -       REG_WRITE(MP1_SMN_C2PMSG_91, 0);
> > +       REG_WRITE(MP1_SMN_C2PMSG_91, VBIOSSMC_Status_BUSY);
> >
> >          /* Set the parameter register for the SMU message, unit is Mhz */
> >          REG_WRITE(MP1_SMN_C2PMSG_83, param);
> > @@ -64,7 +98,9 @@ int rn_vbios_smu_send_msg_with_param(struct clk_mgr_internal *clk_mgr, unsigned
> >          /* Trigger the message transaction by writing the message ID */
> >          REG_WRITE(MP1_SMN_C2PMSG_67, msg_id);
> >
> > -       REG_WAIT(MP1_SMN_C2PMSG_91, CONTENT, 1, 10, 200000);
> > +       result = rn_smu_wait_for_response(clk_mgr, 10, 1000);
> > +
> > +       ASSERT(result == VBIOSSMC_Result_OK);
> >
> >          /* Actual dispclk set is returned in the parameter register */
> >          return REG_READ(MP1_SMN_C2PMSG_83);
> > --
> > 2.25.1
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx