Re: [Freedreno] [PATCH v2] arm64: dts: qcom: sc7280: Add Display Port node
Quoting Kuogee Hsieh (2021-08-30 09:04:49)
> Changes in v2:
> -- break this patch into 3 patches
Are there two more somewhere?
>
> Signed-off-by: Kuogee Hsieh
> ---
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 88
> +++-
> 1 file changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index c29226b..f224029 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3202,6 +3202,13 @@
> remote-endpoint =
> <_in>;
> };
> };
> +
> + port@2 {
> +reg = <2>;
> +dpu_intf0_out: endpoint {
> +remote-endpoint =
> <_in>;
> +};
> +};
> };
>
> mdp_opp_table: mdp-opp-table {
> @@ -3389,6 +3396,78 @@
> };
> };
> };
> +
> + msm_dp: displayport-controller@ae9 {
> + status = "disabled";
> + compatible = "qcom,sc7180-dp",
> "qcom,sc7280-dp";
It should be most specific to least specific from left to right. I'd
rather see "qcom,sc7180-dp" dropped entirely as it will become important
to know that sc7280 has eDP and DP whereas sc7180 only has DP. We should
key that knowledge off the compatible string, so having sc7180-dp here
makes that harder, not easier.
> +
> + reg = <0 0x0ae9 0 0x1400>;
> +
> + interrupt-parent = <>;
> + interrupts = <12>;
> +
> + clocks = < DISP_CC_MDSS_AHB_CLK>,
> +< DISP_CC_MDSS_DP_AUX_CLK>,
> +< DISP_CC_MDSS_DP_LINK_CLK>,
> +<
> DISP_CC_MDSS_DP_LINK_INTF_CLK>,
> +< DISP_CC_MDSS_DP_PIXEL_CLK>;
> + clock-names = "core_iface",
> + "core_aux",
Re: [Freedreno] [PATCH v3] drm/dp_mst: Fix return code on sideband message failure
Quoting Lyude Paul (2021-08-30 09:58:01) > On Mon, 2021-08-30 at 08:56 -0700, khs...@codeaurora.org wrote: > > On 2021-08-25 09:26, Lyude Paul wrote: > > > The patch was pushed yes (was part of drm-misc-next-2021-07-29), seems > > > like it > > > just hasn't trickled down to linus's branch quite yet. > > > > Hi Stephen B, > > > > Would you mind back porting this patch to V5.10 branch? > > It will have lots of helps for us to support display port MST case. > > Thanks, If the patch is tagged for stable then it will automatically be backported to the 5.10 stable release, or at least attempted. If you don't see it in the stable tree but it's in Linus' tree then you can submit the patch directly to stable. See Documentation/process/stable-kernel-rules.rst for more info. > > I'm assuming you're talking to someone else? A little confused because I don't > see a Stephen B in this thread I assume it is me.
Re: [Freedreno] [PATCH v2 1/2] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent
Quoting Marijn Suijten (2021-08-30 15:45:42)
> Hi Stephen,
>
> On 2021-08-30 15:16:13, Stephen Boyd wrote:
> > Quoting Marijn Suijten (2021-08-30 11:24:44)
> > > All DSI PHY/PLL drivers were referencing their VCO parent clock by a
> > > global name, most of which don't exist or have been renamed. These
> > > clock drivers seem to function fine without that except the 14nm driver
> > > for the sdm6xx [1].
> > >
> > > At the same time all DTs provide a "ref" clock as per the requirements
> > > of dsi-phy-common.yaml, but the clock is never used. This patchset puts
> > > that clock to use without relying on a global clock name, so that all
> > > dependencies are explicitly defined in DT (the firmware) in the end.
> > >
> > > Note that msm8974 is the only board not providing this clock, and
> > > apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
> > > pxo). Both have been been addressed in separate patches that are
> > > supposed to land well in advance of this patchset.
> > >
> > > Furthermore not all board-DTs provided this clock initially but that
> > > deficiency has been addressed in followup patches (see the Fixes:
> > > below). Those commits seem to assume that the clock was used, while
> > > nothing in history indicates that this "ref" clock was ever retrieved.
> > >
> > > [1]:
> > > https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1b...@somainline.org/
> > >
> > > Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref
> > > clock of the DSI PHY")
> > > Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref
> > > clock of the DSI PHY")
> > > Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of
> > > the DSI PHYs")
> > > Signed-off-by: Marijn Suijten
> > > ---
> > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 4 +++-
> > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 4 +++-
> > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 4 +++-
> > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 +++-
> > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 4 +++-
> > > 5 files changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > index e46b10fc793a..3cbb1f1475e8 100644
> > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> > > @@ -562,7 +562,9 @@ static int pll_10nm_register(struct dsi_pll_10nm
> > > *pll_10nm, struct clk_hw **prov
> > > char clk_name[32], parent[32], vco_name[32];
> > > char parent2[32], parent3[32], parent4[32];
> > > struct clk_init_data vco_init = {
> > > - .parent_names = (const char *[]){ "xo" },
> > > + .parent_data = &(const struct clk_parent_data) {
> > > + .fw_name = "ref",
> >
> > Please also add .name as the old parent_names value so that newer
> > kernels can be used without having to use new DT.
>
> We discussed that only msm8974 misses this "ref" clock at the time of
> writing. Aforementioned Fixes: patches have all been merged about 3
> years ago, are those DTs still in use with a newer kernel? I suppose
> this patch is only backported to kernels including those DT patches, is
> it reasonable to assume that at least that DT is in use there?
I have no idea.
>
> Besides, not all clock trees provide this global "xo" or "bi_tcxo" clock
> in the first place.
>
It doesn't hurt to also specify a .name to help migrate anything else
over. Unless you're confident it won't cause problems to rely on proper
DT being used?
Re: [Freedreno] [PATCH v2 1/2] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent
Quoting Marijn Suijten (2021-08-30 11:24:44)
> All DSI PHY/PLL drivers were referencing their VCO parent clock by a
> global name, most of which don't exist or have been renamed. These
> clock drivers seem to function fine without that except the 14nm driver
> for the sdm6xx [1].
>
> At the same time all DTs provide a "ref" clock as per the requirements
> of dsi-phy-common.yaml, but the clock is never used. This patchset puts
> that clock to use without relying on a global clock name, so that all
> dependencies are explicitly defined in DT (the firmware) in the end.
>
> Note that msm8974 is the only board not providing this clock, and
> apq8064 was providing the wrong clock (19.2MHz cxo instead of 27MHz
> pxo). Both have been been addressed in separate patches that are
> supposed to land well in advance of this patchset.
>
> Furthermore not all board-DTs provided this clock initially but that
> deficiency has been addressed in followup patches (see the Fixes:
> below). Those commits seem to assume that the clock was used, while
> nothing in history indicates that this "ref" clock was ever retrieved.
>
> [1]:
> https://lore.kernel.org/linux-arm-msm/386db1a6-a1cd-3c7d-a88e-dc83f8a1b...@somainline.org/
>
> Fixes: 79e51645a1dd ("arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock
> of the DSI PHY")
> Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of
> the DSI PHY")
> Fixes: 0c0e72705a33 ("arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the
> DSI PHYs")
> Signed-off-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 4 +++-
> drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 4 +++-
> drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 4 +++-
> drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 +++-
> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 4 +++-
> 5 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> index e46b10fc793a..3cbb1f1475e8 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> @@ -562,7 +562,9 @@ static int pll_10nm_register(struct dsi_pll_10nm
> *pll_10nm, struct clk_hw **prov
> char clk_name[32], parent[32], vco_name[32];
> char parent2[32], parent3[32], parent4[32];
> struct clk_init_data vco_init = {
> - .parent_names = (const char *[]){ "xo" },
> + .parent_data = &(const struct clk_parent_data) {
> + .fw_name = "ref",
Please also add .name as the old parent_names value so that newer
kernels can be used without having to use new DT.
> + },
> .num_parents = 1,
> .name = vco_name,
> .flags = CLK_IGNORE_UNUSED,
Re: [Freedreno] [PATCH] arm64: dts: qcom: sc7280: enable IDP display port
Quoting Kuogee Hsieh (2021-08-27 10:05:34) > Signed-off-by: Kuogee Hsieh > --- Reviewed-by: Stephen Boyd But this must depend on the patch that introduces the phandle to begin with.
Re: [Freedreno] [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-08-25 16:42:31)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 2c7de43f655a..4a6132c18e57 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -78,6 +78,8 @@ struct dp_display_private {
> char *name;
> int irq;
>
> + int id;
> +
> /* state variables */
> bool core_initialized;
> bool hpd_irq_on;
> @@ -115,8 +117,19 @@ struct dp_display_private {
> struct dp_audio *audio;
> };
>
> +
> +struct msm_dp_config {
> + phys_addr_t io_start[3];
Can this be made into another struct, like msm_dp_desc, that also
indicates what type of DP connector it is, i.e. eDP vs DP? That would
help me understand in modetest and /sys/class/drm what sort of connector
is probing. dp_drm_connector_init() would need to pass the type of
connector appropriately. Right now, eDP connectors still show up as DP
instead of eDP in sysfs.
> + size_t num_dp;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> + .io_start = { 0x0ae9 },
> + .num_dp = 1,
> +};
> +
> static const struct of_device_id dp_dt_match[] = {
> - {.compatible = "qcom,sc7180-dp"},
> + { .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
> {}
> };
>
Re: [Freedreno] [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-08-26 09:57:18) > On Thu 26 Aug 00:13 PDT 2021, Stephen Boyd wrote: > > Quoting Bjorn Andersson (2021-08-25 16:42:31) > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > [..] > > > @@ -203,8 +204,8 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, > > > struct drm_minor *minor) > > > dpu_debugfs_vbif_init(dpu_kms, entry); > > > dpu_debugfs_core_irq_init(dpu_kms, entry); > > > > > > - if (priv->dp) > > > - msm_dp_debugfs_init(priv->dp, minor); > > > + for (i = 0; i < ARRAY_SIZE(priv->dp); i++) > > > + msm_dp_debugfs_init(priv->dp[i], minor); > > > > Does this need the same if (!priv->dp) continue check like the other > > loops over priv->dp? > > > [..] > > > @@ -800,7 +809,8 @@ static int dpu_irq_postinstall(struct msm_kms *kms) > > > if (!priv) > > > return -EINVAL; > > > > > > - msm_dp_irq_postinstall(priv->dp); > > > + for (i = 0; i < ARRAY_SIZE(priv->dp); i++) > > > + msm_dp_irq_postinstall(priv->dp[i]); > > > > This one too? Or maybe those gained NULL pointer checks. > > > > This already has a NULL check, that's why I added one to the adjacent > msm_dp_debugfs_init() as well. Ok. > > > > > > > return 0; > > > } > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > > > b/drivers/gpu/drm/msm/dp/dp_display.c > [..] > > > @@ -1194,6 +1230,10 @@ static int dp_display_probe(struct platform_device > > > *pdev) > > > if (!dp) > > > return -ENOMEM; > > > > > > + dp->id = dp_display_get_id(pdev); > > > > Ah ok, it's signed for this error check. Maybe assign dp->id in the > > function and return 0 instead of assigning it here? > > dp_display_assign_id() > > > > I like the fact that the "getter" doesn't have side effects, but making > dp->id unsigned makes sense. So let's pay the price of a local signed > variable here. > Sure. If that's the only change then feel free to add Reviewed-by: Stephen Boyd on the next version.
Re: [Freedreno] [PATCH v2 5/5] drm/msm/dp: Add sc8180x DP controllers
Quoting Bjorn Andersson (2021-08-25 16:42:33) > The sc8180x has 2 DP and 1 eDP controllers, add support for these to the > DP driver. > > Signed-off-by: Bjorn Andersson > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-08-25 16:42:31)
> Based on the removal of the g_dp_display and the movement of the
> priv->dp lookup into the DP code it's now possible to have multiple
> DP instances.
>
> In line with the other controllers in the MSM driver, introduce a
> per-compatible list of base addresses which is used to resolve the
> "instance id" for the given DP controller. This instance id is used as
> index in the priv->dp[] array.
>
> Then extend the initialization code to initialize struct drm_encoder for
> each of the registered priv->dp[] and update the logic for associating
> each struct msm_dp with the struct dpu_encoder_virt.
>
> Lastly, bump the number of struct msm_dp instances carries by priv->dp
> to 3, the currently known maximum number of controllers found in a
> Qualcomm SoC.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v1:
> - Update dpu_encoder_setup() to store the reference to the msm_dp in our
> dpu_enc
>
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 60 +++
> .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 ++-
> drivers/gpu/drm/msm/dp/dp_display.c | 51 ++--
> drivers/gpu/drm/msm/msm_drv.h | 2 +-
> 5 files changed, 90 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index b7f33da2799c..9cd9539a1504 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2173,7 +2173,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct
> drm_encoder *enc,
> dpu_encoder_vsync_event_handler,
> 0);
> else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
> - dpu_enc->dp = priv->dp;
> + dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>
> INIT_DELAYED_WORK(_enc->delayed_off_work,
> dpu_encoder_off_work);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index f655adbc2421..a793cc8a007e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -188,6 +188,7 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms,
> struct drm_minor *minor)
> struct dentry *entry;
> struct drm_device *dev;
> struct msm_drm_private *priv;
> + int i;
>
> if (!p)
> return -EINVAL;
> @@ -203,8 +204,8 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms,
> struct drm_minor *minor)
> dpu_debugfs_vbif_init(dpu_kms, entry);
> dpu_debugfs_core_irq_init(dpu_kms, entry);
>
> - if (priv->dp)
> - msm_dp_debugfs_init(priv->dp, minor);
> + for (i = 0; i < ARRAY_SIZE(priv->dp); i++)
> + msm_dp_debugfs_init(priv->dp[i], minor);
Does this need the same if (!priv->dp) continue check like the other
loops over priv->dp?
>
> return dpu_core_perf_debugfs_init(dpu_kms, entry);
> }
> @@ -545,33 +546,40 @@ static int _dpu_kms_initialize_displayport(struct
> drm_device *dev,
> struct drm_encoder *encoder = NULL;
> struct msm_display_info info;
> int rc = 0;
> + int i;
>
> - if (!priv->dp)
> - return rc;
> + for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
> + if (!priv->dp[i])
> + continue;
Like this one.
>
> - encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
> - if (IS_ERR(encoder)) {
> - DPU_ERROR("encoder init failed for dsi display\n");
> - return PTR_ERR(encoder);
> - }
> + encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
> + if (IS_ERR(encoder)) {
> + DPU_ERROR("encoder init failed for dsi display\n");
> + return PTR_ERR(encoder);
> + }
>
> - memset(, 0, sizeof(info));
> - rc = msm_dp_modeset_init(priv->dp, dev, encoder);
> - if (rc) {
> - DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
> - drm_encoder_cleanup(encoder);
> - return rc;
> - }
> + memset(, 0, sizeof(info));
> + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
> + if (rc) {
> + DPU_ERROR("modeset_init failed for DP, rc = %d\n",
> rc);
> + drm_encoder_cleanup(encoder);
> + return rc;
> + }
>
> - priv->encoders[priv->num_encoders++] = encoder;
> + priv->encoders[priv->num_encoders++] = encoder;
> +
> + info.num_of_h_tiles = 1;
> + info.h_tile_instance[0] = i;
> + info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
> +
Re: [Freedreno] [PATCH v2 2/5] drm/msm/dp: Modify prototype of encoder based API
Quoting Bjorn Andersson (2021-08-25 16:42:30) > Functions in the DisplayPort code that relates to individual instances > (encoders) are passed both the struct msm_dp and the struct drm_encoder. But > in a situation where multiple DP instances would exist this means that > the caller need to resolve which struct msm_dp relates to the struct > drm_encoder at hand. > > Store a reference to the struct msm_dp associated with each > dpu_encoder_virt to allow the particular instance to be associate with > the encoder in the following patch. > > Signed-off-by: Bjorn Andersson > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v2 1/5] drm/msm/dp: Remove global g_dp_display variable
Quoting Bjorn Andersson (2021-08-25 16:42:29) > As the Qualcomm DisplayPort driver only supports a single instance of > the driver the commonly used struct dp_display is kept in a global > variable. As we introduce additional instances this obviously doesn't > work. > > Replace this with a combination of existing references to adjacent > objects and drvdata. > > Signed-off-by: Bjorn Andersson > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
Quoting Bjorn Andersson (2021-07-26 16:13:51)
> eDP panels might need some power sequencing and backlight management,
> so make it possible to associate a drm_panel with a DP instance and
> prepare and enable the panel accordingly.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> This solves my immediate problem on my 8cx laptops, of indirectly controlling
> the backlight during DPMS. But my panel is powered when I boot it and as such
> I
> get the hpd interrupt and I don't actually have to deal with a power on
> sequence - so I'm posting this as an RFC, hoping to get some input on these
> other aspects.
>
> If this is acceptable I'd be happy to write up an accompanying DT binding
> change that marks port 2 of the DP controller's of_graph as a reference to the
> attached panel.
dianders@ mentioned creating a connector (and maybe a bridge) for the DP
connector (not eDP)[1]. I'm not sure that's directly related, but I
think with the aux bus code the panel isn't managed in the encoder
driver. Instead the encoder sees a bridge and tries to power it up and
then query things over the aux bus? It's all a little too fuzzy to me
right now so I could be spewing nonsense but I think we want to take
this bridge route if possible.
-Stephen
[1]
https://lore.kernel.org/r/CAD=FV=xd9fizydxfxyokpj_1fzchp3-roj7k4ipg0g0rq_+...@mail.gmail.com/
>
> drivers/gpu/drm/msm/dp/dp_display.c | 15 +--
> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> drivers/gpu/drm/msm/dp/dp_parser.c | 19 +++
> drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
> 4 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 206bf7806f51..1db5a3f752d2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
>
> #include "msm_drv.h"
> #include "msm_kms.h"
> @@ -252,6 +253,8 @@ static int dp_display_bind(struct device *dev, struct
> device *master,
> goto end;
> }
>
> + dp->dp_display.drm_panel = dp->parser->drm_panel;
> +
> rc = dp_aux_register(dp->aux, drm);
> if (rc) {
> DRM_ERROR("DRM DP AUX register failed\n");
> @@ -867,8 +870,10 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
> return 0;
> }
>
> -static int dp_display_prepare(struct msm_dp *dp)
> +static int dp_display_prepare(struct msm_dp *dp_display)
> {
> + drm_panel_prepare(dp_display->drm_panel);
> +
> return 0;
> }
>
> @@ -886,6 +891,8 @@ static int dp_display_enable(struct dp_display_private
> *dp, u32 data)
> if (!rc)
> dp_display->power_on = true;
>
> + drm_panel_enable(dp_display->drm_panel);
> +
> return rc;
> }
>
> @@ -915,6 +922,8 @@ static int dp_display_disable(struct dp_display_private
> *dp, u32 data)
> if (!dp_display->power_on)
> return 0;
>
> + drm_panel_disable(dp_display->drm_panel);
> +
> /* wait only if audio was enabled */
> if (dp_display->audio_enabled) {
> /* signal the disconnect event */
> @@ -939,8 +948,10 @@ static int dp_display_disable(struct dp_display_private
> *dp, u32 data)
> return 0;
> }
>
> -static int dp_display_unprepare(struct msm_dp *dp)
> +static int dp_display_unprepare(struct msm_dp *dp_display)
> {
> + drm_panel_unprepare(dp_display->drm_panel);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> b/drivers/gpu/drm/msm/dp/dp_display.h
> index 8b47cdabb67e..ce337824c95d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -15,6 +15,7 @@ struct msm_dp {
> struct device *codec_dev;
> struct drm_connector *connector;
> struct drm_encoder *encoder;
> + struct drm_panel *drm_panel;
> bool is_connected;
> bool audio_enabled;
> bool power_on;
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index fc8a6452f641..e6a6e9007bfd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -6,6 +6,7 @@
> #include
> #include
>
> +#include
> #include
>
> #include "dp_parser.h"
> @@ -276,6 +277,20 @@ static int dp_parser_clock(struct dp_parser *parser)
> return 0;
> }
>
> +static int dp_parser_find_panel(struct dp_parser *parser)
> +{
> + struct device_node *np = parser->pdev->dev.of_node;
> + int rc;
> +
> + rc = drm_of_find_panel_or_bridge(np, 2, 0, >drm_panel, NULL);
> + if (rc == -ENODEV)
> + rc = 0;
> + else if (rc)
> + DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> +
> + return rc;
> +}
> +
> static int dp_parser_parse(struct dp_parser *parser)
> {
> int rc = 0;
> @@ -297,6 +312,10 @@ static int
Re: [Freedreno] [PATCH v2 4/5] drm/msm/dp: Store each subblock in the io region
Quoting Bjorn Andersson (2021-08-25 15:25:56)
> Not all platforms has DP_P0 at offset 0x1000 from the beginning of the
> DP block. So split the dss_io_data memory region into a set of
> sub-regions, to make it possible in the next patch to specify each of
> the sub-regions individually.
>
> Signed-off-by: Bjorn Andersson
> ---
One nit below:
Reviewed-by: Stephen Boyd
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
> b/drivers/gpu/drm/msm/dp/dp_parser.h
> index dc62e70b1640..a95b05dbb11c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -25,9 +25,16 @@ enum dp_pm_type {
> DP_MAX_PM
> };
>
> -struct dss_io_data {
> - size_t len;
> +struct dss_io_region {
> void __iomem *base;
> + size_t len;
It flip flops here. Would be nice to the diff if len was where it really
wanted to be.
> +};
> +
> +struct dss_io_data {
> + struct dss_io_region ahb;
> + struct dss_io_region aux;
> + struct dss_io_region link;
> + struct dss_io_region p0;
> };
Re: [Freedreno] [PATCH v2 3/5] drm/msm/dp: Refactor ioremap wrapper
Quoting Bjorn Andersson (2021-08-25 15:25:55)
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index c064ced78278..215065336268 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -19,40 +19,30 @@ static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
> },
> };
>
> -static int msm_dss_ioremap(struct platform_device *pdev,
> - struct dss_io_data *io_data)
> +static void __iomem *dp_ioremap(struct platform_device *pdev, int idx,
> size_t *len)
> {
> - struct resource *res = NULL;
> + struct resource *res;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, idx);
> if (!res) {
> DRM_ERROR("%pS->%s: msm_dss_get_res failed\n",
> __builtin_return_address(0), __func__);
This error can be removed too, right? At least I thought passing in NULL
as 'res' to devm_ioremap_resource() did that. It actually looks like we
can use devm_platform_get_and_ioremap_resource() and then pass in
base = devm_platform_get_and_ioremap_resource(pdev, 0, );
if (!IS_ERR(base))
*len = resource_size(res);
return base;
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
> }
>
> - io_data->len = (u32)resource_size(res);
> - io_data->base = devm_ioremap(>dev, res->start, io_data->len);
> - if (!io_data->base) {
> - DRM_ERROR("%pS->%s: ioremap failed\n",
> - __builtin_return_address(0), __func__);
> - return -EIO;
> - }
> -
> - return 0;
> + *len = resource_size(res);
> + return devm_ioremap_resource(>dev, res);
> }
>
> static int dp_parser_ctrl_res(struct dp_parser *parser)
> {
> - int rc = 0;
> struct platform_device *pdev = parser->pdev;
> struct dp_io *io = >io;
> + struct dss_io_data *dss = >dp_controller;
>
> - rc = msm_dss_ioremap(pdev, >dp_controller);
> - if (rc) {
> - DRM_ERROR("unable to remap dp io resources, rc=%d\n", rc);
> - return rc;
> - }
> + dss->base = dp_ioremap(pdev, 0, >len);
> + if (IS_ERR(dss->base))
> + return PTR_ERR(dss->base);
>
> io->phy = devm_phy_get(>dev, "dp");
> if (IS_ERR(io->phy))
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
> b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 34b49628bbaf..dc62e70b1640 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -26,7 +26,7 @@ enum dp_pm_type {
> };
>
> struct dss_io_data {
> - u32 len;
> + size_t len;
len is a resource_size_t above, not sure if that really matters in
practice to indicate it here though.
> void __iomem *base;
> };
>
Re: [Freedreno] [PATCH v1 2/2] dt-bindings: Add SC7280 compatible string
Quoting Sankeerth Billakanti (2021-08-11 17:08:02) > The Qualcomm SC7280 platform supports an eDP controller, add > compatible string for it to msm/binding. > > Signed-off-by: Sankeerth Billakanti > --- > Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > index 64d8d9e..23b78ac 100644 > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > @@ -17,6 +17,9 @@ properties: >compatible: > enum: >- qcom,sc7180-dp > + - qcom,sc8180x-dp > + - qcom,sc8180x-edp > + - qcom,sc7280-edp Also add qcom,sc7280-dp here?
Re: [Freedreno] [PATCH] phy: qcom-qmp: add support for voltage and pre emphesis swing
Quoting Kuogee Hsieh (2021-08-24 16:29:35)
> Add voltage and pre emphesis swing tables so that voltage and
Is it "pre-emphasis"?
> pre emphsis swing level can be configured base on link rate.
This one is also different.
>
> Signed-off-by: Kuogee Hsieh
Presumably
Fixes: aff188feb5e1 ("phy: qcom-qmp: add support for sm8250-usb3-dp phy")
> ---
> drivers/phy/qualcomm/phy-qcom-qmp.c | 95
> -
> 1 file changed, 82 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 31036aa..52bab6e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -1916,7 +1916,7 @@ static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[]
> = {
> QMP_PHY_INIT_CFG(QSERDES_V4_TX_RES_CODE_LANE_OFFSET_RX, 0x11),
> QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_BAND, 0x4),
> QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_POL_INV, 0x0a),
> - QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x2a),
> + QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x22),
Is 0x22 the better "default"? Can that be described in the commit text?
> QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_EMP_POST1_LVL, 0x20),
> };
>
> @@ -3727,6 +3727,81 @@ static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy
> *qphy)
>
> return 0;
> }
Nitpick: Newline here please.
> +/*
> + * 0x20 deducted from tables
> + *
> + * swing_value |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
> + * pre_emphasis_value |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
> +*/
> +static const u8 qmp_dp_v4_pre_emphasis_hbr3_hbr2[4][4] = {
> + /* p0p1p2p3 */
> + { 0x00, 0x0c, 0x15, 0x1b }, /* s0 */
> + { 0x02, 0x0e, 0x16, 0xff }, /* s1 */
> + { 0x02, 0x11, 0xff, 0xff }, /* s2 */
> + { 0x04, 0xff, 0xff, 0xff } /* s3 */
> +};
> +
> +static const u8 qmp_dp_v4_voltage_swing_hbr3_hbr2[4][4] = {
This looks the same as qmp_dp_v3_voltage_swing_hbr3_hbr2. Can that be
used?
> + /* p0p1p2p3 */
> + { 0x02, 0x12, 0x16, 0x1a }, /* s0 */
> + { 0x09, 0x19, 0x1f, 0xff }, /* s1 */
> + { 0x10, 0x1f, 0xff, 0xff }, /* s2 */
> + { 0x1f, 0xff, 0xff, 0xff } /* s3 */
> +};
> +
> +static const u8 qmp_dp_v4_pre_emphasis_hbr_rbr[4][4] = {
> + /* p0p1p2p3 */
> + { 0x00, 0x0e, 0x15, 0x1b }, /* s0 */
> + { 0x00, 0x0e, 0x15, 0xff }, /* s1 */
> + { 0x00, 0x0e, 0xff, 0xff }, /* s2 */
> + { 0x04, 0xff, 0xff, 0xff } /* s3 */
> +};
> +
> +static const u8 qmp_dp_v4_voltage_swing_hbr_rbr[4][4] = {
> + /* p0p1p2p3 */
> + { 0x08, 0x0f, 0x16, 0x1f }, /* s0 */
> + { 0x11, 0x1e, 0x1f, 0xff }, /* s1 */
> + { 0x16, 0x1f, 0xff, 0xff }, /* s2 */
> + { 0x1f, 0xff, 0xff, 0xff } /* s3 */
Do these comments add any value? Can we drop them?
> +};
> +
> +static int qcom_qmp_v4_phy_configure_dp_swing(struct qmp_phy *qphy,
> + unsigned int drv_lvl_reg, unsigned int emp_post_reg)
> +{
> + const struct phy_configure_opts_dp *dp_opts = >dp_opts;
> + unsigned int v_level = 0, p_level = 0;
> + u8 voltage_swing_cfg, pre_emphasis_cfg;
> + int i;
> +
> + for (i = 0; i < dp_opts->lanes; i++) {
> + v_level = max(v_level, dp_opts->voltage[i]);
> + p_level = max(p_level, dp_opts->pre[i]);
> + }
> +
> +
Nitpick: Drop extra newline.
> + if (dp_opts->link_rate <= 2700) {
> + voltage_swing_cfg =
> qmp_dp_v4_voltage_swing_hbr_rbr[v_level][p_level];
> + pre_emphasis_cfg =
> qmp_dp_v4_pre_emphasis_hbr_rbr[v_level][p_level];
> + } else {
> + voltage_swing_cfg =
> qmp_dp_v4_voltage_swing_hbr3_hbr2[v_level][p_level];
> + pre_emphasis_cfg =
> qmp_dp_v4_pre_emphasis_hbr3_hbr2[v_level][p_level];
> + }
> +
> + /* TODO: Move check to config check */
> + if (voltage_swing_cfg == 0xFF && pre_emphasis_cfg == 0xFF)
> + return -EINVAL;
> +
> + /* Enable MUX to use Cursor values from these registers */
> + voltage_swing_cfg |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
> + pre_emphasis_cfg |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
> +
> + writel(voltage_swing_cfg, qphy->tx + drv_lvl_reg);
> + writel(pre_emphasis_cfg, qphy->tx + emp_post_reg);
> + writel(voltage_swing_cfg, qphy->tx2 + drv_lvl_reg);
> + writel(pre_emphasis_cfg, qphy->tx2 + emp_post_reg);
This is copy/pasted from qcom_qmp_phy_configure_dp_swing() right? How
about making a function
static int
__qcom_qmp_phy_configure_dp_swing(struct qmp_phy *qphy,
unsigned int drv_lvl_reg,
unsigned int emp_post_reg,
const u8 **voltage_rbr_hbr,
const u8 **pre_emphasis_rbr_hbr,
const
Re: [Freedreno] [PATCH] arm64: dts: qcom: sc7280: Add Display Port node
Quoting Kuogee Hsieh (2021-08-24 16:20:26)
> Add display port supported node for sc7280. Also correct dp-phy node
> tx/rx/pcs/tx2/rx2 base reg address to fix aux channel read/write
> failure issue.
>
> Signed-off-by: Kuogee Hsieh
> ---
> arch/arm64/boot/dts/qcom/sc7280-idp2.dts | 9 +++
Please split the idp diff from the sc7280.dts diff so that there are two
patches instead of one. It helps with ignoring the idp diff.
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 98
> +---
> 2 files changed, 100 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
> b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
> index b1cf70e..4aea369 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
> @@ -202,3 +202,12 @@ ap_h1_spi: {};
> backlight = <>;
> };
> };
> +
> +_dp {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <_hot_plug_det>;
> + data-lanes = <0 1>;
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l1b_0p8>;
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index c29226b..a350d84 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2918,15 +2918,11 @@
> dp_phy: dp-phy@88ea200 {
> reg = <0 0x088ea200 0 0x200>,
> <0 0x088ea400 0 0x200>,
> - <0 0x088eac00 0 0x400>,
> + <0 0x088eaa00 0 0x200>,
> <0 0x088ea600 0 0x200>,
> - <0 0x088ea800 0 0x200>,
> - <0 0x088eaa00 0 0x100>;
> + <0 0x088ea800 0 0x200>;
So this was wrong? Best to split that out into another patch with the
appropriate Fixes tag.
> #phy-cells = <0>;
> #clock-cells = <1>;
> - clocks = < GCC_USB3_PRIM_PHY_PIPE_CLK>;
> - clock-names = "pipe0";
> - clock-output-names = "usb3_phy_pipe_clk_src";
And then mention this part in the commit text of the fixing patch.
> };
> };
>
> @@ -3389,6 +3392,74 @@
> };
> };
> };
> +
> + msm_dp: displayport-controller@ae9 {
> + status = "disabled";
> + compatible = "qcom,sc7180-dp";
Can we add qcom,sc7280-dp as well? Just in case anything is wrong with
sc7280 specifically.
> +
> + reg = <0 0x0ae9 0 0x1400>;
> +
> + interrupt-parent = <>;
> + interrupts = <12 IRQ_TYPE_NONE>;
Drop IRQ_TYPE_NONE per the binding it is one cell, not two.
> +
> + clocks = < DISP_CC_MDSS_AHB_CLK>,
> +< DISP_CC_MDSS_DP_AUX_CLK>,
> +< DISP_CC_MDSS_DP_LINK_CLK>,
> +<
> DISP_CC_MDSS_DP_LINK_INTF_CLK>,
> +< DISP_CC_MDSS_DP_PIXEL_CLK>;
> + clock-names = "core_iface", "core_aux",
> "ctrl_link",
> + "ctrl_link_iface",
> "stream_pixel";
Can we get clock-names on one line matching the clocks property please?
That makes it easier to match it up between the two properties.
> + #clock-cells = <1>;
> + assigned-clocks = <
> DISP_CC_MDSS_DP_LINK_CLK_SRC>,
> + <
> DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> + assigned-clock-parents = <_phy 0>,
> <_phy 1>;
> + phys = <_phy>;
> + phy-names = "dp";
> +
> + operating-points-v2 = <_opp_table>;
> + power-domains = < SC7180_CX>;
> +
> + #sound-dai-cells = <0>;
Nitpick: Newline here.
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + dp_in: endpoint {
> + remote-endpoint =
> <_intf0_out>;
> + };
> +
Re: [Freedreno] [PATCH v1 1/4] dt-bindings: msm: add DT bindings for sc7280
Quoting Krishna Manikandan (2021-08-18 03:27:01) > MSM Mobile Display Subsystem (MDSS) encapsulates sub-blocks > like DPU display controller, DSI, EDP etc. Add required DPU > device tree bindings for SC7280. > > Signed-off-by: Krishna Manikandan > --- Please send a cover-letter next time. Do you have the display port dts bits and driver code ready too? Can it be part of this patch series?
Re: [Freedreno] [PATCH v1 4/4] arm64: dts: qcom: sc7280: add edp display dt nodes
Quoting Krishna Manikandan (2021-08-18 03:27:04)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index aadf55d..5be318e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1412,7 +1412,7 @@
> reg = <0 0xaf0 0 0x2>;
> clocks = < RPMH_CXO_CLK>,
> < GCC_DISP_GPLL0_CLK_SRC>,
> -<0>, <0>, <0>, <0>, <0>, <0>;
> +<0>, <0>, <0>, <0>, <_phy 0>, <_phy
> 1>;
> clock-names = "bi_tcxo", "gcc_disp_gpll0_clk",
> "dsi0_phy_pll_out_byteclk",
> "dsi0_phy_pll_out_dsiclk",
> @@ -1493,6 +1493,12 @@
> remote-endpoint =
> <_in>;
> };
> };
Newline here please.
> + port@1 {
> + reg = <1>;
> + dpu_intf5_out: endpoint {
> + remote-endpoint =
> <_in>;
> + };
> + };
> };
>
> mdp_opp_table: mdp-opp-table {
> @@ -1608,6 +1614,101 @@
>
> status = "disabled";
> };
> +
> + msm_edp: edp@aea {
> + status = "disabled";
Please pick a place to put status disabled. I don't know what qcom
maintainers want, but please be consistent.
> + compatible = "qcom,sc7280-edp";
> + reg = <0 0xaea 0 0x200>,
> + <0 0xaea0200 0 0x200>,
> + <0 0xaea0400 0 0xc00>,
> + <0 0xaea1000 0 0x400>;
> +
> + interrupt-parent = <>;
> + interrupts = <14 IRQ_TYPE_NONE>;
Drop flags.
> +
> + clocks = < RPMH_CXO_CLK>,
> +< GCC_EDP_CLKREF_EN>,
> +< DISP_CC_MDSS_AHB_CLK>,
> +< DISP_CC_MDSS_EDP_AUX_CLK>,
> +< DISP_CC_MDSS_EDP_LINK_CLK>,
> +<
> DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
> +< DISP_CC_MDSS_EDP_PIXEL_CLK>;
> + clock-names = "core_xo", "core_ref",
> + "core_iface", "core_aux",
> "ctrl_link",
> + "ctrl_link_iface",
> "stream_pixel";
One line per string please.
> + #clock-cells = <1>;
> + assigned-clocks = <
> DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
> + <
> DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
> + assigned-clock-parents = <_phy 0>,
> <_phy 1>;
> +
> + phys = <_phy>;
> + phy-names = "dp";
> +
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l10c_0p8>;
Can this be done here? Probably needs to move to the board dts/dtsi
file.
> + operating-points-v2 = <_opp_table>;
> + power-domains = < SC7280_CX>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_hot_plug_det>,
> <_panel_power_on>;
> +
> + panel-bklt-gpio = <_gpios 7
> GPIO_ACTIVE_HIGH>;
> + panel-pwm-gpio = <_gpios 8
> GPIO_ACTIVE_HIGH>;
Please no panel-bklt-gpio and panel-pwm-gpio properties.
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + edp_in: endpoint {
> + remote-endpoint =
> <_intf5_out>;
> + };
> + };
> + };
> +
> + edp_opp_table: edp-opp-table {
edp_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> +
Re: [Freedreno] [PATCH v1 3/4] arm64: dts: qcom: sc7280: Add DSI display nodes
Quoting Krishna Manikandan (2021-08-18 03:27:03)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index fd7ff1c..aadf55d 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1507,6 +1519,95 @@
> };
> };
> };
> +
> + dsi0: dsi@ae94000 {
> + compatible = "qcom,mdss-dsi-ctrl";
> + reg = <0 0x0ae94000 0 0x400>;
> + reg-names = "dsi_ctrl";
> +
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
Drop flags as the #interrupt-cells is 0 for mdss
> +
> + clocks = < DISP_CC_MDSS_BYTE0_CLK>,
> +<
> DISP_CC_MDSS_BYTE0_INTF_CLK>,
> +< DISP_CC_MDSS_PCLK0_CLK>,
> +< DISP_CC_MDSS_ESC0_CLK>,
> +< DISP_CC_MDSS_AHB_CLK>,
> +< GCC_DISP_HF_AXI_CLK>;
> + clock-names = "byte",
> + "byte_intf",
> + "pixel",
> + "core",
> + "iface",
> + "bus";
> +
> + operating-points-v2 = <_opp_table>;
> + power-domains = < SC7280_CX>;
> +
> + phys = <_phy>;
> + phy-names = "dsi";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + dsi0_in: endpoint {
> + remote-endpoint =
> <_intf1_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + dsi0_out: endpoint {
> + };
> + };
> + };
> +
> + dsi_opp_table: dsi-opp-table {
dsi_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-18750 {
> + opp-hz = /bits/ 64
> <18750>;
> + required-opps =
> <_opp_low_svs>;
> + };
> +
> + opp-3 {
> + opp-hz = /bits/ 64
> <3>;
> + required-opps =
> <_opp_svs>;
> + };
> +
> + opp-35800 {
> + opp-hz = /bits/ 64
> <35800>;
> + required-opps =
> <_opp_svs_l1>;
> + };
> + };
> + };
> +
> + dsi_phy: dsi-phy@ae94400 {
phy@ae94400
> + compatible = "qcom,sc7280-dsi-phy-7nm";
> + reg = <0 0x0ae94400 0 0x200>,
> + <0 0x0ae94600 0 0x280>,
> + <0 0x0ae94900 0 0x280>;
> + reg-names = "dsi_phy",
> + "dsi_phy_lane",
> + "dsi_pll";
> +
> + #clock-cells = <1>;
> + #phy-cells = <0>;
> +
> + clocks = < DISP_CC_MDSS_AHB_CLK>,
> +< RPMH_CXO_CLK>;
> + clock-names = "iface", "ref";
> +
> + status = "disabled";
> + };
Re: [Freedreno] [PATCH v1 2/4] arm64: dts: qcom: sc7280: add display dt nodes
Quoting Krishna Manikandan (2021-08-18 03:27:02)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 53a21d0..fd7ff1c 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5,6 +5,7 @@
> * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> */
>
> +#include
> #include
> #include
> #include
> @@ -1424,6 +1425,90 @@
> #power-domain-cells = <1>;
> };
>
> + mdss: mdss@ae0 {
subsystem@ae0
> + compatible = "qcom,sc7280-mdss";
> + reg = <0 0x0ae0 0 0x1000>;
> + reg-names = "mdss";
> +
> + power-domains = < DISP_CC_MDSS_CORE_GDSC>;
> +
> + clocks = < GCC_DISP_AHB_CLK>,
> +< DISP_CC_MDSS_AHB_CLK>,
> + < DISP_CC_MDSS_MDP_CLK>;
> + clock-names = "iface", "ahb", "core";
> +
> + assigned-clocks = < DISP_CC_MDSS_MDP_CLK>;
> + assigned-clock-rates = <3>;
> +
> + interrupts = ;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + interconnects = <_noc MASTER_MDP0 0 _virt
> SLAVE_EBI1 0>;
> + interconnect-names = "mdp0-mem";
> +
> + iommus = <_smmu 0x900 0x402>;
> +
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + status = "disabled";
> +
> + mdp: mdp@ae01000 {
display-controller@ae01000
> + compatible = "qcom,sc7280-dpu";
> + reg = <0 0x0ae01000 0 0x8f030>,
> + <0 0x0aeb 0 0x2008>;
> + reg-names = "mdp", "vbif";
> +
> + clocks = < GCC_DISP_HF_AXI_CLK>,
> + < GCC_DISP_SF_AXI_CLK>,
> + < DISP_CC_MDSS_AHB_CLK>,
> + < DISP_CC_MDSS_MDP_LUT_CLK>,
> + < DISP_CC_MDSS_MDP_CLK>,
> + < DISP_CC_MDSS_VSYNC_CLK>;
> + clock-names = "bus", "nrt_bus", "iface",
> "lut", "core",
> + "vsync";
One line per string please.
> + assigned-clocks = <
> DISP_CC_MDSS_MDP_CLK>,
> + <
> DISP_CC_MDSS_VSYNC_CLK>,
> + <
> DISP_CC_MDSS_AHB_CLK>;
> + assigned-clock-rates = <3>,
> + <1920>,
> + <1920>;
> + operating-points-v2 = <_opp_table>;
> + power-domains = < SC7280_CX>;
> +
> + interrupt-parent = <>;
> + interrupts = <0>;
> +
> + status = "disabled";
> +
> + mdp_opp_table: mdp-opp-table {
mdp_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-2 {
> + opp-hz = /bits/ 64
> <2>;
> + required-opps =
> <_opp_low_svs>;
> + };
> +
> + opp-3 {
> + opp-hz = /bits/ 64
> <3>;
> + required-opps =
> <_opp_svs>;
> + };
> +
> + opp-38000 {
> + opp-hz = /bits/ 64
> <38000>;
> + required-opps =
> <_opp_svs_l1>;
> + };
> +
> + opp-50667 {
> + opp-hz = /bits/ 64
> <50667>;
> + required-opps =
> <_opp_nom>;
> + };
> + };
> + };
> + };
> +
Re: [Freedreno] [PATCH v1 1/4] dt-bindings: msm: add DT bindings for sc7280
Quoting Krishna Manikandan (2021-08-18 03:27:01)
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc7280.yaml
> b/Documentation/devicetree/bindings/display/msm/dpu-sc7280.yaml
> new file mode 100644
> index 000..3d256c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dpu-sc7280.yaml
> @@ -0,0 +1,228 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dpu-sc7280.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Display DPU dt properties for SC7280 target
Drop "target"?
> +
> +maintainers:
> + - Krishna Manikandan
> +
> +description: |
> + Device tree bindings for MSM Mobile Display Subsystem(MDSS) that
> encapsulates
Space after Subsystem please.
> + sub-blocks like DPU display controller, DSI and DP interfaces etc. Device
> tree
> + bindings of MDSS and DPU are mentioned for SC7280 target.
Drop "target"?
> +
> +properties:
> + compatible:
> +items:
Will there be anymore? If not, drop items and only have const.
> + - const: qcom,sc7280-mdss
> +
> + reg:
> +maxItems: 1
> +
> + reg-names:
> +const: mdss
> +
> + power-domains:
> +maxItems: 1
> +
> + clocks:
> +items:
> + - description: Display AHB clock from gcc
> + - description: Display AHB clock from dispcc
> + - description: Display core clock
> +
> + clock-names:
> +items:
> + - const: iface
> + - const: ahb
> + - const: core
> +
> + interrupts:
> +maxItems: 1
> +
> + interrupt-controller: true
> +
> + "#address-cells": true
> +
> + "#size-cells": true
> +
> + "#interrupt-cells":
> +const: 1
> +
> + iommus:
> +items:
> + - description: Phandle to apps_smmu node with SID mask for Hard-Fail
> port0
> +
> + ranges: true
> +
> + interconnects:
> +items:
> + - description: Interconnect path specifying the port ids for data bus
> +
> + interconnect-names:
> +const: mdp0-mem
> +
> +patternProperties:
> + "^display-controller@[0-9a-f]+$":
> +type: object
> +description: Node containing the properties of DPU.
> +
> +properties:
> + compatible:
> +items:
> + - const: qcom,sc7280-dpu
Will there be anymore? If not, drop items and only have const.
> +
> + reg:
> +items:
> + - description: Address offset and size for mdp register set
> + - description: Address offset and size for vbif register set
> +
> + reg-names:
> +items:
> + - const: mdp
> + - const: vbif
> +
> + clocks:
> +items:
> + - description: Display hf axi clock
> + - description: Display sf axi clock
> + - description: Display ahb clock
> + - description: Display lut clock
> + - description: Display core clock
> + - description: Display vsync clock
> +
> + clock-names:
> +items:
> + - const: bus
> + - const: nrt_bus
> + - const: iface
> + - const: lut
> + - const: core
> + - const: vsync
> +
> + interrupts:
> +maxItems: 1
> +
> + power-domains:
> +maxItems: 1
> +
> + operating-points-v2: true
> +
> + ports:
> +$ref: /schemas/graph.yaml#/properties/ports
> +description: |
> + Contains the list of output ports from DPU device. These ports
> + connect to interfaces that are external to the DPU hardware,
> + such as DSI, DP etc. Each output port contains an endpoint that
> + describes how it is connected to an external interface.
> +
> +properties:
> + port@0:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: DPU_INTF1 (DSI)
> +
> + port@1:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: DPU_INTF5 (EDP)
> +
> +required:
> + - port@0
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> + - clocks
> + - interrupts
> + - power-domains
> + - operating-points-v2
> + - ports
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> + - power-domains
> + - clocks
> + - interrupts
> + - interrupt-controller
> + - iommus
> + - ranges
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +display-subsystem@ae0 {
Maybe just 'subsystem' as that is generic enough.
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "qcom,sc7280-mdss";
> + reg = <0xae0 0x1000>;
> + reg-names = "mdss";
> + power-domains = < DISP_CC_MDSS_CORE_GDSC>;
> + clocks = < GCC_DISP_AHB_CLK>,
> + < DISP_CC_MDSS_AHB_CLK>,
> + < DISP_CC_MDSS_MDP_CLK>;
> +
Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP
Quoting Sankeerth Billakanti (2021-08-11 17:08:01)
> 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 b131fd37..1096c44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -856,9 +856,9 @@ static const struct dpu_intf_cfg sm8150_intf[] = {
> };
>
> static const struct dpu_intf_cfg sc7280_intf[] = {
> - INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 0, 24, INTF_SC7280_MASK,
> MDP_SSPP_TOP0_INTR, 24, 25),
> + INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 1, 24, INTF_SC7280_MASK,
> MDP_SSPP_TOP0_INTR, 24, 25),
> INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> - INTF_BLK("intf_5", INTF_5, 0x39000, INTF_EDP, 0, 24,
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> + INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, 0, 24, INTF_SC7280_MASK,
> MDP_SSPP_TOP0_INTR, 22, 23),
> };
>
> /*
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index d2569da..06d5a2d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1244,7 +1244,9 @@ static int dp_ctrl_link_train(struct dp_ctrl_private
> *ctrl,
> struct dp_cr_status *cr, int *training_step)
> {
> int ret = 0;
> + u8 *dpcd = ctrl->panel->dpcd;
> u8 encoding = DP_SET_ANSI_8B10B;
> + u8 ssc = 0, assr = 0;
Please don't initialize to zero and then overwrite it before using it.
It hides usage before actual initialization bugs.
> struct dp_link_info link_info = {0};
>
> dp_ctrl_config_ctrl(ctrl);
> @@ -1254,9 +1256,21 @@ static int dp_ctrl_link_train(struct dp_ctrl_private
> *ctrl,
> link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
>
> dp_aux_link_configure(ctrl->aux, _info);
> +
> + if (dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
> + ssc = DP_SPREAD_AMP_0_5;
> + drm_dp_dpcd_write(ctrl->aux, DP_DOWNSPREAD_CTRL, , 1);
> + }
> +
> drm_dp_dpcd_write(ctrl->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
> , 1);
>
> + if (dpcd[DP_EDP_CONFIGURATION_CAP] &
> DP_ALTERNATE_SCRAMBLER_RESET_CAP) {
> + assr = DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
> + drm_dp_dpcd_write(ctrl->aux, DP_EDP_CONFIGURATION_SET,
> + , 1);
> + }
> +
> ret = dp_ctrl_link_train_1(ctrl, cr, training_step);
> if (ret) {
> DRM_ERROR("link training #1 failed. ret=%d\n", ret);
> @@ -1328,9 +1342,11 @@ static int dp_ctrl_enable_mainlink_clocks(struct
> dp_ctrl_private *ctrl)
> struct dp_io *dp_io = >parser->io;
> struct phy *phy = dp_io->phy;
> struct phy_configure_opts_dp *opts_dp = _io->phy_opts.dp;
> + u8 *dpcd = ctrl->panel->dpcd;
const?
>
> opts_dp->lanes = ctrl->link->link_params.num_lanes;
> opts_dp->link_rate = ctrl->link->link_params.rate / 100;
> + opts_dp->ssc = dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5;
> dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
> ctrl->link->link_params.rate * 1000);
>
> @@ -1760,6 +1776,9 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> ctrl->link->link_params.num_lanes = ctrl->panel->link_info.num_lanes;
> ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>
> + if (ctrl->dp_ctrl.pixel_rate == 0)
> + return -EINVAL;
> +
Why are we enabling the stream with a zero pixel clk?
> DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
> ctrl->link->link_params.rate,
> ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index ee5bf64..a772290 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -117,8 +117,36 @@ struct dp_display_private {
> struct dp_audio *audio;
> };
>
> +struct msm_dp_config {
> + phys_addr_t io_start[3];
> + size_t num_dp;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> + .io_start = { 0x0ae9 },
> + .num_dp = 1,
> +};
> +
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> + .io_start = { 0xae9, 0xae98000, 0 },
> + .num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc8180x_edp_cfg = {
> + .io_start = { 0, 0, 0xae9a000 },
> + .num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc7280_edp_cfg = {
> + .io_start = { 0xaea, 0 },
> + .num_dp = 2,
> +};
Are all of these supposed to be here?
> +
> static const struct of_device_id dp_dt_match[] = {
> - {.compatible =
Re: [Freedreno] [PATCH v1 2/2] dt-bindings: Add SC7280 compatible string
Quoting Sankeerth Billakanti (2021-08-11 17:08:02) > The Qualcomm SC7280 platform supports an eDP controller, add > compatible string for it to msm/binding. > > Signed-off-by: Sankeerth Billakanti > --- > Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > index 64d8d9e..23b78ac 100644 > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > @@ -17,6 +17,9 @@ properties: >compatible: > enum: >- qcom,sc7180-dp > + - qcom,sc8180x-dp > + - qcom,sc8180x-edp > + - qcom,sc7280-edp Please sort this alphabetically. - qcom,sc7180-dp - qcom,sc7280-edp - qcom,sc8180x-dp - qcom,sc8180x-edp > >reg: > maxItems: 1
Re: [Freedreno] [PATCH 1/4] drm/bridge: ti-sn65dsi86: Avoid creating multiple connectors
Quoting Rob Clark (2021-08-11 16:52:47)
> From: Rob Clark
>
> If we created our own connector because the driver does not support the
> NO_CONNECTOR flag, we don't want the downstream bridge to *also* create
> a connector. And if this driver did pass the NO_CONNECTOR flag (and we
> supported that mode) this would change nothing.
>
> Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
> Signed-off-by: Rob Clark
> ---
Thanks for saving me the packaging effort.
Reported-by: Stephen Boyd
Tested-by: Stephen Boyd
Re: [Freedreno] [PATCH v5 1/2] arm64: dts: qcom: sc7280: Add gpu support
Quoting Akhil P Oommen (2021-08-11 07:23:54) > Add the necessary dt nodes for gpu support in sc7280. > > Signed-off-by: Akhil P Oommen > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v3 0/6] add fixes to pass DP Link Layer compliance test cases
Quoting Kuogee Hsieh (2021-08-05 13:44:49) > add fixes to pass DP Link Layer compliance test cases > > Kuogee Hsieh (6): > drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run > drm/msm/dp: reduce link rate if failed at link training 1 > drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed. > drm/msm/dp: replug event is converted into an unplug followed by an > plug events > drm/msm/dp: return correct edid checksum after corrupted edid checksum > read > drm/msm/dp: do not end dp link training until video is ready I'm still able to use my Apple dongle with these patches so Tested-by: Stephen Boyd
Re: [Freedreno] [PATCH v3 6/6] drm/msm/dp: do not end dp link training until video is ready
Quoting Kuogee Hsieh (2021-08-05 13:44:55) > Initialize both pre-emphasis and voltage swing level to 0 before > start link training and do not end link training until video is > ready to reduce the period between end of link training and video > start to meet Link Layer CTS requirement. Some dongle main link > symbol may become unlocked again if host did not end link training > soon enough after completion of link training 2. Host have to re > train main link if loss of symbol locked detected before end link > training so that the coming video stream can be transmitted to sink > properly. This fixes Link Layer CTS cases 4.3.2.1, 4.3.2.2, 4.3.2.3 > and 4.3.2.4. > > Changes in v3: > -- merge retrain link if loss of symbol locked happen into this patch > -- replace dp_ctrl_loss_symbol_lock() with dp_ctrl_channel_eq_ok() > > Signed-off-by: Kuogee Hsieh > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v3 5/6] drm/msm/dp: return correct edid checksum after corrupted edid checksum read
Quoting Kuogee Hsieh (2021-08-05 13:44:54) > Response with correct edid checksum saved at connector after corrupted edid > checksum read. This fixes Link Layer CTS cases 4.2.2.3, 4.2.2.6. > > Signed-off-by: Kuogee Hsieh > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v3 4/6] drm/msm/dp: replug event is converted into an unplug followed by an plug events
Quoting Kuogee Hsieh (2021-08-05 13:44:53)
> Remove special handling of replug interrupt and instead treat replug event
> as a sequential unplug followed by a plugin event. This is needed to meet
> the requirements of DP Link Layer CTS test case 4.2.1.3.
>
> Changes in V2:
> -- add fixes statement
>
> Changes in V3:
> -- delete EV_HPD_REPLUG_INT
>
> Fixes: f21c8a276c2d ("drm/msm/dp: handle irq_hpd with sink_count = 0
> correctly")
>
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v3 2/6] drm/msm/dp: reduce link rate if failed at link training 1
Quoting Kuogee Hsieh (2021-08-05 13:44:51) > Reduce link rate and re start link training if link training 1 > failed due to loss of clock recovery done to fix Link Layer > CTS case 4.3.1.7. Also only update voltage and pre-emphasis > swing level after link training started to fix Link Layer CTS > case 4.3.1.6. > > Changes in V2: > -- replaced cr_status with link_status[DP_LINK_STATUS_SIZE] > -- replaced dp_ctrl_any_lane_cr_done() with dp_ctrl_colco_recovery_any_ok() > -- replaced dp_ctrl_any_ane_cr_lose() with !drm_dp_clock_recovery_ok() > > Changes in V3: > -- return failed if lane_count <= 1 > > Signed-off-by: Kuogee Hsieh > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v2] drm/msm/dp: add drm debug logs to dp_pm_resume/suspend
Quoting khs...@codeaurora.org (2021-08-10 12:18:02)
> On 2021-08-10 11:33, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2021-08-10 08:29:22)
> >> Changes in V2:
> >> -- correct Fixes text
> >> -- drop commit text
> >>
> >> Fixes: 601f0479c583 ("drm/msm/dp: add logs across DP driver for ease
> >> of debugging")
> >> Signed-off-by: Kuogee Hsieh
> >> ---
> >> drivers/gpu/drm/msm/dp/dp_display.c | 13 +
> >> 1 file changed, 13 insertions(+)
> >
> > BTW, this conflicts with commit
> > e8a767e04dbc7b201cb17ab99dca723a3488b6d4
> > in msm-next. The resolution is trivial but just wanted to mention it.
>
> I Just fetched msm-next and cherry-pick this patch over, no conflict
> seen.
> Is this conflict need to be fixed?
>
Oh sorry, I mean commit afc9b8b6bab8 ("drm/msm/dp: signal audio plugged
change at dp_pm_resume") which doesn't seem to be in msm-next. Maybe Rob
will resolve the conflict directly.
Re: [Freedreno] [PATCH v2] drm/msm/dp: add drm debug logs to dp_pm_resume/suspend
Quoting Kuogee Hsieh (2021-08-10 08:29:22)
> Changes in V2:
> -- correct Fixes text
> -- drop commit text
>
> Fixes: 601f0479c583 ("drm/msm/dp: add logs across DP driver for ease of
> debugging")
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 13 +
> 1 file changed, 13 insertions(+)
BTW, this conflicts with commit e8a767e04dbc7b201cb17ab99dca723a3488b6d4
in msm-next. The resolution is trivial but just wanted to mention it.
Re: [Freedreno] [PATCH v2] drm/msm/dp: add drm debug logs to dp_pm_resume/suspend
Quoting Kuogee Hsieh (2021-08-10 08:29:22)
> Changes in V2:
> -- correct Fixes text
> -- drop commit text
>
> Fixes: 601f0479c583 ("drm/msm/dp: add logs across DP driver for ease of
> debugging")
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH] drm/msm/dp: add drm debug logs to dp_pm_resume/suspend
Quoting Kuogee Hsieh (2021-08-09 14:58:57)
> Add drm debug logs to dp_pm_resume and dp_pm_suspend to help
> debug suspend/resume issues.
>
> Fixes: 355ab7428f09 ("drm/msm/dp: add debug logs to dp_pm_resume/suspend")
BTW, I have no idea what commit this is. Best to probably just drop it?
Re: [Freedreno] [PATCH] drm/msm/dp: add drm debug logs to dp_pm_resume/suspend
Quoting Kuogee Hsieh (2021-08-09 14:58:57)
> Add drm debug logs to dp_pm_resume and dp_pm_suspend to help
> debug suspend/resume issues.
>
> Fixes: 355ab7428f09 ("drm/msm/dp: add debug logs to dp_pm_resume/suspend")
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v4] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()
Quoting Kuogee Hsieh (2021-08-04 08:51:01)
> Currently at dp_pm_resume() is_connected state is decided base on hpd
> connection
> status only. This will put is_connected in wrongly "true" state at the
> scenario
> that dongle attached to DUT but without hmdi cable connecting to it. Fix this
> problem by adding read sink count from dongle and decided is_connected state
> base
> on both sink count and hpd connection status.
>
> Changes in v2:
> -- remove dp_get_sink_count() cand call drm_dp_read_sink_count()
>
> Changes in v3:
> -- delete status local variable from dp_pm_resume()
>
> Changes in v4:
> -- delete un necessary comment at dp_pm_resume()
>
> Fixes: d9aa6571b28ba ("drm/msm/dp: check sink_count before update
> is_connected status")
> Signed-off-by: Kuogee Hsieh
> ---
Tested-by: Stephen Boyd
Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v3] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()
Quoting khs...@codeaurora.org (2021-08-04 08:48:04) > On 2021-08-03 12:05, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2021-08-03 09:25:13) > >> @@ -1327,14 +1327,26 @@ static int dp_pm_resume(struct device *dev) > >> > >> dp_catalog_ctrl_hpd_config(dp->catalog); > >> > >> - status = dp_catalog_link_is_connected(dp->catalog); > >> + /* > >> +* set sink to normal operation mode -- D0 > >> +* before dpcd read > >> +*/ > >> + dp_link_psm_config(dp->link, >panel->link_info, false); > >> + > >> + /* if sink conencted, do dpcd read sink count */ > > > > s/conencted/connected/ > > > > This also just says what the code is doing. Why do we only read the > > sink > > count if the link is connected? Can we read the sink count even if the > > link isn't connected and then consider sink count as 0 if trying to > > read > > fails? > > > yes, we can do that. > But it will suffer aux time out and retry. > i think it is better to avoid this overhead by check connection first. > Hmm ok. Maybe something is wrong with the aux channel where it doesn't avoid the timeout if the connection is obviously not established yet.
Re: [Freedreno] [v2] drm/msm/disp/dpu1: add safe lut config in dpu driver
Quoting Kalyan Thota (2021-08-03 03:41:47)
> Add safe lut configuration for all the targets in dpu
> driver as per QOS recommendation.
>
> Issue reported on SC7280:
>
> With wait-for-safe feature in smmu enabled, RT client
> buffer levels are checked to be safe before smmu invalidation.
> Since display was always set to unsafe it was delaying the
> invalidaiton process thus impacting the performance on NRT clients
> such as eMMC and NVMe.
>
> Validated this change on SC7280, With this change eMMC performance
> has improved significantly.
>
> Changes in v1:
> - Add fixes tag (Sai)
> - CC stable kernel (Dimtry)
>
> Fixes: cfacf946a464d4(drm/msm/disp/dpu1: add support for display for SC7280
> target)
This is wrong format and commit hash
Fixes: 591e34a091d1 ("drm/msm/disp/dpu1: add support for display for
SC7280 target")
> Signed-off-by: Kalyan Thota
> Tested-by: Sai Prakash Ranjan (sc7280,
> sc7180)
Re: [Freedreno] [PATCH v3] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()
Quoting Kuogee Hsieh (2021-08-03 09:25:13)
> Currently at dp_pm_resume() is_connected state is decided base on hpd
> connection
> status only. This will put is_connected in wrongly "true" state at the
> scenario
> that dongle attached to DUT but without hmdi cable connecting to it. Fix this
> problem by adding read sink count from dongle and decided is_connected state
> base
> on both sink count and hpd connection status.
>
> Changes in v2:
> -- remove dp_get_sink_count() cand call drm_dp_read_sink_count()
>
> Changes in v3:
> -- delete status local variable from dp_pm_resume()
>
> Fixes: d9aa6571b28ba ("drm/msm/dp: check sink_count before update
> is_connected status")
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 78c5301..0f39256 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1313,7 +1313,7 @@ static int dp_pm_resume(struct device *dev)
> struct platform_device *pdev = to_platform_device(dev);
> struct msm_dp *dp_display = platform_get_drvdata(pdev);
> struct dp_display_private *dp;
> - u32 status;
> + int sink_count = 0;
>
> dp = container_of(dp_display, struct dp_display_private, dp_display);
>
> @@ -1327,14 +1327,26 @@ static int dp_pm_resume(struct device *dev)
>
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> - status = dp_catalog_link_is_connected(dp->catalog);
> + /*
> +* set sink to normal operation mode -- D0
> +* before dpcd read
> +*/
> + dp_link_psm_config(dp->link, >panel->link_info, false);
> +
> + /* if sink conencted, do dpcd read sink count */
s/conencted/connected/
This also just says what the code is doing. Why do we only read the sink
count if the link is connected? Can we read the sink count even if the
link isn't connected and then consider sink count as 0 if trying to read
fails?
> + if (dp_catalog_link_is_connected(dp->catalog)) {
> + sink_count = drm_dp_read_sink_count(dp->aux);
> + if (sink_count < 0)
> + sink_count = 0;
> + }
>
> + dp->link->sink_count = sink_count;
> /*
> * can not declared display is connected unless
> * HDMI cable is plugged in and sink_count of
> * dongle become 1
> */
> - if (status && dp->link->sink_count)
> + if (dp->link->sink_count)
> dp->dp_display.is_connected = true;
> else
> dp->dp_display.is_connected = false;
Re: [Freedreno] [PATCH v2] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()
Quoting Kuogee Hsieh (2021-08-02 13:20:55)
> Currently at dp_pm_resume() is_connected state is decided base on hpd
> connection
> status only. This will put is_connected in wrongly "true" state at the
> scenario
> that dongle attached to DUT but without hmdi cable connecting to it. Fix this
> problem by adding read sink count from dongle and decided is_connected state
> base
> on both sink count and hpd connection status.
>
> Changes in v2:
> -- remove dp_get_sink_count() cand call drm_dp_read_sink_count()
>
> Fixes: d9aa6571b28ba ("drm/msm/dp: check sink_count before update
> is_connected status")
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8b69114..6dcb78e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1403,6 +1403,7 @@ static int dp_pm_resume(struct device *dev)
> struct msm_dp *dp_display = platform_get_drvdata(pdev);
> struct dp_display_private *dp;
> u32 status;
'status' is unused now, right? The compiler should be complaining about
unused local variables.
> + int sink_count = 0;
>
> dp = container_of(dp_display, struct dp_display_private, dp_display);
> xlog(__func__, 1,0,0, dp->core_initialized, dp_display->power_on);
> @@ -1417,15 +1418,26 @@ xlog(__func__, 1,0,0, dp->core_initialized,
> dp_display->power_on);
>
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> - status = dp_catalog_link_is_connected(dp->catalog);
> + /*
> +* set sink to normal operation mode -- D0
> +* before dpcd read
> +*/
> + dp_link_psm_config(dp->link, >panel->link_info, false);
> +
> + /* if sink conencted, do dpcd read sink count */
> + if ((status = dp_catalog_link_is_connected(dp->catalog))) {
> + sink_count = drm_dp_read_sink_count(dp->aux);
> + if (sink_count < 0)
> + sink_count = 0;
> + }
>
> + dp->link->sink_count = sink_count;
> /*
> * can not declared display is connected unless
> * HDMI cable is plugged in and sink_count of
> * dongle become 1
> */
> -xlog(__func__, 0x12,0,0, 0, dp->link->sink_count);
> - if (status && dp->link->sink_count)
> + if (dp->link->sink_count)
> dp->dp_display.is_connected = true;
> else
> dp->dp_display.is_connected = false;
Re: [Freedreno] [PATCH] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()
Quoting Kuogee Hsieh (2021-07-28 14:30:54)
> Currently at dp_pm_resume() is_connected state is decided base on hpd
> connection
> status only. This will put is_connected in wrongly "true" state at the
> scenario
> that dongle attached to DUT but without hmdi cable connecting to it. Fix this
> problem by adding read sink count from dongle and decided is_connected state
> base
> on both sink count and hpd connection status.
>
Please add a Fixes tag.
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 23 +--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 2b660e9..9bcb261 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1308,6 +1308,17 @@ static int dp_display_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> +static int dp_get_sink_count(struct dp_display_private *dp)
> +{
> + u8 sink_count;
> +
> + sink_count = drm_dp_read_sink_count(dp->aux);
drm_dp_read_sink_count() returns an int, not a u8. Comparing a u8 to
less than zero doesn't make any sense as it isn't signed.
> + if (sink_count < 0)
> + return 0;
> +
> + return sink_count;
> +}
We can drop this function and just have an int count in dp_pm_resume()
that is compared to < 0 and then ignored.
> +
> static int dp_pm_resume(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -1327,14 +1338,22 @@ static int dp_pm_resume(struct device *dev)
>
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> - status = dp_catalog_link_is_connected(dp->catalog);
> + /*
> +* set sink to normal operation mode -- D0
> +* before dpcd read
> +*/
> + dp_link_psm_config(dp->link, >panel->link_info, false);
>
> + if ((status = dp_catalog_link_is_connected(dp->catalog)))
> + dp->link->sink_count = dp_get_sink_count(dp);
Do we need to call drm_dp_read_sink_count_cap() as well?
> + else
> + dp->link->sink_count = 0;
> /*
> * can not declared display is connected unless
> * HDMI cable is plugged in and sink_count of
> * dongle become 1
> */
> - if (status && dp->link->sink_count)
Is 'status' used anymore? If not, please remove it.
> + if (dp->link->sink_count)
> dp->dp_display.is_connected = true;
> else
> dp->dp_display.is_connected = false;
Re: [Freedreno] [PATCH v2] arm64: dts: qcom: sc7280: Add gpu support
Quoting Akhil P Oommen (2021-07-29 11:57:23) > On 7/29/2021 10:46 PM, Stephen Boyd wrote: > > Quoting Akhil P Oommen (2021-07-28 00:17:45) > >> On 7/27/2021 5:46 AM, Stephen Boyd wrote: > >>> Quoting Akhil P Oommen (2021-07-24 10:29:00) > >>>> Add the necessary dt nodes for gpu support in sc7280. > >>>> > >>>> Signed-off-by: Akhil P Oommen > >>>> --- > >>>> This patch has dependency on the GPUCC bindings patch here: > >>>> https://patchwork.kernel.org/project/linux-arm-msm/patch/1619519590-3019-4-git-send-email-t...@codeaurora.org/ > >>> > >>> To avoid the dependency the plain numbers can be used. > >> > >> But, won't that reduce readability and make things prone to error? > > > > The numbers are not supposed to change so maybe it reduces readability > > but I don't see how it is prone to error. > > I cross check GPU's clock list whenever there is a system level issue > like NoC errors. So it is convenient to have the clock names here, at > least for me. But, I will budge if it is not easy to manage the dependency. To clarify my statement, the defines can be used eventually once the header file is part of the same tree. A duplicate patch between clk and qcom trees is fine or pulling in the clk branch works too. > > > > >> If > >> the other patch doesn't get picked up soon, we should try this option. > >> We like to get this patch merged in v5.15. > > > > The clk binding is already picked up but Bjorn would need to merge it > > into the qcom tree to use it. I don't know what the plan is there. > > > > Bjorn, could you please advise here? > > -Akhil. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 1/2] arm64: dts: qcom: sc7280: Add gpu support
Quoting Rob Clark (2021-07-29 10:35:32) > On Thu, Jul 29, 2021 at 10:19 AM Stephen Boyd wrote: > > > > > > Why is 45000 after 55000? Is it on purpose? If not intended > > please sort by frequency. > > We've used descending order, at least for gpu opp table, on other > gens, fwiw.. not sure if that just means we were doing it wrong > previously > Ah I missed that. I don't think one way or the other is mandated, but we're already sorting other OPP tables in the qcom dtsi files in ascending so this is the only one that is different. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 2/2] arm64: dts: qcom: sc7280: Add gpu thermal zone cooling support
Quoting Akhil P Oommen (2021-07-28 04:54:02) > From: Manaf Meethalavalappu Pallikunhi > > Add cooling-cells property and the cooling maps for the gpu thermal > zones to support GPU thermal cooling. > > Signed-off-by: Manaf Meethalavalappu Pallikunhi > Signed-off-by: Akhil P Oommen > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 1/2] arm64: dts: qcom: sc7280: Add gpu support
Quoting Akhil P Oommen (2021-07-28 04:54:01)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 029723a..c88f366 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -592,6 +593,85 @@
> qcom,bcm-voters = <_bcm_voter>;
> };
>
> + gpu@3d0 {
> + compatible = "qcom,adreno-635.0", "qcom,adreno";
> + #stream-id-cells = <16>;
> + reg = <0 0x03d0 0 0x4>,
> + <0 0x03d9e000 0 0x1000>,
> + <0 0x03d61000 0 0x800>;
> + reg-names = "kgsl_3d0_reg_memory",
> + "cx_mem",
> + "cx_dbgc";
> + interrupts = ;
> + iommus = <_smmu 0 0x401>;
> + operating-points-v2 = <_opp_table>;
> + qcom,gmu = <>;
> + interconnects = <_noc MASTER_GFX3D 0 _virt
> SLAVE_EBI1 0>;
> + interconnect-names = "gfx-mem";
> +
> + gpu_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-55000 {
> + opp-hz = /bits/ 64 <55000>;
> + opp-level =
> ;
> + opp-peak-kBps = <6832000>;
> + };
> +
> + opp-45000 {
Why is 45000 after 55000? Is it on purpose? If not intended
please sort by frequency.
> + opp-hz = /bits/ 64 <45000>;
> + opp-level =
> ;
> + opp-peak-kBps = <4068000>;
> + };
> +
> + opp-31500 {
> + opp-hz = /bits/ 64 <31500>;
> + opp-level =
> ;
> + opp-peak-kBps = <1804000>;
> + };
> + };
> + };
> +
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64: dts: qcom: sc7280: Add gpu support
Quoting Akhil P Oommen (2021-07-28 00:17:45) > On 7/27/2021 5:46 AM, Stephen Boyd wrote: > > Quoting Akhil P Oommen (2021-07-24 10:29:00) > >> Add the necessary dt nodes for gpu support in sc7280. > >> > >> Signed-off-by: Akhil P Oommen > >> --- > >> This patch has dependency on the GPUCC bindings patch here: > >> https://patchwork.kernel.org/project/linux-arm-msm/patch/1619519590-3019-4-git-send-email-t...@codeaurora.org/ > > > > To avoid the dependency the plain numbers can be used. > > But, won't that reduce readability and make things prone to error? The numbers are not supposed to change so maybe it reduces readability but I don't see how it is prone to error. > If > the other patch doesn't get picked up soon, we should try this option. > We like to get this patch merged in v5.15. The clk binding is already picked up but Bjorn would need to merge it into the qcom tree to use it. I don't know what the plan is there. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v6] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitreye (2021-07-26 17:38:18) > From: Maitreyee Rao > > Add trace points across the MSM DP driver to help debug > interop issues. > > Changes in v2: > - Got rid of redundant log messages. > - Added %#x instead of 0x%x wherever required. > - Got rid of __func__ calls in debug messages. > - Added newline wherever missing. > > Changes in v3: > - Got rid of redundant log messages. > - Unstuck colon from printf specifier in various places. > > Changes in v4: > - Changed goto statement and used if else-if > > Changes in v5: > - Changed if else if statement, >to not overwrite the ret variable multiple times. > Changes in v6: > - Changed a wrong log message. > Signed-off-by: Maitreyee Rao > --- Reviewed-by: Stephen Boyd It may also be good to add some logging into the dp_pm_resume() and dp_pm_suspend() functions so that we can figure out what's going on with all these problems due to disconnecting the cable during suspend. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64: dts: qcom: sc7280: Add gpu support
Quoting Akhil P Oommen (2021-07-24 10:29:00)
> Add the necessary dt nodes for gpu support in sc7280.
>
> Signed-off-by: Akhil P Oommen
> ---
> This patch has dependency on the GPUCC bindings patch here:
> https://patchwork.kernel.org/project/linux-arm-msm/patch/1619519590-3019-4-git-send-email-t...@codeaurora.org/
To avoid the dependency the plain numbers can be used.
>
> Changes in v2:
> - formatting update and removed a duplicate header (Stephan)
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 116
> +++
> 1 file changed, 116 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 029723a..524a5e0 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -585,6 +586,121 @@
> #clock-cells = <1>;
> };
>
> + gpu@3d0 {
> + compatible = "qcom,adreno-635.0", "qcom,adreno";
> + #stream-id-cells = <16>;
> + reg = <0 0x03d0 0 0x4>,
> + <0 0x03d9e000 0 0x1000>,
> + <0 0x03d61000 0 0x800>;
> + reg-names = "kgsl_3d0_reg_memory",
> + "cx_mem",
> + "cx_dbgc";
> + interrupts = ;
> + iommus = <_smmu 0 0x401>;
> + operating-points-v2 = <_opp_table>;
> + qcom,gmu = <>;
> + interconnects = <_noc MASTER_GFX3D 0 _virt
> SLAVE_EBI1 0>;
> + interconnect-names = "gfx-mem";
> +
> + gpu_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-55000 {
> + opp-hz = /bits/ 64 <55000>;
> + opp-level =
> ;
> + opp-peak-kBps = <6832000>;
> + };
> +
> + opp-45000 {
> + opp-hz = /bits/ 64 <45000>;
> + opp-level =
> ;
> + opp-peak-kBps = <4068000>;
> + };
> +
> + opp-31500 {
> + opp-hz = /bits/ 64 <31500>;
> + opp-level =
> ;
> + opp-peak-kBps = <1804000>;
> + };
> + };
> + };
> +
> + gmu: gmu@3d69000 {
> + compatible="qcom,adreno-gmu-635.0", "qcom,adreno-gmu";
> + reg = <0 0x03d6a000 0 0x34000>,
> + <0 0x3de 0 0x1>,
> + <0 0x0b29 0 0x1>;
> + reg-names = "gmu", "rscc", "gmu_pdc";
> + interrupts = ,
> + ;
> + interrupt-names = "hfi", "gmu";
> + clocks = < GPU_CC_CX_GMU_CLK>,
> + < GPU_CC_CXO_CLK>,
> + < GCC_DDRSS_GPU_AXI_CLK>,
> + < GCC_GPU_MEMNOC_GFX_CLK>,
> + < GPU_CC_AHB_CLK>,
> + < GPU_CC_HUB_CX_INT_CLK>,
> + <
> GPU_CC_HLOS1_VOTE_GPU_SMMU_CLK>;
> + clock-names = "gmu",
> + "cxo",
> + "axi",
> + "memnoc",
> + "ahb",
> + "hub",
> + "smmu_vote";
> + power-domains = < GPU_CC_CX_GDSC>,
> + < GPU_CC_GX_GDSC>;
> + power-domain-names = "cx",
> +"gx";
> + iommus = <_smmu 5 0x400>;
> + operating-points-v2 = <_opp_table>;
> +
> + gmu_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-2 {
> + opp-hz = /bits/ 64 <2>;
> + opp-level =
> ;
> + };
> + };
> + };
> +
> + adreno_smmu: iommu@3da {
> + compatible = "qcom,sc7280-smmu-500",
> "qcom,adreno-smmu", "arm,mmu-500";
> +
Re: [Freedreno] [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles
Quoting Bjorn Andersson (2021-07-24 21:24:35) > The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add > compatibles for these to the msm/dp binding. > > Signed-off-by: Bjorn Andersson > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-07-24 21:24:33)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 59ffd6c8f41f..92b7646a1bb7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -238,8 +251,11 @@ static int dp_display_bind(struct device *dev, struct
> device *master,
> }
>
> rc = dp_register_audio_driver(dev, dp->audio);
> - if (rc)
> + if (rc) {
> DRM_ERROR("Audio registration Dp failed\n");
> + goto end;
> + }
> +
>
> end:
> return rc;
This hunk looks useless. Drop it?
> @@ -1205,6 +1221,26 @@ int dp_display_request_irq(struct msm_dp *dp_display)
> return 0;
> }
>
> +static int dp_display_get_id(struct platform_device *pdev)
> +{
> + const struct msm_dp_config *cfg =
> of_device_get_match_data(>dev);
> + struct resource *res;
> + int i;
> +
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + for (i = 0; i < cfg->num_dp; i++) {
> + if (cfg->io_start[i] == res->start)
> + return i;
> + }
> +
> + dev_err(>dev, "unknown displayport instance\n");
> + return -EINVAL;
> +}
> +
> static int dp_display_probe(struct platform_device *pdev)
> {
> int rc = 0;
> @@ -1219,6 +1255,10 @@ static int dp_display_probe(struct platform_device
> *pdev)
> if (!dp)
> return -ENOMEM;
>
> + dp->id = dp_display_get_id(pdev);
> + if (dp->id < 0)
> + return -EINVAL;
> +
> dp->pdev = pdev;
> dp->name = "drm_dp";
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index e9232032b266..62d54ef6c2c4 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -161,7 +161,7 @@ struct msm_drm_private {
> /* DSI is shared by mdp4 and mdp5 */
> struct msm_dsi *dsi[2];
>
> - struct msm_dp *dp;
> + struct msm_dp *dp[3];
It would be nice to either make this dynamically sized (probably little
gain), somehow make a BUILD_BUG_ON(), or have a WARN_ON if
ARRAY_SIZE(dp) is less than a num_dp so we know somebody messed up.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/msm/dp: signal audio plugged change at dp_pm_resume
Quoting Kuogee Hsieh (2021-07-23 09:55:39)
> There is a scenario that dp cable is unplugged from DUT during system
> suspended will cause audio option state does not match real connection
> state. Fix this problem by Signaling audio plugged change with realtime
> connection status at dp_pm_resume() so that audio option will be in
> correct state after system resumed.
>
> Changes in V2:
> -- correct Fixes tag commit id.
>
> Fixes: f591dbb5fb8c ("drm/msm/dp: power off DP phy at suspend")
> Signed-off-by: Kuogee Hsieh
> Reviewed-by: Stephen Boyd
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 4
> 1 file changed, 4 insertions(+)
I noticed that with or without this patch I still have a problem with an
apple dongle where if I leave the dongle connected but unplug the HDMI
cable during suspend the audio device is still there when I resume. The
display looks to be connected in that case too, according to modetest. I
don't know if you want to roll that into this patch or make another
follow-up patch to fix it, but it seems like the sink count isn't
updated on resume? Did commit f591dbb5fb8c break a bunch of logic in
here because now the link is powered down properly and so sink_count
isn't updated properly?
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 78c5301..2b660e9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1339,6 +1339,10 @@ static int dp_pm_resume(struct device *dev)
> else
> dp->dp_display.is_connected = false;
>
> + dp_display_handle_plugged_change(g_dp_display,
> + dp->dp_display.is_connected);
> +
> +
There's also a double newline here that we should probably remove.
> mutex_unlock(>event_mutex);
>
> return 0;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v5] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitreye (2021-07-26 10:36:26)
> @@ -509,6 +515,7 @@ static int dp_display_usbpd_attention_cb(struct device
> *dev)
> DRM_ERROR("invalid dev\n");
> return -EINVAL;
> }
> + DRM_DEBUG_DP("sink_request: %d\n", sink_request);
This one is bad. sink_request isn't assigned yet.
>
> dp = container_of(g_dp_display,
> struct dp_display_private, dp_display);
> @@ -523,6 +530,7 @@ static int dp_display_usbpd_attention_cb(struct device
> *dev)
> rc = dp_link_process_request(dp->link);
> if (!rc) {
> sink_request = dp->link->sink_request;
> + DRM_DEBUG_DP("hpd_state=%d sink_count=%d\n", dp->hpd_state,
> sink_request);
Should that say sink_request?
> if (sink_request & DS_PORT_STATUS_CHANGED)
> rc = dp_display_handle_port_ststus_changed(dp);
> else
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/msm/dp: signal audio plugged change at dp_pm_resume
Quoting Kuogee Hsieh (2021-07-23 09:55:39)
> There is a scenario that dp cable is unplugged from DUT during system
> suspended will cause audio option state does not match real connection
> state. Fix this problem by Signaling audio plugged change with realtime
> connection status at dp_pm_resume() so that audio option will be in
> correct state after system resumed.
>
> Changes in V2:
> -- correct Fixes tag commit id.
>
> Fixes: f591dbb5fb8c ("drm/msm/dp: power off DP phy at suspend")
> Signed-off-by: Kuogee Hsieh
> Reviewed-by: Stephen Boyd
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 78c5301..2b660e9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1339,6 +1339,10 @@ static int dp_pm_resume(struct device *dev)
> else
> dp->dp_display.is_connected = false;
>
> + dp_display_handle_plugged_change(g_dp_display,
Can this be dp_display instead of g_dp_display?
> + dp->dp_display.is_connected);
> +
> +
> mutex_unlock(>event_mutex);
>
> return 0;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v5] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitreye (2021-07-26 10:36:26) > From: Maitreyee Rao > > Add trace points across the MSM DP driver to help debug > interop issues. > > Changes in v2: > - Got rid of redundant log messages. > - Added %#x instead of 0x%x wherever required. > - Got rid of __func__ calls in debug messages. > - Added newline wherever missing. > > Changes in v3: > - Got rid of redundant log messages. > - Unstuck colon from printf specifier in various places. > > Changes in v4: > - Changed goto statement and used if else-if > > Changes in v5: > - Changed if else if statement, >to not overwrite the ret variable multiple times. > > Signed-off-by: Maitreyee Rao > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] arm64: dts: qcom: sc7280: Add gpu support
Quoting Akhil P Oommen (2021-07-23 04:20:54)
> Add the necessary dt nodes for gpu support in sc7280.
>
> Signed-off-by: Akhil P Oommen
> ---
> This patch has dependency on the GPUCC bindings patch here:
> https://patchwork.kernel.org/project/linux-arm-msm/patch/1619519590-3019-4-git-send-email-t...@codeaurora.org/
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 107
> +++
> 1 file changed, 107 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 029723a..beb313c 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -16,6 +16,8 @@
> #include
> #include
> #include
> +#include
> +#include
Please sort this alphabetically.
>
> / {
> interrupt-parent = <>;
> @@ -592,6 +594,111 @@
> qcom,bcm-voters = <_bcm_voter>;
> };
>
> + gpu: gpu@3d0 {
Will this label be used? If not, please don't add it.
> + compatible = "qcom,adreno-635.0", "qcom,adreno";
> + #stream-id-cells = <16>;
> + reg = <0 0x03d0 0 0x4>, <0 0x03d9e000 0
> 0x1000>,
> + <0 0x03d61000 0 0x800>;
> + reg-names = "kgsl_3d0_reg_memory", "cx_mem",
> "cx_dbgc";
I'd prefer to see one reg and reg-names per line if the list is longer
than one line.
reg = < >,
< >,
< >;
reg-names = " ",
" ",
" ";
It makes is much easier to figure out which property lines up with which
name.
> + interrupts = ;
> + iommus = <_smmu 0 0x401>;
> + operating-points-v2 = <_opp_table>;
> + qcom,gmu = <>;
> + interconnects = <_noc MASTER_GFX3D 0 _virt
> SLAVE_EBI1 0>;
> + interconnect-names = "gfx-mem";
> +
> + gpu_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-55000 {
> + opp-hz = /bits/ 64 <55000>;
> + opp-level =
> ;
> + opp-peak-kBps = <6832000>;
> + };
> +
> + opp-45000 {
> + opp-hz = /bits/ 64 <45000>;
> + opp-level =
> ;
> + opp-peak-kBps = <4068000>;
> + };
> +
> + opp-31500 {
> + opp-hz = /bits/ 64 <31500>;
> + opp-level =
> ;
> + opp-peak-kBps = <1804000>;
> + };
> + };
> + };
> +
> + adreno_smmu: iommu@3da {
3da comes after 3d69000, please sort this by unit address.
> + compatible = "qcom,sc7280-smmu-500",
> "qcom,adreno-smmu", "arm,mmu-500";
> + reg = <0 0x03da 0 0x2>;
> + #iommu-cells = <2>;
> + #global-interrupts = <2>;
> + interrupts = ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ;
> +
> + clocks = < GCC_GPU_MEMNOC_GFX_CLK>,
> + < GCC_GPU_SNOC_DVM_GFX_CLK>,
> + < GPU_CC_AHB_CLK>,
> + <
> GPU_CC_HLOS1_VOTE_GPU_SMMU_CLK>,
> + < GPU_CC_CX_GMU_CLK>,
> + < GPU_CC_HUB_CX_INT_CLK>,
> + < GPU_CC_HUB_AON_CLK>;
> + clock-names = "gcc_gpu_memnoc_gfx_clk",
> + "gcc_gpu_snoc_dvm_gfx_clk",
> + "gpu_cc_ahb_clk",
> + "gpu_cc_hlos1_vote_gpu_smmu_clk",
> + "gpu_cc_cx_gmu_clk",
> + "gpu_cc_hub_cx_int_clk",
> + "gpu_cc_hub_aon_clk";
> +
> +
Re: [Freedreno] [PATCH v4] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitr...@codeaurora.org (2021-07-22 20:53:37)
> On 2021-07-22 15:09, Stephen Boyd wrote:
> Thank you for the comments .
> > Quoting maitr...@codeaurora.org (2021-07-22 14:33:43)
> >> Thank you Stephen.
> >>
> >> On 2021-07-22 13:31, Stephen Boyd wrote:
> >> > Quoting maitreye (2021-07-21 16:19:40)
> >> >> From: Maitreyee Rao
> >> >>
> >> >> Add trace points across the MSM DP driver to help debug
> >> >> interop issues.
> >> >>
> >> >> Changes in v4:
> >> >> - Changed goto statement and used if else-if
> >> >
> >> > I think drm likes to see all the changelog here to see patch evolution.
> >> >
> >> Yes, I will fix this
> >> >>
> >> >> Signed-off-by: Maitreyee Rao
> >> >> ---
> >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
> >> >> b/drivers/gpu/drm/msm/dp/dp_link.c
> >> >> index be986da..8c98ab7 100644
> >> >> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> >> >> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> >> >> @@ -1036,43 +1036,28 @@ int dp_link_process_request(struct dp_link
> >> >> *dp_link)
> >> >>
> >> >> if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> >> >> dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> >> >> - return ret;
> >> >> }
> >> >> -
> >> >> - ret = dp_link_process_ds_port_status_change(link);
> >> >> - if (!ret) {
> >> >> + else if (!(ret = dp_link_process_ds_port_status_change(link)))
> >> >> {
> >> >> dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> >> >> - return ret;
> >> >> }
> >> >> -
> >> >> - ret = dp_link_process_link_training_request(link);
> >> >> - if (!ret) {
> >> >> + else if (!(ret = dp_link_process_link_training_request(link)))
> >> >> {
> >> >> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> >> >> - return ret;
> >> >> }
> >> >> -
> >> >> - ret = dp_link_process_phy_test_pattern_request(link);
> >> >> - if (!ret) {
> >> >> + else if (!(ret =
> >> >> dp_link_process_phy_test_pattern_request(link))) {
> >> >> dp_link->sink_request |=
> >> >> DP_TEST_LINK_PHY_TEST_PATTERN;
> >> >> - return ret;
> >> >> - }
> >> >> -
> >> >> - ret = dp_link_process_link_status_update(link);
> >> >> - if (!ret) {
> >> >> + }
> >> >> + else if (!(ret = dp_link_process_link_status_update(link))) {
> >> >
> >> > The kernel coding style is to leave the brackets on the same line
> >> >
> >> > if (condition) {
> >> >
> >> > } else if (conditon) {
> >> >
> >> > }
> >> >
> >> > See Documentation/process/coding-style.rst
> >> >
> >> Yes, I will fix this
> >>
> >> > Also, the if (!(ret = dp_link_...)) style is really hard to read. Maybe
> >> > apply this patch before?
> >> >
> >> > 8<
> >> > diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
> >> > b/drivers/gpu/drm/msm/dp/dp_link.c
> >> > index 1195044a7a3b..408cddd90f0f 100644
> >> > --- a/drivers/gpu/drm/msm/dp/dp_link.c
> >> > +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> >> > @@ -1027,41 +1027,22 @@ int dp_link_process_request(struct dp_link
> >> > *dp_link)
> >> >
> >> > if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> >> > dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> >> > - return ret;
> >> > - }
> >> > -
> >> > - ret = dp_link_process_ds_port_status_change(link);
> >> > - if (!ret) {
> >> > + } else if (!dp_link_process_ds_port_status_change(link)) {
> >> > dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> >> > - return ret;
> >> > - }
> >> >
Re: [Freedreno] [PATCH] drm/msm/dp: signal audio plugged change at dp_pm_resume
Quoting Kuogee Hsieh (2021-07-22 15:15:17)
> There is a scenario that dp cable is unplugged from DUT during system
> suspended will cause audio option state does not match real connection
> state. Fix this problem by Signaling audio plugged change with realtime
> connection status at dp_pm_resume() so that audio option will be in
> correct state after system resumed.
>
> Fixes: bd52cfedb5a8 ("drm/msm/dp: power off DP phy at suspend")
This should be
Fixes: f591dbb5fb8c ("drm/msm/dp: power off DP phy at suspend")
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v4] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitr...@codeaurora.org (2021-07-22 14:33:43)
> Thank you Stephen.
>
> On 2021-07-22 13:31, Stephen Boyd wrote:
> > Quoting maitreye (2021-07-21 16:19:40)
> >> From: Maitreyee Rao
> >>
> >> Add trace points across the MSM DP driver to help debug
> >> interop issues.
> >>
> >> Changes in v4:
> >> - Changed goto statement and used if else-if
> >
> > I think drm likes to see all the changelog here to see patch evolution.
> >
> Yes, I will fix this
> >>
> >> Signed-off-by: Maitreyee Rao
> >> ---
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
> >> b/drivers/gpu/drm/msm/dp/dp_link.c
> >> index be986da..8c98ab7 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> >> @@ -1036,43 +1036,28 @@ int dp_link_process_request(struct dp_link
> >> *dp_link)
> >>
> >> if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> >> dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> >> - return ret;
> >> }
> >> -
> >> - ret = dp_link_process_ds_port_status_change(link);
> >> - if (!ret) {
> >> + else if (!(ret = dp_link_process_ds_port_status_change(link)))
> >> {
> >> dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> >> - return ret;
> >> }
> >> -
> >> - ret = dp_link_process_link_training_request(link);
> >> - if (!ret) {
> >> + else if (!(ret = dp_link_process_link_training_request(link)))
> >> {
> >> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> >> - return ret;
> >> }
> >> -
> >> - ret = dp_link_process_phy_test_pattern_request(link);
> >> - if (!ret) {
> >> + else if (!(ret =
> >> dp_link_process_phy_test_pattern_request(link))) {
> >> dp_link->sink_request |=
> >> DP_TEST_LINK_PHY_TEST_PATTERN;
> >> - return ret;
> >> - }
> >> -
> >> - ret = dp_link_process_link_status_update(link);
> >> - if (!ret) {
> >> + }
> >> + else if (!(ret = dp_link_process_link_status_update(link))) {
> >
> > The kernel coding style is to leave the brackets on the same line
> >
> > if (condition) {
> >
> > } else if (conditon) {
> >
> > }
> >
> > See Documentation/process/coding-style.rst
> >
> Yes, I will fix this
>
> > Also, the if (!(ret = dp_link_...)) style is really hard to read. Maybe
> > apply this patch before?
> >
> > 8<
> > diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
> > b/drivers/gpu/drm/msm/dp/dp_link.c
> > index 1195044a7a3b..408cddd90f0f 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_link.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> > @@ -1027,41 +1027,22 @@ int dp_link_process_request(struct dp_link
> > *dp_link)
> >
> > if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> > dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> > - return ret;
> > - }
> > -
> > - ret = dp_link_process_ds_port_status_change(link);
> > - if (!ret) {
> > + } else if (!dp_link_process_ds_port_status_change(link)) {
> > dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> > - return ret;
> > - }
> > -
> > - ret = dp_link_process_link_training_request(link);
> > - if (!ret) {
> > + } else if (!dp_link_process_link_training_request(link)) {
> > dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> > - return ret;
> > - }
> > -
> > - ret = dp_link_process_phy_test_pattern_request(link);
> > - if (!ret) {
> > + } else if (!dp_link_process_phy_test_pattern_request(link)) {
> > dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
> > - return ret;
> > - }
> > -
> > - ret = dp_link_process_link_status_update(link);
> > - if (!ret) {
> > + } else if (!dp_link_process_link_status_update(link)) {
> > dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
> > - return ret;
> > - }
> > -
> > - if (dp_link_is_video_pattern_requested(link)) {
> > -
Re: [Freedreno] [PATCH v4] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitreye (2021-07-21 16:19:40)
> From: Maitreyee Rao
>
> Add trace points across the MSM DP driver to help debug
> interop issues.
>
> Changes in v4:
> - Changed goto statement and used if else-if
I think drm likes to see all the changelog here to see patch evolution.
>
> Signed-off-by: Maitreyee Rao
> ---
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
> b/drivers/gpu/drm/msm/dp/dp_link.c
> index be986da..8c98ab7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -1036,43 +1036,28 @@ int dp_link_process_request(struct dp_link *dp_link)
>
> if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> - return ret;
> }
> -
> - ret = dp_link_process_ds_port_status_change(link);
> - if (!ret) {
> + else if (!(ret = dp_link_process_ds_port_status_change(link))) {
> dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> - return ret;
> }
> -
> - ret = dp_link_process_link_training_request(link);
> - if (!ret) {
> + else if (!(ret = dp_link_process_link_training_request(link))) {
> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> - return ret;
> }
> -
> - ret = dp_link_process_phy_test_pattern_request(link);
> - if (!ret) {
> + else if (!(ret = dp_link_process_phy_test_pattern_request(link))) {
> dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
> - return ret;
> - }
> -
> - ret = dp_link_process_link_status_update(link);
> - if (!ret) {
> + }
> + else if (!(ret = dp_link_process_link_status_update(link))) {
The kernel coding style is to leave the brackets on the same line
if (condition) {
} else if (conditon) {
}
See Documentation/process/coding-style.rst
Also, the if (!(ret = dp_link_...)) style is really hard to read. Maybe
apply this patch before?
8<
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index 1195044a7a3b..408cddd90f0f 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -1027,41 +1027,22 @@ int dp_link_process_request(struct dp_link *dp_link)
if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
- return ret;
- }
-
- ret = dp_link_process_ds_port_status_change(link);
- if (!ret) {
+ } else if (!dp_link_process_ds_port_status_change(link)) {
dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
- return ret;
- }
-
- ret = dp_link_process_link_training_request(link);
- if (!ret) {
+ } else if (!dp_link_process_link_training_request(link)) {
dp_link->sink_request |= DP_TEST_LINK_TRAINING;
- return ret;
- }
-
- ret = dp_link_process_phy_test_pattern_request(link);
- if (!ret) {
+ } else if (!dp_link_process_phy_test_pattern_request(link)) {
dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
- return ret;
- }
-
- ret = dp_link_process_link_status_update(link);
- if (!ret) {
+ } else if (!dp_link_process_link_status_update(link)) {
dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
- return ret;
- }
-
- if (dp_link_is_video_pattern_requested(link)) {
- ret = 0;
- dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
- }
+ } else {
+ if (dp_link_is_video_pattern_requested(link))
+ dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
- if (dp_link_is_audio_pattern_requested(link)) {
- dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
- return -EINVAL;
+ if (dp_link_is_audio_pattern_requested(link)) {
+ dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
+ ret = -EINVAL;
+ }
}
return ret;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 2/2] drm/msm/dsi: add support for dsi test pattern generator
Quoting Abhinav Kumar (2021-07-21 19:50:32) > During board bringups its useful to have a DSI test pattern > generator to isolate a DPU vs a DSI issue and focus on the relevant > hardware block. > > To facilitate this, add an API which triggers the DSI controller > test pattern. The expected output is a rectangular checkered pattern. > > This has been validated on a single DSI video mode panel by calling it > right after drm_panel_enable() which is also the ideal location to use > this as the DSI host and the panel have been initialized by then. > > Further validation on dual DSI and command mode panel is pending. > If there are any fix ups needed for those, it shall be applied on top > of this change. > > Changes in v2: > - generate the new dsi.xml.h and update the bitfield names > > Signed-off-by: Abhinav Kumar > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 1/2] drm/msm/dsi: update dsi register header file for tpg
Quoting Abhinav Kumar (2021-07-21 19:50:31) > Update the DSI controller header XML file to add registers > and bitfields to support rectangular checkered pattern > generator. > > Signed-off-by: Abhinav Kumar > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dp: Initialize the INTF_CONFIG register
Quoting Bjorn Andersson (2021-07-21 19:44:34)
> Some bootloaders set the widebus enable bit in the INTF_CONFIG register,
> but configuration of widebus isn't yet supported ensure that the
> register has a known value, with widebus disabled.
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Bjorn Andersson
> ---
Reviewed-by: Stephen Boyd
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 5/5] drm/msm/dp: Allow sub-regions to be specified in DT
Quoting Bjorn Andersson (2021-07-21 19:42:27) > Not all platforms has P0 at an offset of 0x1000 from the base address, > so add support for specifying each sub-region in DT. The code falls back > to the predefined offsets in the case that only a single reg is > specified, in order to support existing DT. > > Signed-off-by: Bjorn Andersson > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 4/5] drm/msm/dp: Store each subblock in the io region
Quoting Bjorn Andersson (2021-07-21 19:42:26)
> Not all platforms has DP_P0 at offset 0x1000 from the beginning of the
> DP block. So dss_io_data into representing each of the sub-regions, to
"So dss_io_data into" doesn't make sense to me. Is some word or words
missing?
> make it possible in the next patch to specify each of the sub-regions
> individually.
>
> Signed-off-by: Bjorn Andersson
> ---
> drivers/gpu/drm/msm/dp/dp_catalog.c | 64 +
> drivers/gpu/drm/msm/dp/dp_parser.c | 30 --
> drivers/gpu/drm/msm/dp/dp_parser.h | 10 -
> 3 files changed, 54 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index e68dacef547c..1a10901ae574 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -11,6 +11,15 @@
> #include "dp_parser.h"
> #include "dp_reg.h"
>
> +#define DP_DEFAULT_AHB_OFFSET 0x
> +#define DP_DEFAULT_AHB_SIZE0x0200
> +#define DP_DEFAULT_AUX_OFFSET 0x0200
> +#define DP_DEFAULT_AUX_SIZE0x0200
> +#define DP_DEFAULT_LINK_OFFSET 0x0400
> +#define DP_DEFAULT_LINK_SIZE 0x0C00
> +#define DP_DEFAULT_P0_OFFSET 0x1000
> +#define DP_DEFAULT_P0_SIZE 0x0400
> +
> static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
> .num = 2,
> .regs = {
> @@ -48,12 +57,25 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
> struct dp_io *io = >io;
> struct dss_io_data *dss = >dp_controller;
>
> - dss->base = dp_ioremap(pdev, 0, >len);
> - if (IS_ERR(dss->base)) {
> - DRM_ERROR("unable to remap dp io region: %pe\n", dss->base);
> - return PTR_ERR(dss->base);
> + dss->ahb = dp_ioremap(pdev, 0, >ahb_len);
So many layers of gunky goo!
> + if (IS_ERR(dss->ahb)) {
> + DRM_ERROR("unable to remap ahb region: %pe\n", dss->ahb);
> + return PTR_ERR(dss->ahb);
> }
>
> + if (dss->ahb_len < DP_DEFAULT_P0_OFFSET + DP_DEFAULT_P0_SIZE) {
> + DRM_ERROR("legacy memory region not large enough\n");
> + return -EINVAL;
> + }
> +
> + dss->ahb_len = DP_DEFAULT_AHB_SIZE;
> + dss->aux = dss->ahb + DP_DEFAULT_AUX_OFFSET;
> + dss->aux_len = DP_DEFAULT_AUX_SIZE;
> + dss->link = dss->ahb + DP_DEFAULT_LINK_OFFSET;
> + dss->link_len = DP_DEFAULT_LINK_SIZE;
> + dss->p0 = dss->ahb + DP_DEFAULT_P0_OFFSET;
> + dss->p0_len = DP_DEFAULT_P0_SIZE;
> +
> io->phy = devm_phy_get(>dev, "dp");
> if (IS_ERR(io->phy))
> return PTR_ERR(io->phy);
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
> b/drivers/gpu/drm/msm/dp/dp_parser.h
> index dc62e70b1640..3266b529c090 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -26,8 +26,14 @@ enum dp_pm_type {
> };
>
> struct dss_io_data {
> - size_t len;
> - void __iomem *base;
> + void __iomem *ahb;
> + size_t ahb_len;
Maybe make another struct and have a few of them here
struct dss_io_region {
void __iomem *base;
size_t len;
};
then the code reads as aux.base and aux.len and we know they're closely
related.
> + void __iomem *aux;
> + size_t aux_len;
> + void __iomem *link;
> + size_t link_len;
> + void __iomem *p0;
> + size_t p0_len;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 3/5] drm/msm/dp: Refactor ioremap wrapper
Quoting Bjorn Andersson (2021-07-21 19:42:25)
> In order to deal with multiple memory ranges in the following commit
> change the ioremap wrapper to not poke directly into the dss_io_data
> struct.
>
> Signed-off-by: Bjorn Andersson
> ---
> drivers/gpu/drm/msm/dp/dp_parser.c | 28 ++--
> drivers/gpu/drm/msm/dp/dp_parser.h | 2 +-
> 2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index c064ced78278..e68dacef547c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -19,39 +19,39 @@ static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
> },
> };
>
> -static int msm_dss_ioremap(struct platform_device *pdev,
> - struct dss_io_data *io_data)
> +static void __iomem *dp_ioremap(struct platform_device *pdev, int idx,
> size_t *len)
> {
> struct resource *res = NULL;
Can we drop assignment to NULL too?
> + void __iomem *base;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, idx);
> if (!res) {
> DRM_ERROR("%pS->%s: msm_dss_get_res failed\n",
> __builtin_return_address(0), __func__);
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
> }
>
> - io_data->len = (u32)resource_size(res);
> - io_data->base = devm_ioremap(>dev, res->start, io_data->len);
> - if (!io_data->base) {
> + base = devm_ioremap_resource(>dev, res);
> + if (!base) {
devm_ioremap_resource() returns an ERR_PTR on failure, not NULL.
> DRM_ERROR("%pS->%s: ioremap failed\n",
> __builtin_return_address(0), __func__);
> - return -EIO;
> + return ERR_PTR(-EIO);
> }
>
> - return 0;
> + *len = resource_size(res);
> + return base;
> }
>
> static int dp_parser_ctrl_res(struct dp_parser *parser)
> {
> - int rc = 0;
> struct platform_device *pdev = parser->pdev;
> struct dp_io *io = >io;
> + struct dss_io_data *dss = >dp_controller;
>
> - rc = msm_dss_ioremap(pdev, >dp_controller);
> - if (rc) {
> - DRM_ERROR("unable to remap dp io resources, rc=%d\n", rc);
> - return rc;
> + dss->base = dp_ioremap(pdev, 0, >len);
> + if (IS_ERR(dss->base)) {
> + DRM_ERROR("unable to remap dp io region: %pe\n", dss->base);
> + return PTR_ERR(dss->base);
> }
>
> io->phy = devm_phy_get(>dev, "dp");
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/5] drm/msm/dp: Use devres for ioremap()
Quoting Bjorn Andersson (2021-07-21 19:42:24)
> The non-devres version of ioremap is used, which requires manual
> cleanup. But the code paths leading here is mixed with other devres
> users, so rely on this for ioremap as well to simplify the code.
>
> Signed-off-by: Bjorn Andersson
> ---
Reviewed-by: Stephen Boyd
> drivers/gpu/drm/msm/dp/dp_parser.c | 29 -
> 1 file changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 0519dd3ac3c3..c064ced78278 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -32,7 +32,7 @@ static int msm_dss_ioremap(struct platform_device *pdev,
> }
>
> io_data->len = (u32)resource_size(res);
> - io_data->base = ioremap(res->start, io_data->len);
> + io_data->base = devm_ioremap(>dev, res->start, io_data->len);
> if (!io_data->base) {
> DRM_ERROR("%pS->%s: ioremap failed\n",
> __builtin_return_address(0), __func__);
I don't think we need this print either, but that can come later in
addition to using devm_platform_get_and_ioremap_resource().
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/5] dt-bindings: msm/dp: Change reg definition
Quoting Bjorn Andersson (2021-07-21 19:42:23) > reg was defined as one region covering the entire DP block, but the > memory map is actually split in 4 regions and obviously the size of > these regions differs between platforms. > > Switch the reg to require that all four regions are specified instead. > It is expected that the implementation will handle existing DTBs, even > though the schema defines the new layout. > > Signed-off-by: Bjorn Andersson > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 4/7] drm/msm/dp: replug event is converted into an unplug followed by an plug events
Quoting Kuogee Hsieh (2021-07-13 08:54:04)
> Remove special handling of replug interrupt and instead treat replug event
> as a sequential unplug followed by a plugin event. This is needed to meet
> the requirements of DP Link Layer CTS test case 4.2.1.3.
>
> Changes in V2:
> -- add fixes statement
>
> Fixes: f21c8a276c2d ("drm/msm/dp: handle irq_hpd with sink_count = 0
> correctly")
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 78c5301..d089ada 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1146,9 +1146,6 @@ static int hpd_event_thread(void *data)
> case EV_IRQ_HPD_INT:
> dp_irq_hpd_handle(dp_priv, todo->data);
> break;
> - case EV_HPD_REPLUG_INT:
Please remove the enum as well.
> - /* do nothing */
> - break;
> case EV_USER_NOTIFICATION:
> dp_display_send_hpd_notification(dp_priv,
> todo->data);
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 5/7] drm/msm/dp: return correct edid checksum after corrupted edid checksum read
Quoting Kuogee Hsieh (2021-07-13 08:54:05)
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 88196f7..0fdb551 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -303,7 +303,12 @@ void dp_panel_handle_sink_request(struct dp_panel
> *dp_panel)
> panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>
> if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> - u8 checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> + u8 checksum;
> +
> + if (dp_panel->edid)
> + checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> + else
> + checksum = dp_panel->connector->real_edid_checksum;
Is there any reason why we can't use connector->real_edid_checksum all
the time?
>
> dp_link_send_edid_checksum(panel->link, checksum);
> dp_link_send_test_response(panel->link);
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 7/7] drm/msm/dp: retrain link when loss of symbol lock detected
Quoting Kuogee Hsieh (2021-07-13 08:54:07)
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 6a013b0..20951c8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1638,6 +1638,25 @@ static bool dp_ctrl_clock_recovery_any_ok(
> return drm_dp_clock_recovery_ok(link_status, lane_count);
> }
>
> +static bool dp_ctrl_loss_symbol_lock(struct dp_ctrl_private *ctrl)
> +{
> + u8 link_status[DP_LINK_STATUS_SIZE];
> + u8 status;
> + int i;
> + int num_lanes = ctrl->link->link_params.num_lanes;
> +
> + dp_ctrl_read_link_status(ctrl, link_status);
> +
> + for (i = 0; i < num_lanes; i++) {
> + status = link_status[i / 2];
> + status >>= ((i % 2) * 4);
> + if (!(status & DP_LANE_SYMBOL_LOCKED))
> + return true;
> + }
> +
> + return false;
> +}
Can this function move to drivers/gpu/drm/drm_dp_helper.c and be called
drm_dp_symbol_locked()?
> +
> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> {
> int rc = 0;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 3/7] drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed.
Quoting Kuogee Hsieh (2021-07-13 08:54:03) > Aux hardware calibration sequence requires resetting the aux controller > in order for the new setting to take effect. However resetting the AUX > controller will also clear HPD interrupt status which may accidentally > cause pending unplug interrupt to get lost. Therefore reset aux > controller only when link is in connection state when dp_aux_cmd_fifo_tx() > fail. This fixes Link Layer CTS cases 4.2.1.1 and 4.2.1.2. > > Signed-off-by: Kuogee Hsieh > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 2/7] drm/msm/dp: reduce link rate if failed at link training 1
Quoting Kuogee Hsieh (2021-07-13 08:54:02)
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 27fb0f0..92cf331 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1634,6 +1617,24 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl
> *dp_ctrl)
> }
> }
>
> +static bool dp_ctrl_clock_recovery_any_ok(
> + const u8 link_status[DP_LINK_STATUS_SIZE],
> + int lane_count)
> +{
> + int lane_cnt;
> +
> + /*
> +* only interested in the lane number after reduced
> +* lane_cnt = 4, then only interested in 2 lanes
> +* lane_cnt = 2, then only interested in 1 lane
> +*/
> + lane_cnt = lane_count >> 1;
> + if (lane_cnt == 0)
> + return false;
> +
> + return drm_dp_clock_recovery_ok(link_status, lane_count);
This doesn't work? Because drm_dp_clock_recovery_ok() requires every
lane to be OK whereas this function wants any lane to be OK? It may make
sense to have drm_dp_clock_recovery_ok() return false if lane_count == 0
too.
> +}
> +
> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> {
> int rc = 0;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 1/7] drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run
Quoting Kuogee Hsieh (2021-07-13 08:54:01)
> DP cable should always connect to DPU during the entire PHY compliance
> testing run. Since DP PHY compliance test is executed at irq_hpd event
> context, dp_ctrl_off_link_stream() should be used instead of dp_ctrl_off().
> dp_ctrl_off() is used for unplug event which is triggered when DP cable is
> dis connected.
>
> Changes in V2:
> -- add fixes statement
>
> Fixes: f21c8a276c2d ("drm/msm/dp: handle irq_hpd with sink_count = 0
> correctly")
>
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v2 3/3] drm/msm/dsi: Add DSI support for SC7280
Quoting Rajeev Nandan (2021-06-22 05:42:28) > Add support for v2.5.0 DSI block in the SC7280 SoC. > > Signed-off-by: Rajeev Nandan > Reviewed-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v2 2/3] drm/msm/dsi: Add PHY configuration for SC7280
Quoting Rajeev Nandan (2021-06-22 05:42:27) > The SC7280 SoC uses the 7nm (V4.1) DSI PHY driver with > different enable|disable regulator loads. > > Signed-off-by: Rajeev Nandan > Reviewed-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v2 1/3] dt-bindings: msm/dsi: Add sc7280 7nm dsi phy
Quoting Rajeev Nandan (2021-06-22 05:42:26) > The SC7280 SoC uses the 7nm (V4.1) DSI PHY driver. > > Signed-off-by: Rajeev Nandan > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitreye (2021-07-20 15:39:30)
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
> b/drivers/gpu/drm/msm/dp/dp_link.c
> index be986da..316e8e6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -1036,43 +1036,46 @@ int dp_link_process_request(struct dp_link *dp_link)
>
> if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> - return ret;
> + goto out;
> }
>
> ret = dp_link_process_ds_port_status_change(link);
> if (!ret) {
> dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> - return ret;
> + goto out;
> }
>
> ret = dp_link_process_link_training_request(link);
> if (!ret) {
> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> - return ret;
> + goto out;
> }
>
> ret = dp_link_process_phy_test_pattern_request(link);
> if (!ret) {
> dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
> - return ret;
> + goto out;
> }
>
> ret = dp_link_process_link_status_update(link);
if ret == 0 we go into the if below and goto out.
> if (!ret) {
> dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
> - return ret;
> + goto out;
> }
At this point ret != 0 due to the goto above.
>
> if (dp_link_is_video_pattern_requested(link)) {
> - ret = 0;
And now we've removed the ret = 0 assignment from here.
> dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
> + goto out;
And then we goto out. Isn't this a behavior change? Still feels like we
should be using if/else-if logic here instead of this goto maze.
> }
>
> if (dp_link_is_audio_pattern_requested(link)) {
> dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> +out:
> + DRM_DEBUG_DP("sink request=%#x", dp_link->sink_request);
> return ret;
> }
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 7/7] drm/msm/dp: retrain link when loss of symbol lock detected
Quoting khs...@codeaurora.org (2021-07-09 10:16:52) > On 2021-07-08 00:21, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2021-07-06 10:20:20) > >> Main link symbol locked is achieved at end of link training 2. Some > >> dongle main link symbol may become unlocked again if host did not end > >> link training soon enough after completion of link training 2. Host > >> have to re train main link if loss of symbol lock detected before > >> end link training so that the coming video stream can be transmitted > >> to sink properly. > >> > >> Signed-off-by: Kuogee Hsieh > > > > I guess this is a fix for the original driver, so it should be tagged > > with Fixes appropriately. > Actually, this is fix on patch #6 : drm/msm/dp: do not end dp link > training until video is ready > Should i merge patch #6 and #7 together? > Or can you suggest what should I do? Yes if it fixes the patch before this one it should be combined. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/7] drm/msm/dp: reduce link rate if failed at link training 1
Quoting khs...@codeaurora.org (2021-07-09 10:46:41)
> On 2021-07-08 00:33, Stephen Boyd wrote:
> >> +
> >> +static bool dp_ctrl_any_lane_cr_lose(struct dp_ctrl_private *ctrl,
> >> + u8 *cr_status)
> >> +{
> >> + int i;
> >> + u8 status;
> >> + int lane = ctrl->link->link_params.num_lanes;
> >> +
> >> + for (i = 0; i < lane; i++) {
> >> + status = cr_status[i / 2];
> >> + status >>= ((i % 2) * 4);
> >> + if (!(status & DP_LANE_CR_DONE))
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >
> > Why not use !drm_dp_clock_recovery_ok() for dp_ctrl_any_lane_cr_lose()?
> ok,
>
> > And then move dp_ctrl_any_lane_cr_done() next to
> > drm_dp_clock_recovery_ok() and call it drm_dp_clock_recovery_any_ok()?
> no understand how it work, can you elaborate it more?
I'm suggesting to make a new function called
drm_dp_clock_recovery_any_ok(), written next to
drm_dp_clock_recovery_ok(). Then call it from here instead of implement
it locally in the qcom DP driver.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dp: Remove unused variable
Quoting Souptick Joarder (2021-07-08 19:48:34) > Kernel test roobot throws below warning -> > > drivers/gpu/drm/msm/dp/dp_display.c:1017:21: > warning: variable 'drm' set but not used [-Wunused-but-set-variable] > > Removed unused variable drm. > > Reported-by: kernel test robot > Signed-off-by: Souptick Joarder > --- Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitreye (2021-07-08 12:13:44)
> From: Maitreyee Rao
>
> Add trace points across the MSM DP driver to help debug
> interop issues.
>
> Changes in v2:
> - Got rid of redundant log messages.
> - Added %#x instead of 0x%x wherever required.
> - Got rid of __func__ calls in debug messages.
> - Added newline wherever missing.
I think this is the new thing in v3? Adding one missing newline?
>
> Signed-off-by: Maitreyee Rao
> ---
> drivers/gpu/drm/msm/dp/dp_catalog.c | 8 ++--
> drivers/gpu/drm/msm/dp/dp_ctrl.c| 5 -
> drivers/gpu/drm/msm/dp/dp_display.c | 14 ++
> drivers/gpu/drm/msm/dp/dp_link.c| 17 ++---
> drivers/gpu/drm/msm/dp/dp_panel.c | 2 ++
> drivers/gpu/drm/msm/dp/dp_power.c | 3 +++
> 6 files changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 32f3575..292ec2c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -372,6 +372,7 @@ void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog
> *dp_catalog,
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
>
> + DRM_DEBUG_DP("enable=%d\n", enable);
> if (enable) {
> /*
> * To make sure link reg writes happens before other
> operation,
> @@ -580,6 +581,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog
> *dp_catalog,
>
> config = (en ? config | intr_mask : config & ~intr_mask);
>
> + DRM_DEBUG_DP("intr_mask=%#x config=%#x\n", intr_mask, config);
> dp_write_aux(catalog, REG_DP_DP_HPD_INT_MASK,
> config & DP_DP_HPD_INT_MASK);
> }
> @@ -610,6 +612,7 @@ u32 dp_catalog_link_is_connected(struct dp_catalog
> *dp_catalog)
> u32 status;
>
> status = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> + DRM_DEBUG_DP("aux status:%#x\n", status);
Unstick colon from printf specifier?
> status >>= DP_DP_HPD_STATE_STATUS_BITS_SHIFT;
> status &= DP_DP_HPD_STATE_STATUS_BITS_MASK;
>
> @@ -685,6 +688,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog
> *dp_catalog,
> /* Make sure to clear the current pattern before starting a new one */
> dp_write_link(catalog, REG_DP_STATE_CTRL, 0x0);
>
> + DRM_DEBUG_DP("pattern:%#x\n", pattern);
Unstick colon from printf specifier?
> switch (pattern) {
> case DP_PHY_TEST_PATTERN_D10_2:
> dp_write_link(catalog, REG_DP_STATE_CTRL,
> @@ -745,7 +749,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog
> *dp_catalog,
> DP_STATE_CTRL_LINK_TRAINING_PATTERN4);
> break;
> default:
> - DRM_DEBUG_DP("No valid test pattern requested:0x%x\n",
> pattern);
> + DRM_DEBUG_DP("No valid test pattern requested:%#x\n",
> pattern);
Unstick colon from printf specifier?
> break;
> }
> }
> @@ -928,7 +932,7 @@ void dp_catalog_audio_config_acr(struct dp_catalog
> *dp_catalog)
> select = dp_catalog->audio_data;
> acr_ctrl = select << 4 | BIT(31) | BIT(8) | BIT(14);
>
> - DRM_DEBUG_DP("select = 0x%x, acr_ctrl = 0x%x\n", select, acr_ctrl);
> + DRM_DEBUG_DP("select =0x%x, acr_ctrl =0x%x\n", select, acr_ctrl);
This doesn't use %#x? And then it sticks it to the equals sign but
doesn't move select and acr_ctrl to them?
>
> dp_write_link(catalog, MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl);
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 2a8955c..21ad7d3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -122,7 +122,7 @@ void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl)
> IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES))
> pr_warn("PUSH_IDLE pattern timedout\n");
>
> - pr_debug("mainlink off done\n");
> + DRM_DEBUG_DP("PUSH IDLE, mainlink off done\n");
I think we already get the func name as dp_ctrl_push_idle in the print
so is having PUSH IDLE in all caps really useful?
> }
>
> static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
> @@ -1013,6 +1013,8 @@ static int dp_ctrl_update_vx_px(struct dp_ctrl_private
> *ctrl)
> u32 voltage_swing_level = link->phy_params.v_level;
> u32 pre_emphasis_level = link->phy_params.p_level;
>
> + DRM_DEBUG_DP("voltage level: %d emphasis level: %d\n",
> voltage_swing_level,
> + pre_emphasis_level);
> ret = dp_catalog_ctrl_update_vx_px(ctrl->catalog,
> voltage_swing_level, pre_emphasis_level);
>
> @@ -1384,6 +1386,7 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool
> flip, bool reset)
> if (reset)
> dp_catalog_ctrl_reset(ctrl->catalog);
>
> +
Re: [Freedreno] [PATCH 4/7] drm/msm/dp: replug event is converted into an unplug followed by an plug events
Quoting Kuogee Hsieh (2021-07-06 10:20:17) > Remove special handling of replug interrupt and instead treat replug event > as a sequential unplug followed by a plugin event. This is needed to meet > the requirements of DP Link Layer CTS test case 4.2.1.3. > > Signed-off-by: Kuogee Hsieh > --- This needs a Fixes tag of some kind. Reviewed-by: Stephen Boyd ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 3/7] drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed.
Quoting Kuogee Hsieh (2021-07-06 10:20:16)
> Aux hardware calibration sequence requires resetting the aux controller
> in order for the new setting to take effect. However resetting the AUX
> controller will also clear HPD interrupt status which may accidentally
> cause pending unplug interrupt to get lost. Therefore reset aux
> controller only when link is in connection state when dp_aux_cmd_fifo_tx()
> fail. This fixes Link Layer CTS cases 4.2.1.1 and 4.2.1.2.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 4a3293b..eb40d84 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -353,6 +353,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> dp_catalog_aux_update_cfg(aux->catalog);
> }
> + /* reset aux if link is in connected state */
> + if (dp_catalog_link_is_connected(aux->catalog))
How do we avoid resetting aux when hpd is unplugged and then plugged
back in during an aux transfer?
> + dp_catalog_aux_reset(aux->catalog);
> } else {
> aux->retry_cnt = 0;
> switch (aux->aux_error_num) {
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/7] drm/msm/dp: reduce link rate if failed at link training 1
Quoting Kuogee Hsieh (2021-07-06 10:20:15)
> Reduce link rate and re start link training if link training 1
> failed due to loss of clock recovery done to fix Link Layer
> CTS case 4.3.1.7. Also only update voltage and pre-emphasis
> swing level after link training started to fix Link Layer CTS
> case 4.3.1.6.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 86
> ++--
> 1 file changed, 56 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 27fb0f0..6f8443d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -83,13 +83,6 @@ struct dp_ctrl_private {
> struct completion video_comp;
> };
>
> -struct dp_cr_status {
> - u8 lane_0_1;
> - u8 lane_2_3;
> -};
> -
> -#define DP_LANE0_1_CR_DONE 0x11
> -
> static int dp_aux_link_configure(struct drm_dp_aux *aux,
> struct dp_link_info *link)
> {
> @@ -1080,7 +1073,7 @@ static int dp_ctrl_read_link_status(struct
> dp_ctrl_private *ctrl,
> }
>
> static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
> - struct dp_cr_status *cr, int *training_step)
> + u8 *cr, int *training_step)
> {
> int tries, old_v_level, ret = 0;
> u8 link_status[DP_LINK_STATUS_SIZE];
> @@ -1109,8 +1102,8 @@ static int dp_ctrl_link_train_1(struct dp_ctrl_private
> *ctrl,
> if (ret)
> return ret;
>
> - cr->lane_0_1 = link_status[0];
> - cr->lane_2_3 = link_status[1];
> + cr[0] = link_status[0];
> + cr[1] = link_status[1];
>
> if (drm_dp_clock_recovery_ok(link_status,
> ctrl->link->link_params.num_lanes)) {
> @@ -1188,7 +1181,7 @@ static void dp_ctrl_clear_training_pattern(struct
> dp_ctrl_private *ctrl)
> }
>
> static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
> - struct dp_cr_status *cr, int *training_step)
> + u8 *cr, int *training_step)
> {
> int tries = 0, ret = 0;
> char pattern;
> @@ -1204,10 +1197,6 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private
> *ctrl,
> else
> pattern = DP_TRAINING_PATTERN_2;
>
> - ret = dp_ctrl_update_vx_px(ctrl);
> - if (ret)
> - return ret;
> -
> ret = dp_catalog_ctrl_set_pattern(ctrl->catalog, pattern);
> if (ret)
> return ret;
> @@ -1220,8 +1209,8 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private
> *ctrl,
> ret = dp_ctrl_read_link_status(ctrl, link_status);
> if (ret)
> return ret;
> - cr->lane_0_1 = link_status[0];
> - cr->lane_2_3 = link_status[1];
> + cr[0] = link_status[0];
> + cr[1] = link_status[1];
>
> if (drm_dp_channel_eq_ok(link_status,
> ctrl->link->link_params.num_lanes)) {
> @@ -1241,7 +1230,7 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private
> *ctrl,
> static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl);
>
> static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
> - struct dp_cr_status *cr, int *training_step)
> + u8 *cr, int *training_step)
> {
> int ret = 0;
> u8 encoding = DP_SET_ANSI_8B10B;
> @@ -1282,7 +1271,7 @@ static int dp_ctrl_link_train(struct dp_ctrl_private
> *ctrl,
> }
>
> static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
> - struct dp_cr_status *cr, int *training_step)
> + u8 *cr, int *training_step)
> {
> int ret = 0;
>
> @@ -1496,14 +1485,14 @@ static int dp_ctrl_deinitialize_mainlink(struct
> dp_ctrl_private *ctrl)
> static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
> {
> int ret = 0;
> - struct dp_cr_status cr;
> + u8 cr_status[2];
> int training_step = DP_TRAINING_NONE;
>
> dp_ctrl_push_idle(>dp_ctrl);
>
> ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>
> - ret = dp_ctrl_setup_main_link(ctrl, , _step);
> + ret = dp_ctrl_setup_main_link(ctrl, cr_status, _step);
> if (ret)
> goto end;
Do we need to extract the link status information from deep in these
functions? Why not read it again when we need to?
>
> @@ -1634,6 +1623,41 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl
> *dp_ctrl)
> }
> }
>
> +static bool dp_ctrl_any_lane_cr_done(struct dp_ctrl_private *ctrl,
> + u8 *cr_status)
> +
> +{
> + int i;
> + u8 status;
> + int lane = ctrl->link->link_params.num_lanes;
> +
> + for (i = 0; i < lane; i++) {
> + status = cr_status[i / 2];
> + status >>=
Re: [Freedreno] [PATCH 7/7] drm/msm/dp: retrain link when loss of symbol lock detected
Quoting Kuogee Hsieh (2021-07-06 10:20:20)
> Main link symbol locked is achieved at end of link training 2. Some
> dongle main link symbol may become unlocked again if host did not end
> link training soon enough after completion of link training 2. Host
> have to re train main link if loss of symbol lock detected before
> end link training so that the coming video stream can be transmitted
> to sink properly.
>
> Signed-off-by: Kuogee Hsieh
I guess this is a fix for the original driver, so it should be tagged
with Fixes appropriately.
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 0cb01a9..e616ab2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1661,6 +1661,25 @@ static bool dp_ctrl_any_lane_cr_lose(struct
> dp_ctrl_private *ctrl,
> return false;
> }
>
> +static bool dp_ctrl_loss_symbol_lock(struct dp_ctrl_private *ctrl)
> +{
> + u8 link_status[6];
Can we use link_status[DP_LINK_STATUS_SIZE] instead?
> + u8 status;
> + int i;
> + int lane = ctrl->link->link_params.num_lanes;
s/lane/num_lanes/
would make the code easier to read
> +
> + dp_ctrl_read_link_status(ctrl, link_status);
> +
> + for (i = 0; i < lane; i++) {
> + status = link_status[i / 2];
> + status >>= ((i % 2) * 4);
> + if (!(status & DP_LANE_SYMBOL_LOCKED))
> + return true;
> + }
> +
> + return false;
> +}
> +
> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> {
> int rc = 0;
> @@ -1777,6 +1796,17 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> return rc;
> }
>
> +static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
> +{
> + int ret = 0;
Please drop init of ret.
> + u8 cr_status[2];
> + int training_step = DP_TRAINING_NONE;
> +
> + ret = dp_ctrl_setup_main_link(ctrl, cr_status, _step);
as it is assigned here.
> +
> + return ret;
And indeed, it could be 'return dp_ctrl_setup_main_link()' instead.
> +}
> +
> int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> {
> int ret = 0;
> @@ -1802,6 +1832,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> }
> }
>
> + /* if loss symbol lock happen, then retaining the link */
retain or retrain? The comment seems to be saying what the code says "if
loss retrain", so the comment is not very useful.
> + if (dp_ctrl_loss_symbol_lock(ctrl))
> + dp_ctrl_link_retrain(ctrl);
> +
> /* stop txing train pattern to end link training */
> dp_ctrl_clear_training_pattern(ctrl);
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 5/7] drm/msm/dp: return correct edid checksum after corrupted edid checksum read
Quoting Kuogee Hsieh (2021-07-06 10:20:18)
> Response with correct edid checksum saved at connector after corrupted edid
> checksum read. This fixes Link Layer CTS cases 4.2.2.3, 4.2.2.6.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_panel.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 88196f7..0fdb551 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -271,7 +271,7 @@ static u8 dp_panel_get_edid_checksum(struct edid *edid)
> {
> struct edid *last_block;
> u8 *raw_edid;
> - bool is_edid_corrupt;
> + bool is_edid_corrupt = false;
>
> if (!edid) {
> DRM_ERROR("invalid edid input\n");
> @@ -303,7 +303,12 @@ void dp_panel_handle_sink_request(struct dp_panel
> *dp_panel)
> panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>
> if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> - u8 checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> + u8 checksum;
> +
> + if (dp_panel->edid)
> + checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> + else
> + checksum = dp_panel->connector->real_edid_checksum;
>
> dp_link_send_edid_checksum(panel->link, checksum);
It looks like this can be drm_dp_send_real_edid_checksum()? Then we
don't have to look at the connector internals sometimes and can drop
dp_panel_get_edid_checksum() entirely?
> dp_link_send_test_response(panel->link);
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/7] drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run
Quoting Kuogee Hsieh (2021-07-06 10:20:14)
> DP cable should always connect to DPU during the entire PHY compliance
> testing run. Since DP PHY compliance test is executed at irq_hpd event
> context, dp_ctrl_off_link_stream() should be used instead of dp_ctrl_off().
> dp_ctrl_off() is used for unplug event which is triggered when DP cable is
> dis connected.
>
> Signed-off-by: Kuogee Hsieh
> ---
Is this
Fixes: f21c8a276c2d ("drm/msm/dp: handle irq_hpd with sink_count = 0 correctly")
or
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
? It's not clear how dp_ctrl_off() was working for compliance tests
before commit f21c8a276c2d.
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index caf71fa..27fb0f0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1530,7 +1530,7 @@ static int dp_ctrl_process_phy_test_request(struct
> dp_ctrl_private *ctrl)
> * running. Add the global reset just before disabling the
> * link clocks and core clocks.
> */
> - ret = dp_ctrl_off(>dp_ctrl);
> + ret = dp_ctrl_off_link_stream(>dp_ctrl);
> if (ret) {
> DRM_ERROR("failed to disable DP controller\n");
> return ret;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm/dpu: Add newlines to printks
Add some missing newlines to the various DRM printks in this file.
Noticed while looking at logs. While we're here unbreak quoted
strings so grepping them is easier.
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1c04b7cce43e..0e9d3fa1544b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -274,20 +274,20 @@ int dpu_encoder_helper_wait_for_irq(struct
dpu_encoder_phys *phys_enc,
/* return EWOULDBLOCK since we know the wait isn't necessary */
if (phys_enc->enable_state == DPU_ENC_DISABLED) {
- DRM_ERROR("encoder is disabled id=%u, intr=%d, irq=%d",
+ DRM_ERROR("encoder is disabled id=%u, intr=%d, irq=%d\n",
DRMID(phys_enc->parent), intr_idx,
irq->irq_idx);
return -EWOULDBLOCK;
}
if (irq->irq_idx < 0) {
- DRM_DEBUG_KMS("skip irq wait id=%u, intr=%d, irq=%s",
+ DRM_DEBUG_KMS("skip irq wait id=%u, intr=%d, irq=%s\n",
DRMID(phys_enc->parent), intr_idx,
irq->name);
return 0;
}
- DRM_DEBUG_KMS("id=%u, intr=%d, irq=%d, pp=%d, pending_cnt=%d",
+ DRM_DEBUG_KMS("id=%u, intr=%d, irq=%d, pp=%d, pending_cnt=%d\n",
DRMID(phys_enc->parent), intr_idx,
irq->irq_idx, phys_enc->hw_pp->idx - PINGPONG_0,
atomic_read(wait_info->atomic_cnt));
@@ -303,8 +303,7 @@ int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys
*phys_enc,
if (irq_status) {
unsigned long flags;
- DRM_DEBUG_KMS("irq not triggered id=%u, intr=%d, "
- "irq=%d, pp=%d, atomic_cnt=%d",
+ DRM_DEBUG_KMS("irq not triggered id=%u, intr=%d,
irq=%d, pp=%d, atomic_cnt=%d\n",
DRMID(phys_enc->parent), intr_idx,
irq->irq_idx,
phys_enc->hw_pp->idx - PINGPONG_0,
@@ -315,8 +314,7 @@ int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys
*phys_enc,
ret = 0;
} else {
ret = -ETIMEDOUT;
- DRM_DEBUG_KMS("irq timeout id=%u, intr=%d, "
- "irq=%d, pp=%d, atomic_cnt=%d",
+ DRM_DEBUG_KMS("irq timeout id=%u, intr=%d, irq=%d,
pp=%d, atomic_cnt=%d\n",
DRMID(phys_enc->parent), intr_idx,
irq->irq_idx,
phys_enc->hw_pp->idx - PINGPONG_0,
base-commit: e9f1cbc0c4114880090c7a578117d3b9cf184ad4
--
https://chromeos.dev
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v4] arm64: dts: qcom: sc7180: Add DisplayPort node
Quoting Bjorn Andersson (2021-06-25 09:13:45)
> On Thu 03 Jun 17:22 CDT 2021, Kuogee Hsieh wrote:
> > + dp_out: endpoint { };
> > + };
> > + };
> > +
> > + dp_opp_table: dp-opp-table {
>
> I forgot that our discussion about the node name here was on the
> previous revision, _this_ is the patch I will drop the "dp-" from and
> apply.
>
> And as I've looked at this quite a bit now:
>
> Reviewed-by: Bjorn Andersson
>
With that node name fixed
Reviewed-by: Stephen Boyd
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/dp_mst: Fix return code on sideband message failure
+Lyude, author of fixed commit. Please add relevant folks in the future.
Quoting Kuogee Hsieh (2021-06-29 13:08:56)
> From: Rajkumar Subbiah
>
> The commit 2f015ec6eab69301fdcf54d397810d72362d7223 added some debug
Please write
Commit 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
selftests") added some debug
> code for sideband message tracing. But it seems to have unintentionally
> changed the behavior on sideband message failure. It catches and returns
> failure only if DRM_UT_DP is enabled. Otherwise it ignores the error code
> and returns success. So on an MST unplug, the caller is unaware that the
> clear payload message failed and ends up waiting for 4 seconds for the
> response.
>
> This change fixes the issue by returning the proper error code.
$ git grep "This patch" -- Documentation/process
>
> Change-Id: I2887b7ca21355fe84a7968f7619d5e8199cbb0c6
Please replace with
Fixes: 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
selftests")
> Signed-off-by: Rajkumar Subbiah
Should be a Signed-off-by from Kuogee Hsieh here.
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 1590144..8d97430 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2887,11 +2887,13 @@ static int process_single_tx_qlock(struct
> drm_dp_mst_topology_mgr *mgr,
> idx += tosend + 1;
>
> ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx);
> - if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) {
> - struct drm_printer p = drm_debug_printer(DBG_PREFIX);
> + if (unlikely(ret)) {
> + if (drm_debug_enabled(DRM_UT_DP)) {
> + struct drm_printer p = drm_debug_printer(DBG_PREFIX);
>
> - drm_printf(, "sideband msg failed to send\n");
> - drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> + drm_printf(, "sideband msg failed to send\n");
> + drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> + }
> return ret;
> }
>
With the above fixed up
Reviewed-by: Stephen Boyd
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting Bjorn Andersson (2021-06-18 14:41:50) > On Fri 18 Jun 15:49 CDT 2021, Stephen Boyd wrote: > > > Quoting khs...@codeaurora.org (2021-06-10 09:54:05) > > > On 2021-06-08 16:10, Bjorn Andersson wrote: > > > > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote: > > > > > > > >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for > > > >> digital logic. Probably the PLL is the hardware that has some minimum > > > >> CX > > > >> requirement, and that flows down into the various display clks like > > > >> the > > > >> link clk that actually clock the DP controller hardware. The mdss_gdsc > > > >> probably gates CX for the display subsystem (mdss) so if we had proper > > > >> corner aggregation logic we could indicate that mdss_gdsc is a child > > > >> of > > > >> the CX domain and then make requests from the DP driver for particular > > > >> link frequencies on the mdss_gdsc and then have that bubble up to CX > > > >> appropriately. I don't think any of that sort of code is in place > > > >> though, right? > > > > > > > > I haven't checked sc7180, but I'm guessing that it's following the > > > > other > > > > modern platforms, where all the MDSS related pieces (including e.g. > > > > dispcc) lives in the MMCX domain, which is separate from CX. > > > > > > > > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and > > > > the dp-opp-table) tells us that the PLL lives in the CX domain. > > > > Isn't MMCX a "child" of CX? At least my understanding is that MMCX is > > basically a GDSC that clamps all of multimedia hardware block power > > logic so that the leakage is minimized when multimedia isn't in use, > > i.e. the device is suspended. In terms of bumping up the voltage we have > > to pin that on CX though as far as I know because that's the only power > > domain that can actually change voltage, while MMCX merely gates that > > voltage for multimedia. > > > > No, MMCX is a separate rail from CX, which powers the display blocks and > is parent of MDSS_GDSC. But I see in rpmhpd that sc7180 is not one of > these platforms, so I presume this means that the displayport controller > thereby sits in MDSS_GDSC parented by CX. > > But in line with what you're saying, the naming of the supplies to the > QMP indicates that the power for the PLLs is static. As such the only > moving things would be the clock rates in the DP controller and as such > that's what needs to scale the voltage. > > So if the resources we're scaling is the clocks in the DP controller > then the gist of the patch is correct. The only details I see is that > the DP controller actually sits in MDSS_GDSC - while it should control > the level of its parent (CX). Not sure if we can describe that in a > simple way. Right. I'm not sure things could be described any better right now. If we need to change this to be MDSS_GDSC power domain and control the level of the parent then I suppose we'll have to make some sort of DT change and pair that with a driver change. Maybe if that happens we can just pick a new compatible and leave the old code in place. Are you happy enough with this current patch? > > > PS. Why does the node name of the opp-table have to be globally unique? Presumably the opp table node name can be 'opp-table' as long as it lives under the node that's using it. If the opp table is at / or /soc then it will need to be unique. I'd prefer just 'opp-table' if possible. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dsi: Stash away calculated vco frequency on recalc
Quoting Dmitry Baryshkov (2021-06-09 09:03:14)
> On 09/06/2021 01:11, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2021-06-08 14:41:21)
> >> Hi Stephen,
> >>
> >> On 08/06/2021 22:55, Stephen Boyd wrote:
> >>> A problem was reported on CoachZ devices where the display wouldn't come
> >>> up, or it would be distorted. It turns out that the PLL code here wasn't
> >>> getting called once dsi_pll_10nm_vco_recalc_rate() started returning the
> >>> same exact frequency, down to the Hz, that the bootloader was setting
> >>> instead of 0 when the clk was registered with the clk framework.
> >>>
> >>> After commit 001d8dc33875 ("drm/msm/dsi: remove temp data from global
> >>> pll structure") we use a hardcoded value for the parent clk frequency,
> >>> i.e. VCO_REF_CLK_RATE, and we also hardcode the value for FRAC_BITS,
> >>> instead of getting it from the config structure. This combination of
> >>> changes to the recalc function allows us to properly calculate the
> >>> frequency of the PLL regardless of whether or not the PLL has been
> >>> clk_prepare()d or clk_set_rate()d. That's a good improvement.
> >>>
> >>> Unfortunately, this means that now we won't call down into the PLL clk
> >>> driver when we call clk_set_rate() because the frequency calculated in
> >>> the framework matches the frequency that is set in hardware. If the rate
> >>> is the same as what we want it should be OK to not call the set_rate PLL
> >>> op. The real problem is that the prepare op in this driver uses a
> >>> private struct member to stash away the vco frequency so that it can
> >>> call the set_rate op directly during prepare. Once the set_rate op is
> >>> never called because recalc_rate told us the rate is the same, we don't
> >>> set this private struct member before the prepare op runs, so we try to
> >>> call the set_rate function directly with a frequency of 0. This
> >>> effectively kills the PLL and configures it for a rate that won't work.
> >>> Calling set_rate from prepare is really quite bad and will confuse any
> >>> downstream clks about what the rate actually is of their parent. Fixing
> >>> that will be a rather large change though so we leave that to later.
> >>>
> >>> For now, let's stash away the rate we calculate during recalc so that
> >>> the prepare op knows what frequency to set, instead of 0. This way
> >>> things keep working and the display can enable the PLL properly. In the
> >>> future, we should remove that code from the prepare op so that it
> >>> doesn't even try to call the set rate function.
> >>>
> >>> Cc: Dmitry Baryshkov
> >>> Cc: Abhinav Kumar
> >>> Fixes: 001d8dc33875 ("drm/msm/dsi: remove temp data from global pll
> >>> structure")
> >>> Signed-off-by: Stephen Boyd
> >>
> >> Thank you for the lengthy explanation. May I suggest another solution:
> >>- Apply
> >> https://lore.kernel.org/linux-arm-msm/010101750064e17e-3db0087e-fc37-494d-aac9-2c2b9b0a7c5b-000...@us-west-2.amazonses.com/
> >>
> >>- And make save_state for 7nm and 10nm cache vco freq (like 14nm does).
> >>
> >> What do you think?
> >>
> >
> > Maybe that can be done for the next merge window? I'd like to get the
> > smallest possible patch in as a fix for this cycle given that the Fixes
> > tag is a recent regression introduced during the most recent merge
> > window.
> >
> > I honestly have no idea what's going on with the clk driver in these
> > files but from the clk framework perspective there are bigger problems
> > than caching the vco freq properly. As I stated in the commit text
> > above, calling set_rate from prepare is plain bad. That should stop.
>
> Could you please spend few more words, on why calling the clock's
> set_rate() callback from the same clock's prepare callback is bad? I
> don't see how this would affect downstream clocks (as we do not change
> the frequency, we just set the registers).
The clk framework is caching things and we don't want clk providers to
be calling into the clk framework again from within the clk ops. This
recursion into the framework is why we have a nasty recursive aware lock
in the clk framework that we're never going to get rid of if more and
more code keeps recursing into the framework.
I think you're saying that the code is reusing the set rate clk op
without going through the frame
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting khs...@codeaurora.org (2021-06-10 09:54:05) > On 2021-06-08 16:10, Bjorn Andersson wrote: > > On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote: > > > >> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for > >> digital logic. Probably the PLL is the hardware that has some minimum > >> CX > >> requirement, and that flows down into the various display clks like > >> the > >> link clk that actually clock the DP controller hardware. The mdss_gdsc > >> probably gates CX for the display subsystem (mdss) so if we had proper > >> corner aggregation logic we could indicate that mdss_gdsc is a child > >> of > >> the CX domain and then make requests from the DP driver for particular > >> link frequencies on the mdss_gdsc and then have that bubble up to CX > >> appropriately. I don't think any of that sort of code is in place > >> though, right? > > > > I haven't checked sc7180, but I'm guessing that it's following the > > other > > modern platforms, where all the MDSS related pieces (including e.g. > > dispcc) lives in the MMCX domain, which is separate from CX. > > > > So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and > > the dp-opp-table) tells us that the PLL lives in the CX domain. Isn't MMCX a "child" of CX? At least my understanding is that MMCX is basically a GDSC that clamps all of multimedia hardware block power logic so that the leakage is minimized when multimedia isn't in use, i.e. the device is suspended. In terms of bumping up the voltage we have to pin that on CX though as far as I know because that's the only power domain that can actually change voltage, while MMCX merely gates that voltage for multimedia. > > > > > > PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs) > > seems to live in MMCX. > > > > Regards, > > Bjorn > > Dp link clock rate is sourced from phy/pll (vco). However it is possible > that different link clock rate > are sourced from same vco (phy/pll) rate. Therefore I think CX rail > voltage level is more proper to > be decided base on link clock rate. > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
Quoting maitreye (2021-06-16 18:08:54)
> From: Maitreyee Rao
>
> Add trace points across the MSM DP driver to help debug
> interop issues.
>
> Signed-off-by: Maitreyee Rao
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++--
> drivers/gpu/drm/msm/dp/dp_catalog.c | 4
> drivers/gpu/drm/msm/dp/dp_ctrl.c| 7 +++
> drivers/gpu/drm/msm/dp/dp_display.c | 16
> drivers/gpu/drm/msm/dp/dp_link.c| 20 +---
> drivers/gpu/drm/msm/dp/dp_panel.c | 2 ++
> drivers/gpu/drm/msm/dp/dp_power.c | 3 +++
> 7 files changed, 48 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 4a3293b..5fdff18d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -121,9 +121,10 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private
> *aux,
>
> time_left = wait_for_completion_timeout(>comp,
> msecs_to_jiffies(250));
> - if (!time_left)
> + if (!time_left) {
> + DRM_DEBUG_DP("%s aux timeout error timeout:%lu\n", __func__,
> time_left);
This will always print 0 for "no time left". Is that useful to know? I'd
rather we just drop that. Also, __func__ shouldn't be needed given that
__drm_dbg() uses builtin_return_address(). And then, I believe the DP
aux core code already adds logs on the transfer to indicate how it
failed, so probably this whole line can be dropped.
> return -ETIMEDOUT;
> -
> + }
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 32f3575..5de5dcd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -372,6 +372,7 @@ void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog
> *dp_catalog,
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
>
> + DRM_DEBUG_DP("%s enable=0x%x\n", __func__, enable);
Again, drop __func__. 'enable' is a bool, why is printed in hex format?
> if (enable) {
> /*
> * To make sure link reg writes happens before other
> operation,
> @@ -580,6 +581,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog
> *dp_catalog,
>
> config = (en ? config | intr_mask : config & ~intr_mask);
>
> + DRM_DEBUG_DP("%s intr_mask=0x%x config=0x%x\n", __func__, intr_mask,
> config);
> dp_write_aux(catalog, REG_DP_DP_HPD_INT_MASK,
> config & DP_DP_HPD_INT_MASK);
> }
> @@ -610,6 +612,7 @@ u32 dp_catalog_link_is_connected(struct dp_catalog
> *dp_catalog)
> u32 status;
>
> status = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> + DRM_DEBUG_DP("%s aux status:0x%x\n", __func__, status);
> status >>= DP_DP_HPD_STATE_STATUS_BITS_SHIFT;
> status &= DP_DP_HPD_STATE_STATUS_BITS_MASK;
>
> @@ -685,6 +688,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog
> *dp_catalog,
> /* Make sure to clear the current pattern before starting a new one */
> dp_write_link(catalog, REG_DP_STATE_CTRL, 0x0);
>
> + DRM_DEBUG_DP("%s pattern:0x%x\n", __func__, pattern);
> switch (pattern) {
> case DP_PHY_TEST_PATTERN_D10_2:
> dp_write_link(catalog, REG_DP_STATE_CTRL,
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 2a8955c..7fd1e3f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -99,6 +99,7 @@ static int dp_aux_link_configure(struct drm_dp_aux *aux,
> values[0] = drm_dp_link_rate_to_bw_code(link->rate);
> values[1] = link->num_lanes;
>
> + DRM_DEBUG_DP("%s value0:0x%x value1:0x%x\n", __func__, values[0],
> values[1]);
The drm_dp_dpcd_write() soon after should tell us what this is, so is
this necessary?
> if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
> values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>
> @@ -122,6 +123,7 @@ void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl)
> IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES))
> pr_warn("PUSH_IDLE pattern timedout\n");
>
> + DRM_DEBUG_DP("PUSH IDLE\n");
> pr_debug("mainlink off done\n");
Can these two printks be combined into one DRM_DEBUG_DP()?
> }
>
> @@ -1013,6 +1015,8 @@ static int dp_ctrl_update_vx_px(struct dp_ctrl_private
> *ctrl)
> u32 voltage_swing_level = link->phy_params.v_level;
> u32 pre_emphasis_level = link->phy_params.p_level;
>
> + DRM_DEBUG_DP("%s: voltage level:%d emphasis level:%d\n", __func__,
Can we unstick the colon : from the printk format?
voltage level: %d emphasis level: %d
> + voltage_swing_level, pre_emphasis_level);
> ret =
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting Bjorn Andersson (2021-06-08 15:34:01) > On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-06-08 15:26:23) > > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote: > > > > > > > Quoting Bjorn Andersson (2021-06-07 16:31:47) > > > > > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > > > > > Sorry about the confusion. What I meant is that even though DP > > > > > > controller is > > > > > > in the MDSS_GDSC > > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have > > > > > > a direct > > > > > > impact > > > > > > on the CX voltage corners. Therefore, we need to mention the CX > > > > > > power domain > > > > > > here. And, since > > > > > > we can associate only one OPP table with one device, we picked the > > > > > > DP link > > > > > > clock over other > > > > > > clocks. > > > > > > > > > > Thank you, that's a much more useful answer. > > > > > > > > > > Naturally I would think it would make more sense for the PHY/PLL > > > > > driver > > > > > to ensure that CX is appropriately voted for then, but I think that > > > > > would result in it being the clock driver performing such vote and I'm > > > > > unsure how the opp table for that would look. > > > > > > > > > > @Stephen, what do you say? > > > > > > > > > > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the > > > > clk driver, and probably not from the clk ops, but instead come from the > > > > phy ops via phy_enable() and phy_configure(). > > > > > > > > > > If I understand the logic correctly *_configure_dp_phy() will both > > > configure the vco clock and "request" the clock framework to change the > > > rate. > > > > > > So I presume what you're suggesting is that that would be the place to > > > cast the CX corner vote? > > > > Yes that would be a place to make the CX vote. The problem is then I > > don't know where to drop the vote. Is that when the phy is disabled? > > We do pass qcom_qmp_phy_power_off() and power down the DP part as DP > output is being disabled. So that sounds like a reasonable place to drop > the vote for the lowest performance state. > So then will the corner vote be in place when the PHY isn't actually powered up? That will be bad for power. The phy configure code will need to know if the phy is enabled and then only put in the vote when the phy is enabled, otherwise wait for enable to make the corner vote. Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for digital logic. Probably the PLL is the hardware that has some minimum CX requirement, and that flows down into the various display clks like the link clk that actually clock the DP controller hardware. The mdss_gdsc probably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate that mdss_gdsc is a child of the CX domain and then make requests from the DP driver for particular link frequencies on the mdss_gdsc and then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right? ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting Bjorn Andersson (2021-06-08 15:26:23) > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-06-07 16:31:47) > > > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > Sorry about the confusion. What I meant is that even though DP > > > > controller is > > > > in the MDSS_GDSC > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a > > > > direct > > > > impact > > > > on the CX voltage corners. Therefore, we need to mention the CX power > > > > domain > > > > here. And, since > > > > we can associate only one OPP table with one device, we picked the DP > > > > link > > > > clock over other > > > > clocks. > > > > > > Thank you, that's a much more useful answer. > > > > > > Naturally I would think it would make more sense for the PHY/PLL driver > > > to ensure that CX is appropriately voted for then, but I think that > > > would result in it being the clock driver performing such vote and I'm > > > unsure how the opp table for that would look. > > > > > > @Stephen, what do you say? > > > > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the > > clk driver, and probably not from the clk ops, but instead come from the > > phy ops via phy_enable() and phy_configure(). > > > > If I understand the logic correctly *_configure_dp_phy() will both > configure the vco clock and "request" the clock framework to change the > rate. > > So I presume what you're suggesting is that that would be the place to > cast the CX corner vote? Yes that would be a place to make the CX vote. The problem is then I don't know where to drop the vote. Is that when the phy is disabled? ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting Bjorn Andersson (2021-06-07 16:31:47) > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > On 2021-06-05 22:07, Bjorn Andersson wrote: > > > On Thu 03 Jun 16:56 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > On 2021-06-03 09:53, Bjorn Andersson wrote: > > > > > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote: > > > [..] > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > > > [..] > > > > > > + power-domains = < SC7180_CX>; > > > > > > > > > > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need > > > > > to > > > > > mention CX here in order for the opp framework to apply required-opps > > > > > of CX? > > > > > > > > yes, > > > > > > If you want me, or other maintainers, to spend any time reviewing or > > > applying your patches going forward then you need to actually bother > > > replying properly to the questions asked. > > > > > > Thanks, > > > Bjorn > > > > Sorry about the confusion. What I meant is that even though DP controller is > > in the MDSS_GDSC > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct > > impact > > on the CX voltage corners. Therefore, we need to mention the CX power domain > > here. And, since > > we can associate only one OPP table with one device, we picked the DP link > > clock over other > > clocks. > > Thank you, that's a much more useful answer. > > Naturally I would think it would make more sense for the PHY/PLL driver > to ensure that CX is appropriately voted for then, but I think that > would result in it being the clock driver performing such vote and I'm > unsure how the opp table for that would look. > > @Stephen, what do you say? > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the clk driver, and probably not from the clk ops, but instead come from the phy ops via phy_enable() and phy_configure(). By the way, there's nothing wrong with a clk device doing power domain "stuff", except for that we haven't plumbed it into the clk framework properly and I'm fairly certain our usage of runtime PM in the clk framework today underneath the prepare_lock is getting us into trouble or will get us there soon. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dsi: Stash away calculated vco frequency on recalc
Quoting Dmitry Baryshkov (2021-06-08 14:41:21)
> Hi Stephen,
>
> On 08/06/2021 22:55, Stephen Boyd wrote:
> > A problem was reported on CoachZ devices where the display wouldn't come
> > up, or it would be distorted. It turns out that the PLL code here wasn't
> > getting called once dsi_pll_10nm_vco_recalc_rate() started returning the
> > same exact frequency, down to the Hz, that the bootloader was setting
> > instead of 0 when the clk was registered with the clk framework.
> >
> > After commit 001d8dc33875 ("drm/msm/dsi: remove temp data from global
> > pll structure") we use a hardcoded value for the parent clk frequency,
> > i.e. VCO_REF_CLK_RATE, and we also hardcode the value for FRAC_BITS,
> > instead of getting it from the config structure. This combination of
> > changes to the recalc function allows us to properly calculate the
> > frequency of the PLL regardless of whether or not the PLL has been
> > clk_prepare()d or clk_set_rate()d. That's a good improvement.
> >
> > Unfortunately, this means that now we won't call down into the PLL clk
> > driver when we call clk_set_rate() because the frequency calculated in
> > the framework matches the frequency that is set in hardware. If the rate
> > is the same as what we want it should be OK to not call the set_rate PLL
> > op. The real problem is that the prepare op in this driver uses a
> > private struct member to stash away the vco frequency so that it can
> > call the set_rate op directly during prepare. Once the set_rate op is
> > never called because recalc_rate told us the rate is the same, we don't
> > set this private struct member before the prepare op runs, so we try to
> > call the set_rate function directly with a frequency of 0. This
> > effectively kills the PLL and configures it for a rate that won't work.
> > Calling set_rate from prepare is really quite bad and will confuse any
> > downstream clks about what the rate actually is of their parent. Fixing
> > that will be a rather large change though so we leave that to later.
> >
> > For now, let's stash away the rate we calculate during recalc so that
> > the prepare op knows what frequency to set, instead of 0. This way
> > things keep working and the display can enable the PLL properly. In the
> > future, we should remove that code from the prepare op so that it
> > doesn't even try to call the set rate function.
> >
> > Cc: Dmitry Baryshkov
> > Cc: Abhinav Kumar
> > Fixes: 001d8dc33875 ("drm/msm/dsi: remove temp data from global pll
> > structure")
> > Signed-off-by: Stephen Boyd
>
> Thank you for the lengthy explanation. May I suggest another solution:
> - Apply
> https://lore.kernel.org/linux-arm-msm/010101750064e17e-3db0087e-fc37-494d-aac9-2c2b9b0a7c5b-000...@us-west-2.amazonses.com/
>
> - And make save_state for 7nm and 10nm cache vco freq (like 14nm does).
>
> What do you think?
>
Maybe that can be done for the next merge window? I'd like to get the
smallest possible patch in as a fix for this cycle given that the Fixes
tag is a recent regression introduced during the most recent merge
window.
I honestly have no idea what's going on with the clk driver in these
files but from the clk framework perspective there are bigger problems
than caching the vco freq properly. As I stated in the commit text
above, calling set_rate from prepare is plain bad. That should stop.
>From my quick glance, the patch you mention looks like another
workaround instead of a proper fix. Why would we need to save the
registers at boot and then snap them back into place on enable? Maybe we
shouldn't reset the phy after registering the clks? Instead register the
clks after the phy is reset so recalc_rate can accurately calculate the
frequency. I suppose that would break continuous splash screen though
where you want the PLL to stay running the entire boot? But then
issuing a reset would break that, wouldn't it? As you can see I'm pretty
confused about how this is all supposed to work.
Note: my problem isn't about recovering what boot sets, it's mostly
exposing incorrect usage of the clk framework in this driver because it
relies on this chain of events:
1) recalc rate calculates something different than what is
set via clk_set_rate()
2) clk_set_rate() is called with the different rate
3) clk_prepare() is called to actually enable the PLL and wait for it
to start
If clk_prepare() was called before clk_set_rate(), which is totally
valid, then it should similarly fail and think the rate is 0 and the PLL
won't lock. Does implementing save_state fix that? If so, it seems like
we have two pieces of code working around each other, maybe for
suspend/resume purposes.
I admit this pa
[Freedreno] [PATCH] drm/msm/dsi: Stash away calculated vco frequency on recalc
A problem was reported on CoachZ devices where the display wouldn't come
up, or it would be distorted. It turns out that the PLL code here wasn't
getting called once dsi_pll_10nm_vco_recalc_rate() started returning the
same exact frequency, down to the Hz, that the bootloader was setting
instead of 0 when the clk was registered with the clk framework.
After commit 001d8dc33875 ("drm/msm/dsi: remove temp data from global
pll structure") we use a hardcoded value for the parent clk frequency,
i.e. VCO_REF_CLK_RATE, and we also hardcode the value for FRAC_BITS,
instead of getting it from the config structure. This combination of
changes to the recalc function allows us to properly calculate the
frequency of the PLL regardless of whether or not the PLL has been
clk_prepare()d or clk_set_rate()d. That's a good improvement.
Unfortunately, this means that now we won't call down into the PLL clk
driver when we call clk_set_rate() because the frequency calculated in
the framework matches the frequency that is set in hardware. If the rate
is the same as what we want it should be OK to not call the set_rate PLL
op. The real problem is that the prepare op in this driver uses a
private struct member to stash away the vco frequency so that it can
call the set_rate op directly during prepare. Once the set_rate op is
never called because recalc_rate told us the rate is the same, we don't
set this private struct member before the prepare op runs, so we try to
call the set_rate function directly with a frequency of 0. This
effectively kills the PLL and configures it for a rate that won't work.
Calling set_rate from prepare is really quite bad and will confuse any
downstream clks about what the rate actually is of their parent. Fixing
that will be a rather large change though so we leave that to later.
For now, let's stash away the rate we calculate during recalc so that
the prepare op knows what frequency to set, instead of 0. This way
things keep working and the display can enable the PLL properly. In the
future, we should remove that code from the prepare op so that it
doesn't even try to call the set rate function.
Cc: Dmitry Baryshkov
Cc: Abhinav Kumar
Fixes: 001d8dc33875 ("drm/msm/dsi: remove temp data from global pll structure")
Signed-off-by: Stephen Boyd
---
I didn't update the 14nm file as the caching logic looks different. But
between the 7nm and 10nm files it looks practically the same.
drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 1 +
drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 34bc93548fcf..657778889d35 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -432,6 +432,7 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct
clk_hw *hw,
pll_freq += div_u64(tmp64, multiplier);
vco_rate = pll_freq;
+ pll_10nm->vco_current_rate = vco_rate;
DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
pll_10nm->phy->id, (unsigned long)vco_rate, dec, frac);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index e76ce40a12ab..6f96fbac8282 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -460,6 +460,7 @@ static unsigned long dsi_pll_7nm_vco_recalc_rate(struct
clk_hw *hw,
pll_freq += div_u64(tmp64, multiplier);
vco_rate = pll_freq;
+ pll_7nm->vco_current_rate = vco_rate;
DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
pll_7nm->phy->id, (unsigned long)vco_rate, dec, frac);
base-commit: 8124c8a6b35386f73523d27eacb71b5364a68c4c
--
https://chromeos.dev
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
