| Message ID | 20220218145437.18563-1-granquet@baylibre.com |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add mt8195 DisplayPort driver | expand |
Hi, Guillaume: Guillaume Ranquet <granquet@baylibre.com> 於 2022年2月18日 週五 下午10:56寫道： > > Add flexibility by moving the csc_enable bit to board config After replace 'board' with 'SoC', Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > index fcf88dcd8b89d..be99399faf1bb 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -133,6 +133,7 @@ struct mtk_dpi_conf { > u32 hvsize_mask; > u32 channel_swap_shift; > u32 yuv422_en_bit; > + u32 csc_enable_bit; > const struct mtk_dpi_yc_limit *limit; > }; > > @@ -363,7 +364,8 @@ static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool enable) > > static void mtk_dpi_config_csc_enable(struct mtk_dpi *dpi, bool enable) > { > - mtk_dpi_mask(dpi, DPI_CON, enable ? CSC_ENABLE : 0, CSC_ENABLE); > + mtk_dpi_mask(dpi, DPI_CON, enable ? dpi->conf->csc_enable_bit : 0, > + dpi->conf->csc_enable_bit); > } > > static void mtk_dpi_config_swap_input(struct mtk_dpi *dpi, bool enable) > @@ -827,6 +829,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > .yuv422_en_bit = YUV422_EN, > + .csc_enable_bit = CSC_ENABLE, > .limit = &mtk_dpi_limit, > }; > > @@ -843,6 +846,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > .yuv422_en_bit = YUV422_EN, > + .csc_enable_bit = CSC_ENABLE, > .limit = &mtk_dpi_limit, > }; > > @@ -858,6 +862,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > .yuv422_en_bit = YUV422_EN, > + .csc_enable_bit = CSC_ENABLE, > .limit = &mtk_dpi_limit, > }; > > @@ -873,6 +878,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > .yuv422_en_bit = YUV422_EN, > + .csc_enable_bit = CSC_ENABLE, > .limit = &mtk_dpi_limit, > }; > > -- > 2.34.1 >
Il 18/02/22 15:54, Guillaume Ranquet ha scritto: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > Similar to HDMI, DP uses audio infoframes as well which are structured > very similar to the HDMI ones. > > This patch adds a helper function to pack the HDMI audio infoframe for > DP, called hdmi_audio_infoframe_pack_for_dp(). > hdmi_audio_infoframe_pack_only() is split into two parts. One of them > packs the payload only and can be used for HDMI and DP. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > drivers/video/hdmi.c | 83 ++++++++++++++++++++++++++++--------- > include/drm/drm_dp_helper.h | 2 + > include/linux/hdmi.h | 7 +++- > 3 files changed, 72 insertions(+), 20 deletions(-) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index 947be761dfa40..63e74d9fd210e 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -21,6 +21,7 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include <drm/drm_dp_helper.h> > #include <linux/bitops.h> > #include <linux/bug.h> > #include <linux/errno.h> > @@ -381,12 +382,34 @@ static int hdmi_audio_infoframe_check_only(const struct hdmi_audio_infoframe *fr > * > * Returns 0 on success or a negative error code on failure. > */ > -int hdmi_audio_infoframe_check(struct hdmi_audio_infoframe *frame) > +int hdmi_audio_infoframe_check(const struct hdmi_audio_infoframe *frame) > { > return hdmi_audio_infoframe_check_only(frame); > } > EXPORT_SYMBOL(hdmi_audio_infoframe_check); > > +static void > +hdmi_audio_infoframe_pack_payload(const struct hdmi_audio_infoframe *frame, > + u8 *buffer) > +{ > + u8 channels; > + > + if (frame->channels >= 2) > + channels = frame->channels - 1; > + else > + channels = 0; > + > + buffer[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); > + buffer[1] = ((frame->sample_frequency & 0x7) << 2) | > + (frame->sample_size & 0x3); > + buffer[2] = frame->coding_type_ext & 0x1f; > + buffer[3] = frame->channel_allocation; > + buffer[4] = (frame->level_shift_value & 0xf) << 3; > + > + if (frame->downmix_inhibit) > + buffer[4] |= BIT(7); > +} > + > /** > * hdmi_audio_infoframe_pack_only() - write HDMI audio infoframe to binary buffer > * @frame: HDMI audio infoframe > @@ -404,7 +427,6 @@ EXPORT_SYMBOL(hdmi_audio_infoframe_check); > ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, > void *buffer, size_t size) > { > - unsigned char channels; > u8 *ptr = buffer; > size_t length; > int ret; > @@ -420,28 +442,13 @@ ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, > > memset(buffer, 0, size); > > - if (frame->channels >= 2) > - channels = frame->channels - 1; > - else > - channels = 0; > - > ptr[0] = frame->type; > ptr[1] = frame->version; > ptr[2] = frame->length; > ptr[3] = 0; /* checksum */ > > - /* start infoframe payload */ > - ptr += HDMI_INFOFRAME_HEADER_SIZE; > - > - ptr[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); > - ptr[1] = ((frame->sample_frequency & 0x7) << 2) | > - (frame->sample_size & 0x3); > - ptr[2] = frame->coding_type_ext & 0x1f; > - ptr[3] = frame->channel_allocation; > - ptr[4] = (frame->level_shift_value & 0xf) << 3; > - > - if (frame->downmix_inhibit) > - ptr[4] |= BIT(7); > + hdmi_audio_infoframe_pack_payload(frame, > + ptr + HDMI_INFOFRAME_HEADER_SIZE); > > hdmi_infoframe_set_checksum(buffer, length); > > @@ -479,6 +486,44 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, > } > EXPORT_SYMBOL(hdmi_audio_infoframe_pack); > > +/** > + * hdmi_audio_infoframe_pack_for_dp - Pack a HDMI Audio infoframe for > + * displayport This fits in one line (82 cols is ok)... but, anyway, please capitalize D and P in the DisplayPort word. > + * > + * @frame HDMI Audio infoframe You're almost there! You missed a colon after each description. Also, I personally like seeing indented descriptions as, in my opinion, this enhances human readability, as it's a bit more pleasing to the eye... but it's not a requirement, so you be the judge. * @frame: HDMI Audio infoframe * @sdp: Secondary data packet...... * @dp_version: DisplayPort version...... Please fix. > + * @sdp secondary data packet for display port. This is filled with the > + * appropriate data > + * @dp_version Display Port version to be encoded in the header > + * > + * Packs a HDMI Audio Infoframe to be sent over Display Port. This function > + * fills the secondary data packet to be used for Display Port. > + * > + * Return: Number of total written bytes or a negative errno on failure. > + */ > +ssize_t > +hdmi_audio_infoframe_pack_for_dp(const struct hdmi_audio_infoframe *frame, > + struct dp_sdp *sdp, u8 dp_version) > +{ > + int ret; > + > + ret = hdmi_audio_infoframe_check(frame); > + if (ret) > + return ret; > + > + memset(sdp->db, 0, sizeof(sdp->db)); > + > + // Secondary-data packet header Please, C-style comments: /* Secondary-data packet header */ Thanks, Angelo
Il 18/02/22 15:54, Guillaume Ranquet ha scritto: > Add flexibility by moving the yuv422 en bit to board config > s/board/SoC/g After the change, Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
Il 18/02/22 15:54, Guillaume Ranquet ha scritto: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > hot-plug-detection and DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so that > both can work with the same register range. The phy driver sets device > data that is read by the parent to get the phy device that can be used > to control the phy properties. > > This driver is based on an initial version by > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > drivers/gpu/drm/mediatek/Kconfig | 7 + > drivers/gpu/drm/mediatek/Makefile | 2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 2358 ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > 6 files changed, 2937 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > Sorry for the double review, but I've just noticed something critical: the new version of mtk_dp_train_handler is severely misbehaving, producing non-functional display. I also went on with some effort to give you a solution for this, which implies to also tick the DP_STATE with the DP_TRAIN_STATE; This is my take on it: static int mtk_dp_train_handler(struct mtk_dp *mtk_dp) { bool training_done = false; short max_retry = 50; int ret = 0; do { switch (mtk_dp->train_state) { case MTK_DP_TRAIN_STATE_STARTUP: mtk_dp_state_handler(mtk_dp); mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKCAP; break; case MTK_DP_TRAIN_STATE_CHECKCAP: if (mtk_dp_parse_capabilities(mtk_dp)) { mtk_dp->train_info.check_cap_count = 0; mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID; } else { mtk_dp->train_info.check_cap_count++; if (mtk_dp->train_info.check_cap_count > MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT) { mtk_dp->train_info.check_cap_count = 0; mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE; ret = -ETIMEDOUT; } } break; case MTK_DP_TRAIN_STATE_CHECKEDID: mtk_dp->audio_enable = !mtk_dp_edid_parse_audio_capabilities( mtk_dp, &mtk_dp->info.audio_caps); if (!mtk_dp->audio_enable) memset(&mtk_dp->info.audio_caps, 0, sizeof(mtk_dp->info.audio_caps)); mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE; break; case MTK_DP_TRAIN_STATE_TRAINING_PRE: mtk_dp_state_handler(mtk_dp); mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; break; case MTK_DP_TRAIN_STATE_TRAINING: ret = mtk_dp_train_start(mtk_dp); if (ret == 0) { mtk_dp_video_mute(mtk_dp, true); mtk_dp_audio_mute(mtk_dp, true); mtk_dp->train_state = MTK_DP_TRAIN_STATE_NORMAL; mtk_dp_fec_enable(mtk_dp, mtk_dp->has_fec); } else if (ret != -EAGAIN) { mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE; } break; case MTK_DP_TRAIN_STATE_NORMAL: mtk_dp_state_handler(mtk_dp); training_done = true; break; case MTK_DP_TRAIN_STATE_DPIDLE: break; default: break; } if (ret) { if (ret == -EAGAIN) continue; /* * If we get any other error number, it doesn't * make any sense to keep iterating. */ break; } } while (!training_done || --max_retry); return ret; } > +static void mtk_dp_train_handler(struct mtk_dp *mtk_dp) > +{ > + int ret = 0; > + int i = 50; > + > + do { > + if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL) > + continue; > + > + switch (mtk_dp->train_state) { > + case MTK_DP_TRAIN_STATE_STARTUP: > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKCAP; > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKCAP: > + if (mtk_dp_parse_capabilities(mtk_dp)) { > + mtk_dp->train_info.check_cap_count = 0; > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID; > + } else { > + mtk_dp->train_info.check_cap_count++; > + > + if (mtk_dp->train_info.check_cap_count > > + MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT) { > + mtk_dp->train_info.check_cap_count = 0; > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE; > + ret = -ETIMEDOUT; > + } > + } > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKEDID: > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE; > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING_PRE: > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING: > + ret = mtk_dp_train_start(mtk_dp); > + if (!ret) { > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_NORMAL; > + mtk_dp_fec_enable(mtk_dp, mtk_dp->has_fec); > + } else if (ret != -EAGAIN) { > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE; > + } > + > + ret = 0; > + break; > + > + case MTK_DP_TRAIN_STATE_NORMAL: > + break; > + case MTK_DP_TRAIN_STATE_DPIDLE: > + break; > + default: > + break; > + } > + } while (ret && i--); > + > + if (ret) > + drm_err(mtk_dp->drm_dev, "Train handler failed %d\n", ret); > +} > + > +static void mtk_dp_video_enable(struct mtk_dp *mtk_dp, bool enable) > +{ > + if (enable) { > + mtk_dp_set_tx_out(mtk_dp); > + mtk_dp_video_mute(mtk_dp, false); > + } else { > + mtk_dp_video_mute(mtk_dp, true); > + } > +} > + > +static void mtk_dp_video_config(struct mtk_dp *mtk_dp) > +{ > + mtk_dp_mn_overwrite_disable(mtk_dp); > + > + mtk_dp_set_msa(mtk_dp); > + > + mtk_dp_set_color_depth(mtk_dp, mtk_dp->info.depth); > + mtk_dp_set_color_format(mtk_dp, mtk_dp->info.format); > +} > + > +static void mtk_dp_state_handler(struct mtk_dp *mtk_dp) > +{ > + switch (mtk_dp->state) { > + case MTK_DP_STATE_INITIAL: > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->state = MTK_DP_STATE_IDLE; > + break; > + > + case MTK_DP_STATE_IDLE: > + if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL) > + mtk_dp->state = MTK_DP_STATE_PREPARE; > + break; > + > + case MTK_DP_STATE_PREPARE: > + mtk_dp_video_config(mtk_dp); > + mtk_dp_video_enable(mtk_dp, true); > + > + mtk_dp->state = MTK_DP_STATE_NORMAL; > + break; > + > + case MTK_DP_STATE_NORMAL: > + if (mtk_dp->train_state != MTK_DP_TRAIN_STATE_NORMAL) { > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->state = MTK_DP_STATE_IDLE; > + } > + break; > + > + default: > + break; > + } > +} > + Move mtk_dp_train_handler() here..... > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_state) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + struct drm_connector_state *conn_state; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; int ret; > + > + mtk_dp->conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > + bridge->encoder); > + if (!mtk_dp->conn) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector is missing\n"); > + return; > + } > + > + memcpy(mtk_dp->connector_eld, mtk_dp->conn->eld, MAX_ELD_BYTES); > + > + conn_state = > + drm_atomic_get_new_connector_state(old_state->base.state, mtk_dp->conn); > + if (!conn_state) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector state is missing\n"); > + return; > + } > + > + crtc = conn_state->crtc; > + if (!crtc) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector state doesn't have a crtc\n"); > + return; > + } > + > + crtc_state = drm_atomic_get_new_crtc_state(old_state->base.state, crtc); > + if (!crtc_state) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as crtc state is missing\n"); > + return; > + } > + > + mtk_dp_parse_drm_mode_timings(mtk_dp, &crtc_state->adjusted_mode); > + if (!mtk_dp_parse_capabilities(mtk_dp)) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as nothing is plugged in\n"); > + return; > + } > + > + /* Training */ The following is very similar to your previous version of this function: ret = mtk_dp_train_handler(mtk_dp); if (ret) { drm_err(mtk_dp->drm_dev, "Train handler failed %d\n", ret); return; } mtk_dp->enabled = true; mtk_dp_update_plugged_status(mtk_dp) } > + mtk_dp_train_handler(mtk_dp); > + mtk_dp_state_handler(mtk_dp); > + mtk_dp->enabled = true; > +} Regards, Angelo
Hi, Guillaume: On Fri, 2022-02-18 at 15:54 +0100, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > hot-plug-detection and DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > drivers/gpu/drm/mediatek/Kconfig | 7 + > drivers/gpu/drm/mediatek/Makefile | 2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 2358 > ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > 6 files changed, 2937 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > [snip] > + > +static u32 *mtk_dp_bridge_atomic_get_output_bus_fmts(struct > drm_bridge *bridge, > + struct > drm_bridge_state *bridge_state, > + struct > drm_crtc_state *crtc_state, > + struct > drm_connector_state *conn_state, > + unsigned int > *num_output_fmts) > +{ > + u32 *output_fmts; > + > + *num_output_fmts = 0; > + output_fmts = kcalloc(1, sizeof(*output_fmts), GFP_KERNEL); > + if (!output_fmts) > + return NULL; > + *num_output_fmts = 1; > + output_fmts[0] = MEDIA_BUS_FMT_FIXED; > + return output_fmts; > +} > + > +static const u32 mt8195_input_fmts[] = { > + MEDIA_BUS_FMT_RGB888_1X24, > + MEDIA_BUS_FMT_YUV8_1X24, > + MEDIA_BUS_FMT_YUYV8_1X16, > +}; This means DPINTF output format, right? Does DPINTF switch output buffer format? > + > +static u32 *mtk_dp_bridge_atomic_get_input_bus_fmts(struct > drm_bridge *bridge, > + struct > drm_bridge_state *bridge_state, > + struct > drm_crtc_state *crtc_state, > + struct > drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int > *num_input_fmts) > +{ > + u32 *input_fmts; > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + struct drm_display_mode *mode = &crtc_state->adjusted_mode; > + struct drm_display_info *display_info = > + &conn_state->connector->display_info; > + u32 rx_linkrate; > + u32 bpp; > + > + bpp = (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > ? 16 : > + > 24; > + rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000; > + *num_input_fmts = 0; > + input_fmts = kcalloc(ARRAY_SIZE(mt8195_input_fmts), > sizeof(*input_fmts), > + GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + *num_input_fmts = ARRAY_SIZE(mt8195_input_fmts); > + > + memcpy(input_fmts, mt8195_input_fmts, > + sizeof(*input_fmts) * ARRAY_SIZE(mt8195_input_fmts)); > + > + if (((rx_linkrate * mtk_dp->train_info.lane_count) < > + (mode->clock * 24 / 8)) && > + ((rx_linkrate * mtk_dp->train_info.lane_count) > > + (mode->clock * 16 / 8)) && > + (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422)) > { > + kfree(input_fmts); > + input_fmts = kcalloc(1, sizeof(*input_fmts), > GFP_KERNEL); > + *num_input_fmts = 1; > + input_fmts[0] = MEDIA_BUS_FMT_YUYV8_1X16; > + } > + > + return input_fmts; > +} > + [snip] > + > +static int mtk_dp_probe(struct platform_device *pdev) > +{ > + struct mtk_dp *mtk_dp; > + struct device *dev = &pdev->dev; > + int ret; > + int irq_num = 0; > + > + mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL); > + if (!mtk_dp) > + return -ENOMEM; > + > + mtk_dp->dev = dev; > + > + irq_num = platform_get_irq(pdev, 0); > + if (irq_num < 0) { > + dev_err(dev, "failed to request dp irq resource\n"); > + return irq_num; > + } > + > + mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, > 1, 0); > + if (IS_ERR(mtk_dp->next_bridge)) { > + ret = PTR_ERR(mtk_dp->next_bridge); > + dev_err_probe(dev, ret, "Failed to get bridge\n"); > + return ret; > + } > + > + ret = mtk_dp_dt_parse(mtk_dp, pdev); > + if (ret) > + return ret; > + > + mtk_dp_aux_init(mtk_dp); > + > + ret = devm_request_threaded_irq(dev, irq_num, mtk_dp_hpd_event, > + mtk_dp_hpd_event_thread, > + IRQ_TYPE_LEVEL_HIGH, > dev_name(dev), > + mtk_dp); Embedded displayport is always connected, right? Why do we need process hot plug? Move this to the patch of external displayport. > + if (ret) { > + dev_err(dev, "failed to request mediatek dptx irq\n"); > + return -EPROBE_DEFER; > + } > + > + mutex_init(&mtk_dp->dp_lock); Why need dp_lock, please provide the case information. > + mutex_init(&mtk_dp->edid_lock); edid_lock is necessary when irq exist. > + > + platform_set_drvdata(pdev, mtk_dp); > + > + mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek- > dp-phy", > + PLATFORM_DEVID_ > AUTO, > + &mtk_dp->regs, > + sizeof(struct > regmap *)); > + if (IS_ERR(mtk_dp->phy_dev)) { > + dev_err(dev, "Failed to create device mediatek-dp-phy: > %ld\n", > + PTR_ERR(mtk_dp->phy_dev)); > + return PTR_ERR(mtk_dp->phy_dev); > + } > + > + mtk_dp_get_calibration_data(mtk_dp); > + > + mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp"); > + if (IS_ERR(mtk_dp->phy)) { > + dev_err(dev, "Failed to get phy: %ld\n", > PTR_ERR(mtk_dp->phy)); > + platform_device_unregister(mtk_dp->phy_dev); > + return PTR_ERR(mtk_dp->phy); > + } > + > + mtk_dp->bridge.funcs = &mtk_dp_bridge_funcs; > + mtk_dp->bridge.of_node = dev->of_node; > + mtk_dp->bridge.type = DRM_MODE_CONNECTOR_eDP; > + > + mtk_dp->bridge.ops = > + DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | > DRM_BRIDGE_OP_HPD; DRM_BRIDGE_OP_DETECT? DRM_BRIDGE_OP_HPD? > + drm_bridge_add(&mtk_dp->bridge); > + > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); > + > + return 0; > +} > + > [snip] > diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h > b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > new file mode 100644 > index 0000000000000..79952ac30e9e6 > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > @@ -0,0 +1,568 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2019 MediaTek Inc. > + * Copyright (c) 2021 BayLibre > + */ > +#ifndef _MTK_DP_REG_H_ > +#define _MTK_DP_REG_H_ > + > +#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523 > +#define MTK_DP_SIP_ATF_VIDEO_UNMUTE 0x20 > +#define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE 0x21 > + > +#define DP_PHY_GLB_BIAS_GEN_00 0x0000 > +#define RG_XTP_GLB_BIAS_INTR_CTRL (0x1f << 16) > + > +#define DP_PHY_GLB_DPAUX_TX 0x0008 > +#define RG_CKM_PT0_CKTX_IMPSEL (0xf << 20) > + > +#define DP_PHY_LANE_TX_0 0x104 > +#define RG_XTP_LN0_TX_IMPSEL_PMOS (0xf << 12) > +#define RG_XTP_LN0_TX_IMPSEL_NMOS (0xf << 16) > + > +#define DP_PHY_LANE_TX_1 0x204 > +#define RG_XTP_LN1_TX_IMPSEL_PMOS (0xf << 12) > +#define RG_XTP_LN1_TX_IMPSEL_NMOS (0xf << 16) > + > +#define DP_PHY_LANE_TX_2 0x304 > +#define RG_XTP_LN2_TX_IMPSEL_PMOS (0xf << 12) > +#define RG_XTP_LN2_TX_IMPSEL_NMOS (0xf << 16) > + > +#define DP_PHY_LANE_TX_3 0x404 > +#define RG_XTP_LN3_TX_IMPSEL_PMOS (0xf << 12) > +#define RG_XTP_LN3_TX_IMPSEL_NMOS (0xf << 16) These register should be controlled by dp_phy driver? Regards, CK > + > +#define TOP_OFFSET 0x2000 > +#define ENC0_OFFSET 0x3000 > +#define ENC1_OFFSET 0x3200 > +#define TRANS_OFFSET 0x3400 > +#define AUX_OFFSET 0x3600 > +#define SEC_OFFSET 0x4000 > + >
Quoting AngeloGioacchino Del Regno (2022-02-21 15:30:04) > Il 18/02/22 15:54, Guillaume Ranquet ha scritto: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds two helper functions that extract the frequency and word > > length from a struct cea_sad. > > > > For these helper functions new defines are added that help translate the > > 'freq' and 'byte2' fields into real numbers. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > drivers/gpu/drm/drm_edid.c | 74 ++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_edid.h | 18 ++++++++-- > > 2 files changed, 90 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 12893e7be89bb..500279a82167a 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -4747,6 +4747,80 @@ int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb) > > } > > EXPORT_SYMBOL(drm_edid_to_speaker_allocation); > > > > +/** > > + * drm_cea_sad_get_sample_rate - Extract the sample rate from cea_sad > > + * @sad: Pointer to the cea_sad struct > > + * > > + * Extracts the cea_sad frequency field and returns the sample rate in Hz. > > + * > > + * Return: Sample rate in Hz or a negative errno if parsing failed. > > + */ > > +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad) > > +{ > > + switch (sad->freq) { > > + case DRM_CEA_SAD_FREQ_32KHZ: > > + return 32000; > > + case DRM_CEA_SAD_FREQ_44KHZ: > > + return 44100; > > + case DRM_CEA_SAD_FREQ_48KHZ: > > + return 48000; > > + case DRM_CEA_SAD_FREQ_88KHZ: > > + return 88200; > > + case DRM_CEA_SAD_FREQ_96KHZ: > > + return 96000; > > + case DRM_CEA_SAD_FREQ_176KHZ: > > + return 176400; > > + case DRM_CEA_SAD_FREQ_192KHZ: > > + return 192000; > > + default: > > + return -EINVAL; > > + } > > +} > > +EXPORT_SYMBOL(drm_cea_sad_get_sample_rate); > > + > > +static bool drm_cea_sad_is_uncompressed(const struct cea_sad *sad) > > +{ > > + switch (sad->format) { > > + case HDMI_AUDIO_CODING_TYPE_STREAM: > > + case HDMI_AUDIO_CODING_TYPE_PCM: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > +/** > > + * drm_cea_sad_get_uncompressed_word_length - Extract word length > > + * @sad: Pointer to the cea_sad struct > > + * > > + * Extracts the cea_sad byte2 field and returns the word length for an > > + * uncompressed stream. > > + * > > + * Note: This function may only be called for uncompressed audio. > > + * > > + * Return: Word length in bits or a negative errno if parsing failed. > > + */ > > +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad *sad) > > +{ > > + if (!drm_cea_sad_is_uncompressed(sad)) { > > + DRM_WARN("Unable to get the uncompressed word length for a compressed format: %u\n", > > + sad->format); > > + return -EINVAL; > > + } > > + > > + switch (sad->byte2) { > > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT: > > + return 16; > > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT: > > + return 20; > > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT: > > + return 24; > > + default: > > + return -EINVAL; > > + } > > +} > > +EXPORT_SYMBOL(drm_cea_sad_get_uncompressed_word_length); > > + > > /** > > * drm_av_sync_delay - compute the HDMI/DP sink audio-video sync delay > > * @connector: connector associated with the HDMI/DP sink > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > > index 18f6c700f6d02..a30452b313979 100644 > > --- a/include/drm/drm_edid.h > > +++ b/include/drm/drm_edid.h > > @@ -361,12 +361,24 @@ struct edid { > > > > /* Short Audio Descriptor */ > > struct cea_sad { > > - u8 format; > > + u8 format; /* See HDMI_AUDIO_CODING_TYPE_* */ > > Hello Guillaume, > > since you're adding comments to all the rest of the struct members, > I think that a small effort to instead convert this to kerneldoc is > totally worth it. > Can you please do that? > > Thanks, > Angelo > Hello Angelo, Sure, that's a good suggestion. Thx, Guillaume. > > u8 channels; /* max number of channels - 1 */ > > - u8 freq; > > + u8 freq; /* See CEA_SAD_FREQ_* */ > > u8 byte2; /* meaning depends on format */ > > }; > > > > +#define DRM_CEA_SAD_FREQ_32KHZ BIT(0) > > +#define DRM_CEA_SAD_FREQ_44KHZ BIT(1) > > +#define DRM_CEA_SAD_FREQ_48KHZ BIT(2) > > +#define DRM_CEA_SAD_FREQ_88KHZ BIT(3) > > +#define DRM_CEA_SAD_FREQ_96KHZ BIT(4) > > +#define DRM_CEA_SAD_FREQ_176KHZ BIT(5) > > +#define DRM_CEA_SAD_FREQ_192KHZ BIT(6) > > + > > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT BIT(0) > > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT BIT(1) > > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT BIT(2) > > + > > struct drm_encoder; > > struct drm_connector; > > struct drm_connector_state; > > @@ -374,6 +386,8 @@ struct drm_display_mode; > > > > int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); > > int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb); > > +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad); > > +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad *sad); > > int drm_av_sync_delay(struct drm_connector *connector, > > const struct drm_display_mode *mode); > > > >
Quoting AngeloGioacchino Del Regno (2022-02-21 15:30:07) > Il 18/02/22 15:54, Guillaume Ranquet ha scritto: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > Similar to HDMI, DP uses audio infoframes as well which are structured > > very similar to the HDMI ones. > > > > This patch adds a helper function to pack the HDMI audio infoframe for > > DP, called hdmi_audio_infoframe_pack_for_dp(). > > hdmi_audio_infoframe_pack_only() is split into two parts. One of them > > packs the payload only and can be used for HDMI and DP. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > drivers/video/hdmi.c | 83 ++++++++++++++++++++++++++++--------- > > include/drm/drm_dp_helper.h | 2 + > > include/linux/hdmi.h | 7 +++- > > 3 files changed, 72 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > > index 947be761dfa40..63e74d9fd210e 100644 > > --- a/drivers/video/hdmi.c > > +++ b/drivers/video/hdmi.c > > @@ -21,6 +21,7 @@ > > * DEALINGS IN THE SOFTWARE. > > */ > > > > +#include <drm/drm_dp_helper.h> > > #include <linux/bitops.h> > > #include <linux/bug.h> > > #include <linux/errno.h> > > @@ -381,12 +382,34 @@ static int hdmi_audio_infoframe_check_only(const struct hdmi_audio_infoframe *fr > > * > > * Returns 0 on success or a negative error code on failure. > > */ > > -int hdmi_audio_infoframe_check(struct hdmi_audio_infoframe *frame) > > +int hdmi_audio_infoframe_check(const struct hdmi_audio_infoframe *frame) > > { > > return hdmi_audio_infoframe_check_only(frame); > > } > > EXPORT_SYMBOL(hdmi_audio_infoframe_check); > > > > +static void > > +hdmi_audio_infoframe_pack_payload(const struct hdmi_audio_infoframe *frame, > > + u8 *buffer) > > +{ > > + u8 channels; > > + > > + if (frame->channels >= 2) > > + channels = frame->channels - 1; > > + else > > + channels = 0; > > + > > + buffer[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); > > + buffer[1] = ((frame->sample_frequency & 0x7) << 2) | > > + (frame->sample_size & 0x3); > > + buffer[2] = frame->coding_type_ext & 0x1f; > > + buffer[3] = frame->channel_allocation; > > + buffer[4] = (frame->level_shift_value & 0xf) << 3; > > + > > + if (frame->downmix_inhibit) > > + buffer[4] |= BIT(7); > > +} > > + > > /** > > * hdmi_audio_infoframe_pack_only() - write HDMI audio infoframe to binary buffer > > * @frame: HDMI audio infoframe > > @@ -404,7 +427,6 @@ EXPORT_SYMBOL(hdmi_audio_infoframe_check); > > ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, > > void *buffer, size_t size) > > { > > - unsigned char channels; > > u8 *ptr = buffer; > > size_t length; > > int ret; > > @@ -420,28 +442,13 @@ ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, > > > > memset(buffer, 0, size); > > > > - if (frame->channels >= 2) > > - channels = frame->channels - 1; > > - else > > - channels = 0; > > - > > ptr[0] = frame->type; > > ptr[1] = frame->version; > > ptr[2] = frame->length; > > ptr[3] = 0; /* checksum */ > > > > - /* start infoframe payload */ > > - ptr += HDMI_INFOFRAME_HEADER_SIZE; > > - > > - ptr[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); > > - ptr[1] = ((frame->sample_frequency & 0x7) << 2) | > > - (frame->sample_size & 0x3); > > - ptr[2] = frame->coding_type_ext & 0x1f; > > - ptr[3] = frame->channel_allocation; > > - ptr[4] = (frame->level_shift_value & 0xf) << 3; > > - > > - if (frame->downmix_inhibit) > > - ptr[4] |= BIT(7); > > + hdmi_audio_infoframe_pack_payload(frame, > > + ptr + HDMI_INFOFRAME_HEADER_SIZE); > > > > hdmi_infoframe_set_checksum(buffer, length); > > > > @@ -479,6 +486,44 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, > > } > > EXPORT_SYMBOL(hdmi_audio_infoframe_pack); > > > > +/** > > + * hdmi_audio_infoframe_pack_for_dp - Pack a HDMI Audio infoframe for > > + * displayport > > This fits in one line (82 cols is ok)... but, anyway, please capitalize D and P > in the DisplayPort word. > Yeah, I ran clang-format and didn't want to contradict the tool :) I'll fix that for v9 > > + * > > + * @frame HDMI Audio infoframe > > You're almost there! You missed a colon after each description. > Also, I personally like seeing indented descriptions as, in my opinion, this > enhances human readability, as it's a bit more pleasing to the eye... but > it's not a requirement, so you be the judge. > > * @frame: HDMI Audio infoframe > * @sdp: Secondary data packet...... > * @dp_version: DisplayPort version...... > > Please fix. > I completly forgot to generate and check the kerneldoc. Sorry. > > + * @sdp secondary data packet for display port. This is filled with the > > + * appropriate data > > + * @dp_version Display Port version to be encoded in the header > > + * > > + * Packs a HDMI Audio Infoframe to be sent over Display Port. This function > > + * fills the secondary data packet to be used for Display Port. > > + * > > + * Return: Number of total written bytes or a negative errno on failure. > > + */ > > +ssize_t > > +hdmi_audio_infoframe_pack_for_dp(const struct hdmi_audio_infoframe *frame, > > + struct dp_sdp *sdp, u8 dp_version) > > +{ > > + int ret; > > + > > + ret = hdmi_audio_infoframe_check(frame); > > + if (ret) > > + return ret; > > + > > + memset(sdp->db, 0, sizeof(sdp->db)); > > + > > + // Secondary-data packet header > > Please, C-style comments: > > /* Secondary-data packet header */ > > Thanks, > Angelo I think this is not the only case of C++ comment that slipped past me... I'll revise the whole series. Thx, Guillaume.
Quoting AngeloGioacchino Del Regno (2022-02-22 16:16:48) > Il 18/02/22 15:54, Guillaume Ranquet ha scritto: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > Similar to HDMI, DP uses audio infoframes as well which are structured > > very similar to the HDMI ones. > > > > This patch adds a helper function to pack the HDMI audio infoframe for > > DP, called hdmi_audio_infoframe_pack_for_dp(). > > hdmi_audio_infoframe_pack_only() is split into two parts. One of them > > packs the payload only and can be used for HDMI and DP. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Hello Guillaume, > > I've just noticed that this patch will not apply against the latest linux-next, > as the include/drm/drm_dp_helper.h header was moved to > include/drm/dp/drm_dp_helper.h > > Can you please rebase for v9? > > Thanks, > Angelo > I'm sorry, I'm a bit of a noob, I rebased this series against 5.17-rc4 ... I'll rebase v9 against linux-next. Thx, Guillaume. > > --- > > drivers/video/hdmi.c | 83 ++++++++++++++++++++++++++++--------- > > include/drm/drm_dp_helper.h | 2 + > > include/linux/hdmi.h | 7 +++- > > 3 files changed, 72 insertions(+), 20 deletions(-) > >
Quoting Chun-Kuang Hu (2022-02-21 03:32:32) > Hi, Guillaume: > > Guillaume Ranquet <granquet@baylibre.com> 於 2022年2月18日 週五 下午10:56寫道： > > > > Adds a bit of flexibility to support boards without swap_input support > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 545a1337cc899..454f8563efae4 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -126,6 +126,7 @@ struct mtk_dpi_conf { > > const u32 *output_fmts; > > u32 num_output_fmts; > > bool is_ck_de_pol; > > + bool swap_input_support; > > const struct mtk_dpi_yc_limit *limit; > > }; > > > > @@ -378,18 +379,21 @@ static void mtk_dpi_config_color_format(struct mtk_dpi *dpi, > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) { > > mtk_dpi_config_yuv422_enable(dpi, false); > > mtk_dpi_config_csc_enable(dpi, true); > > - mtk_dpi_config_swap_input(dpi, false); > > + if (dpi->conf->swap_input_support) > > + mtk_dpi_config_swap_input(dpi, false); > > mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_BGR); > > } else if ((format == MTK_DPI_COLOR_FORMAT_YCBCR_422) || > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL)) { > > mtk_dpi_config_yuv422_enable(dpi, true); > > mtk_dpi_config_csc_enable(dpi, true); > > - mtk_dpi_config_swap_input(dpi, true); > > + if (dpi->conf->swap_input_support) > > + mtk_dpi_config_swap_input(dpi, true); > > In MT8173, MT2701, MT8183, MT8192, YCBCR_444 would not swap but > YCBCR_422 would swap. But In MT8195, both YCBCR_444 and YCBCR_422 > would not swap, So I think one of these format would be abnormal in > MT8195, right? Or would you provide more information about how this > swap work? > > Regards, > Chun-Kuang. > I'm not sure I have access to that level of information... and my knowledge on mediatek SoC is rather limited, I will circle back with mediatek engineers to have a definite answer. Thx, Guillaume > > mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > } else { > > mtk_dpi_config_yuv422_enable(dpi, false); > > mtk_dpi_config_csc_enable(dpi, false); > > - mtk_dpi_config_swap_input(dpi, false); > > + if (dpi->conf->swap_input_support) > > + mtk_dpi_config_swap_input(dpi, false); > > mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > } > > } > > @@ -808,6 +812,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -819,6 +824,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -829,6 +835,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > > .output_fmts = mt8183_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -839,6 +846,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > > -- > > 2.34.1 > >
Quoting CK Hu (2022-02-25 10:45:26) > Hi, Guillaume: > > On Fri, 2022-02-18 at 15:54 +0100, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > > > It supports the mt8195, the embedded DisplayPort units. It offers > > hot-plug-detection and DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > This driver is based on an initial version by > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Reported-by: kernel test robot <lkp@intel.com> > > --- > > drivers/gpu/drm/mediatek/Kconfig | 7 + > > drivers/gpu/drm/mediatek/Makefile | 2 + > > drivers/gpu/drm/mediatek/mtk_dp.c | 2358 > > ++++++++++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++ > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > > 6 files changed, 2937 insertions(+) > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > > > > > [snip] > > > + > > +static u32 *mtk_dp_bridge_atomic_get_output_bus_fmts(struct > > drm_bridge *bridge, > > + struct > > drm_bridge_state *bridge_state, > > + struct > > drm_crtc_state *crtc_state, > > + struct > > drm_connector_state *conn_state, > > + unsigned int > > *num_output_fmts) > > +{ > > + u32 *output_fmts; > > + > > + *num_output_fmts = 0; > > + output_fmts = kcalloc(1, sizeof(*output_fmts), GFP_KERNEL); > > + if (!output_fmts) > > + return NULL; > > + *num_output_fmts = 1; > > + output_fmts[0] = MEDIA_BUS_FMT_FIXED; > > + return output_fmts; > > +} > > + > > +static const u32 mt8195_input_fmts[] = { > > + MEDIA_BUS_FMT_RGB888_1X24, > > + MEDIA_BUS_FMT_YUV8_1X24, > > + MEDIA_BUS_FMT_YUYV8_1X16, > > +}; > > This means DPINTF output format, right? Does DPINTF switch output > buffer format? > I'll circle back with mediatek engineers, as I don't have a clue here. > > + > > +static u32 *mtk_dp_bridge_atomic_get_input_bus_fmts(struct > > drm_bridge *bridge, > > + struct > > drm_bridge_state *bridge_state, > > + struct > > drm_crtc_state *crtc_state, > > + struct > > drm_connector_state *conn_state, > > + u32 output_fmt, > > + unsigned int > > *num_input_fmts) > > +{ > > + u32 *input_fmts; > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + struct drm_display_mode *mode = &crtc_state->adjusted_mode; > > + struct drm_display_info *display_info = > > + &conn_state->connector->display_info; > > + u32 rx_linkrate; > > + u32 bpp; > > + > > + bpp = (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > > ? 16 : > > + > > 24; > > + rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000; > > + *num_input_fmts = 0; > > + input_fmts = kcalloc(ARRAY_SIZE(mt8195_input_fmts), > > sizeof(*input_fmts), > > + GFP_KERNEL); > > + if (!input_fmts) > > + return NULL; > > + > > + *num_input_fmts = ARRAY_SIZE(mt8195_input_fmts); > > + > > + memcpy(input_fmts, mt8195_input_fmts, > > + sizeof(*input_fmts) * ARRAY_SIZE(mt8195_input_fmts)); > > + > > + if (((rx_linkrate * mtk_dp->train_info.lane_count) < > > + (mode->clock * 24 / 8)) && > > + ((rx_linkrate * mtk_dp->train_info.lane_count) > > > + (mode->clock * 16 / 8)) && > > + (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422)) > > { > > + kfree(input_fmts); > > + input_fmts = kcalloc(1, sizeof(*input_fmts), > > GFP_KERNEL); > > + *num_input_fmts = 1; > > + input_fmts[0] = MEDIA_BUS_FMT_YUYV8_1X16; > > + } > > + > > + return input_fmts; > > +} > > + > > [snip] > > > + > > +static int mtk_dp_probe(struct platform_device *pdev) > > +{ > > + struct mtk_dp *mtk_dp; > > + struct device *dev = &pdev->dev; > > + int ret; > > + int irq_num = 0; > > + > > + mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL); > > + if (!mtk_dp) > > + return -ENOMEM; > > + > > + mtk_dp->dev = dev; > > + > > + irq_num = platform_get_irq(pdev, 0); > > + if (irq_num < 0) { > > + dev_err(dev, "failed to request dp irq resource\n"); > > + return irq_num; > > + } > > + > > + mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, > > 1, 0); > > + if (IS_ERR(mtk_dp->next_bridge)) { > > + ret = PTR_ERR(mtk_dp->next_bridge); > > + dev_err_probe(dev, ret, "Failed to get bridge\n"); > > + return ret; > > + } > > + > > + ret = mtk_dp_dt_parse(mtk_dp, pdev); > > + if (ret) > > + return ret; > > + > > + mtk_dp_aux_init(mtk_dp); > > + > > + ret = devm_request_threaded_irq(dev, irq_num, mtk_dp_hpd_event, > > + mtk_dp_hpd_event_thread, > > + IRQ_TYPE_LEVEL_HIGH, > > dev_name(dev), > > + mtk_dp); > > Embedded displayport is always connected, right? Why do we need process > hot plug? Move this to the patch of external displayport. > That was my initial plan, remove all the HPD related "stuff"... but something is being done in the irq that is needed to get eDP working. I haven't been able to retrieve the EDID with the irq removed. As I'm not knowledgeable in this domain or on the architecture, I couldn't get to the bottom of it with the available documentation I have. Probably something is done in the irq for eDP that should happen outside? I will try for v9 to remove hot plug detection entirely for the initial eDP support. > > + if (ret) { > > + dev_err(dev, "failed to request mediatek dptx irq\n"); > > + return -EPROBE_DEFER; > > + } > > + > > + mutex_init(&mtk_dp->dp_lock); > > Why need dp_lock, please provide the case information. > > > + mutex_init(&mtk_dp->edid_lock); > > edid_lock is necessary when irq exist. > > > + > > + platform_set_drvdata(pdev, mtk_dp); > > + > > + mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek- > > dp-phy", > > + PLATFORM_DEVID_ > > AUTO, > > + &mtk_dp->regs, > > + sizeof(struct > > regmap *)); > > + if (IS_ERR(mtk_dp->phy_dev)) { > > + dev_err(dev, "Failed to create device mediatek-dp-phy: > > %ld\n", > > + PTR_ERR(mtk_dp->phy_dev)); > > + return PTR_ERR(mtk_dp->phy_dev); > > + } > > + > > + mtk_dp_get_calibration_data(mtk_dp); > > + > > + mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp"); > > + if (IS_ERR(mtk_dp->phy)) { > > + dev_err(dev, "Failed to get phy: %ld\n", > > PTR_ERR(mtk_dp->phy)); > > + platform_device_unregister(mtk_dp->phy_dev); > > + return PTR_ERR(mtk_dp->phy); > > + } > > + > > + mtk_dp->bridge.funcs = &mtk_dp_bridge_funcs; > > + mtk_dp->bridge.of_node = dev->of_node; > > + mtk_dp->bridge.type = DRM_MODE_CONNECTOR_eDP; > > + > > + mtk_dp->bridge.ops = > > + DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | > > DRM_BRIDGE_OP_HPD; > > DRM_BRIDGE_OP_DETECT? DRM_BRIDGE_OP_HPD? > > > + drm_bridge_add(&mtk_dp->bridge); > > + > > + pm_runtime_enable(dev); > > + pm_runtime_get_sync(dev); > > + > > + return 0; > > +} > > + > > > > [snip] > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h > > b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > > new file mode 100644 > > index 0000000000000..79952ac30e9e6 > > --- /dev/null > > +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > > @@ -0,0 +1,568 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2019 MediaTek Inc. > > + * Copyright (c) 2021 BayLibre > > + */ > > +#ifndef _MTK_DP_REG_H_ > > +#define _MTK_DP_REG_H_ > > + > > +#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523 > > +#define MTK_DP_SIP_ATF_VIDEO_UNMUTE 0x20 > > +#define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE 0x21 > > + > > +#define DP_PHY_GLB_BIAS_GEN_00 0x0000 > > +#define RG_XTP_GLB_BIAS_INTR_CTRL (0x1f << 16) > > + > > +#define DP_PHY_GLB_DPAUX_TX 0x0008 > > +#define RG_CKM_PT0_CKTX_IMPSEL (0xf << 20) > > + > > +#define DP_PHY_LANE_TX_0 0x104 > > +#define RG_XTP_LN0_TX_IMPSEL_PMOS (0xf << 12) > > +#define RG_XTP_LN0_TX_IMPSEL_NMOS (0xf << 16) > > + > > +#define DP_PHY_LANE_TX_1 0x204 > > +#define RG_XTP_LN1_TX_IMPSEL_PMOS (0xf << 12) > > +#define RG_XTP_LN1_TX_IMPSEL_NMOS (0xf << 16) > > + > > +#define DP_PHY_LANE_TX_2 0x304 > > +#define RG_XTP_LN2_TX_IMPSEL_PMOS (0xf << 12) > > +#define RG_XTP_LN2_TX_IMPSEL_NMOS (0xf << 16) > > + > > +#define DP_PHY_LANE_TX_3 0x404 > > +#define RG_XTP_LN3_TX_IMPSEL_PMOS (0xf << 12) > > +#define RG_XTP_LN3_TX_IMPSEL_NMOS (0xf << 16) > > These register should be controlled by dp_phy driver? > > Regards, > CK > > > + > > +#define TOP_OFFSET 0x2000 > > +#define ENC0_OFFSET 0x3000 > > +#define ENC1_OFFSET 0x3200 > > +#define TRANS_OFFSET 0x3400 > > +#define AUX_OFFSET 0x3600 > > +#define SEC_OFFSET 0x4000 > > + > > >
Hi, Guillaume: On Fri, 2022-02-18 at 15:54 +0100, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > hot-plug-detection and DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > drivers/gpu/drm/mediatek/Kconfig | 7 + > drivers/gpu/drm/mediatek/Makefile | 2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 2358 > ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > 6 files changed, 2937 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > [snip] > + > +struct mtk_dp_train_info { > + bool tps3; > + bool tps4; > + bool sink_ssc; > + bool cable_plugged_in; Move to external display port patch. > + bool cable_state_change; Move to external display port patch. > + bool cr_done; > + bool eq_done; > + > + // link_rate is in multiple of 0.27Gbps > + int link_rate; > + int lane_count; > + > + int irq_status; Move to external display port patch. > + int check_cap_count; > +}; > + > [snip] > + > +// Same values as used for DP Spec MISC0 bits 5,6,7 > +enum mtk_dp_color_depth { > + MTK_DP_COLOR_DEPTH_6BIT = 0, > + MTK_DP_COLOR_DEPTH_8BIT = 1, Only 8bits is used, so drop other definition. > + MTK_DP_COLOR_DEPTH_10BIT = 2, > + MTK_DP_COLOR_DEPTH_12BIT = 3, > + MTK_DP_COLOR_DEPTH_16BIT = 4, > + MTK_DP_COLOR_DEPTH_UNKNOWN = 5, > +}; > + > [snip] > + > +struct mtk_dp { > + struct device *dev; > + struct platform_device *phy_dev; > + struct phy *phy; > + struct dp_cal_data cal_data; > + > + struct drm_device *drm_dev; > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + struct drm_dp_aux aux; > + > + /* Protects edid as it is used in both bridge ops and IRQ > handler */ > + struct mutex edid_lock; > + struct edid *edid; > + > + u8 rx_cap[DP_RECEIVER_CAP_SIZE]; > + > + struct mtk_dp_info info; > + enum mtk_dp_state state; > + > + struct mtk_dp_train_info train_info; > + enum mtk_dp_train_state train_state; > + unsigned int input_fmt; > + > + struct regmap *regs; > + struct clk *dp_tx_clk; > + > + bool enabled; > + > + bool has_fec; > + /* Protects the mtk_dp struct */ > + struct mutex dp_lock; > + > + hdmi_codec_plugged_cb plugged_cb; > + struct device *codec_dev; > + u8 connector_eld[MAX_ELD_BYTES]; Move this to dp audio patch. > + struct drm_connector *conn; > +}; > + > [snip] > + > +static void mtk_dp_set_color_format(struct mtk_dp *mtk_dp, > + enum mtk_dp_color_format > color_format) > +{ > + u32 val; > + > + mtk_dp->info.format = color_format; When call into this function from mtk_dp_video_config(), it calls mtk_dp_set_color_format(mtk_dp, mtk_dp->info.format); It looks weird that you pass mtk_dp->info.format into this function and set it to it. So I would like this function to be pure register setting, and move this variable keeping out of this function. > + > + // Update MISC0 > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > + color_format << DP_TEST_COLOR_FORMAT_SHIFT, > + DP_TEST_COLOR_FORMAT_MASK); > + > + switch (color_format) { > + case MTK_DP_COLOR_FORMAT_YUV_422: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422; > + break; > + case MTK_DP_COLOR_FORMAT_YUV_420: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420; > + break; > + case MTK_DP_COLOR_FORMAT_YONLY: > + case MTK_DP_COLOR_FORMAT_RAW: > + case MTK_DP_COLOR_FORMAT_RESERVED: > + case MTK_DP_COLOR_FORMAT_UNKNOWN: > + drm_warn(mtk_dp->drm_dev, "Unsupported color format: > %d\n", > + color_format); > + fallthrough; > + case MTK_DP_COLOR_FORMAT_RGB_444: > + case MTK_DP_COLOR_FORMAT_YUV_444: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB; > + break; > + } > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val, > + PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK); > +} > + > [snip] > + > +static void mtk_dp_pg_disable(struct mtk_dp *mtk_dp) > +{ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3038, 0, > + VIDEO_SOURCE_SEL_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_31B0, > + 4 << PGEN_PATTERN_SEL_SHIFT, > PGEN_PATTERN_SEL_MASK); > +} > + > +static bool mtk_dp_plug_state(struct mtk_dp *mtk_dp) Move this function to external dp patch. > +{ > + return !!(mtk_dp_read(mtk_dp, MTK_DP_TRANS_P0_3414) & > + HPD_DB_DP_TRANS_P0_MASK); > +} > + > [snip] > + > +static void mtk_dp_train_set_pattern(struct mtk_dp *mtk_dp, int > pattern) > +{ > + if (pattern < 0 || pattern > 4) { Never happen, remove this checking. > + drm_err(mtk_dp->drm_dev, > + "Implementation error, no such pattern %d\n", > pattern); > + return; > + } > + > + if (pattern == 1) // TPS1 > + mtk_dp_set_idle_pattern(mtk_dp, false); > + > + mtk_dp_update_bits(mtk_dp, > + MTK_DP_TRANS_P0_3400, > + pattern ? BIT(pattern - 1) << > PATTERN1_EN_DP_TRANS_P0_SHIFT : 0, > + PATTERN1_EN_DP_TRANS_P0_MASK | > PATTERN2_EN_DP_TRANS_P0_MASK | > + PATTERN3_EN_DP_TRANS_P0_MASK | > + PATTERN4_EN_DP_TRANS_P0_MASK); > +} > + > [snip] > + > +static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp) > +{ > + u32 sram_read_start = MTK_DP_TBC_BUF_READ_START_ADDR; It's not necessary to have a initial value. > + > + if (mtk_dp->train_info.lane_count > 0) { > + sram_read_start = min_t(u32, > + MTK_DP_TBC_BUF_READ_START_ADDR, > + mtk_dp->info.timings.vm.hactive > / > + (mtk_dp->train_info.lane_count > * 4 * 2 * 2)); > + mtk_dp_set_sram_read_start(mtk_dp, sram_read_start); > + } > + > + mtk_dp_setup_encoder(mtk_dp); > +} > + > [snip] > +static void mtk_dp_train_handler(struct mtk_dp *mtk_dp) > +{ > + int ret = 0; > + int i = 50; > + > + do { > + if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL) > + continue; > + > + switch (mtk_dp->train_state) { > + case MTK_DP_TRAIN_STATE_STARTUP: > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_CHECKCAP; > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKCAP: > + if (mtk_dp_parse_capabilities(mtk_dp)) { > + mtk_dp->train_info.check_cap_count = 0; > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_CHECKEDID; > + } else { > + mtk_dp->train_info.check_cap_count++; > + > + if (mtk_dp->train_info.check_cap_count > > > + MTK_DP_CHECK_SINK_CAP_TIMEOUT_C > OUNT) { > + mtk_dp- > >train_info.check_cap_count = 0; > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_DPIDLE; > + ret = -ETIMEDOUT; > + } > + } > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKEDID: > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_TRAINING_PRE; > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING_PRE: > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_TRAINING; > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING: > + ret = mtk_dp_train_start(mtk_dp); > + if (!ret) { > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_NORMAL; > + mtk_dp_fec_enable(mtk_dp, mtk_dp- > >has_fec); > + } else if (ret != -EAGAIN) { > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_DPIDLE; > + } > + > + ret = 0; > + break; > + > + case MTK_DP_TRAIN_STATE_NORMAL: > + break; > + case MTK_DP_TRAIN_STATE_DPIDLE: > + break; > + default: You have list all 7 states, why need default? > + break; > + } > + } while (ret && i--); Why keep in this loop while error happen? > + > + if (ret) > + drm_err(mtk_dp->drm_dev, "Train handler failed %d\n", > ret); > +} > + > [snip] > +static void mtk_dp_state_handler(struct mtk_dp *mtk_dp) > +{ > + switch (mtk_dp->state) { > + case MTK_DP_STATE_INITIAL: > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->state = MTK_DP_STATE_IDLE; > + break; > + > + case MTK_DP_STATE_IDLE: > + if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL) > + mtk_dp->state = MTK_DP_STATE_PREPARE; > + break; > + > + case MTK_DP_STATE_PREPARE: > + mtk_dp_video_config(mtk_dp); > + mtk_dp_video_enable(mtk_dp, true); > + > + mtk_dp->state = MTK_DP_STATE_NORMAL; > + break; > + > + case MTK_DP_STATE_NORMAL: > + if (mtk_dp->train_state != MTK_DP_TRAIN_STATE_NORMAL) { > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->state = MTK_DP_STATE_IDLE; > + } > + break; > + > + default: There is no default case, so remove this. > + break; > + } > +} > + > [snip] > + > +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge, > + struct drm_connector *connector) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + bool enabled = mtk_dp->enabled; > + struct edid *new_edid = NULL; > + > + if (!enabled) Would DRM core make this happen? > + drm_bridge_chain_pre_enable(bridge); > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, > DP_SET_POWER_D0); > + usleep_range(2000, 5000); > + > + if (mtk_dp_plug_state(mtk_dp)) > + new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc); > + > + if (!enabled) > + drm_bridge_chain_post_disable(bridge); > + > + mutex_lock(&mtk_dp->edid_lock); > + kfree(mtk_dp->edid); > + mtk_dp->edid = NULL; > + > + if (!new_edid) { > + mutex_unlock(&mtk_dp->edid_lock); > + return NULL; > + } > + > + mtk_dp->edid = drm_edid_duplicate(new_edid); > + mutex_unlock(&mtk_dp->edid_lock); > + > + return new_edid; > +} > + > [snip] > + > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_bridge_state > *old_state) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + struct drm_connector_state *conn_state; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + > + mtk_dp->conn = > drm_atomic_get_new_connector_for_encoder(old_state->base.state, > + bridge- > >encoder); > + if (!mtk_dp->conn) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector is > missing\n"); > + return; > + } > + > + memcpy(mtk_dp->connector_eld, mtk_dp->conn->eld, > MAX_ELD_BYTES); > + > + conn_state = > + drm_atomic_get_new_connector_state(old_state- > >base.state, mtk_dp->conn); > + if (!conn_state) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector state is > missing\n"); > + return; > + } > + > + crtc = conn_state->crtc; > + if (!crtc) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector state doesn't > have a crtc\n"); > + return; > + } > + > + crtc_state = drm_atomic_get_new_crtc_state(old_state- > >base.state, crtc); > + if (!crtc_state) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as crtc state is > missing\n"); > + return; > + } > + > + mtk_dp_parse_drm_mode_timings(mtk_dp, &crtc_state- > >adjusted_mode); I think this bridge should implement mode_set() and these code is moved to mode_set(). Regards, CK > + if (!mtk_dp_parse_capabilities(mtk_dp)) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as nothing is plugged > in\n"); > + return; > + } > + > + /* Training */ > + mtk_dp_train_handler(mtk_dp); > + mtk_dp_state_handler(mtk_dp); > + mtk_dp->enabled = true; > +} > + >
On Fri, Feb 18, 2022 at 11:15 PM Guillaume Ranquet <granquet@baylibre.com> wrote: > > From: Jitao Shi <jitao.shi@mediatek.com> > > DP 1.4a Section 2.8.7.1.5.6.1: > A DP Source device shall retry at least seven times upon receiving > AUX_DEFER before giving up the AUX transaction. > > Aux should retry to send msg whether how many bytes. > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>