Re: [Freedreno] mmotm 2021-10-05-19-53 uploaded (drivers/gpu/drm/msm/hdmi/hdmi_phy.o)
On 10/5/21 7:53 PM, a...@linux-foundation.org wrote: The mm-of-the-moment snapshot 2021-10-05-19-53 has been uploaded to https://www.ozlabs.org/~akpm/mmotm/ mmotm-readme.txt says README for mm-of-the-moment: https://www.ozlabs.org/~akpm/mmotm/ This is a snapshot of my -mm patch queue. Uploaded at random hopefully more than once a week. You will need quilt to apply these patches to the latest Linus release (5.x or 5.x-rcY). The series file is in broken-out.tar.gz and is duplicated in https://ozlabs.org/~akpm/mmotm/series The file broken-out.tar.gz contains two datestamp files: .DATE and .DATE--mm-dd-hh-mm-ss. Both contain the string -mm-dd-hh-mm-ss, followed by the base kernel version against which this patch series is to be applied. This tree is partially included in linux-next. To see which patches are included in linux-next, consult the `series' file. Only the patches within the #NEXT_PATCHES_START/#NEXT_PATCHES_END markers are included in linux-next. A full copy of the full kernel tree with the linux-next and mmotm patches already applied is available through git within an hour of the mmotm release. Individual mmotm releases are tagged. The master branch always points to the latest release, so it's constantly rebasing. on i386: ld: drivers/gpu/drm/msm/hdmi/hdmi_phy.o:(.rodata+0x3f0): undefined reference to `msm_hdmi_phy_8996_cfg' Full randconfig fle is attached. -- ~Randy config-msm-hdmi.gz Description: application/gzip
Re: [Freedreno] [PATCH 02/11] drm/msm/disp/dpu1: Add support for DSC
On 02-08-21, 16:03, abhin...@codeaurora.org wrote:
> On 2021-07-14 23:51, Vinod Koul wrote:
> > Display Stream Compression (DSC) is one of the hw blocks in dpu, so add
> > support by adding hw blocks for DSC
> >
> > Signed-off-by: Vinod Koul
> > ---
> > Changes since RFC:
> > - Drop unused enums
> >
> > drivers/gpu/drm/msm/Makefile | 1 +
> > .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 13 ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 221 ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 77 ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 13 ++
> > 5 files changed, 325 insertions(+)
> > create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> > create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> >
> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index 610d630326bb..fd8fc57f1f58 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -61,6 +61,7 @@ msm-y := \
> > disp/dpu1/dpu_hw_blk.o \
> > disp/dpu1/dpu_hw_catalog.o \
> > disp/dpu1/dpu_hw_ctl.o \
> > + disp/dpu1/dpu_hw_dsc.o \
> > disp/dpu1/dpu_hw_interrupts.o \
> > disp/dpu1/dpu_hw_intf.o \
> > disp/dpu1/dpu_hw_lm.o \
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > index 4dfd8a20ad5c..b8b4dc36880c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > @@ -547,6 +547,16 @@ struct dpu_merge_3d_cfg {
> > const struct dpu_merge_3d_sub_blks *sblk;
> > };
> >
> > +/**
> > + * struct dpu_dsc_cfg - information of DSC blocks
> > + * @id enum identifying this block
> > + * @base register offset of this block
> > + * @features bit mask identifying sub-blocks/features
> > + */
> > +struct dpu_dsc_cfg {
> > + DPU_HW_BLK_INFO;
> > +};
> > +
> > /**
> > * struct dpu_intf_cfg - information of timing engine blocks
> > * @id enum identifying this block
> > @@ -748,6 +758,9 @@ struct dpu_mdss_cfg {
> > u32 merge_3d_count;
> > const struct dpu_merge_3d_cfg *merge_3d;
> >
> > + u32 dsc_count;
> > + struct dpu_dsc_cfg *dsc;
> > +
> > u32 intf_count;
> > const struct dpu_intf_cfg *intf;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> > new file mode 100644
> > index ..e27e67bd42e8
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020, Linaro Limited
> Copyright year needs an update : 2020-2021?
Thanks for spotting, fixed up
--
~Vinod
Re: [Freedreno] [PATCH 01/11] drm/msm/dsi: add support for dsc data
Hi Abhinav,
On 02-08-21, 15:55, abhin...@codeaurora.org wrote:
> > +static int dsi_populate_dsc_params(struct msm_display_dsc_config *dsc)
> > +{
> > + int mux_words_size;
> > + int groups_per_line, groups_total;
> > + int min_rate_buffer_size;
> > + int hrd_delay;
> > + int pre_num_extra_mux_bits, num_extra_mux_bits;
> > + int slice_bits;
> > + int target_bpp_x16;
> > + int data;
> > + int final_value, final_scale;
> > + int i;
> > +
> > + dsc->drm->rc_model_size = 8192;
> > + dsc->drm->first_line_bpg_offset = 12;
> > + dsc->drm->rc_edge_factor = 6;
> > + dsc->drm->rc_tgt_offset_high = 3;
> > + dsc->drm->rc_tgt_offset_low = 3;
> > + dsc->drm->simple_422 = 0;
> > + dsc->drm->convert_rgb = 1;
> > + dsc->drm->vbr_enable = 0;
> > +
> > + /* handle only bpp = bpc = 8 */
> > + for (i = 0; i < DSC_NUM_BUF_RANGES - 1 ; i++)
> > + dsc->drm->rc_buf_thresh[i] = dsi_dsc_rc_buf_thresh[i];
> > +
> > + for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> > + dsc->drm->rc_range_params[i].range_min_qp = min_qp[i];
> > + dsc->drm->rc_range_params[i].range_max_qp = max_qp[i];
> > + dsc->drm->rc_range_params[i].range_bpg_offset = bpg_offset[i];
> > + }
> > +
> > + dsc->drm->initial_offset = 6144; /* Not bpp 12 */
> > + if (dsc->drm->bits_per_pixel != 8)
> > + dsc->drm->initial_offset = 2048;/* bpp = 12 */
> > +
> > + mux_words_size = 48;/* bpc == 8/10 */
> > + if (dsc->drm->bits_per_component == 12)
> > + mux_words_size = 64;
> > +
> > + dsc->drm->initial_xmit_delay = 512;
> > + dsc->drm->initial_scale_value = 32;
> > + dsc->drm->first_line_bpg_offset = 12;
> > + dsc->drm->line_buf_depth = dsc->drm->bits_per_component + 1;
> > +
> > + /* bpc 8 */
> > + dsc->drm->flatness_min_qp = 3;
> > + dsc->drm->flatness_max_qp = 12;
> > + dsc->det_thresh_flatness = 7 + 2 * (dsc->drm->bits_per_component - 8);
> > + dsc->drm->rc_quant_incr_limit0 = 11;
> > + dsc->drm->rc_quant_incr_limit1 = 11;
> > + dsc->drm->mux_word_size = DSC_MUX_WORD_SIZE_8_10_BPC;
> > +
> > + /* FIXME: need to call drm_dsc_compute_rc_parameters() so that rest of
> > +* params are calculated
> > +*/
> > + dsc->slice_last_group_size = 3 - (dsc->drm->slice_width % 3);
> > + groups_per_line = DIV_ROUND_UP(dsc->drm->slice_width, 3);
> > + dsc->drm->slice_chunk_size = dsc->drm->slice_width *
> > dsc->drm->bits_per_pixel / 8;
> > + if ((dsc->drm->slice_width * dsc->drm->bits_per_pixel) % 8)
> > + dsc->drm->slice_chunk_size++;
> > +
> > + /* rbs-min */
> > + min_rate_buffer_size = dsc->drm->rc_model_size -
> > dsc->drm->initial_offset +
> > + dsc->drm->initial_xmit_delay *
> > dsc->drm->bits_per_pixel +
> > + groups_per_line *
> > dsc->drm->first_line_bpg_offset;
> > +
> > + hrd_delay = DIV_ROUND_UP(min_rate_buffer_size,
> > dsc->drm->bits_per_pixel);
> > +
> > + dsc->drm->initial_dec_delay = hrd_delay -
> > dsc->drm->initial_xmit_delay;
> > +
> > + dsc->drm->initial_scale_value = 8 * dsc->drm->rc_model_size /
> > + (dsc->drm->rc_model_size -
> > dsc->drm->initial_offset);
> > +
> > + slice_bits = 8 * dsc->drm->slice_chunk_size * dsc->drm->slice_height;
> > +
> > + groups_total = groups_per_line * dsc->drm->slice_height;
> > +
> > + data = dsc->drm->first_line_bpg_offset * 2048;
> > +
> > + dsc->drm->nfl_bpg_offset = DIV_ROUND_UP(data, (dsc->drm->slice_height
> > - 1));
> > +
> > + pre_num_extra_mux_bits = 3 * (mux_words_size + (4 *
> > dsc->drm->bits_per_component + 4) - 2);
> > +
> > + num_extra_mux_bits = pre_num_extra_mux_bits - (mux_words_size -
> > +((slice_bits - pre_num_extra_mux_bits) %
> > mux_words_size));
> > +
> > + data = 2048 * (dsc->drm->rc_model_size - dsc->drm->initial_offset +
> > num_extra_mux_bits);
> > + dsc->drm->slice_bpg_offset = DIV_ROUND_UP(data, groups_total);
> > +
> > + /* bpp * 16 + 0.5 */
> > + data = dsc->drm->bits_per_pixel * 16;
> > + data *= 2;
> > + data++;
> > + data /= 2;
> > + target_bpp_x16 = data;
> > +
> > + data = (dsc->drm->initial_xmit_delay * target_bpp_x16) / 16;
> > + final_value = dsc->drm->rc_model_size - data + num_extra_mux_bits;
> As we discussed, can you please check why there is an additional + 8 and /16
> in the upstream final_offset calculation?
> If we can eliminate or root-cause the difference in the calculations, either
> this patch can be substantially reduced or
> we will atleast know for future reference what was the delta and can leave a
> comment.
I am checking this as well, I think there is something more, so will
continue to debug on that.
Meanwhile I propose we continue this and then switch once we have
concluded.
> > +/* DSC config */
> > +struct msm_display_dsc_config {
> > + struct drm_dsc_config *drm;
> > + u8 scr_rev;
> Can scr_rev also move into drm_dsc_config? SCR
Re: [Freedreno] [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-10-05 19:37:52)
> On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct
> > > > > platform_device *pdev)
> > > > > if (!dp)
> > > > > return -ENOMEM;
> > > > >
> > > > > - desc = dp_display_get_desc(pdev);
> > > > > + desc = dp_display_get_desc(pdev, >id);
> > > >
> > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > already have INTF_DP macros, which makes me think that it may be better
> > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > structure and then make sure the 'type' member matches a connector
> > > > type number. Otherwise this code is super fragile.
> > > >
> > >
> > > I'm afraid I don't understand what you're proposing. Or which part you
> > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > move around...
> > >
> > > I have N instances of the DP driver that I need to match to N entries
> > > from the platform specific intf array, I need some stable reference
> > > between them. When I started this journey I figured I could rely on the
> > > of_graph between the DPU and the interface controllers, but the values
> > > used there today are just bogus, so that was a no go.
> > >
> > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > come up with an identifier to put in h_tile_instance[0] so that
> > > dpu_encoder_setup_display() can find the relevant INTF.
> > >
> >
> > To make it more concrete we can look at sc7180
> >
> > static const struct dpu_intf_cfg sc7180_intf[] = {
> > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > ^
> > |
> >
> > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > zero, the number after INTF_DP, and that is very relevant. That number
> > needs to match the dp->id. Somewhere we have a match between
> > controller_id and dp->id in the code.
>
> That number (the 0, not INTF_0) is what the code matches against dp->id
> in _dpu_kms_initialize_displayport(), in order to figure out that this
> is INTF_0 in dpu_encoder_setup_display().
>
> I.e. look at the sc8180x patch:
>
> INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK,
> MDP_SSPP_TOP0_INTR, 24, 25),
> INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK,
> MDP_SSPP_TOP0_INTR, 26, 27),
> INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK,
> MDP_SSPP_TOP0_INTR, 28, 29),
> /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is
> supported */
> INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK,
> MDP_SSPP_TOP0_INTR, 30, 31),
> INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK,
> MDP_SSPP_TOP0_INTR, 20, 21),
> INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK,
> MDP_SSPP_TOP0_INTR, 22, 23),
>
> Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
>
Yep. I'm saying that having to make that number in this intf array match
the order of the register mapping descriptor array is fragile. Why can't
we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
map from the descriptor array to this intf array somehow so that the
order of the descriptor array doesn't matter? Then we don't have to put
the connector type in the descriptor array, and we don't have to keep
the order of the array a certain way to match this intf descriptor.
Maybe
struct msm_dp_desc {
phys_addr_t io_start;
unsigned int id;
};
and then have msm_dp_desc::id equal INTF_ and then look through the
intf from DPU here in the DP driver to find the id and type of connector
that should be used by default? Still sort of fragile because the only
connection is an unsigned int which isn't great, but at least it's
explicit instead of implicit based on the array order.
Re: [Freedreno] [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller
On Tue 05 Oct 19:29 CDT 2021, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-05 16:13:19)
> > As the following patches introduced support for multiple DP blocks in a
> > platform and some of those block might be eDP it becomes useful to be
> > able to specify the connector type per block.
> >
> > Although there's only a single block at this point, the array of descs
> > and the search in dp_display_get_desc() are introduced here to simplify
> > the next patch, that does introduce support for multiple DP blocks.
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> >
> > Changes since v3:
> > - New patch
> > - Extended msm_dp_config with connector_type, wrapped in inner struct
> > - Refactored out of the next patch
> > - Pass the connector_type to drm_connector_init(), from yet another patch
> > - Dropped double newline and unnecessary {}
>
> BTW, I see that we check for the connector type in debugfs.
>
> $ git grep DRM_MODE_CONNECTOR_DisplayPort -- drivers/gpu/drm/msm/dp/
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
>
> So do those need to be updated to handle either connector type?
The debugfs code loops over all the connectors for the DRM device and
skips those that aren't DisplayPort ones. So fixing this up to properly
support multiple instances in the dp_debugfs code as well should resolve
this.
Regards,
Bjorn
Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel
On 2021-10-05 19:09, Bjorn Andersson wrote:
On Tue 05 Oct 17:35 PDT 2021, abhin...@codeaurora.org wrote:
Hi Bjorn
On 2021-10-05 16:13, Bjorn Andersson wrote:
> eDP panels might need some power sequencing and backlight management,
> so make it possible to associate a drm_panel with an eDP instance and
> prepare and enable the panel accordingly.
>
> Now that we know which hardware instance is DP and which is eDP,
> parser->parse() is passed the connector_type and the parser is limited
> to only search for a panel in the eDP case.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v3:
> - Previously posted separately, now added to series
> - Make the search for a panel conditional on it being an eDP connector
> - Move the search to of_graph port 1 (was 2 to distinguish from DP
> link to USB-C connector)
>
> drivers/gpu/drm/msm/dp/dp_display.c | 9 ++---
> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 11 +++
> drivers/gpu/drm/msm/dp/dp_parser.c | 30 -
> drivers/gpu/drm/msm/dp/dp_parser.h | 3 ++-
> 5 files changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index eaf08f9e7d87..bdaf227f05dc 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"
> @@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
>priv = drm->dev_private;
>priv->dp = &(dp->dp_display);
>
> - rc = dp->parser->parse(dp->parser);
> + rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
>if (rc) {
>DRM_ERROR("device tree parsing failed\n");
>goto end;
>}
>
> + dp->dp_display.panel_bridge = dp->parser->panel_bridge;
> +
>dp->aux->drm_dev = drm;
>rc = dp_aux_register(dp->aux);
>if (rc) {
> @@ -822,7 +825,7 @@ 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)
> {
>return 0;
> }
> @@ -896,7 +899,7 @@ 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)
> {
>return 0;
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> b/drivers/gpu/drm/msm/dp/dp_display.h
> index 02999408c052..24aefca66029 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_bridge *panel_bridge;
>bool is_connected;
>bool audio_enabled;
>bool power_on;
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> b/drivers/gpu/drm/msm/dp/dp_drm.c
> index f33e31523f56..76856c4ee1d6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -5,6 +5,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "msm_drv.h"
> @@ -160,5 +161,15 @@ struct drm_connector
> *dp_drm_connector_init(struct msm_dp *dp_display)
>
>drm_connector_attach_encoder(connector, dp_display->encoder);
>
> + if (dp_display->panel_bridge) {
> + ret = drm_bridge_attach(dp_display->encoder,
> + dp_display->panel_bridge, NULL,
> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> + if (ret < 0) {
> + DRM_ERROR("failed to attach panel bridge: %d\n", ret);
> + return ERR_PTR(ret);
> + }
> + }
> +
>return connector;
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 4d6e047f803d..eb6bbfbea484 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"
> @@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser
> *parser)
>return 0;
> }
>
> -static int dp_parser_parse(struct dp_parser *parser)
> +static int dp_parser_find_panel(struct dp_parser *parser)
> +{
> + struct device *dev = >pdev->dev;
> + struct drm_panel *panel;
> + int rc;
> +
> + rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
> + if (rc) {
> + DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> + return rc;
> + }
> +
> + parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> + if (IS_ERR(parser->panel_bridge)) {
> + DRM_ERROR("failed to create panel bridge\n");
> + return PTR_ERR(parser->panel_bridge);
> + }
When we add a bridge using devm_drm_panel_bridge_add(), it will
Re: [Freedreno] [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers
On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct
> > > > platform_device *pdev)
> > > > if (!dp)
> > > > return -ENOMEM;
> > > >
> > > > - desc = dp_display_get_desc(pdev);
> > > > + desc = dp_display_get_desc(pdev, >id);
> > >
> > > I'm sad that dp->id has to match the number in the SoC specific
> > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > already have INTF_DP macros, which makes me think that it may be better
> > > to connect this to those arrays instead of making an msm_dp_desc
> > > structure and then make sure the 'type' member matches a connector
> > > type number. Otherwise this code is super fragile.
> > >
> >
> > I'm afraid I don't understand what you're proposing. Or which part you
> > consider fragile, the indices of the INTF_DP instances aren't going to
> > move around...
> >
> > I have N instances of the DP driver that I need to match to N entries
> > from the platform specific intf array, I need some stable reference
> > between them. When I started this journey I figured I could rely on the
> > of_graph between the DPU and the interface controllers, but the values
> > used there today are just bogus, so that was a no go.
> >
> > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > come up with an identifier to put in h_tile_instance[0] so that
> > dpu_encoder_setup_display() can find the relevant INTF.
> >
>
> To make it more concrete we can look at sc7180
>
> static const struct dpu_intf_cfg sc7180_intf[] = {
> INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> ^
> |
>
> intf0 is irrelevant. Also the address is irrelevant. But here we have a
> zero, the number after INTF_DP, and that is very relevant. That number
> needs to match the dp->id. Somewhere we have a match between
> controller_id and dp->id in the code.
That number (the 0, not INTF_0) is what the code matches against dp->id
in _dpu_kms_initialize_displayport(), in order to figure out that this
is INTF_0 in dpu_encoder_setup_display().
I.e. look at the sc8180x patch:
INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK,
MDP_SSPP_TOP0_INTR, 24, 25),
INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK,
MDP_SSPP_TOP0_INTR, 26, 27),
INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK,
MDP_SSPP_TOP0_INTR, 28, 29),
/* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is
supported */
INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK,
MDP_SSPP_TOP0_INTR, 30, 31),
INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK,
MDP_SSPP_TOP0_INTR, 20, 21),
INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK,
MDP_SSPP_TOP0_INTR, 22, 23),
Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
Regards,
Bjorn
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
On Tue 05 Oct 16:09 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 5, 2021 at 10:33 AM Bjorn Andersson
> wrote:
> >
> > On Tue 05 Oct 08:39 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
> > > wrote:
> > > >
> > > > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > > > wrote:
> > > > > >
> > > > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > +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);
> > > > > > >
> > > > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says
> > > > > > > that
> > > > > > > port 1 is "Output endpoint of the controller". We should just use
> > > > > > > port
> > > > > > > 1 here, right?
> > > > > > >
> > > > > >
> > > > > > Finally got back to this, changed it to 1 and figured out why I
> > > > > > left it
> > > > > > at 2.
> > > > > >
> > > > > > drm_of_find_panel_or_bridge() on a DP controller will find the
> > > > > > of_graph
> > > > > > reference to the USB-C controller, scan through the registered
> > > > > > panels
> > > > > > and conclude that the of_node of the USB-C controller isn't a
> > > > > > registered
> > > > > > panel and return -EPROBE_DEFER.
> > > > >
> > > > > I'm confused, but maybe it would help if I could see something
> > > > > concrete. Is there a specific board this was happening on?
> > > > >
> > > >
> > > > Right, let's make this more concrete with a snippet from the actual
> > > > SC8180x DT.
> > > >
> > > > > Under the DP node in the device tree I expect:
> > > > >
> > > > > ports {
> > > > > port@1 {
> > > > > reg = <1>;
> > > > > edp_out: endpoint {
> > > > > remote-endpoint = <_panel_in>;
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > >
> > > > /* We got a panel */
> > > > panel {
> > > > ...
> > > > ports {
> > > > port {
> > > > auo_b133han05_in: endpoint {
> > > > remote-endpoint = <_edp_out>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > /* And a 2-port USB-C controller */
> > > > type-c-controller {
> > > > ...
> > > > connector@0 {
> > > > ports {
> > > > port@0 {
> > > > reg = <0>;
> > > > ucsi_port_0_dp: endpoint {
> > > > remote-endpoint = <_mode>;
> > > > };
> > > > };
> > > >
> > > > port@1 {
> > > > reg = <1>;
> > > > ucsi_port_0_switch: endpoint {
> > > > remote-endpoint = <_qmp_phy>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > connector@1 {
> > > > ports {
> > > > port@0 {
> > > > reg = <0>;
> > > > ucsi_port_1_dp: endpoint {
> > > > remote-endpoint = <_mode>;
> > > > };
> > > > };
> > > >
> > > > port@1 {
> > > > reg = <1>;
> > > > ucsi_port_1_switch: endpoint {
> > > > remote-endpoint = <_qmp_phy>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > /* And then our 2 DP and single eDP controllers */
> > > > _dp0 {
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > dp0_mode: endpoint {
> > > > remote-endpoint = <_port_0_dp>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > _dp1 {
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > dp1_mode: endpoint {
> > > > remote-endpoint = <_port_1_dp>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > _edp {
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > mdss_edp_out: endpoint {
> > > > remote-endpoint = <_b133han05_in>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > > If you have "port@1" pointing to a USB-C controller but this instance
> > > > > of the DP controller is actually hooked up straight to a panel then
> > > > > you should simply delete the "port@1" that points to the typeC and
> > > > > replace it with one that points to a panel, right?
> > > > >
> > > >
> > > > As you can see, port 1 on _dp0 and _dp1 points to the two UCSI
> > > >
Re: [Freedreno] [PATCH] drm/msm/dp: Shorten SETUP timeout
On Tue 05 Oct 16:04 PDT 2021, khs...@codeaurora.org wrote: > On 2021-10-05 15:36, Stephen Boyd wrote: > > Quoting Bjorn Andersson (2021-10-05 14:40:38) > > > On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote: > > > > > > > Quoting Bjorn Andersson (2021-10-04 19:37:50) > > > > > Found in the middle of a patch from Sankeerth was the reduction of the > > > > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host > > > > > is initalized and HPD interrupt start to be serviced, so in the case > > > > > of > > > > > eDP this reduction improves the user experience dramatically - i.e. > > > > > removes 9.9s of bland screen time at boot. > > > > > > > > > > Suggested-by: Sankeerth Billakanti > > > > > Signed-off-by: Bjorn Andersson > > > > > --- > > > > > > > > Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go > > > > re-read the code a couple times to understand that it's waiting 100ms > > > > times the 'delay' number. What? > > > > > > > > > > I assume you're happy with the current 10s delay on the current > > > devices, so I don't think we should push for this to be backported. > > > I have no need for it to be backported on my side at least. > > > > > > > Sure. Fixes tag != backported to stable trees but it is close. > > > > > > Reviewed-by: Stephen Boyd > > > > dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms > > This patch will prevent usb3 from working due to dp driver initialize phy > earlier than usb3 which cause timeout error at power up usb3 phy when both > edp and dp are enabled. Can you please help me understand what you mean here, I use this on my sc8180x with both eDP and USB-C/DP right now. What is it that doesn't work? Or am I just lucky in some race condition? Thanks, Bjorn > I had prepared a patch (drm/msm/dp: do not initialize combo phy until plugin > interrupt) to fix this problem. > Unfortunately, my patch is depend on Bjorn's patch (PATCH v3 3/5] > drm/msm/dp: Support up to 3 DP controllers). > I will submit my patch for review once Bjorn's patches merged in. > Therefore I would think this patch should go after both Bjorn's patches and > my patch. > > >
Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel
On Tue 05 Oct 17:35 PDT 2021, abhin...@codeaurora.org wrote:
> Hi Bjorn
>
> On 2021-10-05 16:13, Bjorn Andersson wrote:
> > eDP panels might need some power sequencing and backlight management,
> > so make it possible to associate a drm_panel with an eDP instance and
> > prepare and enable the panel accordingly.
> >
> > Now that we know which hardware instance is DP and which is eDP,
> > parser->parse() is passed the connector_type and the parser is limited
> > to only search for a panel in the eDP case.
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> >
> > Changes since v3:
> > - Previously posted separately, now added to series
> > - Make the search for a panel conditional on it being an eDP connector
> > - Move the search to of_graph port 1 (was 2 to distinguish from DP
> > link to USB-C connector)
> >
> > drivers/gpu/drm/msm/dp/dp_display.c | 9 ++---
> > drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> > drivers/gpu/drm/msm/dp/dp_drm.c | 11 +++
> > drivers/gpu/drm/msm/dp/dp_parser.c | 30 -
> > drivers/gpu/drm/msm/dp/dp_parser.h | 3 ++-
> > 5 files changed, 49 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index eaf08f9e7d87..bdaf227f05dc 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"
> > @@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev,
> > struct device *master,
> > priv = drm->dev_private;
> > priv->dp = &(dp->dp_display);
> >
> > - rc = dp->parser->parse(dp->parser);
> > + rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
> > if (rc) {
> > DRM_ERROR("device tree parsing failed\n");
> > goto end;
> > }
> >
> > + dp->dp_display.panel_bridge = dp->parser->panel_bridge;
> > +
> > dp->aux->drm_dev = drm;
> > rc = dp_aux_register(dp->aux);
> > if (rc) {
> > @@ -822,7 +825,7 @@ 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)
> > {
> > return 0;
> > }
> > @@ -896,7 +899,7 @@ 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)
> > {
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> > b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 02999408c052..24aefca66029 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_bridge *panel_bridge;
> > bool is_connected;
> > bool audio_enabled;
> > bool power_on;
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> > b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index f33e31523f56..76856c4ee1d6 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -5,6 +5,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "msm_drv.h"
> > @@ -160,5 +161,15 @@ struct drm_connector
> > *dp_drm_connector_init(struct msm_dp *dp_display)
> >
> > drm_connector_attach_encoder(connector, dp_display->encoder);
> >
> > + if (dp_display->panel_bridge) {
> > + ret = drm_bridge_attach(dp_display->encoder,
> > + dp_display->panel_bridge, NULL,
> > + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > + if (ret < 0) {
> > + DRM_ERROR("failed to attach panel bridge: %d\n", ret);
> > + return ERR_PTR(ret);
> > + }
> > + }
> > +
> > return connector;
> > }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> > b/drivers/gpu/drm/msm/dp/dp_parser.c
> > index 4d6e047f803d..eb6bbfbea484 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"
> > @@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser
> > *parser)
> > return 0;
> > }
> >
> > -static int dp_parser_parse(struct dp_parser *parser)
> > +static int dp_parser_find_panel(struct dp_parser *parser)
> > +{
> > + struct device *dev = >pdev->dev;
> > + struct drm_panel *panel;
> > + int rc;
> > +
> > + rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
> > + if (rc) {
> > + DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> > + return
Re: [Freedreno] [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers
On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index bdaf227f05dc..674cddfee5b0 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -79,6 +79,8 @@ struct dp_display_private {
> > char *name;
> > int irq;
> >
> > + unsigned int id;
> > +
> > /* state variables */
> > bool core_initialized;
> > bool hpd_irq_on;
> > @@ -229,7 +231,7 @@ static int dp_display_bind(struct device *dev, struct
> > device *master,
> >
> > dp->dp_display.drm_dev = drm;
> > priv = drm->dev_private;
> > - priv->dp = &(dp->dp_display);
> > + priv->dp[dp->id] = &(dp->dp_display);
>
> Can we drop the extra parenthesis?
>
Definitely.
> >
> > rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
> > if (rc) {
> > @@ -269,7 +271,7 @@ static void dp_display_unbind(struct device *dev,
> > struct device *master,
> >
> > dp_power_client_deinit(dp->power);
> > dp_aux_unregister(dp->aux);
> > - priv->dp = NULL;
> > + priv->dp[dp->id] = NULL;
> > }
> >
> > static const struct component_ops dp_display_comp_ops = {
> > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device
> > *pdev)
> > if (!dp)
> > return -ENOMEM;
> >
> > - desc = dp_display_get_desc(pdev);
> > + desc = dp_display_get_desc(pdev, >id);
>
> I'm sad that dp->id has to match the number in the SoC specific
> dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> still. Is there any way we can avoid that? Also, notice how those arrays
> already have INTF_DP macros, which makes me think that it may be better
> to connect this to those arrays instead of making an msm_dp_desc
> structure and then make sure the 'type' member matches a connector
> type number. Otherwise this code is super fragile.
>
I'm afraid I don't understand what you're proposing. Or which part you
consider fragile, the indices of the INTF_DP instances aren't going to
move around...
I have N instances of the DP driver that I need to match to N entries
from the platform specific intf array, I need some stable reference
between them. When I started this journey I figured I could rely on the
of_graph between the DPU and the interface controllers, but the values
used there today are just bogus, so that was a no go.
We can use whatever, as long as _dpu_kms_initialize_displayport() can
come up with an identifier to put in h_tile_instance[0] so that
dpu_encoder_setup_display() can find the relevant INTF.
Regards,
Bjorn
> > if (!desc)
> > return -EINVAL;
> >
Re: [Freedreno] [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
Hi Doug, On Fri, Oct 01, 2021 at 11:02:54AM -0700, Doug Anderson wrote: > On Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart wrote: > > > > > > > err_conn_init: > > > > > drm_dp_aux_unregister(>aux); > > > > > return ret; > > > > > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct > > > > > ti_sn65dsi86 *pdata) > > > > > regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); > > > > > } > > > > > > > > > > +/* > > > > > + * Find the connector and fish out the bpc from display_info. It > > > > > would > > > > > + * be nice if we could get this instead from drm_bridge_state, but > > > > > that > > > > > + * doesn't yet appear to be the case. > > > > > > > > You already have a bus format in the bridge state, from which you can > > > > derive the bpp. Could you give it a try ? > > > > > > Possibly the bridge should be converted to ->atomic_enable(), etc.. > > > I'll leave that for another time > > > > It should be fairly straightforward, and would avoid the hack below. > > Given this point of controversy, my inclination is to wait and not > apply this patch now. I don't think there's anything urgent here, > right? Worst case eventually Laurent might pick it up in his patch > series? At least we know it will work with the MSM driver once patch > #1 lands. :-) I've recorded the task for my upcoming work on the ti-sn65dsi86 driver. -- Regards, Laurent Pinchart
Re: [Freedreno] [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-10-05 16:13:21)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index bdaf227f05dc..674cddfee5b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -79,6 +79,8 @@ struct dp_display_private {
> char *name;
> int irq;
>
> + unsigned int id;
> +
> /* state variables */
> bool core_initialized;
> bool hpd_irq_on;
> @@ -229,7 +231,7 @@ static int dp_display_bind(struct device *dev, struct
> device *master,
>
> dp->dp_display.drm_dev = drm;
> priv = drm->dev_private;
> - priv->dp = &(dp->dp_display);
> + priv->dp[dp->id] = &(dp->dp_display);
Can we drop the extra parenthesis?
>
> rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
> if (rc) {
> @@ -269,7 +271,7 @@ static void dp_display_unbind(struct device *dev, struct
> device *master,
>
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> - priv->dp = NULL;
> + priv->dp[dp->id] = NULL;
> }
>
> static const struct component_ops dp_display_comp_ops = {
> @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device
> *pdev)
> if (!dp)
> return -ENOMEM;
>
> - desc = dp_display_get_desc(pdev);
> + desc = dp_display_get_desc(pdev, >id);
I'm sad that dp->id has to match the number in the SoC specific
dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
still. Is there any way we can avoid that? Also, notice how those arrays
already have INTF_DP macros, which makes me think that it may be better
to connect this to those arrays instead of making an msm_dp_desc
structure and then make sure the 'type' member matches a connector
type number. Otherwise this code is super fragile.
> if (!desc)
> return -EINVAL;
>
Re: [Freedreno] [PATCH v4 7/7] drm/msm/dp: Add sc8180x DP controllers
Quoting Bjorn Andersson (2021-10-05 16:13:23)
> The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
> DP driver.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v3:
> - Rebased upon previous patches in series
>
> drivers/gpu/drm/msm/dp/dp_display.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 674cddfee5b0..29c2c1c52ddb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -135,8 +135,19 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> .num_descs = 1,
> };
>
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> + .descs = (struct msm_dp_desc[]) {
const?
> + { .io_start = 0x0ae9, .connector_type =
> DRM_MODE_CONNECTOR_DisplayPort },
> + { .io_start = 0x0ae98000, .connector_type =
> DRM_MODE_CONNECTOR_DisplayPort },
> + { .io_start = 0x0ae9a000, .connector_type =
> DRM_MODE_CONNECTOR_eDP },
> + },
> + .num_descs = 3,
> +};
> +
> static const struct of_device_id dp_dt_match[] = {
> { .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
> + { .compatible = "qcom,sc8180x-dp", .data = _dp_cfg },
> + { .compatible = "qcom,sc8180x-edp", .data = _dp_cfg },
Otherwise
Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel
Hi Bjorn
On 2021-10-05 16:13, Bjorn Andersson wrote:
eDP panels might need some power sequencing and backlight management,
so make it possible to associate a drm_panel with an eDP instance and
prepare and enable the panel accordingly.
Now that we know which hardware instance is DP and which is eDP,
parser->parse() is passed the connector_type and the parser is limited
to only search for a panel in the eDP case.
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- Previously posted separately, now added to series
- Make the search for a panel conditional on it being an eDP connector
- Move the search to of_graph port 1 (was 2 to distinguish from DP
link to USB-C connector)
drivers/gpu/drm/msm/dp/dp_display.c | 9 ++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 11 +++
drivers/gpu/drm/msm/dp/dp_parser.c | 30 -
drivers/gpu/drm/msm/dp/dp_parser.h | 3 ++-
5 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index eaf08f9e7d87..bdaf227f05dc 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"
@@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev,
struct device *master,
priv = drm->dev_private;
priv->dp = &(dp->dp_display);
- rc = dp->parser->parse(dp->parser);
+ rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
if (rc) {
DRM_ERROR("device tree parsing failed\n");
goto end;
}
+ dp->dp_display.panel_bridge = dp->parser->panel_bridge;
+
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -822,7 +825,7 @@ 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)
{
return 0;
}
@@ -896,7 +899,7 @@ 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)
{
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 02999408c052..24aefca66029 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_bridge *panel_bridge;
bool is_connected;
bool audio_enabled;
bool power_on;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
b/drivers/gpu/drm/msm/dp/dp_drm.c
index f33e31523f56..76856c4ee1d6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include "msm_drv.h"
@@ -160,5 +161,15 @@ struct drm_connector
*dp_drm_connector_init(struct msm_dp *dp_display)
drm_connector_attach_encoder(connector, dp_display->encoder);
+ if (dp_display->panel_bridge) {
+ ret = drm_bridge_attach(dp_display->encoder,
+ dp_display->panel_bridge, NULL,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret < 0) {
+ DRM_ERROR("failed to attach panel bridge: %d\n", ret);
+ return ERR_PTR(ret);
+ }
+ }
+
return connector;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index 4d6e047f803d..eb6bbfbea484 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"
@@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser
*parser)
return 0;
}
-static int dp_parser_parse(struct dp_parser *parser)
+static int dp_parser_find_panel(struct dp_parser *parser)
+{
+ struct device *dev = >pdev->dev;
+ struct drm_panel *panel;
+ int rc;
+
+ rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
+ if (rc) {
+ DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
+ return rc;
+ }
+
+ parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+ if (IS_ERR(parser->panel_bridge)) {
+ DRM_ERROR("failed to create panel bridge\n");
+ return PTR_ERR(parser->panel_bridge);
+ }
When we add a bridge using devm_drm_panel_bridge_add(), it will register
with default bridge functions which is fine
because we need the panel power to be controlled here.
140 static const struct drm_bridge_funcs
Re: [Freedreno] [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller
Quoting Bjorn Andersson (2021-10-05 16:13:19)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d3ee5ef07c2..eaf08f9e7d87 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -115,8 +115,25 @@ struct dp_display_private {
> struct dp_audio *audio;
> };
>
> +struct msm_dp_desc {
> + phys_addr_t io_start;
> + int connector_type;
unsigned?
> +};
> +
Re: [Freedreno] [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller
Quoting Bjorn Andersson (2021-10-05 16:13:19)
> As the following patches introduced support for multiple DP blocks in a
> platform and some of those block might be eDP it becomes useful to be
> able to specify the connector type per block.
>
> Although there's only a single block at this point, the array of descs
> and the search in dp_display_get_desc() are introduced here to simplify
> the next patch, that does introduce support for multiple DP blocks.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v3:
> - New patch
> - Extended msm_dp_config with connector_type, wrapped in inner struct
> - Refactored out of the next patch
> - Pass the connector_type to drm_connector_init(), from yet another patch
> - Dropped double newline and unnecessary {}
BTW, I see that we check for the connector type in debugfs.
$ git grep DRM_MODE_CONNECTOR_DisplayPort -- drivers/gpu/drm/msm/dp/
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)
So do those need to be updated to handle either connector type?
Re: [Freedreno] [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller
Quoting Bjorn Andersson (2021-10-05 16:13:19)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d3ee5ef07c2..eaf08f9e7d87 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -115,8 +115,25 @@ struct dp_display_private {
> struct dp_audio *audio;
> };
>
> +struct msm_dp_desc {
> + phys_addr_t io_start;
> + int connector_type;
> +};
> +
> +struct msm_dp_config {
> + struct msm_dp_desc *descs;
const?
> + size_t num_descs;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> + .descs = (struct msm_dp_desc[]) {
const?
> + { .io_start = 0x0ae9, .connector_type =
> DRM_MODE_CONNECTOR_DisplayPort },
> + },
> + .num_descs = 1,
> +};
> +
> static const struct of_device_id dp_dt_match[] = {
> - {.compatible = "qcom,sc7180-dp"},
> + { .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
> {}
> };
>
> @@ -1180,10 +1197,29 @@ int dp_display_request_irq(struct msm_dp *dp_display)
> return 0;
> }
>
> +static struct msm_dp_desc *dp_display_get_desc(struct platform_device *pdev)
const msm_dp_desc?
> +{
> + 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 NULL;
> +
> + for (i = 0; i < cfg->num_descs; i++)
> + if (cfg->descs[i].io_start == res->start)
> + return >descs[i];
> +
> + dev_err(>dev, "unknown displayport instance\n");
> + return NULL;
> +}
> +
> static int dp_display_probe(struct platform_device *pdev)
> {
> int rc = 0;
> struct dp_display_private *dp;
> + struct msm_dp_desc *desc;
const?
>
> if (!pdev || !pdev->dev.of_node) {
> DRM_ERROR("pdev not found\n");
> @@ -1194,8 +1230,13 @@ static int dp_display_probe(struct platform_device
> *pdev)
> if (!dp)
> return -ENOMEM;
>
> + desc = dp_display_get_desc(pdev);
> + if (!desc)
> + return -EINVAL;
> +
> dp->pdev = pdev;
> dp->name = "drm_dp";
> + dp->dp_display.connector_type = desc->connector_type;
>
> rc = dp_init_sub_modules(dp);
> if (rc) {
Re: [Freedreno] [PATCH] drm/msm/dp: Shorten SETUP timeout
Quoting khs...@codeaurora.org (2021-10-05 16:04:40) > On 2021-10-05 15:36, Stephen Boyd wrote: > > Quoting Bjorn Andersson (2021-10-05 14:40:38) > >> On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote: > >> > >> > Quoting Bjorn Andersson (2021-10-04 19:37:50) > >> > > Found in the middle of a patch from Sankeerth was the reduction of the > >> > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host > >> > > is initalized and HPD interrupt start to be serviced, so in the case of > >> > > eDP this reduction improves the user experience dramatically - i.e. > >> > > removes 9.9s of bland screen time at boot. > >> > > > >> > > Suggested-by: Sankeerth Billakanti > >> > > Signed-off-by: Bjorn Andersson > >> > > --- > >> > > >> > Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go > >> > re-read the code a couple times to understand that it's waiting 100ms > >> > times the 'delay' number. What? > >> > > >> > >> I assume you're happy with the current 10s delay on the current > >> devices, so I don't think we should push for this to be backported. > >> I have no need for it to be backported on my side at least. > >> > > > > Sure. Fixes tag != backported to stable trees but it is close. > > > >> > Reviewed-by: Stephen Boyd > >> >dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms > > This patch will prevent usb3 from working due to dp driver initialize > phy earlier than usb3 which cause timeout error at power up usb3 phy > when both edp and dp are enabled. That sounds pretty bad. > I had prepared a patch (drm/msm/dp: do not initialize combo phy until > plugin interrupt) to fix this problem. Great! When were you planning to report this problem on the list? > Unfortunately, my patch is depend on Bjorn's patch (PATCH v3 3/5] > drm/msm/dp: Support up to 3 DP controllers). > I will submit my patch for review once Bjorn's patches merged in. > Therefore I would think this patch should go after both Bjorn's patches > and my patch. > Why can't you send it now? Point to the other patch series as a dependency.
[Freedreno] [PATCH v4 6/7] dt-bindings: msm/dp: Add SC8180x compatibles
The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add compatibles for these to the msm/dp binding. Reviewed-by: Abhinav Kumar Reviewed-by: Stephen Boyd Signed-off-by: Bjorn Andersson --- Changes since v3: - None .../devicetree/bindings/display/msm/dp-controller.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index 6bb424c21340..63e585f48789 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -17,6 +17,8 @@ properties: compatible: enum: - qcom,sc7180-dp + - qcom,sc8180x-dp + - qcom,sc8180x-edp reg: items: -- 2.29.2
[Freedreno] [PATCH v4 7/7] drm/msm/dp: Add sc8180x DP controllers
The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
DP driver.
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- Rebased upon previous patches in series
drivers/gpu/drm/msm/dp/dp_display.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 674cddfee5b0..29c2c1c52ddb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -135,8 +135,19 @@ static const struct msm_dp_config sc7180_dp_cfg = {
.num_descs = 1,
};
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .descs = (struct msm_dp_desc[]) {
+ { .io_start = 0x0ae9, .connector_type =
DRM_MODE_CONNECTOR_DisplayPort },
+ { .io_start = 0x0ae98000, .connector_type =
DRM_MODE_CONNECTOR_DisplayPort },
+ { .io_start = 0x0ae9a000, .connector_type =
DRM_MODE_CONNECTOR_eDP },
+ },
+ .num_descs = 3,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
+ { .compatible = "qcom,sc8180x-dp", .data = _dp_cfg },
+ { .compatible = "qcom,sc8180x-edp", .data = _dp_cfg },
{}
};
--
2.29.2
[Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel
eDP panels might need some power sequencing and backlight management,
so make it possible to associate a drm_panel with an eDP instance and
prepare and enable the panel accordingly.
Now that we know which hardware instance is DP and which is eDP,
parser->parse() is passed the connector_type and the parser is limited
to only search for a panel in the eDP case.
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- Previously posted separately, now added to series
- Make the search for a panel conditional on it being an eDP connector
- Move the search to of_graph port 1 (was 2 to distinguish from DP link to
USB-C connector)
drivers/gpu/drm/msm/dp/dp_display.c | 9 ++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 11 +++
drivers/gpu/drm/msm/dp/dp_parser.c | 30 -
drivers/gpu/drm/msm/dp/dp_parser.h | 3 ++-
5 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index eaf08f9e7d87..bdaf227f05dc 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"
@@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev, struct
device *master,
priv = drm->dev_private;
priv->dp = &(dp->dp_display);
- rc = dp->parser->parse(dp->parser);
+ rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
if (rc) {
DRM_ERROR("device tree parsing failed\n");
goto end;
}
+ dp->dp_display.panel_bridge = dp->parser->panel_bridge;
+
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -822,7 +825,7 @@ 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)
{
return 0;
}
@@ -896,7 +899,7 @@ 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)
{
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 02999408c052..24aefca66029 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_bridge *panel_bridge;
bool is_connected;
bool audio_enabled;
bool power_on;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index f33e31523f56..76856c4ee1d6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include "msm_drv.h"
@@ -160,5 +161,15 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
drm_connector_attach_encoder(connector, dp_display->encoder);
+ if (dp_display->panel_bridge) {
+ ret = drm_bridge_attach(dp_display->encoder,
+ dp_display->panel_bridge, NULL,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret < 0) {
+ DRM_ERROR("failed to attach panel bridge: %d\n", ret);
+ return ERR_PTR(ret);
+ }
+ }
+
return connector;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index 4d6e047f803d..eb6bbfbea484 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"
@@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser *parser)
return 0;
}
-static int dp_parser_parse(struct dp_parser *parser)
+static int dp_parser_find_panel(struct dp_parser *parser)
+{
+ struct device *dev = >pdev->dev;
+ struct drm_panel *panel;
+ int rc;
+
+ rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
+ if (rc) {
+ DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
+ return rc;
+ }
+
+ parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+ if (IS_ERR(parser->panel_bridge)) {
+ DRM_ERROR("failed to create panel bridge\n");
+ return PTR_ERR(parser->panel_bridge);
+ }
+
+ return 0;
+}
+
+static int dp_parser_parse(struct dp_parser *parser, int connector_type)
{
int rc = 0;
@@ -284,6 +306,12 @@ static int dp_parser_parse(struct dp_parser *parser)
if (rc)
return rc;
+ if
[Freedreno] [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers
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 v3:
- Rebased ontop of previous patches
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 +++
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 ++-
drivers/gpu/drm/msm/dp/dp_display.c | 18 +++--
drivers/gpu/drm/msm/msm_drv.h | 2 +-
5 files changed, 59 insertions(+), 37 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..875b07e7183d 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,10 @@ 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++) {
+ if (priv->dp[i])
+ msm_dp_debugfs_init(priv->dp[i], minor);
+ }
return dpu_core_perf_debugfs_init(dpu_kms, entry);
}
@@ -544,35 +547,42 @@ static int _dpu_kms_initialize_displayport(struct
drm_device *dev,
{
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
- int rc = 0;
+ int rc;
+ int i;
- if (!priv->dp)
- return rc;
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
- 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.capabilities = MSM_DISPLAY_CAP_VID_MODE;
- info.intf_type = encoder->encoder_type;
- rc = dpu_encoder_setup(dev, encoder, );
- if (rc)
- DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
- encoder->base.id, rc);
- return rc;
+ info.num_of_h_tiles = 1;
+ info.h_tile_instance[0] = i;
+ info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
+ info.intf_type =
[Freedreno] [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller
As the following patches introduced support for multiple DP blocks in a
platform and some of those block might be eDP it becomes useful to be
able to specify the connector type per block.
Although there's only a single block at this point, the array of descs
and the search in dp_display_get_desc() are introduced here to simplify
the next patch, that does introduce support for multiple DP blocks.
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- New patch
- Extended msm_dp_config with connector_type, wrapped in inner struct
- Refactored out of the next patch
- Pass the connector_type to drm_connector_init(), from yet another patch
- Dropped double newline and unnecessary {}
drivers/gpu/drm/msm/dp/dp_display.c | 43 -
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 2 +-
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 5d3ee5ef07c2..eaf08f9e7d87 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -115,8 +115,25 @@ struct dp_display_private {
struct dp_audio *audio;
};
+struct msm_dp_desc {
+ phys_addr_t io_start;
+ int connector_type;
+};
+
+struct msm_dp_config {
+ struct msm_dp_desc *descs;
+ size_t num_descs;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+ .descs = (struct msm_dp_desc[]) {
+ { .io_start = 0x0ae9, .connector_type =
DRM_MODE_CONNECTOR_DisplayPort },
+ },
+ .num_descs = 1,
+};
+
static const struct of_device_id dp_dt_match[] = {
- {.compatible = "qcom,sc7180-dp"},
+ { .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
{}
};
@@ -1180,10 +1197,29 @@ int dp_display_request_irq(struct msm_dp *dp_display)
return 0;
}
+static struct msm_dp_desc *dp_display_get_desc(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 NULL;
+
+ for (i = 0; i < cfg->num_descs; i++)
+ if (cfg->descs[i].io_start == res->start)
+ return >descs[i];
+
+ dev_err(>dev, "unknown displayport instance\n");
+ return NULL;
+}
+
static int dp_display_probe(struct platform_device *pdev)
{
int rc = 0;
struct dp_display_private *dp;
+ struct msm_dp_desc *desc;
if (!pdev || !pdev->dev.of_node) {
DRM_ERROR("pdev not found\n");
@@ -1194,8 +1230,13 @@ static int dp_display_probe(struct platform_device *pdev)
if (!dp)
return -ENOMEM;
+ desc = dp_display_get_desc(pdev);
+ if (!desc)
+ return -EINVAL;
+
dp->pdev = pdev;
dp->name = "drm_dp";
+ dp->dp_display.connector_type = desc->connector_type;
rc = dp_init_sub_modules(dp);
if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 8b47cdabb67e..02999408c052 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -18,6 +18,7 @@ struct msm_dp {
bool is_connected;
bool audio_enabled;
bool power_on;
+ int connector_type;
hdmi_codec_plugged_cb plugged_cb;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 764f4b81017e..f33e31523f56 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -147,7 +147,7 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
ret = drm_connector_init(dp_display->drm_dev, connector,
_connector_funcs,
- DRM_MODE_CONNECTOR_DisplayPort);
+ dp_display->connector_type);
if (ret)
return ERR_PTR(ret);
--
2.29.2
[Freedreno] [PATCH v4 2/7] drm/msm/dp: Modify prototype of encoder based API
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.
Reviewed-by: Abhinav Kumar
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- None
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 -
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0e9d3fa1544b..b7f33da2799c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -168,6 +168,7 @@ enum dpu_enc_rc_states {
* @vsync_event_work: worker to handle vsync event for autorefresh
* @topology: topology of the display
* @idle_timeout: idle timeout duration in milliseconds
+ * @dp:msm_dp pointer, for DP encoders
*/
struct dpu_encoder_virt {
struct drm_encoder base;
@@ -206,6 +207,8 @@ struct dpu_encoder_virt {
struct msm_display_topology topology;
u32 idle_timeout;
+
+ struct msm_dp *dp;
};
#define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
@@ -1000,8 +1003,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
- msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
+ msm_dp_display_mode_set(dpu_enc->dp, drm_enc, mode, adj_mode);
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
@@ -1182,9 +1185,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder
*drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- ret = msm_dp_display_enable(priv->dp,
- drm_enc);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
if (ret) {
DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
ret);
@@ -1224,8 +1226,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_pre_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
}
@@ -1253,8 +1255,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
}
@@ -2170,7 +2172,8 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
-
+ else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+ dpu_enc->dp = priv->dp;
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
--
2.29.2
[Freedreno] [PATCH v4 1/7] drm/msm/dp: Remove global g_dp_display variable
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.
Reviewed-by: Abhinav Kumar
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
---
Changes since v3:
- None
drivers/gpu/drm/msm/dp/dp_display.c | 80 -
1 file changed, 21 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index fbe4c2cd52a3..5d3ee5ef07c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -27,7 +27,6 @@
#include "dp_audio.h"
#include "dp_debug.h"
-static struct msm_dp *g_dp_display;
#define HPD_STRING_SIZE 30
enum {
@@ -121,6 +120,13 @@ static const struct of_device_id dp_dt_match[] = {
{}
};
+static struct dp_display_private *dev_get_dp_display_private(struct device
*dev)
+{
+ struct msm_dp *dp = dev_get_drvdata(dev);
+
+ return container_of(dp, struct dp_display_private, dp_display);
+}
+
static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
u32 data, u32 delay)
{
@@ -197,15 +203,12 @@ static int dp_display_bind(struct device *dev, struct
device *master,
void *data)
{
int rc = 0;
- struct dp_display_private *dp;
- struct drm_device *drm;
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
struct msm_drm_private *priv;
+ struct drm_device *drm;
drm = dev_get_drvdata(master);
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
priv->dp = &(dp->dp_display);
@@ -240,13 +243,10 @@ static int dp_display_bind(struct device *dev, struct
device *master,
static void dp_display_unbind(struct device *dev, struct device *master,
void *data)
{
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp = NULL;
@@ -379,38 +379,17 @@ static void dp_display_host_deinit(struct
dp_display_private *dp)
static int dp_display_usbpd_configure_cb(struct device *dev)
{
- int rc = 0;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
- goto end;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_display_host_init(dp, false);
- rc = dp_display_process_hpd_high(dp);
-end:
- return rc;
+ return dp_display_process_hpd_high(dp);
}
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
int rc = 0;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
- return rc;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
@@ -472,15 +451,7 @@ static int dp_display_usbpd_attention_cb(struct device
*dev)
{
int rc = 0;
u32 sink_request;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- return -EINVAL;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
@@ -647,7 +618,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
DRM_DEBUG_DP("hpd_state=%d\n", state);
/* signal the disconnect event early to ensure proper teardown */
- dp_display_handle_plugged_change(g_dp_display, false);
+ dp_display_handle_plugged_change(>dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
@@ -842,9 +813,7 @@ static int dp_display_prepare(struct msm_dp *dp)
static int dp_display_enable(struct dp_display_private
[Freedreno] [PATCH v4 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x
The current implementation supports a single DP instance and the DPU code will only match it against INTF_DP instance 0. These patches extends this to allow multiple DP instances and support for matching against DP instances beyond 0. With that in place add SC8180x DP and eDP controllers. Bjorn Andersson (7): drm/msm/dp: Remove global g_dp_display variable drm/msm/dp: Modify prototype of encoder based API drm/msm/dp: Allow specifying connector_type per controller drm/msm/dp: Allow attaching a drm_panel drm/msm/dp: Support up to 3 DP controllers dt-bindings: msm/dp: Add SC8180x compatibles drm/msm/dp: Add sc8180x DP controllers .../bindings/display/msm/dp-controller.yaml | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 +- drivers/gpu/drm/msm/dp/dp_display.c | 153 ++ drivers/gpu/drm/msm/dp/dp_display.h | 2 + drivers/gpu/drm/msm/dp/dp_drm.c | 13 +- drivers/gpu/drm/msm/dp/dp_parser.c| 30 +++- drivers/gpu/drm/msm/dp/dp_parser.h| 3 +- drivers/gpu/drm/msm/msm_drv.h | 2 +- 10 files changed, 194 insertions(+), 108 deletions(-) -- 2.29.2
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
Hi,
On Tue, Oct 5, 2021 at 10:33 AM Bjorn Andersson
wrote:
>
> On Tue 05 Oct 08:39 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
> > wrote:
> > >
> > > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > > wrote:
> > > > >
> > > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > > wrote:
> > > > > > >
> > > > > > > +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);
> > > > > >
> > > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > > port 1 is "Output endpoint of the controller". We should just use
> > > > > > port
> > > > > > 1 here, right?
> > > > > >
> > > > >
> > > > > Finally got back to this, changed it to 1 and figured out why I left
> > > > > it
> > > > > at 2.
> > > > >
> > > > > drm_of_find_panel_or_bridge() on a DP controller will find the
> > > > > of_graph
> > > > > reference to the USB-C controller, scan through the registered panels
> > > > > and conclude that the of_node of the USB-C controller isn't a
> > > > > registered
> > > > > panel and return -EPROBE_DEFER.
> > > >
> > > > I'm confused, but maybe it would help if I could see something
> > > > concrete. Is there a specific board this was happening on?
> > > >
> > >
> > > Right, let's make this more concrete with a snippet from the actual
> > > SC8180x DT.
> > >
> > > > Under the DP node in the device tree I expect:
> > > >
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > edp_out: endpoint {
> > > > remote-endpoint = <_panel_in>;
> > > > };
> > > > };
> > > > };
> > > >
> > >
> > > /* We got a panel */
> > > panel {
> > > ...
> > > ports {
> > > port {
> > > auo_b133han05_in: endpoint {
> > > remote-endpoint = <_edp_out>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > /* And a 2-port USB-C controller */
> > > type-c-controller {
> > > ...
> > > connector@0 {
> > > ports {
> > > port@0 {
> > > reg = <0>;
> > > ucsi_port_0_dp: endpoint {
> > > remote-endpoint = <_mode>;
> > > };
> > > };
> > >
> > > port@1 {
> > > reg = <1>;
> > > ucsi_port_0_switch: endpoint {
> > > remote-endpoint = <_qmp_phy>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > connector@1 {
> > > ports {
> > > port@0 {
> > > reg = <0>;
> > > ucsi_port_1_dp: endpoint {
> > > remote-endpoint = <_mode>;
> > > };
> > > };
> > >
> > > port@1 {
> > > reg = <1>;
> > > ucsi_port_1_switch: endpoint {
> > > remote-endpoint = <_qmp_phy>;
> > > };
> > > };
> > > };
> > > };
> > > };
> > >
> > > /* And then our 2 DP and single eDP controllers */
> > > _dp0 {
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > dp0_mode: endpoint {
> > > remote-endpoint = <_port_0_dp>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > _dp1 {
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > dp1_mode: endpoint {
> > > remote-endpoint = <_port_1_dp>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > _edp {
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > mdss_edp_out: endpoint {
> > > remote-endpoint = <_b133han05_in>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > > If you have "port@1" pointing to a USB-C controller but this instance
> > > > of the DP controller is actually hooked up straight to a panel then
> > > > you should simply delete the "port@1" that points to the typeC and
> > > > replace it with one that points to a panel, right?
> > > >
> > >
> > > As you can see, port 1 on _dp0 and _dp1 points to the two UCSI
> > > connectors and the eDP points to the panel, exactly like we agreed.
> > >
> > > So now I call:
> > > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
> > >
> > > which for the two DP nodes will pass respective UCSI connector to
> > > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > > panel_list.
> > >
> > > There's nothing indicating in the of_graph that the USB connectors
Re: [Freedreno] [PATCH] drm/msm/dp: Shorten SETUP timeout
On 2021-10-05 15:36, Stephen Boyd wrote: Quoting Bjorn Andersson (2021-10-05 14:40:38) On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-10-04 19:37:50) > > Found in the middle of a patch from Sankeerth was the reduction of the > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host > > is initalized and HPD interrupt start to be serviced, so in the case of > > eDP this reduction improves the user experience dramatically - i.e. > > removes 9.9s of bland screen time at boot. > > > > Suggested-by: Sankeerth Billakanti > > Signed-off-by: Bjorn Andersson > > --- > > Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go > re-read the code a couple times to understand that it's waiting 100ms > times the 'delay' number. What? > I assume you're happy with the current 10s delay on the current devices, so I don't think we should push for this to be backported. I have no need for it to be backported on my side at least. Sure. Fixes tag != backported to stable trees but it is close. > Reviewed-by: Stephen Boyd dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms This patch will prevent usb3 from working due to dp driver initialize phy earlier than usb3 which cause timeout error at power up usb3 phy when both edp and dp are enabled. I had prepared a patch (drm/msm/dp: do not initialize combo phy until plugin interrupt) to fix this problem. Unfortunately, my patch is depend on Bjorn's patch (PATCH v3 3/5] drm/msm/dp: Support up to 3 DP controllers). I will submit my patch for review once Bjorn's patches merged in. Therefore I would think this patch should go after both Bjorn's patches and my patch.
Re: [Freedreno] [pull] drm/msm: drm-msm-fixes-2021-10-05 for v5.15-rc5
+ dri-devel, sorry hit 'send' too quickly On Tue, Oct 5, 2021 at 3:45 PM Rob Clark wrote: > > Hi Dave & Daniel, > > A few fixes for v5.15: > > * Fix a new crash on dev file close if the dev file was opened when > GPU is not loaded (such as missing fw in initrd) > * Switch to single drm_sched_entity per priority level per drm_file > to unbreak multi-context userspace > * Serialize GMU access to fix GMU OOB errors > * Various error path fixes > * A couple integer overflow fixes > * Fix mdp5 cursor plane WARNs > > The following changes since commit 5816b3e6577eaa676ceb00a848f0fd65fe2adc29: > > Linux 5.15-rc3 (2021-09-26 14:08:19 -0700) > > are available in the Git repository at: > > https://gitlab.freedesktop.org/drm/msm.git drm-msm-fixes-2021-10-05 > > for you to fetch changes up to c6921fbc88e120b2279c55686a071ca312d058e9: > > drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling > (2021-10-04 08:08:07 -0700) > > > Arnd Bergmann (1): > drm/msm/submit: fix overflow check on 64-bit architectures > > Colin Ian King (1): > drm/msm: Fix null pointer dereference on pointer edp > > Dan Carpenter (4): > drm/msm/a4xx: fix error handling in a4xx_gpu_init() > drm/msm/a3xx: fix error handling in a3xx_gpu_init() > drm/msm/dsi: Fix an error code in msm_dsi_modeset_init() > drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling > > Dmitry Baryshkov (2): > drm/msm/mdp5: fix cursor-related warnings > drm/msm/dsi/phy: fix clock names in 28nm_8960 phy > > Fabio Estevam (1): > drm/msm: Do not run snapshot on non-DPU devices > > Kuogee Hsieh (1): > drm/msm/dp: only signal audio when disconnected detected at dp_pm_resume > > Marek Vasut (1): > drm/msm: Avoid potential overflow in timeout_to_jiffies() > > Marijn Suijten (1): > drm/msm/dsi: dsi_phy_14nm: Take ready-bit into account in poll_for_ready > > Rob Clark (5): > drm/msm: Fix crash on dev file close > drm/msm/a6xx: Serialize GMU communication > drm/msm/a6xx: Track current ctx by seqno > drm/msm: A bit more docs + cleanup > drm/msm: One sched entity per process per priority > > Robert Foss (1): > drm/msm/dpu: Fix address of SM8150 PINGPONG5 IRQ register > > Stephan Gerhold (1): > drm/msm: Fix devfreq NULL pointer dereference on a3xx > > drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 9 ++-- > drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 9 ++-- > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +++ > drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 3 ++ > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 46 > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 11 +++- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 16 ++ > drivers/gpu/drm/msm/dp/dp_display.c | 10 ++-- > drivers/gpu/drm/msm/dsi/dsi.c | 4 +- > drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- > drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 30 +-- > drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 +- > drivers/gpu/drm/msm/edp/edp_ctrl.c | 3 +- > drivers/gpu/drm/msm/msm_drv.c | 15 -- > drivers/gpu/drm/msm/msm_drv.h | 47 +--- > drivers/gpu/drm/msm/msm_gem_submit.c| 7 +-- > drivers/gpu/drm/msm/msm_gpu.h | 66 ++- > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 6 +++ > drivers/gpu/drm/msm/msm_submitqueue.c | 72 > - > 20 files changed, 256 insertions(+), 112 deletions(-)
[Freedreno] [pull] drm/msm: drm-msm-fixes-2021-10-05 for v5.15-rc5
Hi Dave & Daniel, A few fixes for v5.15: * Fix a new crash on dev file close if the dev file was opened when GPU is not loaded (such as missing fw in initrd) * Switch to single drm_sched_entity per priority level per drm_file to unbreak multi-context userspace * Serialize GMU access to fix GMU OOB errors * Various error path fixes * A couple integer overflow fixes * Fix mdp5 cursor plane WARNs The following changes since commit 5816b3e6577eaa676ceb00a848f0fd65fe2adc29: Linux 5.15-rc3 (2021-09-26 14:08:19 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git drm-msm-fixes-2021-10-05 for you to fetch changes up to c6921fbc88e120b2279c55686a071ca312d058e9: drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling (2021-10-04 08:08:07 -0700) Arnd Bergmann (1): drm/msm/submit: fix overflow check on 64-bit architectures Colin Ian King (1): drm/msm: Fix null pointer dereference on pointer edp Dan Carpenter (4): drm/msm/a4xx: fix error handling in a4xx_gpu_init() drm/msm/a3xx: fix error handling in a3xx_gpu_init() drm/msm/dsi: Fix an error code in msm_dsi_modeset_init() drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling Dmitry Baryshkov (2): drm/msm/mdp5: fix cursor-related warnings drm/msm/dsi/phy: fix clock names in 28nm_8960 phy Fabio Estevam (1): drm/msm: Do not run snapshot on non-DPU devices Kuogee Hsieh (1): drm/msm/dp: only signal audio when disconnected detected at dp_pm_resume Marek Vasut (1): drm/msm: Avoid potential overflow in timeout_to_jiffies() Marijn Suijten (1): drm/msm/dsi: dsi_phy_14nm: Take ready-bit into account in poll_for_ready Rob Clark (5): drm/msm: Fix crash on dev file close drm/msm/a6xx: Serialize GMU communication drm/msm/a6xx: Track current ctx by seqno drm/msm: A bit more docs + cleanup drm/msm: One sched entity per process per priority Robert Foss (1): drm/msm/dpu: Fix address of SM8150 PINGPONG5 IRQ register Stephan Gerhold (1): drm/msm: Fix devfreq NULL pointer dereference on a3xx drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 9 ++-- drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 9 ++-- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +++ drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 3 ++ drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 46 drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 11 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 16 ++ drivers/gpu/drm/msm/dp/dp_display.c | 10 ++-- drivers/gpu/drm/msm/dsi/dsi.c | 4 +- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 30 +-- drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 +- drivers/gpu/drm/msm/edp/edp_ctrl.c | 3 +- drivers/gpu/drm/msm/msm_drv.c | 15 -- drivers/gpu/drm/msm/msm_drv.h | 47 +--- drivers/gpu/drm/msm/msm_gem_submit.c| 7 +-- drivers/gpu/drm/msm/msm_gpu.h | 66 ++- drivers/gpu/drm/msm/msm_gpu_devfreq.c | 6 +++ drivers/gpu/drm/msm/msm_submitqueue.c | 72 - 20 files changed, 256 insertions(+), 112 deletions(-)
Re: [Freedreno] [PATCH] drm/msm/dp: Shorten SETUP timeout
Quoting Bjorn Andersson (2021-10-05 14:40:38) > On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-10-04 19:37:50) > > > Found in the middle of a patch from Sankeerth was the reduction of the > > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host > > > is initalized and HPD interrupt start to be serviced, so in the case of > > > eDP this reduction improves the user experience dramatically - i.e. > > > removes 9.9s of bland screen time at boot. > > > > > > Suggested-by: Sankeerth Billakanti > > > Signed-off-by: Bjorn Andersson > > > --- > > > > Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go > > re-read the code a couple times to understand that it's waiting 100ms > > times the 'delay' number. What? > > > > I assume you're happy with the current 10s delay on the current > devices, so I don't think we should push for this to be backported. > I have no need for it to be backported on my side at least. > Sure. Fixes tag != backported to stable trees but it is close. > > Reviewed-by: Stephen Boyd >
Re: [Freedreno] [PATCH] drm/msm/dp: Shorten SETUP timeout
On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-10-04 19:37:50) > > Found in the middle of a patch from Sankeerth was the reduction of the > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host > > is initalized and HPD interrupt start to be serviced, so in the case of > > eDP this reduction improves the user experience dramatically - i.e. > > removes 9.9s of bland screen time at boot. > > > > Suggested-by: Sankeerth Billakanti > > Signed-off-by: Bjorn Andersson > > --- > > Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go > re-read the code a couple times to understand that it's waiting 100ms > times the 'delay' number. What? > I assume you're happy with the current 10s delay on the current devices, so I don't think we should push for this to be backported. I have no need for it to be backported on my side at least. > Reviewed-by: Stephen Boyd Thanks, Bjorn
Re: [Freedreno] [PATCH v3 4/5] drm/msm/dp: Store each subblock in the io region
On 2021-10-01 10:43, Bjorn Andersson wrote:
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.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
Reviewed-by: Abhinav Kumar
---
Changes since v2:
- Skipped the unnecessary reorder in struct dss_io_region
drivers/gpu/drm/msm/dp/dp_catalog.c | 64 +
drivers/gpu/drm/msm/dp/dp_parser.c | 28 +++--
drivers/gpu/drm/msm/dp/dp_parser.h | 9 +++-
3 files changed, 53 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index cc2bb8295329..6ae9b29044b6 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -24,15 +24,6 @@
#define DP_INTERRUPT_STATUS_ACK_SHIFT 1
#define DP_INTERRUPT_STATUS_MASK_SHIFT 2
-#define MSM_DP_CONTROLLER_AHB_OFFSET 0x
-#define MSM_DP_CONTROLLER_AHB_SIZE 0x0200
-#define MSM_DP_CONTROLLER_AUX_OFFSET 0x0200
-#define MSM_DP_CONTROLLER_AUX_SIZE 0x0200
-#define MSM_DP_CONTROLLER_LINK_OFFSET 0x0400
-#define MSM_DP_CONTROLLER_LINK_SIZE0x0C00
-#define MSM_DP_CONTROLLER_P0_OFFSET0x1000
-#define MSM_DP_CONTROLLER_P0_SIZE 0x0400
-
#define DP_INTERRUPT_STATUS1 \
(DP_INTR_AUX_I2C_DONE| \
DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \
@@ -66,82 +57,77 @@ void dp_catalog_snapshot(struct dp_catalog
*dp_catalog, struct msm_disp_state *d
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
+ struct dss_io_data *dss = >io->dp_controller;
- msm_disp_snapshot_add_block(disp_state,
catalog->io->dp_controller.len,
- catalog->io->dp_controller.base, "dp_ctrl");
+ msm_disp_snapshot_add_block(disp_state, dss->ahb.len, dss->ahb.base,
"dp_ahb");
+ msm_disp_snapshot_add_block(disp_state, dss->aux.len, dss->aux.base,
"dp_aux");
+ msm_disp_snapshot_add_block(disp_state, dss->link.len,
dss->link.base, "dp_link");
+ msm_disp_snapshot_add_block(disp_state, dss->p0.len, dss->p0.base,
"dp_p0");
}
I like this part, devcoredump will look pretty clean and well separated
with this.
static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32
offset)
{
- offset += MSM_DP_CONTROLLER_AUX_OFFSET;
- return readl_relaxed(catalog->io->dp_controller.base + offset);
+ return readl_relaxed(catalog->io->dp_controller.aux.base + offset);
}
static inline void dp_write_aux(struct dp_catalog_private *catalog,
u32 offset, u32 data)
{
- offset += MSM_DP_CONTROLLER_AUX_OFFSET;
/*
* To make sure aux reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
*/
- writel(data, catalog->io->dp_controller.base + offset);
+ writel(data, catalog->io->dp_controller.aux.base + offset);
}
static inline u32 dp_read_ahb(struct dp_catalog_private *catalog, u32
offset)
{
- offset += MSM_DP_CONTROLLER_AHB_OFFSET;
- return readl_relaxed(catalog->io->dp_controller.base + offset);
+ return readl_relaxed(catalog->io->dp_controller.ahb.base + offset);
}
static inline void dp_write_ahb(struct dp_catalog_private *catalog,
u32 offset, u32 data)
{
- offset += MSM_DP_CONTROLLER_AHB_OFFSET;
/*
* To make sure phy reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
*/
- writel(data, catalog->io->dp_controller.base + offset);
+ writel(data, catalog->io->dp_controller.ahb.base + offset);
}
static inline void dp_write_p0(struct dp_catalog_private *catalog,
u32 offset, u32 data)
{
- offset += MSM_DP_CONTROLLER_P0_OFFSET;
/*
* To make sure interface reg writes happens before any other
operation,
* this function uses writel() instread of writel_relaxed()
*/
- writel(data, catalog->io->dp_controller.base + offset);
+ writel(data, catalog->io->dp_controller.p0.base + offset);
}
static inline u32 dp_read_p0(struct dp_catalog_private *catalog,
u32 offset)
{
- offset += MSM_DP_CONTROLLER_P0_OFFSET;
/*
* To make sure interface reg writes happens before any other
operation,
* this function uses writel() instread of writel_relaxed()
*/
- return readl_relaxed(catalog->io->dp_controller.base + offset);
+ return readl_relaxed(catalog->io->dp_controller.p0.base + offset);
}
static inline u32 dp_read_link(struct dp_catalog_private *catalog, u32
offset)
{
- offset += MSM_DP_CONTROLLER_LINK_OFFSET;
- return
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Refactor ioremap wrapper
On 2021-10-01 10:43, Bjorn Andersson wrote:
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.
While at it, devm_ioremap_resource() already prints useful error
messages on failure, so omit the unnecessary prints from the caller.
Signed-off-by: Bjorn Andersson
Reviewed-by: Abhinav Kumar
---
Changes since v2:
- Switched to devm_platform_get_and_ioremap_resource()
drivers/gpu/drm/msm/dp/dp_parser.c | 35 ++
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +-
2 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index c064ced78278..c05ba1990218 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -19,40 +19,27 @@ 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;
+ void __iomem *base;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- DRM_ERROR("%pS->%s: msm_dss_get_res failed\n",
- __builtin_return_address(0), __func__);
- return -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;
- }
+ base = devm_platform_get_and_ioremap_resource(pdev, idx, );
+ if (!IS_ERR(base))
+ *len = resource_size(res);
- return 0;
+ 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))
+ 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;
void __iomem *base;
};
Re: [Freedreno] [PATCH v3 1/5] dt-bindings: msm/dp: Change reg definition
On 2021-10-01 10:43, Bjorn Andersson wrote:
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.
Reviewed-by: Stephen Boyd
Reviewed-by: Rob Herring
Signed-off-by: Bjorn Andersson
Reviewed-by: Abhinav Kumar
---
Changes since v2:
- None
.../bindings/display/msm/dp-controller.yaml | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git
a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index d89b3c510c27..6bb424c21340 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -19,7 +19,12 @@ properties:
- qcom,sc7180-dp
reg:
-maxItems: 1
+items:
+ - description: ahb register block
+ - description: aux register block
+ - description: link register block
+ - description: p0 register block
+ - description: p1 register block
interrupts:
maxItems: 1
@@ -99,7 +104,11 @@ examples:
displayport-controller@ae9 {
compatible = "qcom,sc7180-dp";
-reg = <0xae9 0x1400>;
+reg = <0xae9 0x200>,
+ <0xae90200 0x200>,
+ <0xae90400 0xc00>,
+ <0xae91000 0x400>,
+ <0xae91400 0x400>;
interrupt-parent = <>;
interrupts = <12>;
clocks = < DISP_CC_MDSS_AHB_CLK>,
Re: [Freedreno] [PATCH v3 5/5] drm/msm/dp: Add sc8180x DP controllers
On 2021-10-01 11:00, Bjorn Andersson wrote:
The sc8180x has 2 DP and 1 eDP controllers, add support for these to
the
DP driver.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
Reviewed-by: Abhinav Kumar
---
Changes since v2:
- None
drivers/gpu/drm/msm/dp/dp_display.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index ff3477474c5d..56a79aeffed4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -127,8 +127,15 @@ static const struct msm_dp_config sc7180_dp_cfg =
{
.num_descs = 1,
};
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .io_start = { 0xae9, 0xae98000, 0xae9a000 },
+ .num_descs = 3,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
+ { .compatible = "qcom,sc8180x-dp", .data = _dp_cfg },
+ { .compatible = "qcom,sc8180x-edp", .data = _dp_cfg },
{}
};
Re: [Freedreno] [PATCH v3 4/5] dt-bindings: msm/dp: Add SC8180x compatibles
On 2021-10-01 11:00, Bjorn Andersson wrote: The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add compatibles for these to the msm/dp binding. Reviewed-by: Stephen Boyd Signed-off-by: Bjorn Andersson Reviewed-by: Abhinav Kumar --- Changes since v2: - None .../devicetree/bindings/display/msm/dp-controller.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index 6bb424c21340..63e585f48789 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -17,6 +17,8 @@ properties: compatible: enum: - qcom,sc7180-dp + - qcom,sc8180x-dp + - qcom,sc8180x-edp reg: items:
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Support up to 3 DP controllers
On 2021-10-05 13:16, abhin...@codeaurora.org wrote:
On 2021-10-01 11:00, Bjorn Andersson wrote:
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 v2:
- Added MSM_DRM_DP_COUNT to link the two 3s
- Moved NULL check for msm_dp_debugfs_init() to the call site
- Made struct dp_display_private->id unsigned
I also implemented added connector_type to each of the DP instances
and
propagated this to dp_drm_connector_init() but later dropped this
again per
Doug's suggestion that we'll base this on the presence/absence of a
associated
drm bridge or panel.
Hi Bjorn / Doug
I suggest we add the dp_drm_connector_init() part back to this change.
Having the same connector type for DP and eDP is an error and instead of
blocking/pending it on a potential RFC which attaches a panel to DP,
I would rather do it right here. So till thats done, I would retract my
R-B.
Thanks
Abhinav
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66
+++
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 ++-
drivers/gpu/drm/msm/dp/dp_display.c | 44 -
drivers/gpu/drm/msm/msm_drv.h | 4 +-
5 files changed, 90 insertions(+), 34 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]];
At this point this is a nit but not sure if this is right to use the
tile_instance as the index.
In the future if we chose to assign another index to the
h_tile_instance, this would break.
But I cant think of the use-case for that yet.
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..875b07e7183d 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,10 @@ 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++) {
+ if (priv->dp[i])
+ msm_dp_debugfs_init(priv->dp[i], minor);
+ }
return dpu_core_perf_debugfs_init(dpu_kms, entry);
}
@@ -544,35 +547,42 @@ static int
_dpu_kms_initialize_displayport(struct drm_device *dev,
{
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
- int rc = 0;
+ int rc;
+ int i;
- if (!priv->dp)
- return rc;
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
- 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);
-
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Support up to 3 DP controllers
On 2021-10-01 11:00, Bjorn Andersson wrote:
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 v2:
- Added MSM_DRM_DP_COUNT to link the two 3s
- Moved NULL check for msm_dp_debugfs_init() to the call site
- Made struct dp_display_private->id unsigned
I also implemented added connector_type to each of the DP instances and
propagated this to dp_drm_connector_init() but later dropped this again
per
Doug's suggestion that we'll base this on the presence/absence of a
associated
drm bridge or panel.
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 +++
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 ++-
drivers/gpu/drm/msm/dp/dp_display.c | 44 -
drivers/gpu/drm/msm/msm_drv.h | 4 +-
5 files changed, 90 insertions(+), 34 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]];
At this point this is a nit but not sure if this is right to use the
tile_instance as the index.
In the future if we chose to assign another index to the
h_tile_instance, this would break.
But I cant think of the use-case for that yet, Hence
Reviewed-by: Abhinav Kumar
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..875b07e7183d 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,10 @@ 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++) {
+ if (priv->dp[i])
+ msm_dp_debugfs_init(priv->dp[i], minor);
+ }
return dpu_core_perf_debugfs_init(dpu_kms, entry);
}
@@ -544,35 +547,42 @@ static int
_dpu_kms_initialize_displayport(struct drm_device *dev,
{
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
- int rc = 0;
+ int rc;
+ int i;
- if (!priv->dp)
- return rc;
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
- 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);
+
Re: [Freedreno] [PATCH v3 2/5] drm/msm/dp: Modify prototype of encoder based API
On 2021-10-01 11:00, Bjorn Andersson wrote:
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.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
Reviewed-by: Abhinav Kumar
---
Changes since v2:
- None
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 -
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0e9d3fa1544b..b7f33da2799c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -168,6 +168,7 @@ enum dpu_enc_rc_states {
* @vsync_event_work: worker to handle vsync event for autorefresh
* @topology: topology of the display
* @idle_timeout: idle timeout duration in milliseconds
+ * @dp:msm_dp pointer, for DP encoders
*/
struct dpu_encoder_virt {
struct drm_encoder base;
@@ -206,6 +207,8 @@ struct dpu_encoder_virt {
struct msm_display_topology topology;
u32 idle_timeout;
+
+ struct msm_dp *dp;
};
#define to_dpu_encoder_virt(x) container_of(x, struct
dpu_encoder_virt, base)
@@ -1000,8 +1003,8 @@ static void dpu_encoder_virt_mode_set(struct
drm_encoder *drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
- msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
+ msm_dp_display_mode_set(dpu_enc->dp, drm_enc, mode, adj_mode);
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
@@ -1182,9 +1185,8 @@ static void dpu_encoder_virt_enable(struct
drm_encoder *drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- ret = msm_dp_display_enable(priv->dp,
- drm_enc);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
if (ret) {
DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
ret);
@@ -1224,8 +1226,8 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_pre_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
}
@@ -1253,8 +1255,8 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
}
@@ -2170,7 +2172,8 @@ int dpu_encoder_setup(struct drm_device *dev,
struct drm_encoder *enc,
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
-
+ else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+ dpu_enc->dp = priv->dp;
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
Re: [Freedreno] [PATCH v3 1/5] drm/msm/dp: Remove global g_dp_display variable
On 2021-10-01 11:00, Bjorn Andersson wrote:
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.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
Reviewed-by: Abhinav Kumar
---
Changes since v2:
- None
drivers/gpu/drm/msm/dp/dp_display.c | 80 -
1 file changed, 21 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index fbe4c2cd52a3..5d3ee5ef07c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -27,7 +27,6 @@
#include "dp_audio.h"
#include "dp_debug.h"
-static struct msm_dp *g_dp_display;
#define HPD_STRING_SIZE 30
enum {
@@ -121,6 +120,13 @@ static const struct of_device_id dp_dt_match[] = {
{}
};
+static struct dp_display_private *dev_get_dp_display_private(struct
device *dev)
+{
+ struct msm_dp *dp = dev_get_drvdata(dev);
+
+ return container_of(dp, struct dp_display_private, dp_display);
+}
+
static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
u32 data, u32 delay)
{
@@ -197,15 +203,12 @@ static int dp_display_bind(struct device *dev,
struct device *master,
void *data)
{
int rc = 0;
- struct dp_display_private *dp;
- struct drm_device *drm;
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
struct msm_drm_private *priv;
+ struct drm_device *drm;
drm = dev_get_drvdata(master);
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
priv->dp = &(dp->dp_display);
@@ -240,13 +243,10 @@ static int dp_display_bind(struct device *dev,
struct device *master,
static void dp_display_unbind(struct device *dev, struct device
*master,
void *data)
{
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp = NULL;
@@ -379,38 +379,17 @@ static void dp_display_host_deinit(struct
dp_display_private *dp)
static int dp_display_usbpd_configure_cb(struct device *dev)
{
- int rc = 0;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
- goto end;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_display_host_init(dp, false);
- rc = dp_display_process_hpd_high(dp);
-end:
- return rc;
+ return dp_display_process_hpd_high(dp);
}
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
int rc = 0;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
- return rc;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
@@ -472,15 +451,7 @@ static int dp_display_usbpd_attention_cb(struct
device *dev)
{
int rc = 0;
u32 sink_request;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- return -EINVAL;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
@@ -647,7 +618,7 @@ static int dp_hpd_unplug_handle(struct
dp_display_private *dp, u32 data)
DRM_DEBUG_DP("hpd_state=%d\n", state);
/* signal the disconnect event early to ensure proper teardown */
- dp_display_handle_plugged_change(g_dp_display, false);
+ dp_display_handle_plugged_change(>dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK,
true);
@@ -842,9 +813,7 @@ static int dp_display_prepare(struct msm_dp *dp)
static int
Re: [Freedreno] [PATCH] drm/msm/dp: Shorten SETUP timeout
Quoting Bjorn Andersson (2021-10-04 19:37:50) > Found in the middle of a patch from Sankeerth was the reduction of the > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host > is initalized and HPD interrupt start to be serviced, so in the case of > eDP this reduction improves the user experience dramatically - i.e. > removes 9.9s of bland screen time at boot. > > Suggested-by: Sankeerth Billakanti > Signed-off-by: Bjorn Andersson > --- Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go re-read the code a couple times to understand that it's waiting 100ms times the 'delay' number. What? Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH v1] drm/msm: use compatible string to find mdp node
Quoting Krishna Manikandan (2021-10-05 02:44:31)
> In the current implementation, substring comparison
> using device node name is used to find mdp node
> during driver probe. Use compatible string instead
> of node name to get mdp node from the parent mdss node.
>
> Signed-off-by: Krishna Manikandan
> ---
> drivers/gpu/drm/msm/msm_drv.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 2e6fc18..50a23cf 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1241,9 +1241,16 @@ static int add_components_mdp(struct device *mdp_dev,
> return 0;
> }
>
> -static int compare_name_mdp(struct device *dev, void *data)
> +static int find_mdp_node(struct device *dev, void *data)
> {
> - return (strstr(dev_name(dev), "mdp") != NULL);
> + return of_device_is_compatible(dev->of_node, "qcom,mdp4") ||
Why do we care about mdp4? It looks like this function is only called if
get_mdp_ver() returns 5 or DPU, in which case 4 isn't relevant?
> + of_device_is_compatible(dev->of_node, "qcom,mdp5") ||
> + of_device_is_compatible(dev->of_node, "qcom,mdss_mdp") ||
> + of_device_is_compatible(dev->of_node, "qcom,sdm845-dpu") ||
> + of_device_is_compatible(dev->of_node, "qcom,sm8150-dpu") ||
> + of_device_is_compatible(dev->of_node, "qcom,sm8250-dpu") ||
> + of_device_is_compatible(dev->of_node, "qcom,sc7180-dpu") ||
> + of_device_is_compatible(dev->of_node, "qcom,sc7280-dpu");
Instead of this duplicate string check why not use canonical compatible
lists?
return of_match_node(dpu_dt_match, dev->of_node) ||
of_match_node(mdp5_dt_match, dev->of_node);
This way we're not constantly updating this list of compatibles in two
places.
> }
>
> static int add_display_components(struct platform_device *pdev,
> @@ -1268,7 +1275,7 @@ static int add_display_components(struct
> platform_device *pdev,
> return ret;
> }
>
> - mdp_dev = device_find_child(dev, NULL, compare_name_mdp);
> + mdp_dev = device_find_child(dev, NULL, find_mdp_node);
> if (!mdp_dev) {
> DRM_DEV_ERROR(dev, "failed to find MDSS MDP node\n");
> of_platform_depopulate(dev);
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
On Tue 05 Oct 08:39 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
> wrote:
> >
> > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > wrote:
> > > >
> > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > wrote:
> > > > > >
> > > > > > +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);
> > > > >
> > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > > 1 here, right?
> > > > >
> > > >
> > > > Finally got back to this, changed it to 1 and figured out why I left it
> > > > at 2.
> > > >
> > > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > > reference to the USB-C controller, scan through the registered panels
> > > > and conclude that the of_node of the USB-C controller isn't a registered
> > > > panel and return -EPROBE_DEFER.
> > >
> > > I'm confused, but maybe it would help if I could see something
> > > concrete. Is there a specific board this was happening on?
> > >
> >
> > Right, let's make this more concrete with a snippet from the actual
> > SC8180x DT.
> >
> > > Under the DP node in the device tree I expect:
> > >
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > edp_out: endpoint {
> > > remote-endpoint = <_panel_in>;
> > > };
> > > };
> > > };
> > >
> >
> > /* We got a panel */
> > panel {
> > ...
> > ports {
> > port {
> > auo_b133han05_in: endpoint {
> > remote-endpoint = <_edp_out>;
> > };
> > };
> > };
> > };
> >
> > /* And a 2-port USB-C controller */
> > type-c-controller {
> > ...
> > connector@0 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_0_dp: endpoint {
> > remote-endpoint = <_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_0_switch: endpoint {
> > remote-endpoint = <_qmp_phy>;
> > };
> > };
> > };
> > };
> >
> > connector@1 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_1_dp: endpoint {
> > remote-endpoint = <_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_1_switch: endpoint {
> > remote-endpoint = <_qmp_phy>;
> > };
> > };
> > };
> > };
> > };
> >
> > /* And then our 2 DP and single eDP controllers */
> > _dp0 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp0_mode: endpoint {
> > remote-endpoint = <_port_0_dp>;
> > };
> > };
> > };
> > };
> >
> > _dp1 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp1_mode: endpoint {
> > remote-endpoint = <_port_1_dp>;
> > };
> > };
> > };
> > };
> >
> > _edp {
> > ports {
> > port@1 {
> > reg = <1>;
> > mdss_edp_out: endpoint {
> > remote-endpoint = <_b133han05_in>;
> > };
> > };
> > };
> > };
> >
> > > If you have "port@1" pointing to a USB-C controller but this instance
> > > of the DP controller is actually hooked up straight to a panel then
> > > you should simply delete the "port@1" that points to the typeC and
> > > replace it with one that points to a panel, right?
> > >
> >
> > As you can see, port 1 on _dp0 and _dp1 points to the two UCSI
> > connectors and the eDP points to the panel, exactly like we agreed.
> >
> > So now I call:
> > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
> >
> > which for the two DP nodes will pass respective UCSI connector to
> > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > panel_list.
> >
> > There's nothing indicating in the of_graph that the USB connectors
> > aren't panels (or bridges), so I don't see a way to distinguish the two
> > types remotes.
>
> As far as I can tell the way this would be solved would be to actually
> pass in and then make sure that a bridge would be in place for
> the DP connector. In the full DP case you'll get an -EPROBE_DEFER if
> the connector hasn't been probed but once it's probed then it should
> register as a bridge and thus give you the info you
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
Hi,
On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
wrote:
>
> On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > wrote:
> > >
> > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > wrote:
> > > > >
> > > > > +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);
> > > >
> > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > 1 here, right?
> > > >
> > >
> > > Finally got back to this, changed it to 1 and figured out why I left it
> > > at 2.
> > >
> > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > reference to the USB-C controller, scan through the registered panels
> > > and conclude that the of_node of the USB-C controller isn't a registered
> > > panel and return -EPROBE_DEFER.
> >
> > I'm confused, but maybe it would help if I could see something
> > concrete. Is there a specific board this was happening on?
> >
>
> Right, let's make this more concrete with a snippet from the actual
> SC8180x DT.
>
> > Under the DP node in the device tree I expect:
> >
> > ports {
> > port@1 {
> > reg = <1>;
> > edp_out: endpoint {
> > remote-endpoint = <_panel_in>;
> > };
> > };
> > };
> >
>
> /* We got a panel */
> panel {
> ...
> ports {
> port {
> auo_b133han05_in: endpoint {
> remote-endpoint = <_edp_out>;
> };
> };
> };
> };
>
> /* And a 2-port USB-C controller */
> type-c-controller {
> ...
> connector@0 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_0_dp: endpoint {
> remote-endpoint = <_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_0_switch: endpoint {
> remote-endpoint = <_qmp_phy>;
> };
> };
> };
> };
>
> connector@1 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_1_dp: endpoint {
> remote-endpoint = <_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_1_switch: endpoint {
> remote-endpoint = <_qmp_phy>;
> };
> };
> };
> };
> };
>
> /* And then our 2 DP and single eDP controllers */
> _dp0 {
> ports {
> port@1 {
> reg = <1>;
> dp0_mode: endpoint {
> remote-endpoint = <_port_0_dp>;
> };
> };
> };
> };
>
> _dp1 {
> ports {
> port@1 {
> reg = <1>;
> dp1_mode: endpoint {
> remote-endpoint = <_port_1_dp>;
> };
> };
> };
> };
>
> _edp {
> ports {
> port@1 {
> reg = <1>;
> mdss_edp_out: endpoint {
> remote-endpoint = <_b133han05_in>;
> };
> };
> };
> };
>
> > If you have "port@1" pointing to a USB-C controller but this instance
> > of the DP controller is actually hooked up straight to a panel then
> > you should simply delete the "port@1" that points to the typeC and
> > replace it with one that points to a panel, right?
> >
>
> As you can see, port 1 on _dp0 and _dp1 points to the two UCSI
> connectors and the eDP points to the panel, exactly like we agreed.
>
> So now I call:
> drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
>
> which for the two DP nodes will pass respective UCSI connector to
> drm_find_panel() and get EPROBE_DEFER back - because they are not on
> panel_list.
>
> There's nothing indicating in the of_graph that the USB connectors
> aren't panels (or bridges), so I don't see a way to distinguish the two
> types remotes.
As far as I can tell the way this would be solved would be to actually
pass in and then make sure that a bridge would be in place for
the DP connector. In the full DP case you'll get an -EPROBE_DEFER if
the connector hasn't been probed but once it's probed then it should
register as a bridge and thus give you the info you need (AKA that
this isn't a panel).
I haven't done the digging to see how all this works, but according to
Laurent [1]: "Physical connectors are already handled as bridges, see
drivers/gpu/drm/bridge/display-connector.c"
So basically I think this is solvable in code and there's no reason to
mess with the devicetree bindings to solve this problem. Does that
sound right?
[1]
[Freedreno] [PATCH v2 3/3] drm/msm: Extend gpu devcore dumps with pgtbl info
From: Rob Clark
In the case of iova fault triggered devcore dumps, include additional
debug information based on what we think is the current page tables,
including the TTBR0 value (which should match what we have in
adreno_smmu_fault_info unless things have gone horribly wrong), and
the pagetable entries traversed in the process of resolving the
faulting iova.
Signed-off-by: Rob Clark
---
v2: Fix build error on 32b/armv7
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++
drivers/gpu/drm/msm/msm_gpu.c | 10 ++
drivers/gpu/drm/msm/msm_gpu.h | 8
drivers/gpu/drm/msm/msm_iommu.c | 17 +
drivers/gpu/drm/msm/msm_mmu.h | 2 ++
5 files changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 42e522a60623..7bac86b01f30 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -707,6 +707,16 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state
*state,
drm_printf(p, " - dir: %s\n", info->flags & IOMMU_FAULT_WRITE
? "WRITE" : "READ");
drm_printf(p, " - type: %s\n", info->type);
drm_printf(p, " - source: %s\n", info->block);
+
+ /* Information extracted from what we think are the current
+* pgtables. Hopefully the TTBR0 matches what we've extracted
+* from the SMMU registers in smmu_info!
+*/
+ drm_puts(p, "pgtable-fault-info:\n");
+ drm_printf(p, " - ttbr0: %.16llx\n", (u64)info->pgtbl_ttbr0);
+ drm_printf(p, " - asid: %d\n", info->asid);
+ drm_printf(p, " - ptes: %.16llx %.16llx %.16llx %.16llx\n",
+ info->ptes[0], info->ptes[1], info->ptes[2],
info->ptes[3]);
}
drm_printf(p, "rbbm-status: 0x%08x\n", state->rbbm_status);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 8a3a592da3a4..d1a16642ecd5 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -284,6 +284,16 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
if (submit) {
int i, nr = 0;
+ if (state->fault_info.smmu_info.ttbr0) {
+ struct msm_gpu_fault_info *info = >fault_info;
+ struct msm_mmu *mmu = submit->aspace->mmu;
+
+ msm_iommu_pagetable_params(mmu, >pgtbl_ttbr0,
+ >asid);
+ msm_iommu_pagetable_walk(mmu, info->iova, info->ptes,
+ARRAY_SIZE(info->ptes));
+ }
+
/* count # of buffers to dump: */
for (i = 0; i < submit->nr_bos; i++)
if (should_dump(submit, i))
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 0e132795123f..ab4c80065ac5 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -78,6 +78,14 @@ struct msm_gpu_fault_info {
int flags;
const char *type;
const char *block;
+
+ /* Information about what we think/expect is the current SMMU state,
+* for example expected_ttbr0 should match smmu_info.ttbr0 which
+* was read back from SMMU registers.
+*/
+ phys_addr_t pgtbl_ttbr0;
+ u64 ptes[4];
+ int asid;
};
/**
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index bcaddbba564d..0f2924fd2524 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -116,6 +116,23 @@ int msm_iommu_pagetable_params(struct msm_mmu *mmu,
return 0;
}
+int msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova,
+u64 *ptes, int num_ptes)
+{
+ struct msm_iommu_pagetable *pagetable;
+
+ if (mmu->type != MSM_MMU_IOMMU_PAGETABLE)
+ return -EINVAL;
+
+ pagetable = to_pagetable(mmu);
+
+ if (!pagetable->pgtbl_ops->pgtable_walk)
+ return -EINVAL;
+
+ return pagetable->pgtbl_ops->pgtable_walk(pagetable->pgtbl_ops, iova,
+ ptes, _ptes);
+}
+
static const struct msm_mmu_funcs pagetable_funcs = {
.map = msm_iommu_pagetable_map,
.unmap = msm_iommu_pagetable_unmap,
diff --git a/drivers/gpu/drm/msm/msm_mmu.h b/drivers/gpu/drm/msm/msm_mmu.h
index de158e1bf765..519b749c61af 100644
--- a/drivers/gpu/drm/msm/msm_mmu.h
+++ b/drivers/gpu/drm/msm/msm_mmu.h
@@ -58,5 +58,7 @@ void msm_gpummu_params(struct msm_mmu *mmu, dma_addr_t
*pt_base,
int msm_iommu_pagetable_params(struct msm_mmu *mmu, phys_addr_t *ttbr,
int *asid);
+int msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova,
+u64
[Freedreno] [PATCH v2 2/3] drm/msm: Show all smmu info for iova fault devcore dumps
From: Rob Clark
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 25 +
drivers/gpu/drm/msm/msm_gpu.h | 2 +-
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 34fede935ac0..96e0ca986c54 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1282,7 +1282,7 @@ static int a6xx_fault_handler(void *arg, unsigned long
iova, int flags, void *da
/* Turn off the hangcheck timer to keep it from bothering us */
del_timer(>hangcheck_timer);
- gpu->fault_info.ttbr0 = info->ttbr0;
+ gpu->fault_info.smmu_info = *info;
gpu->fault_info.iova = iova;
gpu->fault_info.flags = flags;
gpu->fault_info.type = type;
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 748665232d29..42e522a60623 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -685,19 +685,28 @@ void adreno_show(struct msm_gpu *gpu, struct
msm_gpu_state *state,
adreno_gpu->rev.major, adreno_gpu->rev.minor,
adreno_gpu->rev.patchid);
/*
-* If this is state collected due to iova fault, so fault related info
+* If this is state collected due to iova fault, show fault related
+* info
*
-* TTBR0 would not be zero, so this is a good way to distinguish
+* TTBR0 would not be zero in this case, so this is a good way to
+* distinguish
*/
- if (state->fault_info.ttbr0) {
+ if (state->fault_info.smmu_info.ttbr0) {
const struct msm_gpu_fault_info *info = >fault_info;
+ const struct adreno_smmu_fault_info *smmu_info =
>smmu_info;
drm_puts(p, "fault-info:\n");
- drm_printf(p, " - ttbr0=%.16llx\n", info->ttbr0);
- drm_printf(p, " - iova=%.16lx\n", info->iova);
- drm_printf(p, " - dir=%s\n", info->flags & IOMMU_FAULT_WRITE ?
"WRITE" : "READ");
- drm_printf(p, " - type=%s\n", info->type);
- drm_printf(p, " - source=%s\n", info->block);
+ drm_printf(p, " - far: %.16llx\n", smmu_info->far);
+ drm_printf(p, " - ttbr0: %.16llx\n", smmu_info->ttbr0);
+ drm_printf(p, " - contextidr: %.8x\n", smmu_info->contextidr);
+ drm_printf(p, " - fsr: %.8x\n", smmu_info->fsr);
+ drm_printf(p, " - fsynr0: %.8x\n", smmu_info->fsynr0);
+ drm_printf(p, " - fsynr1: %.8x\n", smmu_info->fsynr1);
+ drm_printf(p, " - cbfrsynra: %.8x\n", smmu_info->cbfrsynra);
+ drm_printf(p, " - iova: %.16lx\n", info->iova);
+ drm_printf(p, " - dir: %s\n", info->flags & IOMMU_FAULT_WRITE
? "WRITE" : "READ");
+ drm_printf(p, " - type: %s\n", info->type);
+ drm_printf(p, " - source: %s\n", info->block);
}
drm_printf(p, "rbbm-status: 0x%08x\n", state->rbbm_status);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 9801a965816c..0e132795123f 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -73,7 +73,7 @@ struct msm_gpu_funcs {
/* Additional state for iommu faults: */
struct msm_gpu_fault_info {
- u64 ttbr0;
+ struct adreno_smmu_fault_info smmu_info;
unsigned long iova;
int flags;
const char *type;
--
2.31.1
[Freedreno] [PATCH v2 1/3] iommu/io-pgtable-arm: Add way to debug pgtable walk
From: Rob Clark
Add an io-pgtable method to retrieve the raw PTEs that would be
traversed for a given iova access.
Signed-off-by: Rob Clark
---
drivers/iommu/io-pgtable-arm.c | 40 +++---
include/linux/io-pgtable.h | 9
2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index dd9e47189d0d..c470fc0b3c2b 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -700,38 +700,61 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops,
unsigned long iova,
return arm_lpae_unmap_pages(ops, iova, size, 1, gather);
}
-static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
-unsigned long iova)
+static int arm_lpae_pgtable_walk(struct io_pgtable_ops *ops, unsigned long
iova,
+void *_ptes, int *num_ptes)
{
struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
arm_lpae_iopte pte, *ptep = data->pgd;
+ arm_lpae_iopte *ptes = _ptes;
+ int max_ptes = *num_ptes;
int lvl = data->start_level;
+ *num_ptes = 0;
+
do {
+ if (*num_ptes >= max_ptes)
+ return -ENOSPC;
+
/* Valid IOPTE pointer? */
if (!ptep)
- return 0;
+ return -EFAULT;
/* Grab the IOPTE we're interested in */
ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
pte = READ_ONCE(*ptep);
+ ptes[(*num_ptes)++] = pte;
+
/* Valid entry? */
if (!pte)
- return 0;
+ return -EFAULT;
/* Leaf entry? */
if (iopte_leaf(pte, lvl, data->iop.fmt))
- goto found_translation;
+ return 0;
/* Take it to the next level */
ptep = iopte_deref(pte, data);
} while (++lvl < ARM_LPAE_MAX_LEVELS);
- /* Ran out of page tables to walk */
- return 0;
+ return -EFAULT;
+}
+
+static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
+unsigned long iova)
+{
+ struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ arm_lpae_iopte pte, ptes[ARM_LPAE_MAX_LEVELS];
+ int lvl, num_ptes = ARM_LPAE_MAX_LEVELS;
+ int ret;
+
+ ret = arm_lpae_pgtable_walk(ops, iova, ptes, _ptes);
+ if (ret)
+ return 0;
+
+ pte = ptes[num_ptes - 1];
+ lvl = num_ptes - 1 + data->start_level;
-found_translation:
iova &= (ARM_LPAE_BLOCK_SIZE(lvl, data) - 1);
return iopte_to_paddr(pte, data) | iova;
}
@@ -816,6 +839,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg)
.unmap = arm_lpae_unmap,
.unmap_pages= arm_lpae_unmap_pages,
.iova_to_phys = arm_lpae_iova_to_phys,
+ .pgtable_walk = arm_lpae_pgtable_walk,
};
return data;
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index 86af6f0a00a2..501f362a929c 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -148,6 +148,13 @@ struct io_pgtable_cfg {
* @unmap:Unmap a physically contiguous memory region.
* @unmap_pages: Unmap a range of virtually contiguous pages of the same size.
* @iova_to_phys: Translate iova to physical address.
+ * @pgtable_walk: Return details of a page table walk for a given iova.
+ *This returns the array of PTEs in a format that is
+ *specific to the page table format. The number of
+ *PTEs can be format specific. The num_ptes parameter
+ *on input specifies the size of the ptes array, and
+ *on output the number of PTEs filled in (which depends
+ *on the number of PTEs walked to resolve the iova)
*
* These functions map directly onto the iommu_ops member functions with
* the same names.
@@ -165,6 +172,8 @@ struct io_pgtable_ops {
struct iommu_iotlb_gather *gather);
phys_addr_t (*iova_to_phys)(struct io_pgtable_ops *ops,
unsigned long iova);
+ int (*pgtable_walk)(struct io_pgtable_ops *ops, unsigned long iova,
+ void *ptes, int *num_ptes);
};
/**
--
2.31.1
[Freedreno] [PATCH v2 0/3] io-pgtable-arm + drm/msm: Extend iova fault debugging
From: Rob Clark This series extends io-pgtable-arm with a method to retrieve the page table entries traversed in the process of address translation, and then beefs up drm/msm gpu devcore dump to include this (and additional info) in the devcore dump. The motivation is tracking down an obscure iova fault triggered crash on the address of the IB1 cmdstream. This is one of the few places where the GPU address written into the cmdstream is soley under control of the kernel mode driver, so I don't think it can be a userspace bug. The logged cmdstream from the devcore's I've looked at look correct, and the TTBR0 read back from arm-smmu agrees with the kernel emitted cmdstream. Unfortunately it happens infrequently enough (something like once per 1000hrs of usage, from what I can tell from our telemetry) that actually reproducing it with an instrumented debug kernel is not an option. So further spiffying out the devcore dumps and hoping we can spot a clue is the plan I'm shooting for. See https://gitlab.freedesktop.org/drm/msm/-/issues/8 for more info on the issue I'm trying to debug. v2: Fix an armv7/32b build error in the last patch Rob Clark (3): iommu/io-pgtable-arm: Add way to debug pgtable walk drm/msm: Show all smmu info for iova fault devcore dumps drm/msm: Extend gpu devcore dumps with pgtbl info drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 35 +- drivers/gpu/drm/msm/msm_gpu.c | 10 +++ drivers/gpu/drm/msm/msm_gpu.h | 10 ++- drivers/gpu/drm/msm/msm_iommu.c | 17 +++ drivers/gpu/drm/msm/msm_mmu.h | 2 ++ drivers/iommu/io-pgtable-arm.c | 40 - include/linux/io-pgtable.h | 9 ++ 8 files changed, 107 insertions(+), 18 deletions(-) -- 2.31.1
Re: [Freedreno] [PATCH v1 2/4] arm64: dts: qcom: sc7280: add display dt nodes
On 2021-10-05 07:21, Stephen Boyd wrote:
Quoting mkri...@codeaurora.org (2021-09-30 23:39:07)
On 2021-09-30 23:28, Stephen Boyd wrote:
> Quoting mkri...@codeaurora.org (2021-09-30 04:56:59)
>> On 2021-08-19 01:27, Stephen Boyd wrote:
>> > 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
>> >> +
>> >> + status = "disabled";
>> >> +
>> >> + mdp: mdp@ae01000 {
>> >
>> > display-controller@ae01000
>>
>> Stephen,
>> In the current driver code, there is a substring comparison for
>> "mdp"
>> in device node name as part of probe sequence. If "mdp" is not present
>> in the node name, it will
>> return an error resulting in probe failure. Can we continue using
>> mdp
>> as nodename instead of display controller?
>>
>
> Can we fix the driver to not look for node names and look for
> compatible
> strings instead? It took me a minute to find compare_name_mdp() in
> drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about.
> Perhaps looking for qcom,mdp5 in there will be sufficient instead of
> looking at the node name.
Sure Stephen. I will make the necessary changes in msm_drv.c to look
for
compatible string instead of node name.
Can I include these two changes (changing mdp--> display controller
and
msm_drv.c changes) in a separate series ?
Sure. So you'll send the drm driver change now and we'll get the DT
change after that with the more generic node name?
Yes Stephen.I have raised the change to fix the driver issue.
https://patchwork.kernel.org/project/linux-arm-msm/patch/1633427071-19523-1-git-send-email-mkri...@codeaurora.org/
Regards,
Krishna
[Freedreno] [PATCH v1] drm/msm: use compatible string to find mdp node
In the current implementation, substring comparison
using device node name is used to find mdp node
during driver probe. Use compatible string instead
of node name to get mdp node from the parent mdss node.
Signed-off-by: Krishna Manikandan
---
drivers/gpu/drm/msm/msm_drv.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 2e6fc18..50a23cf 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1241,9 +1241,16 @@ static int add_components_mdp(struct device *mdp_dev,
return 0;
}
-static int compare_name_mdp(struct device *dev, void *data)
+static int find_mdp_node(struct device *dev, void *data)
{
- return (strstr(dev_name(dev), "mdp") != NULL);
+ return of_device_is_compatible(dev->of_node, "qcom,mdp4") ||
+ of_device_is_compatible(dev->of_node, "qcom,mdp5") ||
+ of_device_is_compatible(dev->of_node, "qcom,mdss_mdp") ||
+ of_device_is_compatible(dev->of_node, "qcom,sdm845-dpu") ||
+ of_device_is_compatible(dev->of_node, "qcom,sm8150-dpu") ||
+ of_device_is_compatible(dev->of_node, "qcom,sm8250-dpu") ||
+ of_device_is_compatible(dev->of_node, "qcom,sc7180-dpu") ||
+ of_device_is_compatible(dev->of_node, "qcom,sc7280-dpu");
}
static int add_display_components(struct platform_device *pdev,
@@ -1268,7 +1275,7 @@ static int add_display_components(struct platform_device
*pdev,
return ret;
}
- mdp_dev = device_find_child(dev, NULL, compare_name_mdp);
+ mdp_dev = device_find_child(dev, NULL, find_mdp_node);
if (!mdp_dev) {
DRM_DEV_ERROR(dev, "failed to find MDSS MDP node\n");
of_platform_depopulate(dev);
--
2.7.4
Re: [Freedreno] [PATCH] drm/msm/disp: fix endian bug in debugfs code
On Tue, Oct 05, 2021 at 02:31:12AM +0300, Dmitry Baryshkov wrote:
> On 04/10/2021 16:47, Dan Carpenter wrote:
> > The "vbif->features" is type unsigned long but the debugfs file
> > is treating it as a u32 type. This will work in little endian
> > systems, but the correct thing is to change the debugfs to use
> > an unsigned long.
> >
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> > Signed-off-by: Dan Carpenter
> > ---
> > You might wonder why this code has so many casts. It's required because
> > this data is const. Which is fine because the file is read only.
> >
> > drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > index 21d20373eb8b..e645a886e3c6 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > @@ -305,8 +305,8 @@ void dpu_debugfs_vbif_init(struct dpu_kms *dpu_kms,
> > struct dentry *debugfs_root)
> > debugfs_vbif = debugfs_create_dir(vbif_name, entry);
> > - debugfs_create_u32("features", 0600, debugfs_vbif,
> > - (u32 *)>features);
> > + debugfs_create_ulong("features", 0600, debugfs_vbif,
> > +(unsigned long *)>features);
>
> As you are converting this to the ulong file, could you please also remove
> the now-unnecessary type cast?
I wanted to remove all the casting but they are required because of the
const.
regards,
dan carpenter
