Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver support
On Thu, Feb 27, 2020 at 01:54:33PM -0800, Matthias Kaehlcke wrote:
> On Mon, Dec 02, 2019 at 01:48:57PM +, Chandan Uddaraju wrote:
> > Add the needed displayPort files to enable DP driver
> > on msm target.
> >
> > "dp_display" module is the main module that calls into
> > other sub-modules. "dp_drm" file represents the interface
> > between DRM framework and DP driver.
> >
> > changes in v2:
> > -- Update copyright markings on all relevant files.
> > -- Change pr_err() to DRM_ERROR()
> > -- Use APIs directly instead of function pointers.
> > -- Use drm_display_mode structure to store link parameters in the driver.
> > -- Use macros for register definitions instead of hardcoded values.
> > -- Replace writel_relaxed/readl_relaxed with writel/readl
> >and remove memory barriers.
> > -- Remove unnecessary NULL checks.
> > -- Use drm helper functions for dpcd read/write.
> > -- Use DRM_DEBUG_DP for debug msgs.
> >
> > changes in V3:
> > -- Removed changes in dpu_io_util.[ch]
> > -- Added locking around "is_connected" flag and removed atomic_set()
> > -- Removed the argument validation checks in all the static functions
> >except initialization functions and few API calls across msm/dp files
> > -- Removed hardcoded values for register reads/writes
> > -- Removed vreg related generic structures.
> > -- Added return values where ever necessary.
> > -- Updated dp_ctrl_on function.
> > -- Calling the ctrl specific catalog functions directly instead of
> >function pointers.
> > -- Added seperate change that adds standard value in drm_dp_helper file.
> > -- Added separate change in this list that is used to initialize
> >displayport in DPU driver.
> > -- Added change to use drm_dp_get_adjust_request_voltage() function.
> >
> > Signed-off-by: Chandan Uddaraju
> > ---
> > +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> >
> > ...
> >
> > +int dp_power_init(struct dp_power *dp_power, bool flip)
> > +{
> > + int rc = 0;
> > + struct dp_power_private *power;
> > +
> > + if (!dp_power) {
> > + DRM_ERROR("invalid power data\n");
> > + rc = -EINVAL;
> > + goto exit;
> > + }
>
> drive-by comment:
>
> this would lead to calling 'pm_runtime_put_sync(>pdev->dev)'
> below with 'power' being NULL, which doesn't seem a good idea.
correction: with 'power' being uninitialized, which isn't a good idea
either.
> It is probably sane to expect that 'dp_power' is not NULL, if that's
> the case the check can be removed. Otherwise the function should just
> return -EINVAL instead of jumping to 'exit'.
>
> > +
> > + power = container_of(dp_power, struct dp_power_private, dp_power);
> > +
> > + pm_runtime_get_sync(>pdev->dev);
> > + rc = dp_power_regulator_enable(power);
> > + if (rc) {
> > + DRM_ERROR("failed to enable regulators, %d\n", rc);
> > + goto exit;
> > + }
> > +
> > + rc = dp_power_pinctrl_set(power, true);
> > + if (rc) {
> > + DRM_ERROR("failed to set pinctrl state, %d\n", rc);
> > + goto err_pinctrl;
> > + }
> > +
> > + rc = dp_power_config_gpios(power, flip);
> > + if (rc) {
> > + DRM_ERROR("failed to enable gpios, %d\n", rc);
> > + goto err_gpio;
> > + }
> > +
> > + rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> > + if (rc) {
> > + DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
> > + goto err_clk;
> > + }
> > +
> > + return 0;
> > +
> > +err_clk:
> > + dp_power_disable_gpios(power);
> > +err_gpio:
> > + dp_power_pinctrl_set(power, false);
> > +err_pinctrl:
> > + dp_power_regulator_disable(power);
> > +exit:
> > + pm_runtime_put_sync(>pdev->dev);
> > + return rc;
> > +}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver support
On Mon, Dec 02, 2019 at 01:48:57PM +, Chandan Uddaraju wrote:
> Add the needed displayPort files to enable DP driver
> on msm target.
>
> "dp_display" module is the main module that calls into
> other sub-modules. "dp_drm" file represents the interface
> between DRM framework and DP driver.
>
> changes in v2:
> -- Update copyright markings on all relevant files.
> -- Change pr_err() to DRM_ERROR()
> -- Use APIs directly instead of function pointers.
> -- Use drm_display_mode structure to store link parameters in the driver.
> -- Use macros for register definitions instead of hardcoded values.
> -- Replace writel_relaxed/readl_relaxed with writel/readl
>and remove memory barriers.
> -- Remove unnecessary NULL checks.
> -- Use drm helper functions for dpcd read/write.
> -- Use DRM_DEBUG_DP for debug msgs.
>
> changes in V3:
> -- Removed changes in dpu_io_util.[ch]
> -- Added locking around "is_connected" flag and removed atomic_set()
> -- Removed the argument validation checks in all the static functions
>except initialization functions and few API calls across msm/dp files
> -- Removed hardcoded values for register reads/writes
> -- Removed vreg related generic structures.
> -- Added return values where ever necessary.
> -- Updated dp_ctrl_on function.
> -- Calling the ctrl specific catalog functions directly instead of
>function pointers.
> -- Added seperate change that adds standard value in drm_dp_helper file.
> -- Added separate change in this list that is used to initialize
>displayport in DPU driver.
> -- Added change to use drm_dp_get_adjust_request_voltage() function.
>
> Signed-off-by: Chandan Uddaraju
> ---
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>
> ...
>
> +int dp_power_init(struct dp_power *dp_power, bool flip)
> +{
> + int rc = 0;
> + struct dp_power_private *power;
> +
> + if (!dp_power) {
> + DRM_ERROR("invalid power data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
drive-by comment:
this would lead to calling 'pm_runtime_put_sync(>pdev->dev)'
below with 'power' being NULL, which doesn't seem a good idea.
It is probably sane to expect that 'dp_power' is not NULL, if that's
the case the check can be removed. Otherwise the function should just
return -EINVAL instead of jumping to 'exit'.
> +
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + pm_runtime_get_sync(>pdev->dev);
> + rc = dp_power_regulator_enable(power);
> + if (rc) {
> + DRM_ERROR("failed to enable regulators, %d\n", rc);
> + goto exit;
> + }
> +
> + rc = dp_power_pinctrl_set(power, true);
> + if (rc) {
> + DRM_ERROR("failed to set pinctrl state, %d\n", rc);
> + goto err_pinctrl;
> + }
> +
> + rc = dp_power_config_gpios(power, flip);
> + if (rc) {
> + DRM_ERROR("failed to enable gpios, %d\n", rc);
> + goto err_gpio;
> + }
> +
> + rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> + if (rc) {
> + DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
> + goto err_clk;
> + }
> +
> + return 0;
> +
> +err_clk:
> + dp_power_disable_gpios(power);
> +err_gpio:
> + dp_power_pinctrl_set(power, false);
> +err_pinctrl:
> + dp_power_regulator_disable(power);
> +exit:
> + pm_runtime_put_sync(>pdev->dev);
> + return rc;
> +}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver support
Hello Rob,
We removed the bridge object for DisplayPort due to review comments in
patch set 1.
Added more details below.
Original Message
Subject: Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver
support
Date: 2019-12-02 08:48
From: Rob Clark
To: Chandan Uddaraju
Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
, linux-arm-msm
, Abhinav Kumar
, Sean Paul ,
dri-devel , "Kristian H. Kristensen"
, freedreno
On Mon, Dec 2, 2019 at 5:48 AM Chandan Uddaraju
wrote:
Add the needed displayPort files to enable DP driver
on msm target.
"dp_display" module is the main module that calls into
other sub-modules. "dp_drm" file represents the interface
between DRM framework and DP driver.
changes in v2:
-- Update copyright markings on all relevant files.
-- Change pr_err() to DRM_ERROR()
-- Use APIs directly instead of function pointers.
-- Use drm_display_mode structure to store link parameters in the
driver.
-- Use macros for register definitions instead of hardcoded values.
-- Replace writel_relaxed/readl_relaxed with writel/readl
and remove memory barriers.
-- Remove unnecessary NULL checks.
-- Use drm helper functions for dpcd read/write.
-- Use DRM_DEBUG_DP for debug msgs.
changes in V3:
-- Removed changes in dpu_io_util.[ch]
-- Added locking around "is_connected" flag and removed atomic_set()
-- Removed the argument validation checks in all the static functions
except initialization functions and few API calls across msm/dp
files
-- Removed hardcoded values for register reads/writes
-- Removed vreg related generic structures.
-- Added return values where ever necessary.
-- Updated dp_ctrl_on function.
-- Calling the ctrl specific catalog functions directly instead of
function pointers.
-- Added seperate change that adds standard value in drm_dp_helper
file.
-- Added separate change in this list that is used to initialize
displayport in DPU driver.
-- Added change to use drm_dp_get_adjust_request_voltage() function.
Signed-off-by: Chandan Uddaraju
---
[snip]
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f96e142..29ac7d3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -967,6 +967,9 @@ static void dpu_encoder_virt_mode_set(struct
drm_encoder *drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
priv->dp)
+ msm_dp_display_mode_set(priv->dp, drm_enc, mode,
adj_mode);
+
for better or for worse, DSI and HDMI backends create an internal
drm_bridge object to avoid all of these shunts over from the encoder.
We might be still the only driver to do this, but IMHO it is better
than making each encoder know about each backend, so we might as well
stick with that.
(same goes for the other 'if (drm_enc->encoder_type ==
DRM_MODE_ENCODER_TMDS)'s)
We had the below comments from Sean Paul to remove the bridge object in
patch set 1 of this change.
** **
+static const struct drm_bridge_funcs dp_bridge_ops = {
+ .mode_fixup = dp_bridge_mode_fixup,
+ .pre_enable = dp_bridge_pre_enable,
+ .enable = dp_bridge_enable,
+ .disable = dp_bridge_disable,
+ .post_disable = dp_bridge_post_disable,
+ .mode_set = dp_bridge_mode_set,
+};
I'm not convinced you need any of this. The only advantage a bridge gets
you is
to be involved in modeset. However the only thing you do in modeset is
save the
mode (which is only used in enable). So why not just use the mode from
the
crtc's state when encoder->enable is called?
That allows you to delete all of the bridge stuff here.
+
+int dp_connector_post_init(struct drm_connector *connector, void
*display)
+{
+ struct msm_dp *dp_display = display;
+
+ if (!dp_display)
+ return -EINVAL;
***
thanks
Chandan
BR,
-R
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
conn = conn_iter;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver support
On Mon, Dec 2, 2019 at 5:48 AM Chandan Uddaraju wrote:
>
> Add the needed displayPort files to enable DP driver
> on msm target.
>
> "dp_display" module is the main module that calls into
> other sub-modules. "dp_drm" file represents the interface
> between DRM framework and DP driver.
>
> changes in v2:
> -- Update copyright markings on all relevant files.
> -- Change pr_err() to DRM_ERROR()
> -- Use APIs directly instead of function pointers.
> -- Use drm_display_mode structure to store link parameters in the driver.
> -- Use macros for register definitions instead of hardcoded values.
> -- Replace writel_relaxed/readl_relaxed with writel/readl
>and remove memory barriers.
> -- Remove unnecessary NULL checks.
> -- Use drm helper functions for dpcd read/write.
> -- Use DRM_DEBUG_DP for debug msgs.
>
> changes in V3:
> -- Removed changes in dpu_io_util.[ch]
> -- Added locking around "is_connected" flag and removed atomic_set()
> -- Removed the argument validation checks in all the static functions
>except initialization functions and few API calls across msm/dp files
> -- Removed hardcoded values for register reads/writes
> -- Removed vreg related generic structures.
> -- Added return values where ever necessary.
> -- Updated dp_ctrl_on function.
> -- Calling the ctrl specific catalog functions directly instead of
>function pointers.
> -- Added seperate change that adds standard value in drm_dp_helper file.
> -- Added separate change in this list that is used to initialize
>displayport in DPU driver.
> -- Added change to use drm_dp_get_adjust_request_voltage() function.
>
> Signed-off-by: Chandan Uddaraju
> ---
[snip]
> +
> +void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog,
> + u32 rate, u32 stream_rate_khz,
> + bool fixed_nvid)
> +{
> + u32 pixel_m, pixel_n;
> + u32 mvid, nvid;
> + u64 mvid_calc;
> + struct dp_catalog_private *catalog = container_of(dp_catalog,
> + struct dp_catalog_private, dp_catalog);
> +
> + if (fixed_nvid) {
> + nvid = DP_LINK_CONSTANT_N_VALUE;
> + DRM_DEBUG_DP("use fixed NVID=0x%x\n", nvid);
> +
> + /*
> +* For intermediate results, use 64 bit arithmetic to avoid
> +* loss of precision.
> +*/
> + mvid_calc = (u64) stream_rate_khz * nvid;
> + mvid_calc = div_u64(mvid_calc, rate);
> +
> + /*
> +* truncate back to 32 bits as this final divided value will
> +* always be within the range of a 32 bit unsigned int.
> +*/
> + mvid = (u32) mvid_calc;
> + DRM_DEBUG_DP("link rate=%dkbps, stream_rate_khz=%uKhz",
> + rate, stream_rate_khz);
> + } else {
> + pixel_m = dp_read_cc(catalog, MMSS_DP_PIXEL_M);
> + pixel_n = dp_read_cc(catalog, MMSS_DP_PIXEL_N);
Can we just calculate m/n from the rate instead. That gets rid of
having to ioremap() the dispcc region, which is really ugly.
BR,
-R
> + DRM_DEBUG_DP("pixel_m=0x%x, pixel_n=0x%x\n", pixel_m,
> pixel_n);
> +
> + mvid = (pixel_m & 0x) * 5;
> + nvid = (0x & (~pixel_n)) + (pixel_m & 0x);
> +
> + DRM_DEBUG_DP("rate = %d\n", rate);
> +
> + switch (drm_dp_link_rate_to_bw_code(rate)) {
> + case DP_LINK_BW_5_4:
> + nvid *= 2;
> + break;
> + case DP_LINK_BW_8_1:
> + nvid *= 3;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + DRM_DEBUG_DP("mvid=0x%x, nvid=0x%x\n", mvid, nvid);
> + dp_write_link(catalog, REG_DP_SOFTWARE_MVID, mvid);
> + dp_write_link(catalog, REG_DP_SOFTWARE_NVID, nvid);
> +}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver support
On Mon, Dec 2, 2019 at 5:48 AM Chandan Uddaraju wrote: > > Add the needed displayPort files to enable DP driver > on msm target. > > "dp_display" module is the main module that calls into > other sub-modules. "dp_drm" file represents the interface > between DRM framework and DP driver. > > changes in v2: > -- Update copyright markings on all relevant files. > -- Change pr_err() to DRM_ERROR() > -- Use APIs directly instead of function pointers. > -- Use drm_display_mode structure to store link parameters in the driver. > -- Use macros for register definitions instead of hardcoded values. > -- Replace writel_relaxed/readl_relaxed with writel/readl >and remove memory barriers. > -- Remove unnecessary NULL checks. > -- Use drm helper functions for dpcd read/write. > -- Use DRM_DEBUG_DP for debug msgs. > > changes in V3: > -- Removed changes in dpu_io_util.[ch] > -- Added locking around "is_connected" flag and removed atomic_set() > -- Removed the argument validation checks in all the static functions >except initialization functions and few API calls across msm/dp files > -- Removed hardcoded values for register reads/writes > -- Removed vreg related generic structures. > -- Added return values where ever necessary. > -- Updated dp_ctrl_on function. > -- Calling the ctrl specific catalog functions directly instead of >function pointers. > -- Added seperate change that adds standard value in drm_dp_helper file. > -- Added separate change in this list that is used to initialize >displayport in DPU driver. > -- Added change to use drm_dp_get_adjust_request_voltage() function. > > Signed-off-by: Chandan Uddaraju > --- [snip] > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index f96e142..29ac7d3 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -967,6 +967,9 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder > *drm_enc, > > trace_dpu_enc_mode_set(DRMID(drm_enc)); > > + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) > + msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode); > + for better or for worse, DSI and HDMI backends create an internal drm_bridge object to avoid all of these shunts over from the encoder. We might be still the only driver to do this, but IMHO it is better than making each encoder know about each backend, so we might as well stick with that. (same goes for the other 'if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)'s) BR, -R > list_for_each_entry(conn_iter, connector_list, head) > if (conn_iter->encoder == drm_enc) > conn = conn_iter; ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
