Re: [Freedreno] [DPU PATCH 2/3] drm/msm/dp: add displayPort driver support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2019-02-14 04:28, chandanu@xxxxxxxxxxxxxx wrote:
Hello Sean
I had few more queries regarding your comments. Can you please provide your feedback? (Queries/responses are present below)

thanks
Chandan

On 2018-10-23 09:28, Sean Paul wrote:
On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote:
Add the needed displayPort files to enable DP driver
on msm target.

"dp_display" module is the main module that calls into
other sub-modules. "dp_drm" file represents the interface
between DRM framework and DP driver.



Hello Sean,
I have responded to all of your comments and queries. I have specified
the comments addressed
in V2 patch series and what I have missed that will be fixed in V3.


Hi Chandan,
A few themes which I've pointed out below, but want to call out at the top so
it's obvious.

- Please remove all superfluous NULL checks at the beginning of functions. Included in this is reflowing initialization of variables and checking that return types still make sense (ie: if a function's only error path is the NULL
  check, make it void).
- Please remove all of the new callbacks and just call functions directly. This should also get rid of the _get/_put functions that allocate/set them up - Please consolidate state structs into one struct with a few substructs,
  there's too much state right now.
- Please #define hardcoded values
- Instead of sprinkling wmb() everywhere, use readl/writel instead of *_relaxed - There are a bunch of races that I found, please audit the code and remove
  races with locks as necessary
- I tried to catch as much dead code as possible, please audit the rest for
  unused structs/variables/functions/etc
- Use drm dp helper structs for link parameter storage


Signed-off-by: Chandan Uddaraju <chandanu@xxxxxxxxxxxxxx>
---
 drivers/gpu/drm/msm/Kconfig                 |    9 +
 drivers/gpu/drm/msm/Makefile                |   15 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c |  206 ++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h |   44 +
 drivers/gpu/drm/msm/dp/dp_aux.c             |  570 ++++++++++
 drivers/gpu/drm/msm/dp/dp_aux.h             |   44 +
drivers/gpu/drm/msm/dp/dp_catalog.c | 1188 ++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_catalog.h         |  144 +++
drivers/gpu/drm/msm/dp/dp_ctrl.c | 1475 +++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_ctrl.h            |   50 +
 drivers/gpu/drm/msm/dp/dp_debug.c           |  507 +++++++++
 drivers/gpu/drm/msm/dp/dp_debug.h           |   81 ++
 drivers/gpu/drm/msm/dp/dp_display.c         |  977 +++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_display.h         |   55 +
 drivers/gpu/drm/msm/dp/dp_drm.c             |  542 ++++++++++
 drivers/gpu/drm/msm/dp/dp_drm.h             |   52 +
 drivers/gpu/drm/msm/dp/dp_extcon.c          |  400 +++++++
 drivers/gpu/drm/msm/dp/dp_extcon.h          |  111 ++
drivers/gpu/drm/msm/dp/dp_link.c | 1549 +++++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_link.h            |  184 ++++
 drivers/gpu/drm/msm/dp/dp_panel.c           |  624 +++++++++++
 drivers/gpu/drm/msm/dp/dp_panel.h           |  121 +++
 drivers/gpu/drm/msm/dp/dp_parser.c          |  679 ++++++++++++
 drivers/gpu/drm/msm/dp/dp_parser.h          |  205 ++++
 drivers/gpu/drm/msm/dp/dp_power.c           |  599 +++++++++++
 drivers/gpu/drm/msm/dp/dp_power.h           |   57 +
 drivers/gpu/drm/msm/dp/dp_reg.h             |  357 ++++++
 drivers/gpu/drm/msm/msm_drv.c               |    2 +
 drivers/gpu/drm/msm/msm_drv.h               |   22 +
 include/drm/drm_dp_helper.h                 |   19 +
 30 files changed, 10887 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_display.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_display.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_link.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_link.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_power.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_power.h
 create mode 100644 drivers/gpu/drm/msm/dp/dp_reg.h



diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
new file mode 100644
index 0000000..5818612
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -0,0 +1,1188 @@
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#define pr_fmt(fmt)	"[drm-dp] %s: " fmt, __func__
+
+#include <linux/delay.h>
+#include <drm/drm_dp_helper.h>
+
+#include "dp_catalog.h"
+#include "dp_reg.h"
+
+#define DP_GET_MSB(x)	(x >> 8)
+#define DP_GET_LSB(x)	(x & 0xff)

These aren't used anywhere

Fixed in V2.

+
+#define dp_read(offset) readl_relaxed((offset))
+#define dp_write(offset, data) writel_relaxed((data), (offset))

These macros aren't useful. I think you'd be much better having a read_/write_
for the different io types, ie:

static inline u32 dp_read_audio(struct dp_catalog *catalog, u32 offset)
{
        return readl_relaxed(catalog->io->dp_link.base + offset);
}

Then you can remove a ton of boilerplate code below getting base;

Fixed in V2.


+
+#define dp_catalog_get_priv(x) { \

static inline struct dp_catalog_private *dp_catalog_get_priv(...)
{
        ...
}

For inline, we have to use 3 different functions (one each for aux,
ctrl and panel).
Looking at "type-checking" w.r.t macro, container_of will through an
error if its not valid pointer.
Please let me know if you still want to change this macro to inline func.



/snip


diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
new file mode 100644
index 0000000..58951df
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -0,0 +1,144 @@
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _DP_CATALOG_H_
+#define _DP_CATALOG_H_
+
+#include "dp_parser.h"
+
+/* interrupts */
+#define DP_INTR_HPD		BIT(0)
+#define DP_INTR_AUX_I2C_DONE	BIT(3)
+#define DP_INTR_WRONG_ADDR	BIT(6)
+#define DP_INTR_TIMEOUT		BIT(9)
+#define DP_INTR_NACK_DEFER	BIT(12)
+#define DP_INTR_WRONG_DATA_CNT	BIT(15)
+#define DP_INTR_I2C_NACK	BIT(18)
+#define DP_INTR_I2C_DEFER	BIT(21)
+#define DP_INTR_PLL_UNLOCKED	BIT(24)
+#define DP_INTR_AUX_ERROR	BIT(27)
+
+#define DP_INTR_READY_FOR_VIDEO		BIT(0)
+#define DP_INTR_IDLE_PATTERN_SENT	BIT(3)
+#define DP_INTR_FRAME_END		BIT(6)
+#define DP_INTR_CRC_UPDATED		BIT(9)
+
+struct dp_catalog_aux {
+	u32 data;
+	u32 isr;
+
+	u32 (*read_data)(struct dp_catalog_aux *aux);
+	int (*write_data)(struct dp_catalog_aux *aux);
+	int (*write_trans)(struct dp_catalog_aux *aux);
+	int (*clear_trans)(struct dp_catalog_aux *aux, bool read);
+	void (*reset)(struct dp_catalog_aux *aux);
+	void (*enable)(struct dp_catalog_aux *aux, bool enable);
+	void (*update_aux_cfg)(struct dp_catalog_aux *aux,
+		struct dp_aux_cfg *cfg, enum dp_phy_aux_config_type type);
+	void (*setup)(struct dp_catalog_aux *aux,
+			struct dp_aux_cfg *aux_cfg);
+	void (*get_irq)(struct dp_catalog_aux *aux, bool cmd_busy);
+};
+
+struct dp_catalog_ctrl {
+	u32 dp_tu;
+	u32 valid_boundary;
+	u32 valid_boundary2;
+	u32 isr;
+
+	void (*state_ctrl)(struct dp_catalog_ctrl *ctrl, u32 state);
+	void (*config_ctrl)(struct dp_catalog_ctrl *ctrl, u32 config);
+	void (*lane_mapping)(struct dp_catalog_ctrl *ctrl);
+	void (*mainlink_ctrl)(struct dp_catalog_ctrl *ctrl, bool enable);
+	void (*config_misc)(struct dp_catalog_ctrl *ctrl, u32 cc, u32 tb);
+	void (*config_msa)(struct dp_catalog_ctrl *ctrl, u32 rate,
+				u32 stream_rate_khz, bool fixed_nvid);
+	void (*set_pattern)(struct dp_catalog_ctrl *ctrl, u32 pattern);
+	void (*reset)(struct dp_catalog_ctrl *ctrl);
+	void (*usb_reset)(struct dp_catalog_ctrl *ctrl, bool flip);
+	bool (*mainlink_ready)(struct dp_catalog_ctrl *ctrl);
+	void (*enable_irq)(struct dp_catalog_ctrl *ctrl, bool enable);
+	void (*hpd_config)(struct dp_catalog_ctrl *ctrl, bool enable);
+	void (*phy_reset)(struct dp_catalog_ctrl *ctrl);
+	void (*phy_lane_cfg)(struct dp_catalog_ctrl *ctrl, bool flipped,
+				u8 lane_cnt);
+	void (*update_vx_px)(struct dp_catalog_ctrl *ctrl, u8 v_level,
+				u8 p_level);
+	void (*get_interrupt)(struct dp_catalog_ctrl *ctrl);
+	void (*update_transfer_unit)(struct dp_catalog_ctrl *ctrl);
+	void (*send_phy_pattern)(struct dp_catalog_ctrl *ctrl,
+			u32 pattern);
+	u32 (*read_phy_pattern)(struct dp_catalog_ctrl *ctrl);
+};
+
+enum dp_catalog_audio_sdp_type {
+	DP_AUDIO_SDP_STREAM,
+	DP_AUDIO_SDP_TIMESTAMP,
+	DP_AUDIO_SDP_INFOFRAME,
+	DP_AUDIO_SDP_COPYMANAGEMENT,
+	DP_AUDIO_SDP_ISRC,
+	DP_AUDIO_SDP_MAX,
+};
+
+enum dp_catalog_audio_header_type {
+	DP_AUDIO_SDP_HEADER_1,
+	DP_AUDIO_SDP_HEADER_2,
+	DP_AUDIO_SDP_HEADER_3,
+	DP_AUDIO_SDP_HEADER_MAX,
+};
+
+struct dp_catalog_audio {
+	enum dp_catalog_audio_sdp_type sdp_type;
+	enum dp_catalog_audio_header_type sdp_header;
+	u32 data;
+
+	void (*init)(struct dp_catalog_audio *audio);
+	void (*enable)(struct dp_catalog_audio *audio);
+	void (*config_acr)(struct dp_catalog_audio *audio);
+	void (*config_sdp)(struct dp_catalog_audio *audio);
+	void (*set_header)(struct dp_catalog_audio *audio);
+	void (*get_header)(struct dp_catalog_audio *audio);
+	void (*safe_to_exit_level)(struct dp_catalog_audio *audio);

None of these are called? So get rid of this entire structure, the functions
that aren't called, and the audio enums above.

Removed all the Audio related APIs in V2.

+};
+
+struct dp_catalog_panel {
+	u32 total;
+	u32 sync_start;
+	u32 width_blanking;
+	u32 dp_active;
+
+	/* TPG */
+	u32 hsync_period;
+	u32 vsync_period;
+	u32 display_v_start;
+	u32 display_v_end;
+	u32 v_sync_width;
+	u32 hsync_ctl;
+	u32 display_hctl;

All of this information is already available from drm core, so it can go.

Will address this in V3.



/snip

+
+static void dp_ctrl_setup_tr_unit(struct dp_ctrl_private *ctrl)
+{
+	u32 dp_tu = 0x0;
+	u32 valid_boundary = 0x0;
+	u32 valid_boundary2 = 0x0;
+	struct dp_vc_tu_mapping_table tu_calc_table;
+
+	dp_ctrl_calc_tu_parameters(ctrl, &tu_calc_table);
+
+	dp_tu |= tu_calc_table.tu_size_minus1;
+	valid_boundary |= tu_calc_table.valid_boundary_link;
+	valid_boundary |= (tu_calc_table.delay_start_link << 16);
+
+	valid_boundary2 |= (tu_calc_table.valid_lower_boundary_link << 1);
+	valid_boundary2 |= (tu_calc_table.upper_boundary_count << 16);
+	valid_boundary2 |= (tu_calc_table.lower_boundary_count << 20);
+
+	if (tu_calc_table.boundary_moderation_en)
+		valid_boundary2 |= BIT(0);
+
+	pr_debug("dp_tu=0x%x, valid_boundary=0x%x, valid_boundary2=0x%x\n",
+			dp_tu, valid_boundary, valid_boundary2);
+
+	ctrl->catalog->dp_tu = dp_tu;
+	ctrl->catalog->valid_boundary = valid_boundary;
+	ctrl->catalog->valid_boundary2 = valid_boundary2;
+
+	ctrl->catalog->update_transfer_unit(ctrl->catalog);

Again here, if you just call the function directly, you can avoid the callback.
If you add function arguments, you can avoid having to store
dp_tu/valid_boundary/
valid_boundary2 entirely.

Will remove the dp_tu/valid_boundary/valid_boundary2 in V3.
+}

After removing all the function ptrs in each structure and removing dp_tu/valid_boundary/valid_boundary2 for dp_catalog_ctrl structure, we can get rid of dp_catalog_ctrl structure. Similarly, even the other aux and panel structures have minimal data that can be removed. If we remove these structures and get read of dp_catalog structure, we have to find a different way to access dp_catalog_private structure present in dp_catalog.c file. Having "static dp_catalog_private *dp_catalog_private_ptr" is acceptable in upstream? Please share if you have
any better suggestion?

+
+static int dp_ctrl_wait4video_ready(struct dp_ctrl_private *ctrl)
+{
+	int ret = 0;
+
+	ret = wait_for_completion_timeout(&ctrl->video_comp, HZ / 2);

Break out the timeout into a #define

Fixed in V2.


+	if (ret <= 0) {
+		pr_err("Link Train timedout\n");
+		ret = -EINVAL;
+	}
+
+	return ret;

You're not checking the return value at the callsite.

Fixed in V2.

+}

/snip


+
+static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel,
+		u32 mode_edid_bpp, u32 mode_pclk_khz)
+{
+	struct drm_dp_link *link_info;
+	const u32 max_supported_bpp = 30, min_supported_bpp = 18;
+	u32 bpp = 0, data_rate_khz = 0;
+
+	bpp = min_t(u32, mode_edid_bpp, max_supported_bpp);
+
+	link_info = &dp_panel->link_info;
+	data_rate_khz = link_info->num_lanes * link_info->rate * 8;
+
+	while (bpp > min_supported_bpp) {
+		if (mode_pclk_khz * bpp <= data_rate_khz)
+			break;
+		bpp -= 6;

Watch out for underflow here.

Will add check in V3 patch series.


Since min_supported_bpp is 18, bpp can never go below 12. I don't think we need to check underflow here.


+	}
+
+	return bpp;
+}
+
+static u32 dp_panel_get_mode_bpp(struct dp_panel *dp_panel,
+		u32 mode_edid_bpp, u32 mode_pclk_khz)
+{
+	struct dp_panel_private *panel;
+	u32 bpp = mode_edid_bpp;
+
+	if (!dp_panel || !mode_edid_bpp || !mode_pclk_khz) {
+		pr_err("invalid input\n");
+		return 0;
+	}
+
+	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+
+	if (dp_panel->video_test)
+		bpp = dp_link_bit_depth_to_bpp(
+				panel->link->test_video.test_bit_depth);
+	else
+		bpp = dp_panel_get_supported_bpp(dp_panel, mode_edid_bpp,
+				mode_pclk_khz);
+
+	return bpp;
+}
+
+static void dp_panel_set_test_mode(struct dp_panel_private *panel,
+		struct dp_display_mode *mode)
+{
+	struct dp_panel_info *pinfo = NULL;


/snip

+
+struct dp_display_mode {
+	struct dp_panel_info timing;
+	u32 capabilities;
+};
+
+struct dp_panel_in {
+	struct device *dev;
+	struct dp_aux *aux;
+	struct dp_link *link;
+	struct dp_catalog_panel *catalog;
+};
+
+struct dp_panel {
+	/* dpcd raw data */
+	u8 dpcd[DP_RECEIVER_CAP_SIZE + 1];

Why + 1?

Will follow up on this in V3.

DP_RECIEVER_CAP_SIZE is 0xf. We need 16 bytes for dpcd read.




+	u8 ds_ports[DP_MAX_DOWNSTREAM_PORTS];
+
+	struct drm_dp_link link_info;
+	struct edid *edid;
+	struct drm_connector *connector;
+	struct dp_panel_info pinfo;
+	bool video_test;
+
+	u32 vic;
+	u32 max_pclk_khz;
+
+	u32 max_bw_code;
+	int (*init_info)(struct dp_panel *dp_panel);
+	int (*deinit)(struct dp_panel *dp_panel);
+	int (*timing_cfg)(struct dp_panel *dp_panel);
+	int (*read_sink_caps)(struct dp_panel *dp_panel,
+		struct drm_connector *connector);
+	u32 (*get_min_req_link_rate)(struct dp_panel *dp_panel);

This function isn't called anywhere

Removed in V2.


/Snip



+
+static int dp_power_pinctrl_set(struct dp_power_private *power, bool active)
+{
+	int rc = -EFAULT;
+	struct pinctrl_state *pin_state;
+	struct dp_parser *parser;
+
+	parser = power->parser;
+
+	if (IS_ERR_OR_NULL(parser->pinctrl.pin))
+		return PTR_ERR(parser->pinctrl.pin);
+
+	pin_state = active ? parser->pinctrl.state_active
+				: parser->pinctrl.state_suspend;
+	if (!IS_ERR_OR_NULL(pin_state)) {

How is this not possible? You check these on initialization.

Will remove this in V3.

GPIO/pin_ctrl entries are optional entries in dtsi for DP since we don't need them for specific target. During init, we allow the probe to go through even though we initialize pin_state or not. So, we have to check this again here.


+		rc = pinctrl_select_state(parser->pinctrl.pin,
+				pin_state);

Combine on 1 line

Fixed in V2.

+		if (rc)
+			pr_err("can not set %s pins\n",
+			       active ? "dp_active"
+			       : "dp_sleep");


Combine on one line. Also return immediately if this is the case.

Fixed in V2.

+	} else {
+		pr_err("invalid '%s' pinstate\n",
+		       active ? "dp_active"
+		       : "dp_sleep");
+	}
+
+	return rc;
+}
+
+static int dp_power_clk_init(struct dp_power_private *power, bool enable)

Split this into init/deinit, the paths don't share code.

Fixed in V2.



/snip



 #define DP_TEST_SINK			    0x270
 # define DP_TEST_SINK_START		    (1 << 0)
+#define DP_TEST_AUDIO_MODE		    0x271
+#define DP_TEST_AUDIO_PATTERN_TYPE	    0x272
+#define DP_TEST_AUDIO_PERIOD_CH1	    0x273
+#define DP_TEST_AUDIO_PERIOD_CH2	    0x274
+#define DP_TEST_AUDIO_PERIOD_CH3	    0x275
+#define DP_TEST_AUDIO_PERIOD_CH4	    0x276
+#define DP_TEST_AUDIO_PERIOD_CH5	    0x277
+#define DP_TEST_AUDIO_PERIOD_CH6	    0x278
+#define DP_TEST_AUDIO_PERIOD_CH7	    0x279
+#define DP_TEST_AUDIO_PERIOD_CH8	    0x27A

 #define DP_FEC_STATUS			    0x280    /* 1.4 */
 # define DP_FEC_DECODE_EN_DETECTED	    (1 << 0)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/freedreno



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux