[Freedreno] 回复: Re: [PATCH RESEND] drm/msm/dsi: fix the inconsistent indenting
OK, I am sorry. Thanks
主 题：Re: [PATCH RESEND] drm/msm/dsi: fix the inconsistent indenting
日 期：2022-08-27 00:10
发件人：Abhinav Kumar
收件人：孙立明christian.koenig@amd.comrobdclark@gmail.comdmitry.barysh...@linaro.org
On 8/26/2022 1:49 AM, sunliming wrote:> Fix the inconsistent indenting in function msm_dsi_dphy_timing_calc_v3().> > Fix the following smatch warnings:> > drivers/gpu/drm/msm/dsi/phy/dsi_phy.c:350 msm_dsi_dphy_timing_calc_v3() warn: inconsistent indenting> > Fixes: f1fa7ff44056 ("drm/msm/dsi: implement auto PHY timing calculator for 10nm PHY")> Reported-by: kernel test robot > Signed-off-by: sunliming > Reviewed-by: Abhinav Kumar There is no need to resend this. This was already applied to msm-fixeshttps://gitlab.freedesktop.org/drm/msm/-/commit/2f25a1fb4ec516c5ad67afd754334b491b9f09a5> ---> drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 +-> 1 file changed, 1 insertion(+), 1 deletion(-)> > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c> index a39de3bdc7fa..56dfa2d24be1 100644> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c> @@ -347,7 +347,7 @@ int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing *timing,> } else {> timing->shared_timings.clk_pre => linear_inter(tmax, tmin, pcnt2, 0, false);> - timing->shared_timings.clk_pre_inc_by_2 = 0;> + timing->shared_timings.clk_pre_inc_by_2 = 0;> }> > timing->ta_go = 3;
Re: [Freedreno] [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge
Hi Dmitry, On Fri, Aug 26, 2022 at 04:52:12PM +0300, Dmitry Baryshkov wrote: > On 26/08/2022 14:55, Laurent Pinchart wrote: > > On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote: > >> On 22/08/2022 19:31, Dmitry Baryshkov wrote: > >>> On 09/08/2022 22:40, Laurent Pinchart wrote: > On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar wrote: > > adv7533 bridge tries to dynamically switch lanes based on the > > mode by detaching and attaching the mipi dsi device. > > > > This approach is incorrect because as per the DSI spec the > > number of lanes is fixed at the time of system design or initial > > configuration and may not change dynamically. > > Is that really so ? The number of lanes connected on the board is > certainlyset at design time, but a lower number of lanes can be used at > runtime. It shouldn't change dynamically while the display is on, but it > could change at mode set time. > >>> > >>> I'm not sure if I interpreted the standard correctly, but I tended to > >>> have the same interpretation as Abhinav did. > >>> > >>> Anyway, even if we allow the drivers to switch the amount of lanes, this > >>> should not happen in the form of detach()/attach() cycle. The drivers > > > > Agreed. > > > >>> use mipi_dsi_attach() as way to signal the DSI hosts that the sink > >>> device is ready. Some of DSI hosts (including MSM one) would bind > >>> components from the attach callback. > >>> > >>> If we were to support dynamically changing the amount of lanes, I would > >>> ask for additional mipi_dsi API call telling the host to switch the > >>> amount of lanes. And note that this can open the can of worms. Different > >>> hosts might have different requirements here. For example for the MSM > >>> platform the amount of lanes is programmed during bridge_pre_enable > >>> chain call, so it is possible to just set the amount of lanes following > >>> the msm_dsi_device::lanes. Other hosts might have other requirements. > > > > Conceptually, I would expect the number of effective lanes to be > > selected at mode set time, because it has to be done while the output is > > disabled. > > There is one tightly coupled question. The dual-DSI (or bonded-DSI) > mode. Currently it is exposed as two independent DSI hosts. If we allow > changing the amount of DSI lanes at runtime, bonded DSI mode would > become complicated by fixing amount of lanes for each of outputs (or > switching them in tight loop). And then comes the question of switching > between single-DSI and bonded-DSI at runtime. Maybe we can leave dynamic selection of the number of lanes for dual-DSI out at this point ? I have no experienced with bonded DSI, is it typical to have to switch between single and bonded links at runtime (to be precise, at mode set time, not while the display is on) ? > > With the atomic API for bridges the .mode_set() operation is > > deprecated, so .pre_enable() sounds best, but there's a potential issue: > > the .pre_enable() operation is called from sink to source. It means that > > a DSI sink .pre_enable() operation would need to call a DSI host > > operation to set (or maybe negotiate instead of just setting a fixed > > value) the number of lanes first if it wants to control the sink through > > DCS at .pre_enable() time. We'd have to see how that fits. > > What is the fate of the patchset that implemented 'parent-first' opt-in > for the drm_bridge chains? It was supposed to solve this this kind of > issues. I'm asking because until it is merged some DSI hosts (e.g. the > msm dsi) turn on the power in .mode_set() to allow the pre_enable() > callbacks work when the DSI link is in LP11 mode. > > Back then I voted for the explicit mipi_dsi_power_on kind of calls, > which would allow the sink bridge to prepare for the DSI powerup (e.g. > by setting the amount of lanes), power up the DSI host, putting the link > into LP11 and after that communicate with the sink using the DSI data > lanes. A long time ago, I worked on converting the omapdrm driver to the DRM bridge API. It was using internal bridge and panel drivers with an API specific to omapdrm, and it was based on a similar principle: enabling or disabling an output went from the last component in the chain, which was the responsible for calling into its parent explicitly, with a bus-specific API. DRM bridge, on the other hand, doesn't use a recursive model but sequences the whole pipeline with a fixed order. This has led to be pre-enable/enable split, and even that isn't enough. Moving from the omapdrm model to the DRM bridge model was difficult and took lots of time and effort, and I'm now increasingly thinking the omapdrm model got it right, only too early to convince enough people. > >>> Thus said I'd suggest accepting this patch and maybe working on the > >>> API/support for the lane switching as a followup. > > > > I don't have a personal need for the ADV7533 so I won't really c
Re: [Freedreno] [PATCH] drm/msm/dp: add atomic_check to bridge ops
On 8/26/2022 1:19 AM, Dmitry Baryshkov wrote: On 24/08/2022 22:16, Abhinav Kumar wrote: On 8/24/2022 1:25 AM, Dmitry Baryshkov wrote: On Wed, 24 Aug 2022 at 01:59, Abhinav Kumar wrote: On 8/23/2022 3:41 PM, Dmitry Baryshkov wrote: On Wed, 24 Aug 2022 at 01:07, Abhinav Kumar wrote: On 8/22/2022 11:33 AM, Dmitry Baryshkov wrote: On 22/08/2022 20:32, Abhinav Kumar wrote: On 8/22/2022 9:49 AM, Dmitry Baryshkov wrote: On 22/08/2022 19:38, Abhinav Kumar wrote: Hi Dmitry On 8/22/2022 9:18 AM, Dmitry Baryshkov wrote: On 17/08/2022 21:01, Kuogee Hsieh wrote: DRM commit_tails() will disable downstream crtc/encoder/bridge if both disable crtc is required and crtc->active is set before pushing a new frame downstream. There is a rare case that user space display manager issue an extra screen update immediately followed by close DRM device while down stream display interface is disabled. This extra screen update will timeout due to the downstream interface is disabled but will cause crtc->active be set. Hence the followed commit_tails() called by drm_release() will pass the disable downstream crtc/encoder/bridge conditions checking even downstream interface is disabled. This cause the crash to happen at dp_bridge_disable() due to it trying to access the main link register to push the idle pattern out while main link clocks is disabled. This patch adds atomic_check to prevent the extra frame will not be pushed down if display interface is down so that crtc->active will not be set neither. This will fail the conditions checking of disabling down stream crtc/encoder/bridge which prevent drm_release() from calling dp_bridge_disable() so that crash at dp_bridge_disable() prevented. I must admit I had troubles parsing this description. However if I got you right, I think the check that the main link clock is running in the dp_bridge_disable() or dp_ctrl_push_idle() would be a better fix. Originally, thats what was posted https://patchwork.freedesktop.org/patch/496984/. This patch is also not so correct from my POV. It checks for the hpd status, while in reality it should check for main link clocks being enabled. We can push another fix to check for the clk state instead of the hpd status. But I must say we are again just masking something which the fwk should have avoided isnt it? As per the doc in the include/drm/drm_bridge.h it says, "* * The bridge can assume that the display pipe (i.e. clocks and timing * signals) feeding it is still running when this callback is called. *" Yes, that's what I meant about this chunk begging to go to the core. In my opinion, if we are talking about the disconnected sinks, it is the framework who should disallow submitting the frames to the disconnected sinks. By adding an extra layers of protection in the driver, we are just avoiding another issue but the commit should not have been issued in the first place. So shouldnt we do both then? That is add protection to check if clock is ON and also, reject commits when display is disconnected. Then it seemed like we were just protecting against an issue in the framework which was allowing the frames to be pushed even after the display was disconnected. The DP driver did send out the disconnect event correctly and as per the logs, this frame came down after that and the DRM fwk did allow it. So after discussing on IRC with Rob, we came up with this approach that if the display is not connected, then atomic_check should fail. That way the commit will not happen. Just seemed a bit cleaner instead of adding all our protections. The check to fail atomic_check if display is not connected seems out of place. In its current way it begs go to the upper layer, forbidding using disconnected sinks for all the drivers. There is nothing special in the MSM DP driver with respect to the HPD events processing and failing atomic_check() based on that. Why all the drivers? This is only for MSM DP bridge. Yes, we change the MSM DRM driver. But the check is generic enough. I'm not actually insisting on pushing the check to the core, just trying to understand the real cause here. I actually wanted to push this to the core and thats what I had originally asked on IRC because it does seem to be generic enough that it should belong to the core but after discussion with Rob on freedreno, he felt this was a better approach because for some of the legacy connectors like VGA, this need not belong to the DRM core, hence we went with this approach. It might be better to whitelist such connectors (S-VIDEO/composite comes to my mind rather than VGA). I am fine with that approach, if Rob is onboard with that. SError Interrupt on CPU7, code 0xbe000411 -- SError CPU: 7 PID: 3878 Comm: Xorg Not tainted 5.19.0-stb-cbq #19 Hardware name: Google Lazor (rev3 - 8) (DT) pstate: a04000c9 (NzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : __cmpxchg_
Re: [Freedreno] [PATCH RESEND] drm/msm/dsi: fix the inconsistent indenting
On 8/26/2022 1:49 AM, sunliming wrote:
Fix the inconsistent indenting in function msm_dsi_dphy_timing_calc_v3().
Fix the following smatch warnings:
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c:350 msm_dsi_dphy_timing_calc_v3() warn:
inconsistent indenting
Fixes: f1fa7ff44056 ("drm/msm/dsi: implement auto PHY timing calculator for 10nm
PHY")
Reported-by: kernel test robot
Signed-off-by: sunliming
Reviewed-by: Abhinav Kumar
There is no need to resend this. This was already applied to msm-fixes
https://gitlab.freedesktop.org/drm/msm/-/commit/2f25a1fb4ec516c5ad67afd754334b491b9f09a5
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index a39de3bdc7fa..56dfa2d24be1 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -347,7 +347,7 @@ int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing
*timing,
} else {
timing->shared_timings.clk_pre =
linear_inter(tmax, tmin, pcnt2, 0, false);
- timing->shared_timings.clk_pre_inc_by_2 = 0;
+ timing->shared_timings.clk_pre_inc_by_2 = 0;
}
timing->ta_go = 3;
Re: [Freedreno] [PATCH v3 3/3] arm64: dts: qcom: msm8996: add #clock-cells and XO clock to the HDMI PHY node
On 04/07/2022 19:11, Dmitry Baryshkov wrote:
Add #clock-cells property to the HDMI PHY device node to let other nodes
resolve the hdmipll clock. While we are at it, also add the XO clock to
the device node.
Acked-by: Krzysztof Kozlowski
Signed-off-by: Dmitry Baryshkov
Bjorn, I'm picking the patches 1,2 into msm-next. Could you please pick
this patch into your dts-for-6.1?
---
arch/arm64/boot/dts/qcom/msm8996.dtsi | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi
b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 25d6b26fab60..b72385ffecc6 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -1049,9 +1049,13 @@ hdmi_phy: hdmi-phy@9a0600 {
"hdmi_phy";
clocks = <&mmcc MDSS_AHB_CLK>,
-<&gcc GCC_HDMI_CLKREF_CLK>;
+<&gcc GCC_HDMI_CLKREF_CLK>,
+<&xo_board>;
clock-names = "iface",
- "ref";
+ "ref",
+ "xo";
+
+ #clock-cells = <0>;
status = "disabled";
};
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v1 1/4] drm/msm/mdp5: stop overriding drvdata
On 24/08/2022 20:20, Abhinav Kumar wrote:
On 8/24/2022 1:29 AM, Dmitry Baryshkov wrote:
On Wed, 24 Aug 2022 at 04:25, Abhinav Kumar
wrote:
On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote:
The rest of the code expects that master's device drvdata is the
struct msm_drm_private instance. Do not override the mdp5's drvdata.
Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
components")
Signed-off-by: Dmitry Baryshkov
Is this just for consistency across mdp5/dpu drivers?
What issue was seen if mdp5's platform data is overwritten?
I think there was a crash in mdp5_destroy, but I did not capture the
log at the moment.
As you can see, the mdp5_destroy() expects to get mdp5_kms pointer
from the drvdata. However the msm_drv_probe sets the drvdata to
msm_drm_private instance. Boom.
Yes, I see that msm_drv_probe sets the drvdata to msm_drm_private.
But I also see that mdp5_init then sets it to
platform_set_drvdata(pdev, mdp5_kms);
Does this not override it then?
It does. But then the mdp5_pm_ops use msm_pm_prepare()/_complete(). And
these calls expect the msm_drm_private instance in the drvdata. Maybe I
stumbled upon this. I don't remember exactly, unfortunately.
Also seems like the commit which introduced it is present since april,
this should have happened even earlier then right?
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index c668a4b27cc6..daf5b5ca7233 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -203,7 +203,7 @@ static int mdp5_set_split_display(struct msm_kms
*kms,
slave_encoder);
}
-static void mdp5_destroy(struct platform_device *pdev);
+static void mdp5_destroy(struct mdp5_kms *mdp5_kms);
static void mdp5_kms_destroy(struct msm_kms *kms)
{
@@ -223,7 +223,7 @@ static void mdp5_kms_destroy(struct msm_kms *kms)
}
mdp_kms_destroy(&mdp5_kms->base);
- mdp5_destroy(mdp5_kms->pdev);
+ mdp5_destroy(mdp5_kms);
}
#ifdef CONFIG_DEBUG_FS
@@ -651,9 +651,8 @@ static int mdp5_kms_init(struct drm_device *dev)
return ret;
}
-static void mdp5_destroy(struct platform_device *pdev)
+static void mdp5_destroy(struct mdp5_kms *mdp5_kms)
{
- struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
int i;
if (mdp5_kms->ctlm)
@@ -667,7 +666,7 @@ static void mdp5_destroy(struct platform_device
*pdev)
kfree(mdp5_kms->intfs[i]);
if (mdp5_kms->rpm_enabled)
- pm_runtime_disable(&pdev->dev);
+ pm_runtime_disable(&mdp5_kms->pdev->dev);
drm_atomic_private_obj_fini(&mdp5_kms->glob_state);
drm_modeset_lock_fini(&mdp5_kms->glob_state_lock);
@@ -816,8 +815,6 @@ static int mdp5_init(struct platform_device
*pdev, struct drm_device *dev)
goto fail;
}
- platform_set_drvdata(pdev, mdp5_kms);
-
spin_lock_init(&mdp5_kms->resource_lock);
mdp5_kms->dev = dev;
@@ -915,7 +912,7 @@ static int mdp5_init(struct platform_device
*pdev, struct drm_device *dev)
return 0;
fail:
if (mdp5_kms)
- mdp5_destroy(pdev);
+ mdp5_destroy(mdp5_kms);
return ret;
}
@@ -975,7 +972,8 @@ static int mdp5_dev_remove(struct
platform_device *pdev)
static __maybe_unused int mdp5_runtime_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
DBG("");
@@ -985,7 +983,8 @@ static __maybe_unused int
mdp5_runtime_suspend(struct device *dev)
static __maybe_unused int mdp5_runtime_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
+ struct msm_drm_private *priv = platform_get_drvdata(pdev);
+ struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
DBG("");
--
With best wishes
Dmitry
Re: [Freedreno] [v1 2/2] drm/msm/disp/dpu1: enable crtc color management based on encoder topology
On 27/06/2022 14:56, Kalyan Thota wrote:
Thanks for the comments, Dmitry. I haven't noticed mode->hdisplay being used.
My idea was to run thru the topology and tie up the encoders with dspp to the
CRTCs.
Since mode is available only in the commit, we cannot use the
dpu_encoder_get_topology during initialization sequence.
The requirement here is that when we initialize the crtc, we need to enable
drm_crtc_enable_color_mgmt only for the crtcs that support it. As I understand
from Rob, chrome framework will check for the enablement in order to exercise
the feature.
Do you have any ideas on how to handle this requirement ? Since we will reserve
the dspp's only when a commit is issued, I guess it will be too late to enable
color management by then.
I have been thinking about this for quite a while.
Basically I fear you have two options:
- Register the color management for all CRTCs. In dpu_rm_reserve() check
for the ctm, allocate LMs taking the available DSPPs into account. Fail
the atomic_check() if there are no available LMs with required
capabilities. Additional bonus point for moving LM/DSPP resource
allocation from dpu_encoder into dpu_crtc.
- Register CRTCs and it's colormanagement properties according to exact
available hardware. Let the userspace composer select the CRTC for the
connector basing on the availability of the CTM support.
I'd vote strongly against any attempt to put the policy ('e.g. enable
CTM only for the eDP and first DSI display') into the kernel, because we
can not predict the actual usecases the user needs. It well might be
that the user of the laptop will work with DP displays only and thus
require color management for DP.
@robdcl...@gmail.com
Is it okay, if we disable color management for all the crtcs during
initialization and enable it when we have dspps available during modeset. Can
we framework code query for the property before issuing a commit for the frame
after modeset ?
--
With best wishes
Dmitry
Re: [Freedreno] [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge
On 26/08/2022 14:55, Laurent Pinchart wrote: Hello, On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote: On 22/08/2022 19:31, Dmitry Baryshkov wrote: On 09/08/2022 22:40, Laurent Pinchart wrote: On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar wrote: adv7533 bridge tries to dynamically switch lanes based on the mode by detaching and attaching the mipi dsi device. This approach is incorrect because as per the DSI spec the number of lanes is fixed at the time of system design or initial configuration and may not change dynamically. Is that really so ? The number of lanes connected on the board is certainlyset at design time, but a lower number of lanes can be used at runtime. It shouldn't change dynamically while the display is on, but it could change at mode set time. I'm not sure if I interpreted the standard correctly, but I tended to have the same interpretation as Abhinav did. Anyway, even if we allow the drivers to switch the amount of lanes, this should not happen in the form of detach()/attach() cycle. The drivers Agreed. use mipi_dsi_attach() as way to signal the DSI hosts that the sink device is ready. Some of DSI hosts (including MSM one) would bind components from the attach callback. If we were to support dynamically changing the amount of lanes, I would ask for additional mipi_dsi API call telling the host to switch the amount of lanes. And note that this can open the can of worms. Different hosts might have different requirements here. For example for the MSM platform the amount of lanes is programmed during bridge_pre_enable chain call, so it is possible to just set the amount of lanes following the msm_dsi_device::lanes. Other hosts might have other requirements. Conceptually, I would expect the number of effective lanes to be selected at mode set time, because it has to be done while the output is disabled. There is one tightly coupled question. The dual-DSI (or bonded-DSI) mode. Currently it is exposed as two independent DSI hosts. If we allow changing the amount of DSI lanes at runtime, bonded DSI mode would become complicated by fixing amount of lanes for each of outputs (or switching them in tight loop). And then comes the question of switching between single-DSI and bonded-DSI at runtime. With the atomic API for bridges the .mode_set() operation is deprecated, so .pre_enable() sounds best, but there's a potential issue: the .pre_enable() operation is called from sink to source. It means that a DSI sink .pre_enable() operation would need to call a DSI host operation to set (or maybe negotiate instead of just setting a fixed value) the number of lanes first if it wants to control the sink through DCS at .pre_enable() time. We'd have to see how that fits. What is the fate of the patchset that implemented 'parent-first' opt-in for the drm_bridge chains? It was supposed to solve this this kind of issues. I'm asking because until it is merged some DSI hosts (e.g. the msm dsi) turn on the power in .mode_set() to allow the pre_enable() callbacks work when the DSI link is in LP11 mode. Back then I voted for the explicit mipi_dsi_power_on kind of calls, which would allow the sink bridge to prepare for the DSI powerup (e.g. by setting the amount of lanes), power up the DSI host, putting the link into LP11 and after that communicate with the sink using the DSI data lanes. Thus said I'd suggest accepting this patch and maybe working on the API/support for the lane switching as a followup. I don't have a personal need for the ADV7533 so I won't really complain about any potential regression this may introduce (given that it fixes a deadlock maybe things are completely broken already and nothing can regress). I'd like to see this fixed though, doing it as a follow up is too often a way to avoid doing it at all :-) I don't know if this sounds like a promise, we are supporting several devices which use adv75xx (including famous dragonboard410c and less known Inforce ifc6510). So it might be (*) in our interest to restore this functionality. However as it requires adding additional API, design & negotiations might take some time. (*) might be if it limits the functionality of the device by limiting support for different modes. If not... why care then? In any case, the commit message should be reworded to explain the rationale and what needs to be done. Adding a TODO or FIXME comment in the code would also help. -- With best wishes Dmitry
Re: [Freedreno] [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge
Hi Abhinav,
On Tue, Aug 09, 2022 at 02:47:32PM -0700, Abhinav Kumar wrote:
> On 8/9/2022 12:40 PM, Laurent Pinchart wrote:
> > On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar wrote:
> >> adv7533 bridge tries to dynamically switch lanes based on the
> >> mode by detaching and attaching the mipi dsi device.
> >>
> >> This approach is incorrect because as per the DSI spec the
> >> number of lanes is fixed at the time of system design or initial
> >> configuration and may not change dynamically.
> >
> > Is that really so ? The number of lanes connected on the board is
> > certainlyset at design time, but a lower number of lanes can be used at
> > runtime. It shouldn't change dynamically while the display is on, but it
> > could change at mode set time.
>
> So as per the spec it says this:
>
> "The number of Lanes used shall be a static parameter. It shall be fixed
> at the time of system design or initial configuration and may not change
> dynamically. Typically, the peripheral’s bandwidth requirement and its
> corresponding Lane configuration establishes the number of Lanes used in
> a system."
>
> But I do agree with you that this does not prohibit us from changing the
> lanes during mode_set time because display might have been off.
Yes, I would consider mode set time as "initial configuration".
> Although, I really did not see any other bridges doing it this way.
>
> At the same time, detach() and attach() cycle seems the incorrect way to
> do it here.
I fully agree.
> We did think of another approach of having a new mipi_dsi_op to
> reconfigure the host without the component_del()/component_add() because
> most of the host drivers also do component_del()/component_add() in
> detach()/attach().
Anything that avoids the usage of the component framework is likely a
good idea :-) I'd really like to see it go.
> But that would not work here either because this bridge is after the DSI
> controller's bridge in the chain.
>
> Hence DSI controller's modeset would happen earlier than this one.
With the atomic bridge API the .mode_set() operation is deprecated in
favour of the atomic version of the enable and pre-enable operations.
Pre-enable would likely be a good time to handle this.
> So even if we do have another op to reconfigure the host , this bridge's
> modeset would be too late to call that new op.
>
> It complicates things a bit, so we thought that not supporting dynamic
> lane switching would be the better way forward unless there is another
> suggestion on how to support this.
>
> >> In addition this method of dynamic switch of detaching and
> >> attaching the mipi dsi device also results in removing
> >> and adding the component which is not necessary.
> >
> > Yes, that doesn't look good, and the .mode_valid() operation is
> > definitely not the right point where to set the number of lanes.
>
> I didnt follow this. Did you mean mode_set() is not the right point to
> set the number of lanes?
Mode set time is conceptually the right point (but the .mode_set()
operation isn't, given the above), but .mode_valid() isn't.
.mode_valid() validates a mode, it should not affect the hardware
configuration in any way.
> So without this change, adv7533 changes the number of lanes during
> mode_set time followed by a detach/attach cycle.
>
> >> This approach is also prone to deadlocks. So for example, on the
> >> db410c whenever this path is executed with lockdep enabled,
> >> this results in a deadlock due to below ordering of locks.
> >>
> >> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
> >> lock_acquire+0x6c/0x90
> >> drm_modeset_acquire_init+0xf4/0x150
> >> drmm_mode_config_init+0x220/0x770
> >> msm_drm_bind+0x13c/0x654
> >> try_to_bring_up_aggregate_device+0x164/0x1d0
> >> __component_add+0xa8/0x174
> >> component_add+0x18/0x2c
> >> dsi_dev_attach+0x24/0x30
> >> dsi_host_attach+0x98/0x14c
> >> devm_mipi_dsi_attach+0x38/0xb0
> >> adv7533_attach_dsi+0x8c/0x110
> >> adv7511_probe+0x5a0/0x930
> >> i2c_device_probe+0x30c/0x350
> >> really_probe.part.0+0x9c/0x2b0
> >> __driver_probe_device+0x98/0x144
> >> driver_probe_device+0xac/0x14c
> >> __device_attach_driver+0xbc/0x124
> >> bus_for_each_drv+0x78/0xd0
> >> __device_attach+0xa8/0x1c0
> >> device_initial_probe+0x18/0x24
> >> bus_probe_device+0xa0/0xac
> >> deferred_probe_work_func+0x90/0xd0
> >> process_one_work+0x28c/0x6b0
> >> worker_thread+0x240/0x444
> >> kthread+0x110/0x114
> >> ret_from_fork+0x10/0x20
> >>
> >> -> #0 (component_mutex){+.+.}-{3:3}:
> >> __lock_acquire+0x1280/0x20ac
> >> lock_acquire.part.0+0xe0/0x230
> >> lock_acquire+0x6c/0x90
> >> __mutex_lock+0x84/0x400
> >> mutex_lock_nested+0x3c/0x70
> >> component_del+0x34/0x170
>
Re: [Freedreno] [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge
Hello,
On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote:
> On 22/08/2022 19:31, Dmitry Baryshkov wrote:
> > On 09/08/2022 22:40, Laurent Pinchart wrote:
> >> On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar wrote:
> >>> adv7533 bridge tries to dynamically switch lanes based on the
> >>> mode by detaching and attaching the mipi dsi device.
> >>>
> >>> This approach is incorrect because as per the DSI spec the
> >>> number of lanes is fixed at the time of system design or initial
> >>> configuration and may not change dynamically.
> >>
> >> Is that really so ? The number of lanes connected on the board is
> >> certainlyset at design time, but a lower number of lanes can be used at
> >> runtime. It shouldn't change dynamically while the display is on, but it
> >> could change at mode set time.
> >
> > I'm not sure if I interpreted the standard correctly, but I tended to
> > have the same interpretation as Abhinav did.
> >
> > Anyway, even if we allow the drivers to switch the amount of lanes, this
> > should not happen in the form of detach()/attach() cycle. The drivers
Agreed.
> > use mipi_dsi_attach() as way to signal the DSI hosts that the sink
> > device is ready. Some of DSI hosts (including MSM one) would bind
> > components from the attach callback.
> >
> > If we were to support dynamically changing the amount of lanes, I would
> > ask for additional mipi_dsi API call telling the host to switch the
> > amount of lanes. And note that this can open the can of worms. Different
> > hosts might have different requirements here. For example for the MSM
> > platform the amount of lanes is programmed during bridge_pre_enable
> > chain call, so it is possible to just set the amount of lanes following
> > the msm_dsi_device::lanes. Other hosts might have other requirements.
Conceptually, I would expect the number of effective lanes to be
selected at mode set time, because it has to be done while the output is
disabled. With the atomic API for bridges the .mode_set() operation is
deprecated, so .pre_enable() sounds best, but there's a potential issue:
the .pre_enable() operation is called from sink to source. It means that
a DSI sink .pre_enable() operation would need to call a DSI host
operation to set (or maybe negotiate instead of just setting a fixed
value) the number of lanes first if it wants to control the sink through
DCS at .pre_enable() time. We'd have to see how that fits.
> > Thus said I'd suggest accepting this patch and maybe working on the
> > API/support for the lane switching as a followup.
I don't have a personal need for the ADV7533 so I won't really complain
about any potential regression this may introduce (given that it fixes a
deadlock maybe things are completely broken already and nothing can
regress). I'd like to see this fixed though, doing it as a follow up is
too often a way to avoid doing it at all :-)
In any case, the commit message should be reworded to explain the
rationale and what needs to be done. Adding a TODO or FIXME comment in
the code would also help.
> Laurent, gracious ping for your response.
Done :-)
> >>> In addition this method of dynamic switch of detaching and
> >>> attaching the mipi dsi device also results in removing
> >>> and adding the component which is not necessary.
> >>
> >> Yes, that doesn't look good, and the .mode_valid() operation is
> >> definitely not the right point where to set the number of lanes.
> >
> > .mode_set()?
> >
> >>> This approach is also prone to deadlocks. So for example, on the
> >>> db410c whenever this path is executed with lockdep enabled,
> >>> this results in a deadlock due to below ordering of locks.
> >
> > Even without the deadlock, we are pulling the whole DRM device from
> > beneath the DSI panel by unbinding all the components. This is not going
> > to work correctly.
> >
> >>>
> >>> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
> >>> lock_acquire+0x6c/0x90
> >>> drm_modeset_acquire_init+0xf4/0x150
> >>> drmm_mode_config_init+0x220/0x770
> >>> msm_drm_bind+0x13c/0x654
> >>> try_to_bring_up_aggregate_device+0x164/0x1d0
> >>> __component_add+0xa8/0x174
> >>> component_add+0x18/0x2c
> >>> dsi_dev_attach+0x24/0x30
> >>> dsi_host_attach+0x98/0x14c
> >>> devm_mipi_dsi_attach+0x38/0xb0
> >>> adv7533_attach_dsi+0x8c/0x110
> >>> adv7511_probe+0x5a0/0x930
> >>> i2c_device_probe+0x30c/0x350
> >>> really_probe.part.0+0x9c/0x2b0
> >>> __driver_probe_device+0x98/0x144
> >>> driver_probe_device+0xac/0x14c
> >>> __device_attach_driver+0xbc/0x124
> >>> bus_for_each_drv+0x78/0xd0
> >>> __device_attach+0xa8/0x1c0
> >>> device_initial_probe+0x18/0x24
> >>> bus_probe_device+0xa0/0xac
> >>> deferred_probe_work_func+0x90/0xd0
> >>> process_one_work+0x28c/0x6b0
> >>> worker_thread+0x240
Re: [Freedreno] [v1] drm/msm/disp/dpu1: add support for hierarchical flush for dspp in sc7280
On 08/08/2022 13:44, Kalyan Thota wrote:
-Original Message-
From: Dmitry Baryshkov
Sent: Thursday, August 4, 2022 9:29 PM
To: Kalyan Thota (QUIC)
Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
freedreno@lists.freedesktop.org; devicet...@vger.kernel.org; linux-
ker...@vger.kernel.org; robdcl...@gmail.com; diand...@chromium.org;
swb...@chromium.org; Vinod Polimera (QUIC) ;
Abhinav Kumar (QUIC)
Subject: Re: [v1] drm/msm/disp/dpu1: add support for hierarchical flush for dspp
in sc7280
WARNING: This email originated from outside of Qualcomm. Please be wary of
any links or attachments, and do not enable macros.
On Thu, 4 Aug 2022 at 13:29, Kalyan Thota wrote:
Flush mechanism for DSPP blocks has changed in sc7280 family, it
allows individual sub blocks to be flushed in coordination with master
flush control.
representation: master_flush && (PCC_flush | IGC_flush .. etc )
This change adds necessary support for the above design.
Signed-off-by: Kalyan Thota
I'd like to land at least patches 6-8 from [1] next cycle. They clean up the CTL
interface. Could you please rebase your patch on top of them?
Sure I'll wait for the series to rebase. @Doug can you comment if this is okay
and this patch is not needed immediately ?
The respective patches have been picked up for 6.1 and were pushed to
https://gitlab.freedesktop.org/lumag/msm.git msm-next-lumag . Could you
please rebase your patch on top of them?
All other comments also needs addressing.
[1] https://patchwork.freedesktop.org/series/99909/
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 40
+-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 3 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h| 7 +
6 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 7763558..4eca317 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -703,6 +703,10 @@ static void _dpu_crtc_setup_cp_blocks(struct
drm_crtc *crtc)
mixer[i].flush_mask |= ctl->ops.get_bitmask_dspp(ctl,
mixer[i].hw_dspp->idx);
+ if(ctl->ops.set_dspp_hierarchical_flush)
+ ctl->ops.set_dspp_hierarchical_flush(ctl,
+ mixer[i].hw_dspp->idx,
+ DSPP_SUB_PCC);
+
/* stage config flush mask */
ctl->ops.update_pending_flush(ctl,
mixer[i].flush_mask);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 021eb2f..3b27a87 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -58,7 +58,10 @@
(PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
#define CTL_SC7280_MASK \
- (BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) |
BIT(DPU_CTL_VM_CFG))
+ (BIT(DPU_CTL_ACTIVE_CFG) | \
+BIT(DPU_CTL_FETCH_ACTIVE) | \
+BIT(DPU_CTL_VM_CFG) | \
+BIT(DPU_CTL_HIERARCHICAL_FLUSH))
#define MERGE_3D_SM8150_MASK (0)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index b85b24b..7922f6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -185,6 +185,7 @@ enum {
* @DPU_CTL_SPLIT_DISPLAY: CTL supports video mode split display
* @DPU_CTL_FETCH_ACTIVE: Active CTL for fetch HW (SSPPs)
* @DPU_CTL_VM_CFG:CTL config to support multiple VMs
+ * @DPU_CTL_HIERARCHICAL_FLUSH: CTL config to support hierarchical
+ flush
* @DPU_CTL_MAX
*/
enum {
@@ -192,6 +193,7 @@ enum {
DPU_CTL_ACTIVE_CFG,
DPU_CTL_FETCH_ACTIVE,
DPU_CTL_VM_CFG,
+ DPU_CTL_HIERARCHICAL_FLUSH,
DPU_CTL_MAX
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 3584f5e..b34fc30 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -28,6 +28,8 @@
#define CTL_INTF_FLUSH0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
+#define CTL_DSPP_0_FLUSH 0x13C
Please change to CTL_DSPP_n_FLUSH(n).
+
#define CTL_MIXER_BORDER_OUTBIT(24)
#define CTL_FLUSH_MASK_CTL BIT(17)
@@ -292,6 +294,36 @@ static uint32_t dpu_hw_ctl_get_bitmask_dspp(struct
dpu_hw_ctl *ctx,
return flushbits;
}
+static uint32_t dpu_hw_ctl_get_bitmask_dspp_v1(struct dpu_hw_ctl *ctx,
+ enum dpu_dspp dspp)
+{
+ return BIT(29);
+}
+
+static void dpu_hw_c
Re: [Freedreno] [RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge
On 22/08/2022 19:31, Dmitry Baryshkov wrote:
On 09/08/2022 22:40, Laurent Pinchart wrote:
Hi Abhinav,
Thank you for the patch.
On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar wrote:
adv7533 bridge tries to dynamically switch lanes based on the
mode by detaching and attaching the mipi dsi device.
This approach is incorrect because as per the DSI spec the
number of lanes is fixed at the time of system design or initial
configuration and may not change dynamically.
Is that really so ? The number of lanes connected on the board is
certainlyset at design time, but a lower number of lanes can be used at
runtime. It shouldn't change dynamically while the display is on, but it
could change at mode set time.
I'm not sure if I interpreted the standard correctly, but I tended to
have the same interpretation as Abhinav did.
Anyway, even if we allow the drivers to switch the amount of lanes, this
should not happen in the form of detach()/attach() cycle. The drivers
use mipi_dsi_attach() as way to signal the DSI hosts that the sink
device is ready. Some of DSI hosts (including MSM one) would bind
components from the attach callback.
If we were to support dynamically changing the amount of lanes, I would
ask for additional mipi_dsi API call telling the host to switch the
amount of lanes. And note that this can open the can of worms. Different
hosts might have different requirements here. For example for the MSM
platform the amount of lanes is programmed during bridge_pre_enable
chain call, so it is possible to just set the amount of lanes following
the msm_dsi_device::lanes. Other hosts might have other requirements.
Thus said I'd suggest accepting this patch and maybe working on the
API/support for the lane switching as a followup.
Laurent, gracious ping for your response.
In addition this method of dynamic switch of detaching and
attaching the mipi dsi device also results in removing
and adding the component which is not necessary.
Yes, that doesn't look good, and the .mode_valid() operation is
definitely not the right point where to set the number of lanes.
.mode_set()?
This approach is also prone to deadlocks. So for example, on the
db410c whenever this path is executed with lockdep enabled,
this results in a deadlock due to below ordering of locks.
Even without the deadlock, we are pulling the whole DRM device from
beneath the DSI panel by unbinding all the components. This is not going
to work correctly.
-> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
lock_acquire+0x6c/0x90
drm_modeset_acquire_init+0xf4/0x150
drmm_mode_config_init+0x220/0x770
msm_drm_bind+0x13c/0x654
try_to_bring_up_aggregate_device+0x164/0x1d0
__component_add+0xa8/0x174
component_add+0x18/0x2c
dsi_dev_attach+0x24/0x30
dsi_host_attach+0x98/0x14c
devm_mipi_dsi_attach+0x38/0xb0
adv7533_attach_dsi+0x8c/0x110
adv7511_probe+0x5a0/0x930
i2c_device_probe+0x30c/0x350
really_probe.part.0+0x9c/0x2b0
__driver_probe_device+0x98/0x144
driver_probe_device+0xac/0x14c
__device_attach_driver+0xbc/0x124
bus_for_each_drv+0x78/0xd0
__device_attach+0xa8/0x1c0
device_initial_probe+0x18/0x24
bus_probe_device+0xa0/0xac
deferred_probe_work_func+0x90/0xd0
process_one_work+0x28c/0x6b0
worker_thread+0x240/0x444
kthread+0x110/0x114
ret_from_fork+0x10/0x20
-> #0 (component_mutex){+.+.}-{3:3}:
__lock_acquire+0x1280/0x20ac
lock_acquire.part.0+0xe0/0x230
lock_acquire+0x6c/0x90
__mutex_lock+0x84/0x400
mutex_lock_nested+0x3c/0x70
component_del+0x34/0x170
dsi_dev_detach+0x24/0x30
dsi_host_detach+0x20/0x64
mipi_dsi_detach+0x2c/0x40
adv7533_mode_set+0x64/0x90
adv7511_bridge_mode_set+0x210/0x214
drm_bridge_chain_mode_set+0x5c/0x84
crtc_set_mode+0x18c/0x1dc
drm_atomic_helper_commit_modeset_disables+0x40/0x50
msm_atomic_commit_tail+0x1d0/0x6e0
commit_tail+0xa4/0x180
drm_atomic_helper_commit+0x178/0x3b0
drm_atomic_commit+0xa4/0xe0
drm_client_modeset_commit_atomic+0x228/0x284
drm_client_modeset_commit_locked+0x64/0x1d0
drm_client_modeset_commit+0x34/0x60
drm_fb_helper_lastclose+0x74/0xcc
drm_lastclose+0x3c/0x80
drm_release+0xfc/0x114
__fput+0x70/0x224
fput+0x14/0x20
task_work_run+0x88/0x1a0
do_exit+0x350/0xa50
do_group_exit+0x38/0xa4
__wake_up_parent+0x0/0x34
invoke_syscall+0x48/0x114
el0_svc_common.constprop.0+0x60/0x11c
do_el0_svc+0x30/0xc0
el0_svc+0x58/0x100
el0t_64_sync_handler+0x1b0/0x1bc
el0t_64_sync+0x18c/0x190
Due to above reasons, remove the dynamic lane switching
code
Re: [Freedreno] [PATCH v3] drm/msm/dp: correct 1.62G link rate at dp_catalog_ctrl_config_msa()
On 24/08/2022 23:15, Kuogee Hsieh wrote:
At current implementation there is an extra 0 at 1.62G link rate which cause
no correct pixel_div selected for 1.62G link rate to calculate mvid and nvid.
This patch delete the extra 0 to have mvid and nvid be calculated correctly.
Changes in v2:
-- fix Fixes tag's text
Changes in v3:
-- fix misspelling of "Reviewed-by"
Fixes: 937f941ca06f ("drm/msm/dp: Use qmp phy for DP PLL and PHY")
Signed-off-by: Kuogee Hsieh
No extra empty lines between the tags please. I'll correct this manually
while applying.
Reviewed-by: Stephen Boyd
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 7257515..676279d 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -431,7 +431,7 @@ void dp_catalog_ctrl_config_msa(struct dp_catalog
*dp_catalog,
if (rate == link_rate_hbr3)
pixel_div = 6;
- else if (rate == 162 || rate == 27)
+ else if (rate == 162000 || rate == 27)
pixel_div = 2;
else if (rate == link_rate_hbr2)
pixel_div = 4;
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH -next] drm/msm/adreno: Switch to memdup_user_nul() helper
On 26/08/2022 11:45, Yang Yingliang wrote:
Use memdup_user_nul() helper instead of open-coding to
simplify the code.
Signed-off-by: Yang Yingliang
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 382fb7f9e497..50b33e14237b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -339,17 +339,9 @@ int adreno_set_param(struct msm_gpu *gpu, struct
msm_file_private *ctx,
case MSM_PARAM_CMDLINE: {
char *str, **paramp;
- str = kmalloc(len + 1, GFP_KERNEL);
- if (!str)
- return -ENOMEM;
-
- if (copy_from_user(str, u64_to_user_ptr(value), len)) {
- kfree(str);
- return -EFAULT;
- }
-
- /* Ensure string is null terminated: */
- str[len] = '\0';
+ str = memdup_user_nul(u64_to_user_ptr(value), len);
+ if (IS_ERR(str))
+ return PTR_ERR(str);
if (param == MSM_PARAM_COMM) {
paramp = &ctx->comm;
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] dt-bindings: display: Add missing (unevaluated|additional)Properties on child nodes
On 23/08/2022 17:56, Rob Herring wrote: In order to ensure only documented properties are present, node schemas must have unevaluatedProperties or additionalProperties set to false (typically). Signed-off-by: Rob Herring --- Documentation/devicetree/bindings/display/arm,komeda.yaml| 1 + Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml | 1 + Documentation/devicetree/bindings/display/msm/gpu.yaml | 1 + Reviewed-by: Dmitry Baryshkov # msm .../bindings/display/samsung/samsung,exynos7-decon.yaml | 1 + .../devicetree/bindings/display/samsung/samsung,fimd.yaml| 1 + 5 files changed, 5 insertions(+) -- With best wishes Dmitry
Re: [Freedreno] [PATCH] msm/adreno: fix repeated words in comments
On 26/08/2022 12:53, Dan Carpenter wrote: On Fri, Aug 26, 2022 at 12:45:09PM +0300, Dmitry Baryshkov wrote: On 24/07/2022 10:36, wangjianli wrote: Delete the redundant word 'in'. Could you please: - adjust the commit subject to follow the rest of commit messages, - drop the extra whitespace at the beginning of the commit message, - add a correct Fixes tag. This doesn't fix a bug so the fixes tag is inappropriate. Well, it fixes a typo, but I see your point. Let's not insist on Fixes for the comment fixes. -- With best wishes Dmitry
Re: [Freedreno] [PATCH] msm/adreno: fix repeated words in comments
On Fri, Aug 26, 2022 at 12:45:09PM +0300, Dmitry Baryshkov wrote: > On 24/07/2022 10:36, wangjianli wrote: > > Delete the redundant word 'in'. > > Could you please: > - adjust the commit subject to follow the rest of commit messages, > - drop the extra whitespace at the beginning of the commit message, > - add a correct Fixes tag. This doesn't fix a bug so the fixes tag is inappropriate. regards, dan carpenter
Re: [Freedreno] [PATCH v2 5/5] dt-bindings: display/msm: dpu-sdm845: add missing DPU opp-table
On 17/08/2022 09:20, Krzysztof Kozlowski wrote:
The 'display-controller' child (DPU) of Display SubSystem (MDSS) uses
opp-table, so reference it which allows restricting DPU schema to fixed
list of properties.
Fixes: 3d7a0dd8f39b ("dt-bindings: msm: disp: add yaml schemas for DPU
bindings")
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 4/5] dt-bindings: display/msm: dpu-sc7280: add missing DPU opp-table
On 17/08/2022 09:20, Krzysztof Kozlowski wrote:
The 'display-controller' child (DPU) of Display SubSystem (MDSS) uses
opp-table, so reference it which allows restricting DPU schema to fixed
list of properties.
Fixes: 57fd4f34ddac ("dt-bindings: msm: add DT bindings for sc7280")
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 3/5] dt-bindings: display/msm: dpu-sc7180: add missing DPU opp-table
On 17/08/2022 09:20, Krzysztof Kozlowski wrote:
The 'display-controller' child (DPU) of Display SubSystem (MDSS) uses
opp-table, so reference it which allows restricting DPU schema to fixed
list of properties.
Fixes: 3d7a0dd8f39b ("dt-bindings: msm: disp: add yaml schemas for DPU
bindings")
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 2/5] dt-bindings: display/msm: dpu-qcm2290: add missing DPU opp-table
On 17/08/2022 09:20, Krzysztof Kozlowski wrote:
The 'display-controller' child (DPU) of Display SubSystem (MDSS) uses
opp-table, so reference it which allows restricting DPU schema to fixed
list of properties.
Fixes: 164f69d9d45a ("dt-bindings: msm: disp: add yaml schemas for QCM2290 DPU
bindings")
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 1/5] dt-bindings: display/msm: dpu-msm8998: add missing DPU opp-table
On 17/08/2022 09:20, Krzysztof Kozlowski wrote:
The 'display-controller' child (DPU) of Display SubSystem (MDSS) uses
opp-table, so reference it which allows restricting DPU schema to fixed
list of properties.
Fixes: 6e986a8f1cf1 ("dt-bindings: display: msm: Add binding for msm8998 dpu")
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 5/5] dt-bindings: display: drop minItems equal to maxItems
On 25/08/2022 14:33, Krzysztof Kozlowski wrote: minItems, if missing, are implicitly equal to maxItems, so drop redundant piece to reduce size of code. Signed-off-by: Krzysztof Kozlowski --- Documentation/devicetree/bindings/display/bridge/fsl,ldb.yaml | 1 - .../devicetree/bindings/display/msm/dsi-controller-main.yaml| 2 -- Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml | 2 -- For msm changes: Reviewed-by: Dmitry Baryshkov .../bindings/display/samsung/samsung,exynos5433-decon.yaml | 2 -- .../bindings/display/samsung/samsung,exynos5433-mic.yaml| 1 - .../bindings/display/samsung/samsung,exynos7-decon.yaml | 1 - .../devicetree/bindings/display/samsung/samsung,fimd.yaml | 1 - .../devicetree/bindings/display/tegra/nvidia,tegra20-gr3d.yaml | 1 - .../devicetree/bindings/display/tegra/nvidia,tegra20-mpe.yaml | 2 -- 9 files changed, 13 deletions(-) -- With best wishes Dmitry
Re: [Freedreno] [PATCH] msm/adreno: fix repeated words in comments
On 23/08/2022 14:56, Jilin Yuan wrote:
Delete the redundant word 'power'.
Delete the redundant word 'in'.
Delete the redundant word 'for'.
Signed-off-by: Jilin Yuan
Could you please:
- adjust the commit subject to follow the rest of commit messages,
- drop the extra whitespace at the beginning of the commit message,
- add a correct Fixes tag.
Thank you
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 9f76f5b15759..32ecb783c3c1 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -352,7 +352,7 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET, 1 << bit);
}
-/* Enable CPU control of SPTP power power collapse */
+/* Enable CPU control of SPTP power collapse */
static int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
{
int ret;
@@ -374,7 +374,7 @@ static int a6xx_sptprac_enable(struct a6xx_gmu *gmu)
return 0;
}
-/* Disable CPU control of SPTP power power collapse */
+/* Disable CPU control of SPTP power collapse */
static void a6xx_sptprac_disable(struct a6xx_gmu *gmu)
{
u32 val;
@@ -1277,7 +1277,7 @@ static int a6xx_gmu_rpmh_arc_votes_init(struct device
*dev, u32 *votes,
}
/*
-* Look for a level in in the secondary list that matches. If
+* Look for a level in the secondary list that matches. If
* nothing fits, use the maximum non zero vote
*/
@@ -1559,7 +1559,7 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
goto err_memory;
}
- /* Allocate memory for for the HFI queues */
+ /* Allocate memory for the HFI queues */
ret = a6xx_gmu_memory_alloc(gmu, &gmu->hfi, SZ_16K, 0, "hfi");
if (ret)
goto err_memory;
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] msm/adreno: fix repeated words in comments
On 24/07/2022 10:36, wangjianli wrote: Delete the redundant word 'in'. Could you please: - adjust the commit subject to follow the rest of commit messages, - drop the extra whitespace at the beginning of the commit message, - add a correct Fixes tag. Thank you Signed-off-by: wangjianli --- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index 9f76f5b15759..9303a011b81d 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -1277,7 +1277,7 @@ static int a6xx_gmu_rpmh_arc_votes_init(struct device *dev, u32 *votes, } /* -* Look for a level in in the secondary list that matches. If +* Look for a level in the secondary list that matches. If * nothing fits, use the maximum non zero vote */ -- With best wishes Dmitry
Re: [Freedreno] [PATCH] drm/msm: fix repeated words in comments
On 23/08/2022 14:54, Jilin Yuan wrote:
Delete the redundant word 'one'.
The whitespace is unnecessary.
Signed-off-by: Jilin Yuan
Reviewed-by: Dmitry Baryshkov
Fixes: 7198e6b03155 ("drm/msm: add a3xx gpu support")
---
drivers/gpu/drm/msm/msm_gem.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index c75d3b879a53..e300c70e8904 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -118,7 +118,7 @@ struct msm_gem_object {
* An object is either:
* inactive - on priv->inactive_dontneed or priv->inactive_willneed
* (depending on purgeability status)
-* active - on one one of the gpu's active_list.. well, at
+* active - on one of the gpu's active_list.. well, at
* least for now we don't have (I don't think) hw sync between
* 2d and 3d one devices which have both, meaning we need to
* block on submit if a bo is already on other ring
--
With best wishes
Dmitry
[Freedreno] [PATCH v2 5/5] drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe()
To continue the idea of failing the probe() rather than failing the
bind(), move the call to msm_hdmi_get_phy() function to
msm_hdmi_dev_probe(), so that the driver fails the probe if PHY is not
yet available rather than succeeding the probe and then failing the
bind() with -EPROBE_DEFER.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 40 ++---
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 92627425..adaa67d9a78d 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -68,14 +68,17 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
destroy_workqueue(hdmi->workq);
msm_hdmi_hdcp_destroy(hdmi);
+ if (hdmi->i2c)
+ msm_hdmi_i2c_destroy(hdmi->i2c);
+}
+
+static void msm_hdmi_put_phy(struct hdmi *hdmi)
+{
if (hdmi->phy_dev) {
put_device(hdmi->phy_dev);
hdmi->phy = NULL;
hdmi->phy_dev = NULL;
}
-
- if (hdmi->i2c)
- msm_hdmi_i2c_destroy(hdmi->i2c);
}
static int msm_hdmi_get_phy(struct hdmi *hdmi)
@@ -91,19 +94,15 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
}
phy_pdev = of_find_device_by_node(phy_node);
- if (phy_pdev)
- hdmi->phy = platform_get_drvdata(phy_pdev);
-
of_node_put(phy_node);
- if (!phy_pdev) {
- DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
- return -EPROBE_DEFER;
- }
+ if (!phy_pdev)
+ return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is
not ready\n");
+
+ hdmi->phy = platform_get_drvdata(phy_pdev);
if (!hdmi->phy) {
- DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
put_device(&phy_pdev->dev);
- return -EPROBE_DEFER;
+ return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is
not ready\n");
}
hdmi->phy_dev = &phy_pdev->dev;
@@ -130,12 +129,6 @@ static int msm_hdmi_init(struct hdmi *hdmi)
goto fail;
}
- ret = msm_hdmi_get_phy(hdmi);
- if (ret) {
- DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
- goto fail;
- }
-
hdmi->hdcp_ctrl = msm_hdmi_hdcp_init(hdmi);
if (IS_ERR(hdmi->hdcp_ctrl)) {
dev_warn(&pdev->dev, "failed to init hdcp: disabled\n");
@@ -528,6 +521,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
if (hdmi->hpd_gpiod)
gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
+ ret = msm_hdmi_get_phy(hdmi);
+ if (ret) {
+ DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
+ return ret;
+ }
+
ret = devm_pm_runtime_enable(&pdev->dev);
if (ret)
return ret;
@@ -539,7 +538,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
static int msm_hdmi_dev_remove(struct platform_device *pdev)
{
+ struct hdmi *hdmi = dev_get_drvdata(&pdev->dev);
+
component_del(&pdev->dev, &msm_hdmi_ops);
+
+ msm_hdmi_put_phy(hdmi);
+
return 0;
}
--
2.35.1
[Freedreno] [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function
Rather than having all resource allocation happen in the _bind function
(resulting in possible EPROBE_DEFER returns and component bind/unbind
cycles) allocate and check all resources in _probe function. While we
are at it, use platform_get_irq() to get the IRQ rather than going
through the irq_of_parse_and_map().
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++-
1 file changed, 138 insertions(+), 165 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 4a364d8f4c0b..c298a36f3b42 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
if (hdmi->i2c)
msm_hdmi_i2c_destroy(hdmi->i2c);
-
- platform_set_drvdata(hdmi->pdev, NULL);
}
static int msm_hdmi_get_phy(struct hdmi *hdmi)
@@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
* we are to EPROBE_DEFER we want to do it here, rather than later
* at modeset_init() time
*/
-static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
+static int msm_hdmi_init(struct hdmi *hdmi)
{
- struct hdmi_platform_config *config = pdev->dev.platform_data;
- struct hdmi *hdmi = NULL;
- struct resource *res;
- int i, ret;
-
- hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
- if (!hdmi) {
- ret = -ENOMEM;
- goto fail;
- }
-
- hdmi->pdev = pdev;
- hdmi->config = config;
- spin_lock_init(&hdmi->reg_lock);
-
- ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL,
&hdmi->next_bridge);
- if (ret && ret != -ENODEV)
- goto fail;
-
- hdmi->mmio = msm_ioremap(pdev, "core_physical");
- if (IS_ERR(hdmi->mmio)) {
- ret = PTR_ERR(hdmi->mmio);
- goto fail;
- }
-
- /* HDCP needs physical address of hdmi register */
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- "core_physical");
- if (!res) {
- ret = -EINVAL;
- goto fail;
- }
- hdmi->mmio_phy_addr = res->start;
-
- hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
- if (IS_ERR(hdmi->qfprom_mmio)) {
- DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
- hdmi->qfprom_mmio = NULL;
- }
-
- hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
- config->hpd_reg_cnt,
- sizeof(hdmi->hpd_regs[0]),
- GFP_KERNEL);
- if (!hdmi->hpd_regs) {
- ret = -ENOMEM;
- goto fail;
- }
- for (i = 0; i < config->hpd_reg_cnt; i++)
- hdmi->hpd_regs[i].supply = config->hpd_reg_names[i];
-
- ret = devm_regulator_bulk_get(&pdev->dev, config->hpd_reg_cnt,
hdmi->hpd_regs);
- if (ret) {
- DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %d\n",
ret);
- goto fail;
- }
-
- hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
- config->pwr_reg_cnt,
- sizeof(hdmi->pwr_regs[0]),
- GFP_KERNEL);
- if (!hdmi->pwr_regs) {
- ret = -ENOMEM;
- goto fail;
- }
-
- for (i = 0; i < config->pwr_reg_cnt; i++)
- hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
-
- ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt,
hdmi->pwr_regs);
- if (ret) {
- DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: %d\n",
ret);
- goto fail;
- }
-
- hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
- config->hpd_clk_cnt,
- sizeof(hdmi->hpd_clks[0]),
- GFP_KERNEL);
- if (!hdmi->hpd_clks) {
- ret = -ENOMEM;
- goto fail;
- }
- for (i = 0; i < config->hpd_clk_cnt; i++) {
- struct clk *clk;
-
- clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
- if (IS_ERR(clk)) {
- ret = PTR_ERR(clk);
- DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s
(%d)\n",
- config->hpd_clk_names[i], ret);
- goto fail;
- }
-
- hdmi->hpd_clks[i] = clk;
- }
-
- hdmi->pwr_clks = devm_kcalloc(&pdev->dev,
- config->pwr_clk_cnt,
- sizeof(hdmi->pwr_clks[0]),
- GFP_KERNEL);
- if (!hdmi->pwr_clks) {
- ret = -ENOMEM;
- goto fail;
- }
- for (i = 0;
[Freedreno] [PATCH v2 4/5] drm/msm/hdmi: don't take extra reference on PHY device
The of_find_device_by_node() already increments the device's usage count, so there is no need to increment it again using get_device(). Drop this extra get_device(). Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index c298a36f3b42..92627425 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -106,7 +106,7 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi) return -EPROBE_DEFER; } - hdmi->phy_dev = get_device(&phy_pdev->dev); + hdmi->phy_dev = &phy_pdev->dev; return 0; } -- 2.35.1
[Freedreno] [PATCH v2 1/5] drm/msm/hdmi: use devres helper for runtime PM management
Use devm_pm_runtime_enable() to enable runtime PM. This way its effect
will be reverted on device unbind/destruction.
Fixes: 6ed9ed484d04 ("drm/msm/hdmi: Set up runtime PM for HDMI")
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 93fe61b86967..1d4557de6872 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -252,7 +252,7 @@ static struct hdmi *msm_hdmi_init(struct platform_device
*pdev)
if (hdmi->hpd_gpiod)
gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
- pm_runtime_enable(&pdev->dev);
+ devm_pm_runtime_enable(&pdev->dev);
hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
--
2.35.1
[Freedreno] [PATCH v2 2/5] drm/msm/hdmi: drop constant resource names from platform config
All MSM HDMI devices use "core_physical" and "qfprom_physical" names for
register areas. Drop them from the platform config.
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 9 +++--
drivers/gpu/drm/msm/hdmi/hdmi.h | 3 ---
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 1d4557de6872..4a364d8f4c0b 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -138,7 +138,7 @@ static struct hdmi *msm_hdmi_init(struct platform_device
*pdev)
if (ret && ret != -ENODEV)
goto fail;
- hdmi->mmio = msm_ioremap(pdev, config->mmio_name);
+ hdmi->mmio = msm_ioremap(pdev, "core_physical");
if (IS_ERR(hdmi->mmio)) {
ret = PTR_ERR(hdmi->mmio);
goto fail;
@@ -146,14 +146,14 @@ static struct hdmi *msm_hdmi_init(struct platform_device
*pdev)
/* HDCP needs physical address of hdmi register */
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- config->mmio_name);
+ "core_physical");
if (!res) {
ret = -EINVAL;
goto fail;
}
hdmi->mmio_phy_addr = res->start;
- hdmi->qfprom_mmio = msm_ioremap(pdev, config->qfprom_mmio_name);
+ hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
if (IS_ERR(hdmi->qfprom_mmio)) {
DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
hdmi->qfprom_mmio = NULL;
@@ -524,9 +524,6 @@ static int msm_hdmi_bind(struct device *dev, struct device
*master, void *data)
return -ENXIO;
}
- hdmi_cfg->mmio_name = "core_physical";
- hdmi_cfg->qfprom_mmio_name = "qfprom_physical";
-
dev->platform_data = hdmi_cfg;
hdmi = msm_hdmi_init(to_platform_device(dev));
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 04a74381aaf7..e8dbee50637f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -86,9 +86,6 @@ struct hdmi {
/* platform config data (ie. from DT, or pdata) */
struct hdmi_platform_config {
- const char *mmio_name;
- const char *qfprom_mmio_name;
-
/* regulators that need to be on for hpd: */
const char **hpd_reg_names;
int hpd_reg_cnt;
--
2.35.1
[Freedreno] [PATCH v2 0/5] drm/msm/hdmi: move resource allocation to probe function
As pointed several times in the discussions, start moving resource allocation from component bind to the probe function. This simplifies boot process, as the component will not be registered until all resources (clocks, regulators, IRQ, etc.) are not registered. Changes since v1: - Moved a call to msm_hdmi_get_phy() to msm_hdmi_dev_probe() too. Dmitry Baryshkov (5): drm/msm/hdmi: use devres helper for runtime PM management drm/msm/hdmi: drop constant resource names from platform config drm/msm/hdmi: move resource allocation to probe function drm/msm/hdmi: don't take extra reference on PHY device drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe() drivers/gpu/drm/msm/hdmi/hdmi.c | 348 +++- drivers/gpu/drm/msm/hdmi/hdmi.h | 3 - 2 files changed, 161 insertions(+), 190 deletions(-) -- 2.35.1
Re: [Freedreno] [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices
On 24/08/2022 00:31, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-08-05 04:56:30)
diff --git a/drivers/gpu/drm/msm/msm_io_utils.c
b/drivers/gpu/drm/msm/msm_io_utils.c
index 7b504617833a..d02cd29ce829 100644
--- a/drivers/gpu/drm/msm/msm_io_utils.c
+++ b/drivers/gpu/drm/msm/msm_io_utils.c
@@ -124,3 +126,23 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
work->worker = worker;
kthread_init_work(&work->work, fn);
}
+
+struct icc_path *msm_icc_get(struct device *dev, const char *name)
+{
+ struct device *mdss_dev = dev->parent;
+ struct icc_path *path;
+
+ path = of_icc_get(dev, name);
+ if (path)
+ return path;
+
+ /*
+* If there are no interconnects attached to the corresponding device
+* node, of_icc_get() will return NULL.
+*
+* If the MDP5/DPU device node doesn't have interconnects, lookup the
+* path in the parent (MDSS) device.
+*/
+ return of_icc_get(mdss_dev, name);
Perhaps this would be better served by having another icc_get() API that
looks in the device and also the parent? Or maybe there should be
interconnect-ranges (similar to clock-ranges) so that interconnects can
be mapped to child nodes in DT.
I was not sure how common this situation is, so I did not add the
helper/API. Typically the driver knows exactly, which node has the
interconnects. In our case this is complicated because we are
effectively merging two different driver generations and two different
bindings. Thus I suppose this situation is quite unique.
--
With best wishes
Dmitry
Re: [Freedreno] [RFC PATCH] drm/msm: lookup the ICC paths in both mdp5/dpu and mdss devices
On 05/08/2022 15:24, Marijn Suijten wrote:
On 2022-08-05 14:56:30, Dmitry Baryshkov wrote:
The commit 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master
components") changed the MDP5 driver to look for the interconnect paths
in the MDSS device rather than in the MDP5 device itself. This was left
unnoticed since on my testing devices the interconnects probably didn't
reach the sync state.
Rather than just using the MDP5 device for ICC path lookups for the MDP5
devices, introduce an additional helper to check both MDP5/DPU and MDSS
nodes. This will be helpful for the MDP5->DPU conversion, since the
driver will have to check both nodes.
Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
Reported-by: Marijn Suijten
Reported-by: Yassine Oudjana
Signed-off-by: Dmitry Baryshkov
Tested-by: Marijn Suijten # On sdm630
But I'm not sure about giving my Reviewed-by to this, as I'd rather
*correct* the DT bindings for sdm630 and msm8996 to provide
interconnects in the MDSS node unless there are strong reasons not to
(and I don't consider "backwards compatibility" to be one, this binding
"never even existed" if mdp5.txt is to be believed).
As a kind of a joke, I'd prefer to have interconnects in the mdp/dpu
device node. In the end, the interconnects describe the path between the
display controller and the DDR, not the path between the whole MDSS and DDR.
So, for next chipsets I'd vote to move icc to dpu/mdp node (and maybe
even move existing inerconnects to the dpu node).
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH RESEND] drm/msm/dsi: fix the inconsistent indenting
On 26/08/2022 11:49, sunliming wrote:
Fix the inconsistent indenting in function msm_dsi_dphy_timing_calc_v3().
Fix the following smatch warnings:
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c:350 msm_dsi_dphy_timing_calc_v3() warn:
inconsistent indenting
Fixes: f1fa7ff44056 ("drm/msm/dsi: implement auto PHY timing calculator for 10nm
PHY")
Reported-by: kernel test robot
Signed-off-by: sunliming
Reviewed-by: Abhinav Kumar
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm/dpu: populate wb or intf before reset_intf_cfg
On 15/07/2022 22:14, Abhinav Kumar wrote:
dpu_encoder_helper_phys_cleanup() was not populating neither
wb or intf to the intf_cfg before calling the reset_intf_cfg().
This causes the reset of the active bits of wb/intf to be
skipped which is incorrect.
Fix this by populating the relevant wb or intf indices correctly.
Fixes: ae4d721ce100 ("drm/msm/dpu: add an API to reset the encoder related hw
blocks")
Signed-off-by: Abhinav Kumar
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 3/3] drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()
On 23/06/2022 03:25, Stephen Boyd wrote: This API isn't really more than a couple lines now that we don't store the pixel_rate to the struct member. Inline it into the caller. Cc: Kuogee Hsieh Signed-off-by: Stephen Boyd Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dp/dp_ctrl.c | 44 1 file changed, 17 insertions(+), 27 deletions(-) -- With best wishes Dmitry
Re: [Freedreno] [PATCH v2 2/3] drm/msm/dp: Remove pixel_rate from struct dp_ctrl
On 23/06/2022 03:25, Stephen Boyd wrote: This struct member is stored to in the function that calls the function which uses it. That's possible with a function argument instead of storing to a struct member. Pass the pixel_rate as an argument instead to simplify the code. Note that dp_ctrl_link_maintenance() was storing the pixel_rate but never using it so we just remove the assignment from there. Cc: Kuogee Hsieh Signed-off-by: Stephen Boyd Reviewed-by: Dmitry Baryshkov --- dp_ctrl_on_link() almost doesn't even use the pixel_clk either. It just prints the value. I kept it around because maybe it is useful? But if not, then we can remove even more code. Feel free to submit a patch and check if anybody (Kuogee? Abhinav?) complains. -- With best wishes Dmitry
Re: [Freedreno] [PATCH 3/3] drm/msm/hdmi: move resource allocation to probe function
On 24/08/2022 02:58, Abhinav Kumar wrote:
On 6/16/2022 12:59 AM, Dmitry Baryshkov wrote:
Rather than having all resource allocation happen in the _bind function
(resulting in possible EPROBE_DEFER returns and component bind/unbind
cycles) allocate and check all resources in _probe function. While we
are at it, use platform_get_irq() to get the IRQ rather than going
through the irq_of_parse_and_map().
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 295 +++-
1 file changed, 134 insertions(+), 161 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 8dfe5690366b..429abd622c81 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -75,8 +75,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
if (hdmi->i2c)
msm_hdmi_i2c_destroy(hdmi->i2c);
-
- platform_set_drvdata(hdmi->pdev, NULL);
Do we still not need to do this in .remove?
}
static int msm_hdmi_get_phy(struct hdmi *hdmi)
@@ -116,138 +114,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
* we are to EPROBE_DEFER we want to do it here, rather than later
* at modeset_init() time
*/
-static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
+static int msm_hdmi_init(struct hdmi *hdmi)
{
- struct hdmi_platform_config *config = pdev->dev.platform_data;
- struct hdmi *hdmi = NULL;
- struct resource *res;
- int i, ret;
-
- hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
- if (!hdmi) {
- ret = -ENOMEM;
- goto fail;
- }
-
- hdmi->pdev = pdev;
- hdmi->config = config;
- spin_lock_init(&hdmi->reg_lock);
-
- hdmi->mmio = msm_ioremap(pdev, "core_physical");
- if (IS_ERR(hdmi->mmio)) {
- ret = PTR_ERR(hdmi->mmio);
- goto fail;
- }
-
- /* HDCP needs physical address of hdmi register */
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- "core_physical");
- if (!res) {
- ret = -EINVAL;
- goto fail;
- }
- hdmi->mmio_phy_addr = res->start;
-
- hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
- if (IS_ERR(hdmi->qfprom_mmio)) {
- DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
- hdmi->qfprom_mmio = NULL;
- }
-
- hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
- config->hpd_reg_cnt,
- sizeof(hdmi->hpd_regs[0]),
- GFP_KERNEL);
- if (!hdmi->hpd_regs) {
- ret = -ENOMEM;
- goto fail;
- }
- for (i = 0; i < config->hpd_reg_cnt; i++)
- hdmi->hpd_regs[i].supply = config->hpd_reg_names[i];
-
- ret = devm_regulator_bulk_get(&pdev->dev, config->hpd_reg_cnt,
hdmi->hpd_regs);
- if (ret) {
- DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator:
%d\n", ret);
- goto fail;
- }
-
- hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
- config->pwr_reg_cnt,
- sizeof(hdmi->pwr_regs[0]),
- GFP_KERNEL);
- if (!hdmi->pwr_regs) {
- ret = -ENOMEM;
- goto fail;
- }
-
- for (i = 0; i < config->pwr_reg_cnt; i++)
- hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
-
- ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt,
hdmi->pwr_regs);
- if (ret) {
- DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator:
%d\n", ret);
- goto fail;
- }
-
- hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
- config->hpd_clk_cnt,
- sizeof(hdmi->hpd_clks[0]),
- GFP_KERNEL);
- if (!hdmi->hpd_clks) {
- ret = -ENOMEM;
- goto fail;
- }
- for (i = 0; i < config->hpd_clk_cnt; i++) {
- struct clk *clk;
-
- clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
- if (IS_ERR(clk)) {
- ret = PTR_ERR(clk);
- DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s
(%d)\n",
- config->hpd_clk_names[i], ret);
- goto fail;
- }
-
- hdmi->hpd_clks[i] = clk;
- }
-
- hdmi->pwr_clks = devm_kcalloc(&pdev->dev,
- config->pwr_clk_cnt,
- sizeof(hdmi->pwr_clks[0]),
- GFP_KERNEL);
- if (!hdmi->pwr_clks) {
- ret = -ENOMEM;
- goto fail;
- }
- for (i = 0; i < config->pwr_clk_cnt; i++) {
- struct clk *clk;
-
- clk = msm_clk_get(pdev, config->pwr_clk_names[i]);
- if (IS_ERR(clk)) {
- ret = PTR_ERR(clk);
- DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s
(%d)\n",
- config->pwr_clk_names[i], ret);
- goto fail;
- }
-
- hdmi->pwr_clks[i] = clk;
- }
-
- hdmi->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd",
GPIOD_IN);
- /* This will catch e.g. -EPROBE_DEFER */
- if (IS_ERR(hdmi->hpd_gpiod)) {
- ret = PTR_ERR(hdm
[Freedreno] [PATCH RESEND] drm/msm/dsi: fix the inconsistent indenting
Fix the inconsistent indenting in function msm_dsi_dphy_timing_calc_v3().
Fix the following smatch warnings:
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c:350 msm_dsi_dphy_timing_calc_v3() warn:
inconsistent indenting
Fixes: f1fa7ff44056 ("drm/msm/dsi: implement auto PHY timing calculator for
10nm PHY")
Reported-by: kernel test robot
Signed-off-by: sunliming
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index a39de3bdc7fa..56dfa2d24be1 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -347,7 +347,7 @@ int msm_dsi_dphy_timing_calc_v3(struct msm_dsi_dphy_timing
*timing,
} else {
timing->shared_timings.clk_pre =
linear_inter(tmax, tmin, pcnt2, 0, false);
- timing->shared_timings.clk_pre_inc_by_2 = 0;
+ timing->shared_timings.clk_pre_inc_by_2 = 0;
}
timing->ta_go = 3;
--
2.25.1
Re: [Freedreno] [v2] drm/msm: add null checks for drm device to avoid crash during probe defer
On 15/06/2022 15:23, Dmitry Baryshkov wrote:
On 03/06/2022 12:42, Vinod Polimera wrote:
During probe defer, drm device is not initialized and an external
trigger to shutdown is trying to clean up drm device leading to crash.
Add checks to avoid drm device cleanup in such cases.
BUG: unable to handle kernel NULL pointer dereference at virtual
address 00b8
Call trace:
drm_atomic_helper_shutdown+0x44/0x144
msm_pdev_shutdown+0x2c/0x38
platform_shutdown+0x2c/0x38
device_shutdown+0x158/0x210
kernel_restart_prepare+0x40/0x4c
kernel_restart+0x20/0x6c
__arm64_sys_reboot+0x194/0x23c
invoke_syscall+0x50/0x13c
el0_svc_common+0xa0/0x17c
do_el0_svc_compat+0x28/0x34
el0_svc_compat+0x20/0x70
el0t_32_sync_handler+0xa8/0xcc
el0t_32_sync+0x1a8/0x1ac
Changes in v2:
- Add fixes tag.
Fixes: 623f279c778 ("drm/msm: fix shutdown hook in case GPU components
failed to bind")
Signed-off-by: Vinod Polimera
---
drivers/gpu/drm/msm/msm_drv.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c
b/drivers/gpu/drm/msm/msm_drv.c
index 4448536..d62ac66 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -142,6 +142,9 @@ static void msm_irq_uninstall(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct msm_kms *kms = priv->kms;
+ if (!irq_has_action(kms->irq))
+ return;
As a second thought I'd still prefer a variable here. irq_has_action
would check that there is _any_ IRQ handler for this IRQ. While we do
not have anybody sharing this IRQ, I'd prefer to be clear here, that we
do not want to uninstall our IRQ handler rather than any IRQ handler.
Vinod, do we still want to pursue this fix? If so, could you please
update it according to the comment.
+
kms->funcs->irq_uninstall(kms);
if (kms->irq_requested)
free_irq(kms->irq, dev);
@@ -259,6 +262,7 @@ static int msm_drm_uninit(struct device *dev)
ddev->dev_private = NULL;
drm_dev_put(ddev);
+ priv->dev = NULL;
destroy_workqueue(priv->wq);
@@ -1167,7 +1171,7 @@ void msm_drv_shutdown(struct platform_device *pdev)
struct msm_drm_private *priv = platform_get_drvdata(pdev);
struct drm_device *drm = priv ? priv->dev : NULL;
- if (!priv || !priv->kms)
+ if (!priv || !priv->kms || !drm)
return;
drm_atomic_helper_shutdown(drm);
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH linux-next] drm/msm/dsi: Remove the unneeded result variable
On 26/08/2022 10:28, cgel@gmail.com wrote: From: ye xingchen Return the value msm_dsi_phy_enable() directly instead of storing it in another redundant variable. Reported-by: Zeal Robot Signed-off-by: ye xingchen Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [Freedreno] [PATCH] drm/msm/dp: add atomic_check to bridge ops
On 24/08/2022 22:16, Abhinav Kumar wrote: On 8/24/2022 1:25 AM, Dmitry Baryshkov wrote: On Wed, 24 Aug 2022 at 01:59, Abhinav Kumar wrote: On 8/23/2022 3:41 PM, Dmitry Baryshkov wrote: On Wed, 24 Aug 2022 at 01:07, Abhinav Kumar wrote: On 8/22/2022 11:33 AM, Dmitry Baryshkov wrote: On 22/08/2022 20:32, Abhinav Kumar wrote: On 8/22/2022 9:49 AM, Dmitry Baryshkov wrote: On 22/08/2022 19:38, Abhinav Kumar wrote: Hi Dmitry On 8/22/2022 9:18 AM, Dmitry Baryshkov wrote: On 17/08/2022 21:01, Kuogee Hsieh wrote: DRM commit_tails() will disable downstream crtc/encoder/bridge if both disable crtc is required and crtc->active is set before pushing a new frame downstream. There is a rare case that user space display manager issue an extra screen update immediately followed by close DRM device while down stream display interface is disabled. This extra screen update will timeout due to the downstream interface is disabled but will cause crtc->active be set. Hence the followed commit_tails() called by drm_release() will pass the disable downstream crtc/encoder/bridge conditions checking even downstream interface is disabled. This cause the crash to happen at dp_bridge_disable() due to it trying to access the main link register to push the idle pattern out while main link clocks is disabled. This patch adds atomic_check to prevent the extra frame will not be pushed down if display interface is down so that crtc->active will not be set neither. This will fail the conditions checking of disabling down stream crtc/encoder/bridge which prevent drm_release() from calling dp_bridge_disable() so that crash at dp_bridge_disable() prevented. I must admit I had troubles parsing this description. However if I got you right, I think the check that the main link clock is running in the dp_bridge_disable() or dp_ctrl_push_idle() would be a better fix. Originally, thats what was posted https://patchwork.freedesktop.org/patch/496984/. This patch is also not so correct from my POV. It checks for the hpd status, while in reality it should check for main link clocks being enabled. We can push another fix to check for the clk state instead of the hpd status. But I must say we are again just masking something which the fwk should have avoided isnt it? As per the doc in the include/drm/drm_bridge.h it says, "* * The bridge can assume that the display pipe (i.e. clocks and timing * signals) feeding it is still running when this callback is called. *" Yes, that's what I meant about this chunk begging to go to the core. In my opinion, if we are talking about the disconnected sinks, it is the framework who should disallow submitting the frames to the disconnected sinks. By adding an extra layers of protection in the driver, we are just avoiding another issue but the commit should not have been issued in the first place. So shouldnt we do both then? That is add protection to check if clock is ON and also, reject commits when display is disconnected. Then it seemed like we were just protecting against an issue in the framework which was allowing the frames to be pushed even after the display was disconnected. The DP driver did send out the disconnect event correctly and as per the logs, this frame came down after that and the DRM fwk did allow it. So after discussing on IRC with Rob, we came up with this approach that if the display is not connected, then atomic_check should fail. That way the commit will not happen. Just seemed a bit cleaner instead of adding all our protections. The check to fail atomic_check if display is not connected seems out of place. In its current way it begs go to the upper layer, forbidding using disconnected sinks for all the drivers. There is nothing special in the MSM DP driver with respect to the HPD events processing and failing atomic_check() based on that. Why all the drivers? This is only for MSM DP bridge. Yes, we change the MSM DRM driver. But the check is generic enough. I'm not actually insisting on pushing the check to the core, just trying to understand the real cause here. I actually wanted to push this to the core and thats what I had originally asked on IRC because it does seem to be generic enough that it should belong to the core but after discussion with Rob on freedreno, he felt this was a better approach because for some of the legacy connectors like VGA, this need not belong to the DRM core, hence we went with this approach. It might be better to whitelist such connectors (S-VIDEO/composite comes to my mind rather than VGA). I am fine with that approach, if Rob is onboard with that. SError Interrupt on CPU7, code 0xbe000411 -- SError CPU: 7 PID: 3878 Comm: Xorg Not tainted 5.19.0-stb-cbq #19 Hardware name: Google Lazor (rev3 - 8) (DT) pstate: a04000c9 (NzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : __cmpxchg_case_acq_32+0x14/0x2c lr : do_raw_spin_lock+0xa4/0xdc sp : f
[Freedreno] [PATCH linux-next] drm/msm/dsi: Remove the unneeded result variable
From: ye xingchen
Return the value msm_dsi_phy_enable() directly instead of storing it in
another redundant variable.
Reported-by: Zeal Robot
Signed-off-by: ye xingchen
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index cb84d185d73a..0b516a04945f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -141,14 +141,11 @@ static int enable_phy(struct msm_dsi *msm_dsi,
struct msm_dsi_phy_shared_timings *shared_timings)
{
struct msm_dsi_phy_clk_request clk_req;
- int ret;
bool is_bonded_dsi = IS_BONDED_DSI();
msm_dsi_host_get_phy_clk_req(msm_dsi->host, &clk_req, is_bonded_dsi);
- ret = msm_dsi_phy_enable(msm_dsi->phy, &clk_req, shared_timings);
-
- return ret;
+ return msm_dsi_phy_enable(msm_dsi->phy, &clk_req, shared_timings);
}
static int
--
2.25.1
