[Freedreno] [PATCH] drm/msm/dpu: check for null return of devm_kzalloc() in dpu_writeback_init()
Because of the possilble failure of devm_kzalloc(), dpu_wb_conn might be NULL and will cause null pointer derefrence later. Therefore, it might be better to check it and directly return -ENOMEM. Fixes: 77b001acdcfe ("drm/msm/dpu: add the writeback connector layer") Signed-off-by: Hui Tang --- drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c index 088ec990a2f2..2a5a68366582 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c @@ -70,6 +70,8 @@ int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc, int rc = 0; dpu_wb_conn = devm_kzalloc(dev->dev, sizeof(*dpu_wb_conn), GFP_KERNEL); + if (!dpu_wb_conn) + return -ENOMEM; drm_connector_helper_add(_wb_conn->base.base, _wb_conn_helper_funcs); -- 2.17.1
Re: [Freedreno] [PATCH v3 10/12] arm64: dts: qcom: sc8280xp: Define some of the display blocks
On 10/25/2022 8:26 PM, Bjorn Andersson wrote: From: Bjorn Andersson Define the display clock controllers, the MDSS instances, the DP phys and connect these together. Signed-off-by: Bjorn Andersson Signed-off-by: Bjorn Andersson --- Changes since v2: - New patch on list arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 838 + 1 file changed, 838 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi index ed806a6e20f6..8526ed74b7be 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi @@ -4,6 +4,7 @@ * Copyright (c) 2022, Linaro Limited */ +#include #include #include #include @@ -1245,6 +1246,44 @@ usb_1_ssphy: usb3-phy@8903400 { }; }; + mdss1_dp0_phy: phy@8909a00 { + compatible = "qcom,sc8280xp-dp-phy"; + reg = <0 0x08909a00 0 0x19c>, + <0 0x08909200 0 0xec>, + <0 0x08909600 0 0xec>, + <0 0x08909000 0 0x1c8>; + + clocks = < DISP_CC_MDSS_DPTX0_AUX_CLK>, +< DISP_CC_MDSS_AHB_CLK>; + clock-names = "aux", "cfg_ahb"; + + power-domains = < SC8280XP_MX>; + + #clock-cells = <1>; + #phy-cells = <0>; + + status = "disabled"; + }; + + mdss1_dp1_phy: phy@890ca00 { + compatible = "qcom,sc8280xp-dp-phy"; + reg = <0 0x0890ca00 0 0x19c>, + <0 0x0890c200 0 0xec>, + <0 0x0890c600 0 0xec>, + <0 0x0890c000 0 0x1c8>; + + clocks = < DISP_CC_MDSS_DPTX1_AUX_CLK>, +< DISP_CC_MDSS_AHB_CLK>; + clock-names = "aux", "cfg_ahb"; + + power-domains = < SC8280XP_MX>; + + #clock-cells = <1>; + #phy-cells = <0>; + + status = "disabled"; + }; + system-cache-controller@920 { compatible = "qcom,sc8280xp-llcc"; reg = <0 0x0920 0 0x58000>, <0 0x0960 0 0x58000>; @@ -1360,6 +1399,326 @@ usb_1_dwc3: usb@a80 { }; }; + mdss0: display-subsystem@ae0 { + compatible = "qcom,sc8280xp-mdss"; + reg = <0 0x0ae0 0 0x1000>; + reg-names = "mdss"; + + power-domains = < MDSS_GDSC>; + + clocks = < GCC_DISP_AHB_CLK>, +< DISP_CC_MDSS_AHB_CLK>, +< DISP_CC_MDSS_MDP_CLK>; + clock-names = "iface", + "ahb", + "core"; + + resets = < DISP_CC_MDSS_CORE_BCR>; + + interrupts = ; + interrupt-controller; + #interrupt-cells = <1>; + + interconnects = <_noc MASTER_MDP0 0 _virt SLAVE_EBI1 0>, + <_noc MASTER_MDP1 0 _virt SLAVE_EBI1 0>; + interconnect-names = "mdp0-mem", "mdp1-mem"; + + iommus = <_smmu 0x1000 0x402>; + + status = "disabled"; + + #address-cells = <2>; + #size-cells = <2>; + ranges; + + mdss0_mdp: display-controller@ae01000 { + compatible = "qcom,sc8280xp-dpu"; + reg = <0 0x0ae01000 0 0x8f000>, + <0 0x0aeb 0 0x2008>; + reg-names = "mdp", "vbif"; + + clocks = < GCC_DISP_HF_AXI_CLK>, +< GCC_DISP_SF_AXI_CLK>, +< DISP_CC_MDSS_AHB_CLK>, +< DISP_CC_MDSS_MDP_LUT_CLK>, +< DISP_CC_MDSS_MDP_CLK>, +< DISP_CC_MDSS_VSYNC_CLK>; + clock-names = "bus", + "nrt_bus", + "iface", + "lut", + "core", + "vsync"; + + assigned-clocks = < DISP_CC_MDSS_MDP_CLK>, + < DISP_CC_MDSS_VSYNC_CLK>; +
Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
On 11/11/2022 1:45 AM, Daniel Vetter wrote: On Wed, Nov 09, 2022 at 05:44:37PM -0800, Jessica Zhang wrote: On 11/9/2022 5:59 AM, Daniel Vetter wrote: On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote: On 09/11/2022 16:52, Daniel Vetter wrote: On Tue, Nov 08, 2022 at 06:25:29PM +, Simon Ser wrote: On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov wrote: On 29/10/2022 01:59, Jessica Zhang wrote: Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for drm_plane. In addition, add support for setting and getting the values of these properties. COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT represents the format of the color fill. Userspace can set enable solid fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning the COLOR_FILL_FORMAT property to a uint32_t value, and setting the framebuffer to NULL. I suppose that COLOR_FILL should override framebuffer rather than requiring that FB is set to NULL. In other words, if color_filL_format is non-zero, it would make sense to ignore the FB. Then one can use the color_fill_format property to quickly switch between filled plane and FB-backed one. That would be inconsistent with the rest of the KMS uAPI. For instance, the kernel will error out if CRTC has active=0 but a connector is still linked to the CRTC. IOW, the current uAPI errors out if the KMS state is inconsistent. So if the use-case here really is to solid-fill a plane (and not just provide a background color for the crtc overall), then I guess we could also extend addfb to make that happen. We've talked in the past about propertery-fying framebuffer objects, and that would sort out this uapi wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least. But if the use-cases are all background color then just doing the crtc background color would be tons simpler (and likely also easier to support for more hardware). No. The hardware supports multiple color-filled planes, which do not have to cover the whole CRTC. The use case here means the userspace use-case. What the hw can do on any given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not meant to reflect 100% exactly what a specific chip can do, but instead: - provide features userspace actually needs. If you want per-plane fill, you need userspace that makes use of per-plane fill, and if all you have is crtc background, then that's it. Hey Daniel, The userspace use case we're trying to support is the Android HWC SOLID_FILL hint here [1], which is specifying per-plane fill. Does surfaceflinger actually use this for more than background fills? Yes I'm annoying, but if we can simplify the kernel driver implementation burden by asking compositors to do the math and simplify things, then I think we should. AFAIK surfaceflinger allows apps to use this for cases beyond just background fill -- an app, for example, can pass the hint for a plane that only partially covers a screen and the driver would be expected to fill just that ROI. We also need an open source implementation for this that works and is tested end-to-end. There's the drm_hwc project, but last time I've checked there's really not much happpening there unfortunately. FWIW, Simon mentioned in a separate reply that Wayland supports a 1x1 FB support protocol [1] for a similar purpose as this RFC series. I can also create an IGT test meanwhile showing an example of E2E usage. Thanks, Jessica [1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104 -Daniel Thanks, Jessica Zhang [1] https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52 - we should create uapi with an eye towards what's actually possible on a reasonable set of drivers and hw. Sometimes that means a slightly more restricted set so that it's possible to implement in more places, especially if that restricted feature set still gets the job done for userspace. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [Freedreno] [PATCH v4 0/2] Add data-lanes and link-frequencies to dp_out endpoint
On 18/11/2022 18:36, Kuogee Hsieh wrote: Add DP both data-lanes and link-frequencies property to dp_out endpoint and support functions to DP driver. Kuogee Hsieh (2): arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint drm/msm/dp: add support of max dp link rate arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 +++- arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 10 +++- arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 + drivers/gpu/drm/msm/dp/dp_display.c| 4 drivers/gpu/drm/msm/dp/dp_panel.c | 7 +++--- drivers/gpu/drm/msm/dp/dp_panel.h | 1 + drivers/gpu/drm/msm/dp/dp_parser.c | 32 +++--- drivers/gpu/drm/msm/dp/dp_parser.h | 2 ++ 9 files changed, 53 insertions(+), 23 deletions(-) NAK. See comments to v3. They all apply to v4, except the removed pr_err. -- With best wishes Dmitry
[Freedreno] [PATCH v4 2/2] drm/msm/dp: add support of max dp link rate
dp_out endpoint contains both data-lanes and link-frequencies properties. This patch parser dp_out endpoint properties and acquire dp_max_lanes and dp_max_link_rate from respective property. Finally, comparing them against both data lane and link rate read back from sink to ensure both data lane and link rate are supported by platform. In the case there is no data-lanes or link-frequencies property defined at dp_out endpoint, the default value are 4 data lanes with 5.4 Ghz link rate. Changes in v2: -- add max link rate from dtsi Changes in v3: -- parser max_data_lanes and max_dp_link_rate from dp_out endpoint Changes in v4: -- remove unnecessary pr_err() -- add assign default to both max_dp_lanes and max_dp_link_rate Signed-off-by: Kuogee Hsieh --- arch/arm64/boot/dts/qcom/sc7280.dtsi | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 4 drivers/gpu/drm/msm/dp/dp_panel.c| 7 --- drivers/gpu/drm/msm/dp/dp_panel.h| 1 + drivers/gpu/drm/msm/dp/dp_parser.c | 32 drivers/gpu/drm/msm/dp/dp_parser.h | 2 ++ 6 files changed, 36 insertions(+), 11 deletions(-) diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index 1990cc96..7ff9d51a 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -4130,6 +4130,7 @@ reg = <0>; dp_in: endpoint { remote-endpoint = <_intf0_out>; + data-lanes = <0 1 2 3>; }; }; }; diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index bfd0aef..edee550 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -390,6 +390,10 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) struct edid *edid; dp->panel->max_dp_lanes = dp->parser->max_dp_lanes; + dp->panel->max_dp_link_rate = dp->parser->max_dp_link_rate; + + drm_dbg_dp(dp->drm_dev, "max_lanes=%d max_link_rate=%d\n", + dp->panel->max_dp_lanes, dp->panel->max_dp_link_rate); rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector); if (rc) diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 5149ceb..933fa9c 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -75,12 +75,13 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel) link_info->rate = drm_dp_bw_code_to_link_rate(dpcd[DP_MAX_LINK_RATE]); link_info->num_lanes = dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK; + /* Limit data lanes from data-lanes of endpoint properity of dtsi */ if (link_info->num_lanes > dp_panel->max_dp_lanes) link_info->num_lanes = dp_panel->max_dp_lanes; - /* Limit support upto HBR2 until HBR3 support is added */ - if (link_info->rate >= (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4))) - link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4); + /* Limit link rate from link-frequencies of endpoint properity of dtsi */ + if (link_info->rate > dp_panel->max_dp_link_rate) + link_info->rate = dp_panel->max_dp_link_rate; drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor); drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate); diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h index d861197a..f04d021 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.h +++ b/drivers/gpu/drm/msm/dp/dp_panel.h @@ -50,6 +50,7 @@ struct dp_panel { u32 vic; u32 max_dp_lanes; + u32 max_dp_link_rate; u32 max_bw_code; }; diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index dd73221..1895c79 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -94,16 +94,32 @@ static int dp_parser_ctrl_res(struct dp_parser *parser) static int dp_parser_misc(struct dp_parser *parser) { struct device_node *of_node = parser->pdev->dev.of_node; - int len; - - len = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES); - if (len < 0) { - DRM_WARN("Invalid property \"data-lanes\", default max DP lanes = %d\n", -DP_MAX_NUM_DP_LANES); - len = DP_MAX_NUM_DP_LANES; + struct device_node *endpoint; + int cnt; + u32 frequence = 0; + + endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* port@1 */ + + if (endpoint) { + cnt = of_property_count_u32_elems(endpoint, "data-lanes"); + if (cnt < 0) +
[Freedreno] [PATCH v4 0/2] Add data-lanes and link-frequencies to dp_out endpoint
Add DP both data-lanes and link-frequencies property to dp_out endpoint and support functions to DP driver. Kuogee Hsieh (2): arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint drm/msm/dp: add support of max dp link rate arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 +++- arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 10 +++- arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 + drivers/gpu/drm/msm/dp/dp_display.c| 4 drivers/gpu/drm/msm/dp/dp_panel.c | 7 +++--- drivers/gpu/drm/msm/dp/dp_panel.h | 1 + drivers/gpu/drm/msm/dp/dp_parser.c | 32 +++--- drivers/gpu/drm/msm/dp/dp_parser.h | 2 ++ 9 files changed, 53 insertions(+), 23 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[Freedreno] [PATCH v4 1/2] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint
Add both data-lanes and link-frequencies property to dp_out endpoint. Also set link-frequencies to 81 khz at herobrine platform to have max link rate limited at 81 khz (HBR3). Signed-off-by: Kuogee Hsieh --- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 - arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 - arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 10 +- arch/arm64/boot/dts/qcom/sc7280.dtsi | 5 - 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi index eae22e6..b0d6ae9 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi @@ -814,7 +814,14 @@ hp_i2c: { status = "okay"; pinctrl-names = "default"; pinctrl-0 = <_hot_plug_det>; - data-lanes = <0 1>; + ports { + port@1 { + reg = <1>; + dp_out: endpoint { + data-lanes = <0 1>; + }; + }; + }; }; _adc { diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index 58976a1b..2946d8a 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -3116,11 +3116,6 @@ remote-endpoint = <_intf0_out>; }; }; - - port@1 { - reg = <1>; - dp_out: endpoint { }; - }; }; dp_opp_table: opp-table { diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi index c11e371..fc18ab9 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi @@ -442,7 +442,15 @@ ap_i2c_tpm: { status = "okay"; pinctrl-names = "default"; pinctrl-0 = <_hot_plug_det>; - data-lanes = <0 1>; + ports { + port@1 { + reg = <1>; + dp_out: endpoint { + data-lanes = <0 1>; + link-frequencies=<81>; + }; + }; + }; }; _mdp { diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index 2125803..1990cc96 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -4132,11 +4132,6 @@ remote-endpoint = <_intf0_out>; }; }; - - port@1 { - reg = <1>; - dp_out: endpoint { }; - }; }; dp_opp_table: opp-table { -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [Freedreno] [PATCH v3 2/3] drm/msm/disp/dpu1: add helper to know if display is builtin
On 18/11/2022 16:37, Kalyan Thota wrote: -Original Message- From: Dmitry Baryshkov Sent: Friday, November 18, 2022 6:09 PM To: Kalyan Thota (QUIC) ; dri- de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org; freedreno@lists.freedesktop.org; devicet...@vger.kernel.org Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org; diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC) ; Abhinav Kumar (QUIC) Subject: Re: [PATCH v3 2/3] drm/msm/disp/dpu1: add helper to know if display is builtin WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. On 18/11/2022 15:16, Kalyan Thota wrote: Since DRM encoder type for few encoders can be similar (like eDP and DP) find out if the interface supports HPD from encoder bridge to differentiate between builtin and pluggable displays. Changes in v1: - add connector type in the disp_info (Dmitry) - add helper functions to know encoder type - update commit text reflecting the change Changes in v2: - avoid hardcode of connector type for DSI as it may not be true (Dmitry) - get the HPD information from encoder bridge Changes in v3: - use bridge type instead of bridge ops in determining connector (Dmitry) Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 ++ 2 files changed, 33 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 9c6817b..574f2b0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "msm_drv.h" #include "dpu_kms.h" @@ -217,6 +218,32 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 }; +bool dpu_encoder_is_builtin(struct drm_encoder *encoder) { + struct drm_bridge *bridge; + int ops = 0; + + if (!encoder) + return false; + + /* Get last bridge in the chain to determine connector type */ + drm_for_each_bridge_in_chain(encoder, bridge) + if (!drm_bridge_get_next_bridge(bridge)) + ops = bridge->type; Why don't we check the connector type directly? You should not assume that connector's type is equal to the latest bridge's type. if we need to get the type from connector, need to do something as below. Are you thinking on the same lines ? "to_drm_bridge_connector" macro needs to be moved to drm_bridge_connector.h struct drm_bridge_connector *bridge_connector; drm_connector_list_iter_begin(dev, _iter); drm_for_each_connector_iter(connector, _iter) { bridge_connector = to_drm_bridge_connector(connector); if (bridge_connector->encoder == encoder) { type = connector->connector_type; break; } } drm_connector_list_iter_end(_iter); No. You can not depend on the idea that every connector is drm_bridge_connector. Some bridges might create their own connectors. However you can do it in the following way: drm_connector_list_iter_begin(dev, ); drm_for_each_connector_iter(connector, ) { if (connector->possible_encoders & drm_encoder_mask(encoder)) { builtin = (connector->connector_type == DRM_MODE_CONNECTOR_LVDS) || ...; break; } } drm_connector_list_iter_end(); return builtin; + + switch (ops) { + case DRM_MODE_CONNECTOR_Unknown: + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + case DRM_MODE_CONNECTOR_DSI: + case DRM_MODE_CONNECTOR_DPI: + case DRM_MODE_CONNECTOR_WRITEBACK: + case DRM_MODE_CONNECTOR_VIRTUAL: Unknown, WRITEBACK and VIRTUAL are not builtin (for the point of CTM at least). + return true; + default: + return false; + } +} bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 9e7236e..7f3d823 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -224,4 +224,10 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc, */ bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc); +/** + * dpu_encoder_is_builtin - find if the encoder is of type builtin + * @drm_enc:Pointer to previously created drm encoder structure + */ +bool dpu_encoder_is_builtin(struct drm_encoder *drm_enc); + #endif /* __DPU_ENCODER_H__ */ -- With best wishes Dmitry -- With best wishes Dmitry
Re: [Freedreno] [PATCH v2] drm/msm/hdmi: remove unnecessary NULL check
On 11/18/2022 5:03 AM, Dan Carpenter wrote: This code was recently refactored in commit and now the "hdmi" pointer can't be NULL. Checking for NULL leads to a Smatch warning: drivers/gpu/drm/msm/hdmi/hdmi.c:141 msm_hdmi_init() warn: variable dereferenced before check 'hdmi' (see line 119) Fixes: 69a88d8633ec ("drm/msm/hdmi: move resource allocation to probe function") Signed-off-by: Dan Carpenter Reviewed-by: Abhinav Kumar --- v2: Add a Fixes tag. Re-work the commit message. drivers/gpu/drm/msm/hdmi/hdmi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index 7001fabd0977..4d3fdc806bef 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -138,8 +138,7 @@ static int msm_hdmi_init(struct hdmi *hdmi) return 0; fail: - if (hdmi) - msm_hdmi_destroy(hdmi); + msm_hdmi_destroy(hdmi); return ret; }
Re: [Freedreno] [PATCH v3 2/3] drm/msm/disp/dpu1: add helper to know if display is builtin
>-Original Message- >From: Dmitry Baryshkov >Sent: Friday, November 18, 2022 6:09 PM >To: Kalyan Thota (QUIC) ; dri- >de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org; >freedreno@lists.freedesktop.org; devicet...@vger.kernel.org >Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org; >diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC) >; Abhinav Kumar (QUIC) > >Subject: Re: [PATCH v3 2/3] drm/msm/disp/dpu1: add helper to know if display is >builtin > >WARNING: This email originated from outside of Qualcomm. Please be wary of >any links or attachments, and do not enable macros. > >On 18/11/2022 15:16, Kalyan Thota wrote: >> Since DRM encoder type for few encoders can be similar (like eDP and >> DP) find out if the interface supports HPD from encoder bridge to >> differentiate between builtin and pluggable displays. >> >> Changes in v1: >> - add connector type in the disp_info (Dmitry) >> - add helper functions to know encoder type >> - update commit text reflecting the change >> >> Changes in v2: >> - avoid hardcode of connector type for DSI as it may not be true >> (Dmitry) >> - get the HPD information from encoder bridge >> >> Changes in v3: >> - use bridge type instead of bridge ops in determining connector >> (Dmitry) >> >> Signed-off-by: Kalyan Thota >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 >+++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 ++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 9c6817b..574f2b0 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -15,6 +15,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "msm_drv.h" >> #include "dpu_kms.h" >> @@ -217,6 +218,32 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { >> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 >> }; >> >> +bool dpu_encoder_is_builtin(struct drm_encoder *encoder) { >> + struct drm_bridge *bridge; >> + int ops = 0; >> + >> + if (!encoder) >> + return false; >> + >> + /* Get last bridge in the chain to determine connector type */ >> + drm_for_each_bridge_in_chain(encoder, bridge) >> + if (!drm_bridge_get_next_bridge(bridge)) >> + ops = bridge->type; > >Why don't we check the connector type directly? You should not assume that >connector's type is equal to the latest bridge's type. if we need to get the type from connector, need to do something as below. Are you thinking on the same lines ? "to_drm_bridge_connector" macro needs to be moved to drm_bridge_connector.h struct drm_bridge_connector *bridge_connector; drm_connector_list_iter_begin(dev, _iter); drm_for_each_connector_iter(connector, _iter) { bridge_connector = to_drm_bridge_connector(connector); if (bridge_connector->encoder == encoder) { type = connector->connector_type; break; } } drm_connector_list_iter_end(_iter); >> + >> + switch (ops) { >> + case DRM_MODE_CONNECTOR_Unknown: >> + case DRM_MODE_CONNECTOR_LVDS: >> + case DRM_MODE_CONNECTOR_eDP: >> + case DRM_MODE_CONNECTOR_DSI: >> + case DRM_MODE_CONNECTOR_DPI: >> + case DRM_MODE_CONNECTOR_WRITEBACK: >> + case DRM_MODE_CONNECTOR_VIRTUAL: > >Unknown, WRITEBACK and VIRTUAL are not builtin (for the point of CTM at >least). > >> + return true; >> + default: >> + return false; >> + } >> +} >> >> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) >> { >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> index 9e7236e..7f3d823 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> @@ -224,4 +224,10 @@ void dpu_encoder_cleanup_wb_job(struct >drm_encoder *drm_enc, >>*/ >> bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc); >> >> +/** >> + * dpu_encoder_is_builtin - find if the encoder is of type builtin >> + * @drm_enc:Pointer to previously created drm encoder structure >> + */ >> +bool dpu_encoder_is_builtin(struct drm_encoder *drm_enc); >> + >> #endif /* __DPU_ENCODER_H__ */ > >-- >With best wishes >Dmitry
Re: [Freedreno] [PATCH v2 07/18] dt-bindings: msm: dsi-controller-main: Add compatible strings for every current SoC
On 18/11/2022 15:29, Bryan O'Donoghue wrote: On 08/11/2022 12:46, Dmitry Baryshkov wrote: On 08/11/2022 02:56, Bryan O'Donoghue wrote: Currently we do not differentiate between the various users of the qcom,mdss-dsi-ctrl. The driver is flexible enough to operate from one compatible string but, the hardware does have some significant differences in the number of clocks. To facilitate documenting the clocks add the following compatible strings - qcom,mdss-dsi-ctrl-apq8064 Generic comment: I think we'd better follow the arm/qcom-soc.yaml and use qcom,soc-something as compat string. This would leave us with qcom,apq8064-dsi-ctrl I'm not sure if we want to follow the qcm2290 approach and encode the DSI ctrl revision here (6g vs v2). For qcm2290 I'm thinking qcm2290-dsi-ctrl - without the 6g piece. This sounds good too. a) Nobody is using the compat at the moment b) I'm not sure what - if any real information the silicon version number conveys here. + Loic, Shawn --- bod -- With best wishes Dmitry
Re: [Freedreno] [PATCH v1 00/10] iommu/arm-smmu-qcom: Rework Qualcomm SMMU bindings and implementation
On Fri, Nov 18, 2022 at 01:41:24PM +0100, Krzysztof Kozlowski wrote: > On 14/11/2022 18:06, Dmitry Baryshkov wrote: > > The main goal of this patchset is to define a generic qcom,smmu-500 > > binding to be used by newer Qualcomm platforms instead of defining each > > and every SoC line with no actual differences between the compats. > > > > While preparing this change it was required to cleanup the existing > > bindings and to rework the way the arm-smmu-qcom implementation handles > > binding to IOMMU devices. > > > > Changes since RFC v2: > > - Dropped the dts patch, picked up by Bjorn > > - Fixed minor nits in commit messages and in-file comments (noted by > >Krzysztof and Richard Acayan) > > > > Changes since RFC v1: > > - Added the dts patch fixing order of clocks in msm8996.dtsi > > - Fixed the DT bot errors > > - Added separate clause for Google Cheza devices > > Please continue the version numbering. RFC is also a patch and also a > version. This is v3. Your next will be v4. I queued this already, so hopefully there won't be a next version! Will
Re: [Freedreno] [PATCH v2 07/18] dt-bindings: msm: dsi-controller-main: Add compatible strings for every current SoC
On 08/11/2022 12:46, Dmitry Baryshkov wrote: On 08/11/2022 02:56, Bryan O'Donoghue wrote: Currently we do not differentiate between the various users of the qcom,mdss-dsi-ctrl. The driver is flexible enough to operate from one compatible string but, the hardware does have some significant differences in the number of clocks. To facilitate documenting the clocks add the following compatible strings - qcom,mdss-dsi-ctrl-apq8064 Generic comment: I think we'd better follow the arm/qcom-soc.yaml and use qcom,soc-something as compat string. This would leave us with qcom,apq8064-dsi-ctrl I'm not sure if we want to follow the qcm2290 approach and encode the DSI ctrl revision here (6g vs v2). For qcm2290 I'm thinking qcm2290-dsi-ctrl - without the 6g piece. a) Nobody is using the compat at the moment b) I'm not sure what - if any real information the silicon version number conveys here. + Loic, Shawn --- bod
[Freedreno] [PATCH v2] drm/msm/hdmi: remove unnecessary NULL check
This code was recently refactored in commit and now the "hdmi" pointer can't be NULL. Checking for NULL leads to a Smatch warning: drivers/gpu/drm/msm/hdmi/hdmi.c:141 msm_hdmi_init() warn: variable dereferenced before check 'hdmi' (see line 119) Fixes: 69a88d8633ec ("drm/msm/hdmi: move resource allocation to probe function") Signed-off-by: Dan Carpenter --- v2: Add a Fixes tag. Re-work the commit message. drivers/gpu/drm/msm/hdmi/hdmi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index 7001fabd0977..4d3fdc806bef 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -138,8 +138,7 @@ static int msm_hdmi_init(struct hdmi *hdmi) return 0; fail: - if (hdmi) - msm_hdmi_destroy(hdmi); + msm_hdmi_destroy(hdmi); return ret; } -- 2.35.1
Re: [Freedreno] [PATCH v1 03/10] dt-bindings: arm-smmu: add special case for Google Cheza platform
On 14/11/2022 18:06, Dmitry Baryshkov wrote: > Cheza fw does not properly program the GPU aperture to allow the > GPU to update the SMMU pagetables for context switches. The board file > works around this by dropping the "qcom,adreno-smmu" compat string. > Add this usecase to arm,smmu.yaml schema. > > Signed-off-by: Dmitry Baryshkov You already got my tag. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [Freedreno] [PATCH v1 00/10] iommu/arm-smmu-qcom: Rework Qualcomm SMMU bindings and implementation
On 14/11/2022 18:06, Dmitry Baryshkov wrote: > The main goal of this patchset is to define a generic qcom,smmu-500 > binding to be used by newer Qualcomm platforms instead of defining each > and every SoC line with no actual differences between the compats. > > While preparing this change it was required to cleanup the existing > bindings and to rework the way the arm-smmu-qcom implementation handles > binding to IOMMU devices. > > Changes since RFC v2: > - Dropped the dts patch, picked up by Bjorn > - Fixed minor nits in commit messages and in-file comments (noted by >Krzysztof and Richard Acayan) > > Changes since RFC v1: > - Added the dts patch fixing order of clocks in msm8996.dtsi > - Fixed the DT bot errors > - Added separate clause for Google Cheza devices Please continue the version numbering. RFC is also a patch and also a version. This is v3. Your next will be v4. Best regards, Krzysztof
Re: [Freedreno] [PATCH v3 3/3] drm/msm/disp/dpu1: add color management support for the crtc
On 18/11/2022 15:16, Kalyan Thota wrote: Add color management support for the crtc provided there are enough dspps that can be allocated from the catalog. Changes in v1: - cache color enabled state in the dpu crtc obj (Dmitry) - simplify dspp allocation while creating crtc (Dmitry) - register for color when crtc is created (Dmitry) Changes in v2: - avoid primary encoders in the documentation (Dmitry) Changes in v3: - add ctm for builtin encoders (Dmitry) Signed-off-by: Kalyan Thota With two minor nits (stated below) fixed: Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 5 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 6 -- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 7 ++- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 4170fbe..6cacaaf 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1571,7 +1571,7 @@ static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = { /* initialize crtc */ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, - struct drm_plane *cursor) + struct drm_plane *cursor, bool ctm) { struct drm_crtc *crtc = NULL; struct dpu_crtc *dpu_crtc = NULL; @@ -1583,6 +1583,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, crtc = _crtc->base; crtc->dev = dev; + dpu_crtc->color_enabled = ctm; spin_lock_init(_crtc->spin_lock); atomic_set(_crtc->frame_pending, 0); @@ -1604,7 +1605,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, drm_crtc_helper_add(crtc, _crtc_helper_funcs); - drm_crtc_enable_color_mgmt(crtc, 0, true, 0); + drm_crtc_enable_color_mgmt(crtc, 0, dpu_crtc->color_enabled, 0); /* save user friendly CRTC name for later */ snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 539b68b..1ec9517 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -136,6 +136,7 @@ struct dpu_crtc_frame_event { * @enabled : whether the DPU CRTC is currently enabled. updated in the * commit-thread, not state-swap time which is earlier, so * safe to make decisions on during VBLANK on/off work + * @color_enabled : whether crtc supports color management * @feature_list : list of color processing features supported on a crtc * @active_list : list of color processing features are active * @dirty_list: list of color processing features are dirty @@ -164,7 +165,7 @@ struct dpu_crtc { u64 play_count; ktime_t vblank_cb_time; bool enabled; - + bool color_enabled; Keep the empty line after color_enabled please. struct list_head feature_list; struct list_head active_list; struct list_head dirty_list; @@ -269,10 +270,11 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc); * @dev: dpu device * @plane: base plane * @cursor: cursor plane + * @ctm: ctm flag * @Return: new crtc object or error */ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, - struct drm_plane *cursor); + struct drm_plane *cursor, bool ctm); /** * dpu_crtc_register_custom_event - api for enabling/disabling crtc event diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 574f2b0..102612c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -572,6 +572,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc) static struct msm_display_topology dpu_encoder_get_topology( struct dpu_encoder_virt *dpu_enc, struct dpu_kms *dpu_kms, + struct dpu_crtc *dpu_crtc, struct drm_display_mode *mode) { struct msm_display_topology topology = {0}; @@ -600,7 +601,7 @@ static struct msm_display_topology dpu_encoder_get_topology( else topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1; - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) { + if (dpu_crtc->color_enabled) { if (dpu_kms->catalog->dspp && (dpu_kms->catalog->dspp_count >= topology.num_lm)) topology.num_dspp = topology.num_lm; @@ -635,6 +636,7 @@ static int dpu_encoder_virt_atomic_check( struct drm_display_mode *adj_mode;
Re: [Freedreno] [PATCH v3 2/3] drm/msm/disp/dpu1: add helper to know if display is builtin
On 18/11/2022 15:16, Kalyan Thota wrote: Since DRM encoder type for few encoders can be similar (like eDP and DP) find out if the interface supports HPD from encoder bridge to differentiate between builtin and pluggable displays. Changes in v1: - add connector type in the disp_info (Dmitry) - add helper functions to know encoder type - update commit text reflecting the change Changes in v2: - avoid hardcode of connector type for DSI as it may not be true (Dmitry) - get the HPD information from encoder bridge Changes in v3: - use bridge type instead of bridge ops in determining connector (Dmitry) Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 ++ 2 files changed, 33 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 9c6817b..574f2b0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "msm_drv.h" #include "dpu_kms.h" @@ -217,6 +218,32 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 }; +bool dpu_encoder_is_builtin(struct drm_encoder *encoder) +{ + struct drm_bridge *bridge; + int ops = 0; + + if (!encoder) + return false; + + /* Get last bridge in the chain to determine connector type */ + drm_for_each_bridge_in_chain(encoder, bridge) + if (!drm_bridge_get_next_bridge(bridge)) + ops = bridge->type; Why don't we check the connector type directly? You should not assume that connector's type is equal to the latest bridge's type. + + switch (ops) { + case DRM_MODE_CONNECTOR_Unknown: + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + case DRM_MODE_CONNECTOR_DSI: + case DRM_MODE_CONNECTOR_DPI: + case DRM_MODE_CONNECTOR_WRITEBACK: + case DRM_MODE_CONNECTOR_VIRTUAL: Unknown, WRITEBACK and VIRTUAL are not builtin (for the point of CTM at least). + return true; + default: + return false; + } +} bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 9e7236e..7f3d823 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -224,4 +224,10 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc, */ bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc); +/** + * dpu_encoder_is_builtin - find if the encoder is of type builtin + * @drm_enc:Pointer to previously created drm encoder structure + */ +bool dpu_encoder_is_builtin(struct drm_encoder *drm_enc); + #endif /* __DPU_ENCODER_H__ */ -- With best wishes Dmitry
Re: [Freedreno] [PATCH v3 1/3] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder
On 18/11/2022 15:16, Kalyan Thota wrote: Pin each crtc with one encoder. This arrangement will disallow crtc switching between encoders and also will facilitate to advertise certain features on crtc based on encoder type. Changes in v1: - use drm_for_each_encoder macro while iterating through encoder list (Dmitry) Changes in v2: - make sure no encoder miss to have a crtc (Dmitry) - revisit various factors in deciding the crtc count such as num_mixers, num_sspp (Dmitry) Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 17 ++--- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 7a5fabc..4784db8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -763,7 +763,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) drm_for_each_encoder(encoder, dev) num_encoders++; - max_crtc_count = min(catalog->mixer_count, num_encoders); + max_crtc_count = num_encoders; /* Create the planes, keeping track of one primary/cursor per crtc */ for (i = 0; i < catalog->sspp_count; i++) { @@ -795,22 +795,25 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) primary_planes[primary_planes_idx++] = plane; } - max_crtc_count = min(max_crtc_count, primary_planes_idx); + /* +* All the platforms should have at least 1 primary plane for an +* encoder. The below warn should help in setting up the catalog +*/ + WARN_ON(num_encoders > primary_planes_idx); WARN_ON(max_crtc_count > primary_planes_idx) We do not care about encoders number, we care about CRTCs number here. With that fixed: Reviewed-by: Dmitry Baryshkov /* Create one CRTC per encoder */ - for (i = 0; i < max_crtc_count; i++) { + i = 0; + drm_for_each_encoder(encoder, dev) { crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]); if (IS_ERR(crtc)) { ret = PTR_ERR(crtc); return ret; } priv->crtcs[priv->num_crtcs++] = crtc; + encoder->possible_crtcs = 1 << drm_crtc_index(crtc); + i++; } - /* All CRTCs are compatible with all encoders */ - drm_for_each_encoder(encoder, dev) - encoder->possible_crtcs = (1 << priv->num_crtcs) - 1; - return 0; } -- With best wishes Dmitry
[Freedreno] [PATCH v3 2/3] drm/msm/disp/dpu1: add helper to know if display is builtin
Since DRM encoder type for few encoders can be similar (like eDP and DP) find out if the interface supports HPD from encoder bridge to differentiate between builtin and pluggable displays. Changes in v1: - add connector type in the disp_info (Dmitry) - add helper functions to know encoder type - update commit text reflecting the change Changes in v2: - avoid hardcode of connector type for DSI as it may not be true (Dmitry) - get the HPD information from encoder bridge Changes in v3: - use bridge type instead of bridge ops in determining connector (Dmitry) Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 ++ 2 files changed, 33 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 9c6817b..574f2b0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "msm_drv.h" #include "dpu_kms.h" @@ -217,6 +218,32 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 }; +bool dpu_encoder_is_builtin(struct drm_encoder *encoder) +{ + struct drm_bridge *bridge; + int ops = 0; + + if (!encoder) + return false; + + /* Get last bridge in the chain to determine connector type */ + drm_for_each_bridge_in_chain(encoder, bridge) + if (!drm_bridge_get_next_bridge(bridge)) + ops = bridge->type; + + switch (ops) { + case DRM_MODE_CONNECTOR_Unknown: + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + case DRM_MODE_CONNECTOR_DSI: + case DRM_MODE_CONNECTOR_DPI: + case DRM_MODE_CONNECTOR_WRITEBACK: + case DRM_MODE_CONNECTOR_VIRTUAL: + return true; + default: + return false; + } +} bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 9e7236e..7f3d823 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -224,4 +224,10 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc, */ bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc); +/** + * dpu_encoder_is_builtin - find if the encoder is of type builtin + * @drm_enc:Pointer to previously created drm encoder structure + */ +bool dpu_encoder_is_builtin(struct drm_encoder *drm_enc); + #endif /* __DPU_ENCODER_H__ */ -- 2.7.4
[Freedreno] [PATCH v3 0/3] add color management support for the crtc
Add color management support for the crtc provided there are enough dspps that can be allocated from the catalog Kalyan Thota (3): drm/msm/disp/dpu1: pin 1 crtc to 1 encoder drm/msm/disp/dpu1: add helper to know if display is builtin drm/msm/disp/dpu1: add color management support for the crtc drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 5 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 6 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 34 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 24 +--- 5 files changed, 61 insertions(+), 14 deletions(-) -- 2.7.4
[Freedreno] [PATCH v3 3/3] drm/msm/disp/dpu1: add color management support for the crtc
Add color management support for the crtc provided there are enough dspps that can be allocated from the catalog. Changes in v1: - cache color enabled state in the dpu crtc obj (Dmitry) - simplify dspp allocation while creating crtc (Dmitry) - register for color when crtc is created (Dmitry) Changes in v2: - avoid primary encoders in the documentation (Dmitry) Changes in v3: - add ctm for builtin encoders (Dmitry) Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 5 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 6 -- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 7 ++- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 4170fbe..6cacaaf 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1571,7 +1571,7 @@ static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = { /* initialize crtc */ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, - struct drm_plane *cursor) + struct drm_plane *cursor, bool ctm) { struct drm_crtc *crtc = NULL; struct dpu_crtc *dpu_crtc = NULL; @@ -1583,6 +1583,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, crtc = _crtc->base; crtc->dev = dev; + dpu_crtc->color_enabled = ctm; spin_lock_init(_crtc->spin_lock); atomic_set(_crtc->frame_pending, 0); @@ -1604,7 +1605,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, drm_crtc_helper_add(crtc, _crtc_helper_funcs); - drm_crtc_enable_color_mgmt(crtc, 0, true, 0); + drm_crtc_enable_color_mgmt(crtc, 0, dpu_crtc->color_enabled, 0); /* save user friendly CRTC name for later */ snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 539b68b..1ec9517 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -136,6 +136,7 @@ struct dpu_crtc_frame_event { * @enabled : whether the DPU CRTC is currently enabled. updated in the * commit-thread, not state-swap time which is earlier, so * safe to make decisions on during VBLANK on/off work + * @color_enabled : whether crtc supports color management * @feature_list : list of color processing features supported on a crtc * @active_list : list of color processing features are active * @dirty_list: list of color processing features are dirty @@ -164,7 +165,7 @@ struct dpu_crtc { u64 play_count; ktime_t vblank_cb_time; bool enabled; - + bool color_enabled; struct list_head feature_list; struct list_head active_list; struct list_head dirty_list; @@ -269,10 +270,11 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc); * @dev: dpu device * @plane: base plane * @cursor: cursor plane + * @ctm: ctm flag * @Return: new crtc object or error */ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, - struct drm_plane *cursor); + struct drm_plane *cursor, bool ctm); /** * dpu_crtc_register_custom_event - api for enabling/disabling crtc event diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 574f2b0..102612c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -572,6 +572,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc) static struct msm_display_topology dpu_encoder_get_topology( struct dpu_encoder_virt *dpu_enc, struct dpu_kms *dpu_kms, + struct dpu_crtc *dpu_crtc, struct drm_display_mode *mode) { struct msm_display_topology topology = {0}; @@ -600,7 +601,7 @@ static struct msm_display_topology dpu_encoder_get_topology( else topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1; - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) { + if (dpu_crtc->color_enabled) { if (dpu_kms->catalog->dspp && (dpu_kms->catalog->dspp_count >= topology.num_lm)) topology.num_dspp = topology.num_lm; @@ -635,6 +636,7 @@ static int dpu_encoder_virt_atomic_check( struct drm_display_mode *adj_mode; struct msm_display_topology topology; struct dpu_global_state *global_state; + struct dpu_crtc *dpu_crtc; int i = 0; int ret = 0; @@ -645,6 +647,7 @@
[Freedreno] [PATCH v3 1/3] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder
Pin each crtc with one encoder. This arrangement will disallow crtc switching between encoders and also will facilitate to advertise certain features on crtc based on encoder type. Changes in v1: - use drm_for_each_encoder macro while iterating through encoder list (Dmitry) Changes in v2: - make sure no encoder miss to have a crtc (Dmitry) - revisit various factors in deciding the crtc count such as num_mixers, num_sspp (Dmitry) Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 17 ++--- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 7a5fabc..4784db8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -763,7 +763,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) drm_for_each_encoder(encoder, dev) num_encoders++; - max_crtc_count = min(catalog->mixer_count, num_encoders); + max_crtc_count = num_encoders; /* Create the planes, keeping track of one primary/cursor per crtc */ for (i = 0; i < catalog->sspp_count; i++) { @@ -795,22 +795,25 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) primary_planes[primary_planes_idx++] = plane; } - max_crtc_count = min(max_crtc_count, primary_planes_idx); + /* +* All the platforms should have at least 1 primary plane for an +* encoder. The below warn should help in setting up the catalog +*/ + WARN_ON(num_encoders > primary_planes_idx); /* Create one CRTC per encoder */ - for (i = 0; i < max_crtc_count; i++) { + i = 0; + drm_for_each_encoder(encoder, dev) { crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]); if (IS_ERR(crtc)) { ret = PTR_ERR(crtc); return ret; } priv->crtcs[priv->num_crtcs++] = crtc; + encoder->possible_crtcs = 1 << drm_crtc_index(crtc); + i++; } - /* All CRTCs are compatible with all encoders */ - drm_for_each_encoder(encoder, dev) - encoder->possible_crtcs = (1 << priv->num_crtcs) - 1; - return 0; } -- 2.7.4
Re: [Freedreno] [PATCH v3 2/2] drm/msm/dp: add support of max dp link rate
On 18/11/2022 01:49, Kuogee Hsieh wrote: dp_out endpoint contains both data-lanes and link-frequencies properties. This patch parser dp_out endpoint properties and acquire dp_max_lanes and dp_max_link_rate from respective property. Finally, comparing them against both data lane and link rate read back from sink to ensure both data lane and link rate are supported by platform. In the case there is no data-lanes or link-frequencies property defined at dp_out endpoint, the default value are 4 data lanes with 5.4 Ghz link rate. Changes in v2: -- add max link rate from dtsi Changes in v3: -- parser max_data_lanes and max_dp_link_rate from dp_out endpoint Signed-off-by: Kuogee Hsieh --- arch/arm64/boot/dts/qcom/sc7280.dtsi | 1 + Should not be a part of this patch. drivers/gpu/drm/msm/dp/dp_display.c | 4 drivers/gpu/drm/msm/dp/dp_panel.c| 7 --- drivers/gpu/drm/msm/dp/dp_panel.h| 1 + drivers/gpu/drm/msm/dp/dp_parser.c | 30 ++ drivers/gpu/drm/msm/dp/dp_parser.h | 2 ++ 6 files changed, 34 insertions(+), 11 deletions(-) diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index 4afe53b..d456e76 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -3897,6 +3897,7 @@ reg = <0>; dp_in: endpoint { remote-endpoint = <_intf0_out>; + data-lanes = <0 1 2 3>; }; }; }; diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 29c9845..4fe2092 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -390,6 +390,10 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) struct edid *edid; dp->panel->max_dp_lanes = dp->parser->max_dp_lanes; + dp->panel->max_dp_link_rate = dp->parser->max_dp_link_rate; + + drm_dbg_dp(dp->drm_dev, "max_lanes=%d max_link_rate=%d\n", + dp->panel->max_dp_lanes, dp->panel->max_dp_link_rate); rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector); if (rc) diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 5149ceb..933fa9c 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -75,12 +75,13 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel) link_info->rate = drm_dp_bw_code_to_link_rate(dpcd[DP_MAX_LINK_RATE]); link_info->num_lanes = dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK; + /* Limit data lanes from data-lanes of endpoint properity of dtsi */ if (link_info->num_lanes > dp_panel->max_dp_lanes) link_info->num_lanes = dp_panel->max_dp_lanes; - /* Limit support upto HBR2 until HBR3 support is added */ - if (link_info->rate >= (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4))) - link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4); + /* Limit link rate from link-frequencies of endpoint properity of dtsi */ + if (link_info->rate > dp_panel->max_dp_link_rate) + link_info->rate = dp_panel->max_dp_link_rate; drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor); drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate); diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h index d861197a..f04d021 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.h +++ b/drivers/gpu/drm/msm/dp/dp_panel.h @@ -50,6 +50,7 @@ struct dp_panel { u32 vic; u32 max_dp_lanes; + u32 max_dp_link_rate; u32 max_bw_code; }; diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index dd73221..667981e 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -94,16 +94,30 @@ static int dp_parser_ctrl_res(struct dp_parser *parser) static int dp_parser_misc(struct dp_parser *parser) { struct device_node *of_node = parser->pdev->dev.of_node; - int len; - - len = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES); - if (len < 0) { - DRM_WARN("Invalid property \"data-lanes\", default max DP lanes = %d\n", -DP_MAX_NUM_DP_LANES); - len = DP_MAX_NUM_DP_LANES; + struct device_node *endpoint; + int cnt; + u32 frequence = 0; + + endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); + + if (endpoint) { + cnt = of_property_count_u32_elems(endpoint, "data-lanes"); + if (cnt < 0) + parser->max_dp_lanes =
Re: [Freedreno] [PATCH v3 1/2] arm64: dts: qcom: add data-lanes and link-freuencies into dp_out endpoint
On Fri, 18 Nov 2022 at 00:50, Kuogee Hsieh wrote: > > Add both data-lanes and link-frequencies property to dp_out endpoint. Bindings update? Deprecate the old data-lanes property? > Also set link-frequencies to 81 khz at herobrine platform to have > max link rate limited at 81 khz (HBR3). No. As I stated before, the link-frequencies should list all supported frequencies (min/max in case the frequencies are continuous). Stating just maximum is against the property description. > > Signed-off-by: Kuogee Hsieh > --- > arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 - > arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 - > arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 10 +- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 5 - > 4 files changed, 17 insertions(+), 12 deletions(-) > [skipped the sc7180 here. All comments noted against sc7280 apply to sc7180 too]. > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > index 93e39fc..e8fca18 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > @@ -440,7 +440,15 @@ ap_i2c_tpm: { > status = "okay"; > pinctrl-names = "default"; > pinctrl-0 = <_hot_plug_det>; > - data-lanes = <0 1>; > + ports { > + port@1 { > + reg = <1>; > + dp_out: endpoint { > + data-lanes = <0 1>; > + link-frequencies=<81>; Following the existing examples is nice. Not following them is frowned upon. > + }; > + }; > + }; Just: _out { data-lanes = <0 1>; link-frequencies = /bits/ 64 <16000 27000 54000 81000>; }; > }; > > _mdp { > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index a646405..4afe53b 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -3899,11 +3899,6 @@ > remote-endpoint = > <_intf0_out>; > }; > }; > - > - port@1 { > - reg = <1>; > - dp_out: endpoint { }; > - }; Please leave it here. It is a part of the SoC, so it should be in SoC dtsi. > }; > > dp_opp_table: opp-table { -- With best wishes Dmitry