| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Feb 2022 10:39:26 -0800
From: Kuogee Hsieh <quic_khsieh@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <dri-devel@...ts.freedesktop.org>, <robdclark@...il.com>,
<sean@...rly.run>, <swboyd@...omium.org>, <vkoul@...nel.org>,
<daniel@...ll.ch>, <airlied@...ux.ie>, <agross@...nel.org>,
<bjorn.andersson@...aro.org>, <quic_abhinavk@...cinc.com>,
<quic_aravindh@...cinc.com>, <quic_sbillaka@...cinc.com>,
<freedreno@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] drm/msm/dp: enable widebus feature for display
port
On 2/15/2022 10:30 AM, Dmitry Baryshkov wrote:
> On Tue, 15 Feb 2022 at 20:49, Kuogee Hsieh <quic_khsieh@...cinc.com> wrote:
>>
>> On 2/15/2022 5:34 AM, Dmitry Baryshkov wrote:
>>> On 15/02/2022 01:39, Kuogee Hsieh wrote:
>>>> Widebus feature will transmit two pixel data per pixel clock to
>>>> interface.
>>>> This feature now is required to be enabled to easy migrant to higher
>>>> resolution applications in future. However since some legacy chipsets
>>>> does not support this feature, this feature is enabled base on chip's
>>>> hardware revision.
>>>>
>>>> changes in v2:
>>>> -- remove compression related code from timing
>>>> -- remove op_info from struct msm_drm_private
>>>> -- remove unnecessary wide_bus_en variables
>>>> -- pass wide_bus_en into timing configuration by struct msm_dp
>>>>
>>>> Changes in v3:
>>>> -- split patch into 3 patches
>>>> -- enable widebus feature base on chip hardware revision
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 +++-
>>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 36
>>>> +++++++++++++++++++++++++++--
>>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 3 ++-
>>>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 13 +++++++----
>>>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
>>>> drivers/gpu/drm/msm/dp/dp_display.c | 30
>>>> ++++++++++++++++++++++++
>>>> drivers/gpu/drm/msm/dp/dp_display.h | 2 ++
>>>> drivers/gpu/drm/msm/dp/dp_panel.c | 4 ++--
>>>> drivers/gpu/drm/msm/dp/dp_panel.h | 2 +-
>>>> drivers/gpu/drm/msm/msm_drv.h | 6 +++++
>>>> 10 files changed, 90 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 0c22839..b2d23c2 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -2167,8 +2167,10 @@ int dpu_encoder_setup(struct drm_device *dev,
>>>> struct drm_encoder *enc,
>>>> timer_setup(&dpu_enc->vsync_event_timer,
>>>> dpu_encoder_vsync_event_handler,
>>>> 0);
>>>> - else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>>> + else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
>>>> dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>> + dpu_enc->wide_bus_en = msm_dp_wide_bus_enable(dpu_enc->dp);
>>>> + }
>>> Please revert the order of the patches and move this chunk into the
>>> DPU patch.
>> There is chicken and egg issue here.
>>
>> I think we should create an communication channel to pass dp/dsi driver
>> info to drm, such as wide_bus_en and dsc compression, etc.
>>
>> By function calls like this will create more unnecessary tangles between
>> driver and drm layer.
> I'm fine with the function call. I'm suggesting another thing. If you
> change the order of these two patches (#1 - dp, #2 - dpu), the
> msm_dp_wide_bus_enable() will get added in patch#1 and will be
> available to be called in the patch#2.
By doing that, we have wile widebus enabled at (#1 -dp) patch but has no
timing engine changes at (#2 -dpu) to support it. This means (#1 -dp)
will break dp until (#2 -dpu) merged.
Is this ok?
>>>> INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>>> dpu_encoder_off_work);
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> index 64f0b26..99d087e 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> @@ -483,6 +483,27 @@ int dp_catalog_ctrl_set_pattern_state_bit(struct
>>>> dp_catalog *dp_catalog,
>>>> }
>>>> /**
>>>> + * dp_catalog_hw_revision() - retrieve DP hw revision
>>>> + *
>>>> + * @dp_catalog: DP catalog structure
>>>> + *
>>>> + * return: u32
>>>> + *
>>>> + * This function return the DP controller hw revision
>>>> + *
>>>> + */
>>>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog)
>>>> +{
>>>> + u32 revision;
>>>> + struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>> + struct dp_catalog_private, dp_catalog);
>>>> +
>>>> + revision = dp_read_ahb(catalog, REG_DP_HW_VERSION);
>>>> +
>>>> + return revision;
>>>> +}
>>>> +
>>>> +/**
>>>> * dp_catalog_ctrl_reset() - reset DP controller
>>>> *
>>>> * @dp_catalog: DP catalog structure
>>>> @@ -739,10 +760,11 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct
>>>> dp_catalog *dp_catalog)
>>>> }
>>>> /* panel related catalog functions */
>>>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog)
>>>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
>>>> wide_bus_en)
>>>> {
>>>> struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>> struct dp_catalog_private, dp_catalog);
>>>> + u32 reg;
>>>> dp_write_link(catalog, REG_DP_TOTAL_HOR_VER,
>>>> dp_catalog->total);
>>>> @@ -751,7 +773,17 @@ int dp_catalog_panel_timing_cfg(struct
>>>> dp_catalog *dp_catalog)
>>>> dp_write_link(catalog, REG_DP_HSYNC_VSYNC_WIDTH_POLARITY,
>>>> dp_catalog->width_blanking);
>>>> dp_write_link(catalog, REG_DP_ACTIVE_HOR_VER,
>>>> dp_catalog->dp_active);
>>>> - dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0);
>>>> +
>>>> + reg = dp_read_p0(catalog, MMSS_DP_INTF_CONFIG);
>>>> +
>>>> + if (wide_bus_en)
>>>> + reg |= BIT(4); /* DATABUS_WIDEN */
>>>> + else
>>>> + reg &= ~BIT(4);
>>>> +
>>>> + DRM_DEBUG_DP("wide_bus_en=%d reg=%x\n", wide_bus_en, reg);
>>>> +
>>>> + dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, reg);
>>>> return 0;
>>>> }
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> index 7dea101..a3a0129 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> @@ -95,6 +95,7 @@ void dp_catalog_ctrl_config_misc(struct dp_catalog
>>>> *dp_catalog, u32 cc, u32 tb);
>>>> void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32
>>>> rate,
>>>> u32 stream_rate_khz, bool fixed_nvid);
>>>> int dp_catalog_ctrl_set_pattern_state_bit(struct dp_catalog
>>>> *dp_catalog, u32 pattern);
>>>> +u32 dp_catalog_hw_revision(struct dp_catalog *dp_catalog);
>>>> void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog);
>>>> bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>>>> void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool
>>>> enable);
>>>> @@ -115,7 +116,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct
>>>> dp_catalog *dp_catalog,
>>>> u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog);
>>>> /* DP Panel APIs */
>>>> -int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog);
>>>> +int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog, bool
>>>> wide_bus_en);
>>>> void dp_catalog_dump_regs(struct dp_catalog *dp_catalog);
>>>> void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog,
>>>> struct drm_display_mode *drm_mode);
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> index 245e1b9..1c4cf9d 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> @@ -154,7 +154,7 @@ static void dp_ctrl_config_ctrl(struct
>>>> dp_ctrl_private *ctrl)
>>>> dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
>>>> }
>>>> -static void dp_ctrl_configure_source_params(struct dp_ctrl_private
>>>> *ctrl)
>>>> +static void dp_ctrl_configure_source_params(struct dp_ctrl_private
>>>> *ctrl, bool wide_bus_en)
>>>> {
>>>> u32 cc, tb;
>>>> @@ -167,7 +167,7 @@ static void
>>>> dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl)
>>>> ctrl->panel->dp_mode.bpp);
>>>> cc = dp_link_get_colorimetry_config(ctrl->link);
>>>> dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
>>>> - dp_panel_timing_cfg(ctrl->panel);
>>>> + dp_panel_timing_cfg(ctrl->panel, wide_bus_en);
>>>> }
>>>> /*
>>>> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>> int ret = 0;
>>>> bool mainlink_ready = false;
>>>> struct dp_ctrl_private *ctrl;
>>>> + u32 pixel_rate_orig;
>>>> if (!dp_ctrl)
>>>> return -EINVAL;
>>>> @@ -1804,6 +1805,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>> ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>>>> + pixel_rate_orig = ctrl->dp_ctrl.pixel_rate;
>>>> + if (dp_ctrl->wide_bus_en)
>>>> + ctrl->dp_ctrl.pixel_rate >>= 1;
>>>> +
>>>> DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>>>> ctrl->link->link_params.rate,
>>>> ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
>>>> @@ -1839,11 +1844,11 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>>> */
>>>> reinit_completion(&ctrl->video_comp);
>>>> - dp_ctrl_configure_source_params(ctrl);
>>>> + dp_ctrl_configure_source_params(ctrl, dp_ctrl->wide_bus_en);
>>>> dp_catalog_ctrl_config_msa(ctrl->catalog,
>>>> ctrl->link->link_params.rate,
>>>> - ctrl->dp_ctrl.pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
>>>> + pixel_rate_orig, dp_ctrl_use_fixed_nvid(ctrl));
>>>> dp_ctrl_setup_tr_unit(ctrl);
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> index 2433edb..4dff44d 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>>>> @@ -17,6 +17,7 @@ struct dp_ctrl {
>>>> bool orientation;
>>>> atomic_t aborted;
>>>> u32 pixel_rate;
>>>> + bool wide_bus_en;
>>>> };
>>>> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index e89556ad..d45a3aa 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -117,6 +117,8 @@ struct dp_display_private {
>>>> struct dp_event event_list[DP_EVENT_Q_MAX];
>>>> spinlock_t event_lock;
>>>> + bool wide_bus_en;
>>>> +
>>>> struct dp_audio *audio;
>>>> };
>>>> @@ -845,6 +847,8 @@ static int dp_display_enable(struct
>>>> dp_display_private *dp, u32 data)
>>>> return 0;
>>>> }
>>>> + dp->ctrl->wide_bus_en = dp->wide_bus_en;
>>>> +
>>>> rc = dp_ctrl_on_stream(dp->ctrl);
>>>> if (!rc)
>>>> dp_display->power_on = true;
>>>> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>>>> dp->connector, dp_mode);
>>>> if (dp_mode->drm_mode.clock)
>>>> dp->max_pclk_khz = dp_mode->drm_mode.clock;
>>>> +
>>>> return ret;
>>>> }
>>>> @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>> *dp_display)
>>>> }
>>>> }
>>>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>>>> +{
>>>> + struct dp_display_private *dp;
>>>> + u32 revision, major, minor;
>>>> +
>>>> + dp = container_of(dp_display, struct dp_display_private,
>>>> dp_display);
>>>> +
>>>> + /* for the time being widebus only support on DP */
>>>> + if (dp_display->connector_type ==
>>>> DRM_MODE_CONNECTOR_DisplayPort) {
>>>> + revision = dp_catalog_hw_revision(dp->catalog);
>>>> + major = ((revision >> 28) & 0x0ff);
>>>> + minor = ((revision >> 16) & 0x0fff);
>>>> +
>>>> + DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
>>>> +
>>>> + if (major >= 1 && minor >= 2)
>>>> + return true;
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
>>>> drm_minor *minor)
>>>> {
>>>> struct dp_display_private *dp;
>>>> @@ -1505,6 +1532,9 @@ int msm_dp_modeset_init(struct msm_dp
>>>> *dp_display, struct drm_device *dev,
>>>> dp_priv->panel->connector = dp_display->connector;
>>>> priv->connectors[priv->num_connectors++] =
>>>> dp_display->connector;
>>>> +
>>>> + dp_priv->wide_bus_en = msm_dp_wide_bus_enable(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 8e80e3b..d9cb9ee 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>>>> @@ -23,6 +23,8 @@ struct msm_dp {
>>>> hdmi_codec_plugged_cb plugged_cb;
>>>> + bool wide_bus_en;
>>>> +
>>>> u32 max_pclk_khz;
>>>> u32 max_dp_lanes;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> index 71db10c..71deb1e 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> @@ -353,7 +353,7 @@ void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>> dp_catalog_dump_regs(catalog);
>>>> }
>>>> -int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en)
>>>> {
>>>> u32 data, total_ver, total_hor;
>>>> struct dp_catalog *catalog;
>>>> @@ -404,7 +404,7 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
>>>> catalog->dp_active = data;
>>>> - dp_catalog_panel_timing_cfg(catalog);
>>>> + dp_catalog_panel_timing_cfg(catalog, wide_bus_en);
>>>> panel->panel_on = true;
>>>> return 0;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> index 9023e5b..5ec341a 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> @@ -57,7 +57,7 @@ struct dp_panel {
>>>> int dp_panel_init_panel_info(struct dp_panel *dp_panel);
>>>> int dp_panel_deinit(struct dp_panel *dp_panel);
>>>> -int dp_panel_timing_cfg(struct dp_panel *dp_panel);
>>>> +int dp_panel_timing_cfg(struct dp_panel *dp_panel, bool wide_bus_en);
>>>> void dp_panel_dump_regs(struct dp_panel *dp_panel);
>>>> int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>>>> struct drm_connector *connector);
>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
>>>> b/drivers/gpu/drm/msm/msm_drv.h
>>>> index 07f6c41..667f3a8 100644
>>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>>> @@ -398,6 +398,7 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>> *dp_display);
>>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct
>>>> msm_dp *dp_display);
>>>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct
>>>> drm_minor *minor);
>>>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);
>>>> #else
>>>> static inline int __init msm_dp_register(void)
>>>> @@ -448,6 +449,11 @@ static inline void msm_dp_debugfs_init(struct
>>>> msm_dp *dp_display,
>>>> {
>>>> }
>>>> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
>>>> +{
>>>> + return false;
>>>> +}
>>>> +
>>>> #endif
>>>> void __init msm_mdp_register(void);
>>>
>
>
Powered by blists - more mailing lists