Re: [PATCH] drm/amd/display: Add get_dig_frontend implementation for DCEx

[Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>]

On 12/15, Alex Deucher wrote:
> On Tue, Dec 15, 2020 at 10:45 AM Rodrigo Siqueira
> <Rodrigo.Siqueira@xxxxxxx> wrote:
> >
> > Some old ASICs might not implement/require get_dig_frontend helper; in
> > this scenario, we can have a NULL pointer exception when we try to call
> > it inside vbios disable operation. For example, this situation might
> > happen when using Polaris12 with an eDP panel. This commit avoids this
> > situation by adding a specific get_dig_frontend implementation for DCEx.
> >
> > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > Cc: Borislav Petkov <bp@xxxxxxxxx>
> > Cc: Harry Wentland <Harry.Wentland@xxxxxxx>
> > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@xxxxxxx>
> > Cc: Chiawen Huang <chiawen.huang@xxxxxxx>
> > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
> > ---
> >  .../drm/amd/display/dc/dce/dce_link_encoder.c | 43 ++++++++++++++++++-
> >  .../drm/amd/display/dc/dce/dce_link_encoder.h |  2 +
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.c b/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.c
> > index 4592ccdfa9b0..f355cd1e9090 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.c
> > @@ -120,6 +120,7 @@ static const struct link_encoder_funcs dce110_lnk_enc_funcs = {
> >         .is_dig_enabled = dce110_is_dig_enabled,
> >         .destroy = dce110_link_encoder_destroy,
> >         .get_max_link_cap = dce110_link_encoder_get_max_link_cap,
> > +       .get_dig_frontend = dce110_get_dig_frontend,
> >  };
> >
> >  static enum bp_result link_transmitter_control(
> > @@ -235,6 +236,45 @@ static void set_link_training_complete(
> >
> >  }
> >
> > +unsigned int dce110_get_dig_frontend(struct link_encoder *enc)
> > +{
> > +       struct dce110_link_encoder *enc110 = TO_DCE110_LINK_ENC(enc);
> > +       u32 value;
> > +       enum engine_id result;
> > +
> > +       REG_GET(DIG_BE_CNTL, DIG_FE_SOURCE_SELECT, &value);
> > +
> > +       switch (value) {
> > +       case DCE110_DIG_FE_SOURCE_SELECT_DIGA:
> > +               result = ENGINE_ID_DIGA;
> > +               break;
> > +       case DCE110_DIG_FE_SOURCE_SELECT_DIGB:
> > +               result = ENGINE_ID_DIGB;
> > +               break;
> > +       case DCE110_DIG_FE_SOURCE_SELECT_DIGC:
> > +               result = ENGINE_ID_DIGC;
> > +               break;
> > +       case DCE110_DIG_FE_SOURCE_SELECT_DIGD:
> > +               result = ENGINE_ID_DIGD;
> > +               break;
> > +       case DCE110_DIG_FE_SOURCE_SELECT_DIGE:
> > +               result = ENGINE_ID_DIGE;
> > +               break;
> > +       case DCE110_DIG_FE_SOURCE_SELECT_DIGF:
> > +               result = ENGINE_ID_DIGF;
> > +               break;
> > +       case DCE110_DIG_FE_SOURCE_SELECT_DIGG:
> > +               result = ENGINE_ID_DIGG;
> > +               break;
> > +       default:
> > +               // invalid source select DIG
> > +               ASSERT(false);
> > +               result = ENGINE_ID_UNKNOWN;
> 
> Do we really want the ASSERT?  The same function for DCN asserts all
> the time on newer APUs if the register has it's default value (e.g.,
> if a particular output was not used by the vbios).  IMHO, we should
> remove the assert both here and for DCN.

Make sense to me, and I can drop the ASSERT before apply the patch.

Nick, Harry,
Do you have any objection to drop this ASSERT?
 
> Alex
> 
> 
> > +       }
> > +
> > +       return result;
> > +}
> > +
> >  void dce110_link_encoder_set_dp_phy_pattern_training_pattern(
> >         struct link_encoder *enc,
> >         uint32_t index)
> > @@ -1665,7 +1705,8 @@ static const struct link_encoder_funcs dce60_lnk_enc_funcs = {
> >         .disable_hpd = dce110_link_encoder_disable_hpd,
> >         .is_dig_enabled = dce110_is_dig_enabled,
> >         .destroy = dce110_link_encoder_destroy,
> > -       .get_max_link_cap = dce110_link_encoder_get_max_link_cap
> > +       .get_max_link_cap = dce110_link_encoder_get_max_link_cap,
> > +       .get_dig_frontend = dce110_get_dig_frontend
> >  };
> >
> >  void dce60_link_encoder_construct(
> > diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.h b/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.h
> > index cb714a48b171..fc6ade824c23 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.h
> > +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.h
> > @@ -295,6 +295,8 @@ void dce110_link_encoder_connect_dig_be_to_fe(
> >         enum engine_id engine,
> >         bool connect);
> >
> > +unsigned int dce110_get_dig_frontend(struct link_encoder *enc);
> > +
> >  void dce110_link_encoder_set_dp_phy_pattern_training_pattern(
> >         struct link_encoder *enc,
> >         uint32_t index);
> > --
> > 2.29.2
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CRodrigo.Siqueira%40amd.com%7Ce4e5be182f2e462a5fd908d8a11cfc67%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637436493154372722%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=%2BVbEKhRiSaQCDYYGo2aa8BADRJiposg4VJM9G0Z1Mw4%3D&amp;reserved=0

-- 
Rodrigo Siqueira
https://siqueira.tech

Attachment: signature.asc
Description: PGP signature

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