On Sat, 12 Feb 2022 at 03:59, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> On 2/11/2022 4:08 PM, Dmitry Baryshkov wrote:
> > On 12/02/2022 02:23, Kuogee Hsieh wrote:
> >> intf_audio_select() callback function use to configure
> >> HDMI_DP_CORE_SELECT to decide audio output routes to HDMI or DP
> >> interface. HDMI is obsoleted at newer chipset. To keep supporting
> >> legacy hdmi application, intf_audio_select call back function have
> >> to be populated base on hardware chip capability where legacy
> >> chipsets have has_audio_select flag set to true.
> >
> > So, after thinking more about the patch, I have a bunch of questions:
> >
> > You are enabling this callback only for sdm845 and sm8150.
> >
> > Does this register exist on other (newer) platforms (but just defaults
> > to DP)?
>
> The register itself exists but there is no logic associated with it. Its
> a no-op.
Ack, thanks
>
> >
> > Neither sdm845 nor sm8150 support INTF_HDMI. What's the purpose of the
> > register on these platforms?
>
> Yes we also had a similar thought earlier that this register has meaning
> only on chipsets which have HDMI and DP but our hardware team suggested
> sm8250 and its derivatives should be the cut-off point to stop using
> this register. So we are just following that.
Ack
>
> >
> > Does that mean that we should program the register for HDMI (e.g. on 8998)?
> >
> Yes, we should program this for HDMI 8998 ( although the default value
> of the register is 0 for HDMI ).
ok, so ideally we should add an argument switching between HDMI and DP audio.
This can be done in a separate patch.
>
> > And, as you are touching this piece of code, how do we control audio
> > routing on newer platforms which have several hardware DP interfaces?
> >
> Thats unrelated to this register because on newer chipsets which have
> two DPs there is no HDMI and hence this register remains a no-op.
Yep. I just wondered whether this register would be reused to switch
between DPs.
It doesn't.
>
> But coming to the overall question on multi-DP audio.
>
> This is not a new question. I had first asked about this to Bjorn for
> sc8180x. The current hdmi-codec interface which is used for single DP
> audio will have to be extended to support this to support which stream
> to pass the audio on. This is an open item which was left to be done
> later on because the only chipset which has multi-DP in upstream is
> sc8180x. We dont have that hardware with us for development. When we
> start working on that, we will have to implement what I just mentioned.
The implementation depends on the question whether the SoC can output
several DP audio streams in parallel,
but as the register in question is not used, it's definitely a
separate question.
>
> Thanks
>
> Abhinav
>
> >
> >>
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 ++
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 +
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 9 ++++++---
> >> 3 files changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> index 272b14b..23680e7 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> @@ -201,6 +201,7 @@ static const struct dpu_caps sdm845_dpu_caps = {
> >> .has_dim_layer = true,
> >> .has_idle_pc = true,
> >> .has_3d_merge = true,
> >> + .has_audio_select = true,
> >> .max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
> >> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> >> .max_hdeci_exp = MAX_HORZ_DECIMATION,
> >> @@ -229,6 +230,7 @@ static const struct dpu_caps sm8150_dpu_caps = {
> >> .has_dim_layer = true,
> >> .has_idle_pc = true,
> >> .has_3d_merge = true,
> >> + .has_audio_select = true,
> >> .max_linewidth = 4096,
> >> .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> >> .max_hdeci_exp = MAX_HORZ_DECIMATION,
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >> index e5a96d6..b33f91b 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >> @@ -357,6 +357,7 @@ struct dpu_caps {
> >> bool has_dim_layer;
> >> bool has_idle_pc;
> >> bool has_3d_merge;
> >> + bool has_audio_select;
> >> /* SSPP limits */
> >> u32 max_linewidth;
> >> u32 pixel_ram_size;
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> >> index 282e3c6..e608f4d 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> >> @@ -261,14 +261,17 @@ static void dpu_hw_intf_audio_select(struct
> >> dpu_hw_mdp *mdp)
> >> }
> >> static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
> >> - unsigned long cap)
> >> + unsigned long cap,
> >> + const struct dpu_mdss_cfg *m)
> >> {
> >> ops->setup_split_pipe = dpu_hw_setup_split_pipe;
> >> ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl;
> >> ops->get_danger_status = dpu_hw_get_danger_status;
> >> ops->setup_vsync_source = dpu_hw_setup_vsync_source;
> >> ops->get_safe_status = dpu_hw_get_safe_status;
> >> - ops->intf_audio_select = dpu_hw_intf_audio_select;
> >> +
> >> + if (m->caps->has_audio_select)
> >> + ops->intf_audio_select = dpu_hw_intf_audio_select;
> >> }
> >> static const struct dpu_mdp_cfg *_top_offset(enum dpu_mdp mdp,
> >> @@ -320,7 +323,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(enum dpu_mdp
> >> idx,
> >> */
> >> mdp->idx = idx;
> >> mdp->caps = cfg;
> >> - _setup_mdp_ops(&mdp->ops, mdp->caps->features);
> >> + _setup_mdp_ops(&mdp->ops, mdp->caps->features, m);
> >> return mdp;
> >> }
> >
> >
--
With best wishes
Dmitry
