[Freedreno] [PATCH] drm/msm/dpu: check for null return of devm_kzalloc() in dpu_writeback_init()

2022-11-18 Thread Hui Tang
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

2022-11-18 Thread Kuogee Hsieh



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

2022-11-18 Thread Jessica Zhang




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

2022-11-18 Thread Dmitry Baryshkov

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

2022-11-18 Thread Kuogee Hsieh
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

2022-11-18 Thread Kuogee Hsieh
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

2022-11-18 Thread Kuogee Hsieh
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

2022-11-18 Thread Dmitry Baryshkov

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

2022-11-18 Thread Abhinav Kumar




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

2022-11-18 Thread Kalyan Thota


>-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

2022-11-18 Thread Dmitry Baryshkov

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

2022-11-18 Thread Will Deacon
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

2022-11-18 Thread Bryan O'Donoghue

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

2022-11-18 Thread Dan Carpenter
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

2022-11-18 Thread Krzysztof Kozlowski
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

2022-11-18 Thread Krzysztof Kozlowski
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

2022-11-18 Thread Dmitry Baryshkov

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

2022-11-18 Thread Dmitry Baryshkov

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

2022-11-18 Thread Dmitry Baryshkov

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

2022-11-18 Thread Kalyan Thota
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

2022-11-18 Thread Kalyan Thota
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

2022-11-18 Thread Kalyan Thota
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

2022-11-18 Thread Kalyan Thota
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

2022-11-18 Thread Dmitry Baryshkov

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

2022-11-18 Thread Dmitry Baryshkov
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