Re: [PATCH] drm/msm: populate intf_audio_select() base on hardware capability

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 14/02/2022 20:55, Kuogee Hsieh wrote:
> 
> On 2/11/2022 3:36 PM, Dmitry Baryshkov wrote:
>> On Sat, 12 Feb 2022 at 02:23, Kuogee Hsieh<quic_khsieh@quicinc.com>  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.
>>>
>>> Signed-off-by: Kuogee Hsieh<quic_khsieh@quicinc.com>
>>> ---
>>>   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;
>> I'd suggest adding a bit to dpu_mdp_cfg's features instead, following
>> the example of other hardware blocks.
> 
> it may cause mis leading if we put this in hardware level since 
> MDP_HDMI_DP_SELECT  bit has not be connected
> 
> to logic for a while even it still present at ipcat. Also this bit but 
> will be  removed at next release.

I'm sorry, I can't quite catch you. Why would you like to remove this 
bit at the next release?

> 
> Is dpu_caps level more proper than in hardware feature level?

A priciple of the least surprise. In other hardware-dependent virtual 
functions selection we use block's caps->features rather than boolean flags.

> 
> 
>>>          /* 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