| Message ID | 20240613-dpu-handle-te-signal-v2-0-67a0116b5366@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dpu: handle non-default TE source pins | expand |
On 2024-06-13 20:05:10, Dmitry Baryshkov wrote: > Make the DPU driver use the TE source specified in the DT. If none is > specified, the driver defaults to the first GPIO (mdp_vsync0). mdp_vsync_p? > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 44 ++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index e9991f3756d4..6fcb3cf4a382 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -505,6 +505,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask) > dpu_kms_wait_for_commit_done(kms, crtc); > } > > +static const char *dpu_vsync_sources[] = { > + [DPU_VSYNC_SOURCE_GPIO_0] = "mdp_vsync_p", > + [DPU_VSYNC_SOURCE_GPIO_1] = "mdp_vsync_s", > + [DPU_VSYNC_SOURCE_GPIO_2] = "mdp_vsync_e", > + [DPU_VSYNC_SOURCE_INTF_0] = "mdp_intf0", > + [DPU_VSYNC_SOURCE_INTF_1] = "mdp_intf1", > + [DPU_VSYNC_SOURCE_INTF_2] = "mdp_intf2", > + [DPU_VSYNC_SOURCE_INTF_3] = "mdp_intf3", > + [DPU_VSYNC_SOURCE_WD_TIMER_0] = "timer0", > + [DPU_VSYNC_SOURCE_WD_TIMER_1] = "timer1", > + [DPU_VSYNC_SOURCE_WD_TIMER_2] = "timer2", > + [DPU_VSYNC_SOURCE_WD_TIMER_3] = "timer3", > + [DPU_VSYNC_SOURCE_WD_TIMER_4] = "timer4", > +}; > + > +static int dpu_kms_dsi_set_te_source(struct msm_display_info *info, > + struct msm_dsi *dsi) > +{ > + const char *te_source = msm_dsi_get_te_source(dsi); Just checking: if the TE source is different and one has dual-DSI, it must be set on both controllers? > + int i; > + > + if (!te_source) { > + info->vsync_source = DPU_VSYNC_SOURCE_GPIO_0; > + return 0; > + } > + > + /* we can not use match_string since dpu_vsync_sources is a sparse array */ Instead of having gaps in the array, you could also store both the vsync_source and name as the array elements? > + for (i = 0; i < ARRAY_SIZE(dpu_vsync_sources); i++) { > + if (dpu_vsync_sources[i] && > + !strcmp(dpu_vsync_sources[i], te_source)) { > + info->vsync_source = i; > + return 0; > + } > + } > + > + return -EINVAL; > +} > + > static int _dpu_kms_initialize_dsi(struct drm_device *dev, > struct msm_drm_private *priv, > struct dpu_kms *dpu_kms) > @@ -543,7 +581,11 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev, > > info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]); > > - info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0; > + rc = dpu_kms_dsi_set_te_source(&info, priv->dsi[i]); > + if (rc) { > + DPU_ERROR("failed to identify TE source for dsi display\n"); > + return rc; > + } > > encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info); > if (IS_ERR(encoder)) { > > -- > 2.39.2 >
On 2024-06-13 20:05:04, Dmitry Baryshkov wrote: > Command mode panels provide TE signal back to the DSI host to signal > that the frame display has completed and update of the image will not > cause tearing. Usually it is connected to the first GPIO with the > mdp_vsync function, which is the default. In such case the property can > be skipped. > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../bindings/display/msm/dsi-controller-main.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml > index 1fa28e976559..e1cb3a1fee81 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml > @@ -162,6 +162,22 @@ properties: > items: > enum: [ 0, 1, 2, 3 ] > > + qcom,te-source: > + $ref: /schemas/types.yaml#/definitions/string > + description: > + Specifies the source of vsync signal from the panel used for > + tearing elimination. > + default: mdp_vsync_p > + enum: > + - mdp_vsync_p > + - mdp_vsync_s > + - mdp_vsync_e When discussing that these should be renamed, was it also documented what the suffix means? I can only guess something like primary/secondary/e...? Are the mdp_intfX variants missing here that you're handling in patch 7/8? > + - timer0 > + - timer1 > + - timer2 > + - timer3 > + - timer4 > + > required: > - port@0 > - port@1 > @@ -452,6 +468,7 @@ examples: > dsi0_out: endpoint { > remote-endpoint = <&sn65dsi86_in>; > data-lanes = <0 1 2 3>; > + qcom,te-source = "mdp_vsync_e"; > }; > }; > }; > > -- > 2.39.2 >
On 2024-06-13 20:05:07, Dmitry Baryshkov wrote: > Setting vsync source makes sense only for DSI CMD panels. Pull the > is_cmd_mode condition out of the function into the calling code, so that > it becomes more explicit. > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 4988a1029431..bd37a56b4d03 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -736,8 +736,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc, > return; > } > > - if (hw_mdptop->ops.setup_vsync_source && > - disp_info->is_cmd_mode) { > + if (hw_mdptop->ops.setup_vsync_source) { > for (i = 0; i < dpu_enc->num_phys_encs; i++) > vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx; > > @@ -1226,7 +1225,8 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc) > dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select( > dpu_enc->cur_master->hw_mdptop); > > - _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info); > + if (dpu_enc->disp_info.is_cmd_mode) > + _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info); > > if (dpu_enc->disp_info.intf_type == INTF_DSI && > !WARN_ON(dpu_enc->num_phys_encs == 0)) { > > -- > 2.39.2 >
On 2024-06-13 20:05:09, Dmitry Baryshkov wrote: > Allow board's device tree to specify the vsync source (aka TE source). > If the property is omitted, the display controller driver will use the > default setting. Well, that specific default handling is not really part of this patch, but how a followup patch is going to respond when msm_dsi_get_te_source() returns NULL. (Or how that followup patch is expected to deal with that - worth a doc-comment?) > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > drivers/gpu/drm/msm/dsi/dsi.h | 1 + > drivers/gpu/drm/msm/dsi/dsi_host.c | 11 +++++++++++ > drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 +++++ > drivers/gpu/drm/msm/msm_drv.h | 6 ++++++ > 4 files changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h > index afc290408ba4..87496db203d6 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi.h > +++ b/drivers/gpu/drm/msm/dsi/dsi.h > @@ -37,6 +37,7 @@ struct msm_dsi { > > struct mipi_dsi_host *host; > struct msm_dsi_phy *phy; > + const char *te_source; > > struct drm_bridge *next_bridge; > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index c4d72562c95a..c26ad0fed54d 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -1786,9 +1786,11 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc > > static int dsi_host_parse_dt(struct msm_dsi_host *msm_host) > { > + struct msm_dsi *msm_dsi = platform_get_drvdata(msm_host->pdev); > struct device *dev = &msm_host->pdev->dev; > struct device_node *np = dev->of_node; > struct device_node *endpoint; > + const char *te_source; > int ret = 0; > > /* > @@ -1811,6 +1813,15 @@ static int dsi_host_parse_dt(struct msm_dsi_host *msm_host) > goto err; > } > > + ret = of_property_read_string(endpoint, "qcom,te-source", &te_source); > + if (ret && ret != -EINVAL) { > + DRM_DEV_ERROR(dev, "%s: invalid TE source configuration %d\n", > + __func__, ret); > + goto err; > + } > + if (!ret) > + msm_dsi->te_source = devm_kstrdup(dev, te_source, GFP_KERNEL); > + > if (of_property_read_bool(np, "syscon-sfpb")) { > msm_host->sfpb = syscon_regmap_lookup_by_phandle(np, > "syscon-sfpb"); > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index 5b3f3068fd92..a210b7c9e5ca 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -603,3 +603,8 @@ bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi) > { > return IS_MASTER_DSI_LINK(msm_dsi->id); > } > + > +const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi) > +{ > + return msm_dsi->te_source; > +} > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index 912ebaa5df84..afd98dffea99 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -330,6 +330,7 @@ bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi); > bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi); > bool msm_dsi_wide_bus_enabled(struct msm_dsi *msm_dsi); > struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi); > +const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi); > #else > static inline void __init msm_dsi_register(void) > { > @@ -367,6 +368,11 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_ > { > return NULL; > } > + > +static inline const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi) > +{ > + return NULL; > +} > #endif > > #ifdef CONFIG_DRM_MSM_DP > > -- > 2.39.2 >
On 6/13/2024 11:14 AM, Marijn Suijten wrote: > On 2024-06-13 20:05:10, Dmitry Baryshkov wrote: >> Make the DPU driver use the TE source specified in the DT. If none is >> specified, the driver defaults to the first GPIO (mdp_vsync0). > > mdp_vsync_p? > >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 44 ++++++++++++++++++++++++++++++++- >> 1 file changed, 43 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> index e9991f3756d4..6fcb3cf4a382 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> @@ -505,6 +505,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask) >> dpu_kms_wait_for_commit_done(kms, crtc); >> } >> >> +static const char *dpu_vsync_sources[] = { >> + [DPU_VSYNC_SOURCE_GPIO_0] = "mdp_vsync_p", >> + [DPU_VSYNC_SOURCE_GPIO_1] = "mdp_vsync_s", >> + [DPU_VSYNC_SOURCE_GPIO_2] = "mdp_vsync_e", >> + [DPU_VSYNC_SOURCE_INTF_0] = "mdp_intf0", >> + [DPU_VSYNC_SOURCE_INTF_1] = "mdp_intf1", >> + [DPU_VSYNC_SOURCE_INTF_2] = "mdp_intf2", >> + [DPU_VSYNC_SOURCE_INTF_3] = "mdp_intf3", >> + [DPU_VSYNC_SOURCE_WD_TIMER_0] = "timer0", >> + [DPU_VSYNC_SOURCE_WD_TIMER_1] = "timer1", >> + [DPU_VSYNC_SOURCE_WD_TIMER_2] = "timer2", >> + [DPU_VSYNC_SOURCE_WD_TIMER_3] = "timer3", >> + [DPU_VSYNC_SOURCE_WD_TIMER_4] = "timer4", >> +}; >> + >> +static int dpu_kms_dsi_set_te_source(struct msm_display_info *info, >> + struct msm_dsi *dsi) >> +{ >> + const char *te_source = msm_dsi_get_te_source(dsi); > > Just checking: if the TE source is different and one has dual-DSI, it must be > set on both controllers? > If we use dual-dsi (in NON-bonded mode), then yes we will have two TE sources - one for each controller. >> + int i; >> + >> + if (!te_source) { >> + info->vsync_source = DPU_VSYNC_SOURCE_GPIO_0; >> + return 0; >> + } >> + >> + /* we can not use match_string since dpu_vsync_sources is a sparse array */ > > Instead of having gaps in the array, you could also store both the vsync_source > and name as the array elements? > Yes, there is a gap because the DPU_VSYNC_* macros have a gap. Can you pls explain your suggestion to remove the gap a little more? I didnt follow this part very well. >> + for (i = 0; i < ARRAY_SIZE(dpu_vsync_sources); i++) { >> + if (dpu_vsync_sources[i] && >> + !strcmp(dpu_vsync_sources[i], te_source)) { >> + info->vsync_source = i; >> + return 0; >> + } >> + } >> + >> + return -EINVAL; >> +} >> + >> static int _dpu_kms_initialize_dsi(struct drm_device *dev, >> struct msm_drm_private *priv, >> struct dpu_kms *dpu_kms) >> @@ -543,7 +581,11 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev, >> >> info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]); >> >> - info.vsync_source = DPU_VSYNC_SOURCE_GPIO_0; >> + rc = dpu_kms_dsi_set_te_source(&info, priv->dsi[i]); >> + if (rc) { >> + DPU_ERROR("failed to identify TE source for dsi display\n"); >> + return rc; >> + } >> >> encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info); >> if (IS_ERR(encoder)) { >> >> -- >> 2.39.2 >>
Command-mode DSI panels need to signal the display controlller when vsync happens, so that the device can start sending the next frame. Some devices (Google Pixel 3) use a non-default pin, so additional configuration is required. Add a way to specify this information in DT and handle it in the DSI and DPU drivers. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Changes in v2: - In DT bindings renamed mdp_gpioN to mdp_vsync_p/_s/_e per pins name (Abhinav) - Extended bindings to include default: mdp_vsync_p (Rob) - Renamed dpu_hw_setup_vsync_source() and dpu_hw_setup_vsync_source_and_vsync_sel() to match the implementation (Abhinav) - Link to v1: https://lore.kernel.org/r/20240520-dpu-handle-te-signal-v1-0-f273b42a089c@linaro.org --- Dmitry Baryshkov (8): dt-bindings: display/msm/dsi: allow specifying TE source drm/msm/dpu: convert vsync source defines to the enum drm/msm/dsi: drop unused GPIOs handling drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source() drm/msm/dpu: rework vsync_source handling drm/msm/dsi: parse vsync source from device tree drm/msm/dpu: support setting the TE source drm/msm/dpu: rename dpu_hw_setup_vsync_source functions .../bindings/display/msm/dsi-controller-main.yaml | 17 ++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 5 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 26 ++++++------ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 14 +++---- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 44 ++++++++++++++++++++ drivers/gpu/drm/msm/dsi/dsi.h | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c | 48 +++++----------------- drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 +++ drivers/gpu/drm/msm/msm_drv.h | 6 +++ 13 files changed, 114 insertions(+), 69 deletions(-) --- base-commit: 03d44168cbd7fc57d5de56a3730427db758fc7f6 change-id: 20240514-dpu-handle-te-signal-82663c0211bd Best regards,