Re: [Freedreno] [PATCH 1/4] drm/msm/dp: Simplify the mvid/nvid calculation
Hi Bjorn On 2021-06-08 13:13, Bjorn Andersson wrote: On Fri 28 May 18:11 CDT 2021, abhin...@codeaurora.org wrote: Hi Bjorn On 2021-05-10 21:20, Bjorn Andersson wrote: > In the search for causes to timing issues seen during implementation of > eDP support for SC8180x a fair amount of time was spent concluding why > the calculated mvid/nvid values where wrong. > > The overall conclusion is that the ratio of MVID/NVID describes, and > should match, the ratio between the pixel and link clock. > > Downstream this calculation reads the M and N values off the pixel clock > straight from DISP_CC and are then adjusted based on knowledge of how > the link and vco_div (parent of the pixel clock) are derrived from the > common VCO. > > While upstreaming, and then extracting the PHY driver, the resulting > function performs the following steps: > > 1) Adjust the passed link rate based on the VCO divider used in the PHY >driver, and multiply this by 10 based on the link rate divider. > 2) Pick reasonable choices of M and N, by calculating the ratio between >this new clock and the pixel clock. > 3) Subtract M from N and flip the bits, to match the encoding of the N >register in DISP_CC. > 4) Flip the bits of N and add M, to get the value of N back. > 5) Multiply M with 5, per the documentation. > 6) Scale the values such that N is close to 0x8000 (or larger) > 7) Multply M with 2 or 3 depending on the link rate of HBR2 or HBR3. > > Presumably step 3) was added to provide step 4) with expected input, so > the two cancel each other out. The factor of 10 from step 1) goes into > the denominator and is partially cancelled by the 5 in the numerator in > step 5), resulting in step 7) simply cancelling out step 1). > Both the multiplication of M with 5 and N with 2 or 3 is coming because of the ratio between the vco clk and the link clk. So we could have 2.7, 5.4 or 8.1 Gbps link clks and the factor of 2 or 3 gets added because hbr2 is 2 * hbr and hbr3 is 3 * hbr. Thanks for explaining the origin of these numbers, I had quite a difficult time figuring out where the "magic" came from. Your summary is pretty much right otherwise. Let me add some more points here: 1) Originally we removed reading the M_VID and N_VID from the DISPCC regs because of previous upstream comments that we can potentially just recalculate whatever the clk driver is programming by using rational_best_approximation https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/clk/qcom/clk-rcg2.c#L1160 Not having to read from DISPCC register is also useful because we dont have to maintain the register offset of the M_VID and N_VID which keeps changing across chipsets. Right, so downstream we do all the math and then we scale the denominator by 2x or 3x to compensate for the fact that we didn't account for the division as the clock left the PLL. As this was reworked upstream for some reason this compensation was retained, so the denominator would always be 2x or 3x to large for HBR2 and HBR3. So the way this was solved was to divide by 2x or 3x before calculating the ratio. However we discussed this again after viewing this patch. So the clk driver always operates on the vco clk and calculates the pixel clk from it and sets the M_VID and N_VID based on that. In terms of accuracy, the best way is still to re-use the M_VID and N_VID which the clk driver sets because the pixel clock was generated based on that and that is the actual pixel clock we are going to get. So even before this change we lost some accuracy because the pixel clock we are giving here to recalculate the M_VID and N_VID is a theoretical value. Although for most values of pixel clk, theoretical and actual should match. There could be corner cases of pixel clock where its a bit different. Hence ideally, re-using the M_VID and N_VID which the clk driver set would have been the best but not having to hard-code M_VID and N_VID offsets was a good enough reason to not go back to that again. Now, coming to this change. Here its trying to again re-calculate the M_VID and N_VID by using the same API which the clk driver uses but uses link clk and pixel clk as the parameters Vs the clk driver uses vco clk and actual pixel clock to calculate this. So even though this cleanup eliminates the adjustments we need to make to account for the VCO clk to link clk ratio, it also could bring additional difference between what was actually set by the clk driver and what we are calculating here because clk driver used vco clk as the input vs here we use link clk after this change. There might be some pixel clock rates of some resolutions where this difference could be risky. Hence the overall conclusion here was to keep using vco clk as the input to rational_best_approximation and not make more changes to this. So what you're saying is that the reason for this obfuscation is to replicate any rounding errors happening in the path of the link
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-06-08 15:34:01) > > On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote: > > > > > Quoting Bjorn Andersson (2021-06-08 15:26:23) > > > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote: > > > > > > > > > Quoting Bjorn Andersson (2021-06-07 16:31:47) > > > > > > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > > > > > > > Sorry about the confusion. What I meant is that even though DP > > > > > > > controller is > > > > > > > in the MDSS_GDSC > > > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks > > > > > > > have a direct > > > > > > > impact > > > > > > > on the CX voltage corners. Therefore, we need to mention the CX > > > > > > > power domain > > > > > > > here. And, since > > > > > > > we can associate only one OPP table with one device, we picked > > > > > > > the DP link > > > > > > > clock over other > > > > > > > clocks. > > > > > > > > > > > > Thank you, that's a much more useful answer. > > > > > > > > > > > > Naturally I would think it would make more sense for the PHY/PLL > > > > > > driver > > > > > > to ensure that CX is appropriately voted for then, but I think that > > > > > > would result in it being the clock driver performing such vote and > > > > > > I'm > > > > > > unsure how the opp table for that would look. > > > > > > > > > > > > @Stephen, what do you say? > > > > > > > > > > > > > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the > > > > > clk driver, and probably not from the clk ops, but instead come from > > > > > the > > > > > phy ops via phy_enable() and phy_configure(). > > > > > > > > > > > > > If I understand the logic correctly *_configure_dp_phy() will both > > > > configure the vco clock and "request" the clock framework to change the > > > > rate. > > > > > > > > So I presume what you're suggesting is that that would be the place to > > > > cast the CX corner vote? > > > > > > Yes that would be a place to make the CX vote. The problem is then I > > > don't know where to drop the vote. Is that when the phy is disabled? > > > > We do pass qcom_qmp_phy_power_off() and power down the DP part as DP > > output is being disabled. So that sounds like a reasonable place to drop > > the vote for the lowest performance state. > > > > So then will the corner vote be in place when the PHY isn't actually > powered up? That will be bad for power. The phy configure code will need > to know if the phy is enabled and then only put in the vote when the phy > is enabled, otherwise wait for enable to make the corner vote. > If we vote for a corner based on the link rate in *_configure_dp_phy() and put the vote for lowest corner we'd get the corner part sorted out afaict. We'd still have to make sure that the PHY doesn't hang on to the cx vote beyond that though - and implicitly in the non-DP cases... > Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for > digital logic. Probably the PLL is the hardware that has some minimum CX > requirement, and that flows down into the various display clks like the > link clk that actually clock the DP controller hardware. The mdss_gdsc > probably gates CX for the display subsystem (mdss) so if we had proper > corner aggregation logic we could indicate that mdss_gdsc is a child of > the CX domain and then make requests from the DP driver for particular > link frequencies on the mdss_gdsc and then have that bubble up to CX > appropriately. I don't think any of that sort of code is in place > though, right? I haven't checked sc7180, but I'm guessing that it's following the other modern platforms, where all the MDSS related pieces (including e.g. dispcc) lives in the MMCX domain, which is separate from CX. So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and the dp-opp-table) tells us that the PLL lives in the CX domain. PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs) seems to live in MMCX. Regards, Bjorn ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting Bjorn Andersson (2021-06-08 15:34:01) > On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-06-08 15:26:23) > > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote: > > > > > > > Quoting Bjorn Andersson (2021-06-07 16:31:47) > > > > > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > > > > > Sorry about the confusion. What I meant is that even though DP > > > > > > controller is > > > > > > in the MDSS_GDSC > > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have > > > > > > a direct > > > > > > impact > > > > > > on the CX voltage corners. Therefore, we need to mention the CX > > > > > > power domain > > > > > > here. And, since > > > > > > we can associate only one OPP table with one device, we picked the > > > > > > DP link > > > > > > clock over other > > > > > > clocks. > > > > > > > > > > Thank you, that's a much more useful answer. > > > > > > > > > > Naturally I would think it would make more sense for the PHY/PLL > > > > > driver > > > > > to ensure that CX is appropriately voted for then, but I think that > > > > > would result in it being the clock driver performing such vote and I'm > > > > > unsure how the opp table for that would look. > > > > > > > > > > @Stephen, what do you say? > > > > > > > > > > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the > > > > clk driver, and probably not from the clk ops, but instead come from the > > > > phy ops via phy_enable() and phy_configure(). > > > > > > > > > > If I understand the logic correctly *_configure_dp_phy() will both > > > configure the vco clock and "request" the clock framework to change the > > > rate. > > > > > > So I presume what you're suggesting is that that would be the place to > > > cast the CX corner vote? > > > > Yes that would be a place to make the CX vote. The problem is then I > > don't know where to drop the vote. Is that when the phy is disabled? > > We do pass qcom_qmp_phy_power_off() and power down the DP part as DP > output is being disabled. So that sounds like a reasonable place to drop > the vote for the lowest performance state. > So then will the corner vote be in place when the PHY isn't actually powered up? That will be bad for power. The phy configure code will need to know if the phy is enabled and then only put in the vote when the phy is enabled, otherwise wait for enable to make the corner vote. Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for digital logic. Probably the PLL is the hardware that has some minimum CX requirement, and that flows down into the various display clks like the link clk that actually clock the DP controller hardware. The mdss_gdsc probably gates CX for the display subsystem (mdss) so if we had proper corner aggregation logic we could indicate that mdss_gdsc is a child of the CX domain and then make requests from the DP driver for particular link frequencies on the mdss_gdsc and then have that bubble up to CX appropriately. I don't think any of that sort of code is in place though, right? ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-06-08 15:26:23) > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote: > > > > > Quoting Bjorn Andersson (2021-06-07 16:31:47) > > > > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > > > Sorry about the confusion. What I meant is that even though DP > > > > > controller is > > > > > in the MDSS_GDSC > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a > > > > > direct > > > > > impact > > > > > on the CX voltage corners. Therefore, we need to mention the CX power > > > > > domain > > > > > here. And, since > > > > > we can associate only one OPP table with one device, we picked the DP > > > > > link > > > > > clock over other > > > > > clocks. > > > > > > > > Thank you, that's a much more useful answer. > > > > > > > > Naturally I would think it would make more sense for the PHY/PLL driver > > > > to ensure that CX is appropriately voted for then, but I think that > > > > would result in it being the clock driver performing such vote and I'm > > > > unsure how the opp table for that would look. > > > > > > > > @Stephen, what do you say? > > > > > > > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the > > > clk driver, and probably not from the clk ops, but instead come from the > > > phy ops via phy_enable() and phy_configure(). > > > > > > > If I understand the logic correctly *_configure_dp_phy() will both > > configure the vco clock and "request" the clock framework to change the > > rate. > > > > So I presume what you're suggesting is that that would be the place to > > cast the CX corner vote? > > Yes that would be a place to make the CX vote. The problem is then I > don't know where to drop the vote. Is that when the phy is disabled? We do pass qcom_qmp_phy_power_off() and power down the DP part as DP output is being disabled. So that sounds like a reasonable place to drop the vote for the lowest performance state. Regards, Bjorn ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting Bjorn Andersson (2021-06-08 15:26:23) > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-06-07 16:31:47) > > > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > Sorry about the confusion. What I meant is that even though DP > > > > controller is > > > > in the MDSS_GDSC > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a > > > > direct > > > > impact > > > > on the CX voltage corners. Therefore, we need to mention the CX power > > > > domain > > > > here. And, since > > > > we can associate only one OPP table with one device, we picked the DP > > > > link > > > > clock over other > > > > clocks. > > > > > > Thank you, that's a much more useful answer. > > > > > > Naturally I would think it would make more sense for the PHY/PLL driver > > > to ensure that CX is appropriately voted for then, but I think that > > > would result in it being the clock driver performing such vote and I'm > > > unsure how the opp table for that would look. > > > > > > @Stephen, what do you say? > > > > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the > > clk driver, and probably not from the clk ops, but instead come from the > > phy ops via phy_enable() and phy_configure(). > > > > If I understand the logic correctly *_configure_dp_phy() will both > configure the vco clock and "request" the clock framework to change the > rate. > > So I presume what you're suggesting is that that would be the place to > cast the CX corner vote? Yes that would be a place to make the CX vote. The problem is then I don't know where to drop the vote. Is that when the phy is disabled? ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-06-07 16:31:47) > > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > > > On 2021-06-05 22:07, Bjorn Andersson wrote: > > > > On Thu 03 Jun 16:56 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > > > On 2021-06-03 09:53, Bjorn Andersson wrote: > > > > > > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote: > > > > [..] > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > > > > [..] > > > > > > > + power-domains = < SC7180_CX>; > > > > > > > > > > > > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we > > > > > > need to > > > > > > mention CX here in order for the opp framework to apply > > > > > > required-opps > > > > > > of CX? > > > > > > > > > > yes, > > > > > > > > If you want me, or other maintainers, to spend any time reviewing or > > > > applying your patches going forward then you need to actually bother > > > > replying properly to the questions asked. > > > > > > > > Thanks, > > > > Bjorn > > > > > > Sorry about the confusion. What I meant is that even though DP controller > > > is > > > in the MDSS_GDSC > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a > > > direct > > > impact > > > on the CX voltage corners. Therefore, we need to mention the CX power > > > domain > > > here. And, since > > > we can associate only one OPP table with one device, we picked the DP link > > > clock over other > > > clocks. > > > > Thank you, that's a much more useful answer. > > > > Naturally I would think it would make more sense for the PHY/PLL driver > > to ensure that CX is appropriately voted for then, but I think that > > would result in it being the clock driver performing such vote and I'm > > unsure how the opp table for that would look. > > > > @Stephen, what do you say? > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the > clk driver, and probably not from the clk ops, but instead come from the > phy ops via phy_enable() and phy_configure(). > If I understand the logic correctly *_configure_dp_phy() will both configure the vco clock and "request" the clock framework to change the rate. So I presume what you're suggesting is that that would be the place to cast the CX corner vote? > By the way, there's nothing wrong with a clk device doing power domain > "stuff", except for that we haven't plumbed it into the clk framework > properly and I'm fairly certain our usage of runtime PM in the clk > framework today underneath the prepare_lock is getting us into trouble > or will get us there soon. On the bright side, it's wonderful that we're at a point where this is not only a theoretical problem :) Regards, Bjorn ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Quoting Bjorn Andersson (2021-06-07 16:31:47) > On Mon 07 Jun 12:48 CDT 2021, khs...@codeaurora.org wrote: > > > On 2021-06-05 22:07, Bjorn Andersson wrote: > > > On Thu 03 Jun 16:56 CDT 2021, khs...@codeaurora.org wrote: > > > > > > > On 2021-06-03 09:53, Bjorn Andersson wrote: > > > > > On Thu 03 Jun 11:09 CDT 2021, Kuogee Hsieh wrote: > > > [..] > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > > > [..] > > > > > > + power-domains = < SC7180_CX>; > > > > > > > > > > Just curious, but isn't the DP block in the MDSS_GDCS? Or do we need > > > > > to > > > > > mention CX here in order for the opp framework to apply required-opps > > > > > of CX? > > > > > > > > yes, > > > > > > If you want me, or other maintainers, to spend any time reviewing or > > > applying your patches going forward then you need to actually bother > > > replying properly to the questions asked. > > > > > > Thanks, > > > Bjorn > > > > Sorry about the confusion. What I meant is that even though DP controller is > > in the MDSS_GDSC > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct > > impact > > on the CX voltage corners. Therefore, we need to mention the CX power domain > > here. And, since > > we can associate only one OPP table with one device, we picked the DP link > > clock over other > > clocks. > > Thank you, that's a much more useful answer. > > Naturally I would think it would make more sense for the PHY/PLL driver > to ensure that CX is appropriately voted for then, but I think that > would result in it being the clock driver performing such vote and I'm > unsure how the opp table for that would look. > > @Stephen, what do you say? > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the clk driver, and probably not from the clk ops, but instead come from the phy ops via phy_enable() and phy_configure(). By the way, there's nothing wrong with a clk device doing power domain "stuff", except for that we haven't plumbed it into the clk framework properly and I'm fairly certain our usage of runtime PM in the clk framework today underneath the prepare_lock is getting us into trouble or will get us there soon. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dsi: Stash away calculated vco frequency on recalc
Quoting Dmitry Baryshkov (2021-06-08 14:41:21)
> Hi Stephen,
>
> On 08/06/2021 22:55, Stephen Boyd wrote:
> > A problem was reported on CoachZ devices where the display wouldn't come
> > up, or it would be distorted. It turns out that the PLL code here wasn't
> > getting called once dsi_pll_10nm_vco_recalc_rate() started returning the
> > same exact frequency, down to the Hz, that the bootloader was setting
> > instead of 0 when the clk was registered with the clk framework.
> >
> > After commit 001d8dc33875 ("drm/msm/dsi: remove temp data from global
> > pll structure") we use a hardcoded value for the parent clk frequency,
> > i.e. VCO_REF_CLK_RATE, and we also hardcode the value for FRAC_BITS,
> > instead of getting it from the config structure. This combination of
> > changes to the recalc function allows us to properly calculate the
> > frequency of the PLL regardless of whether or not the PLL has been
> > clk_prepare()d or clk_set_rate()d. That's a good improvement.
> >
> > Unfortunately, this means that now we won't call down into the PLL clk
> > driver when we call clk_set_rate() because the frequency calculated in
> > the framework matches the frequency that is set in hardware. If the rate
> > is the same as what we want it should be OK to not call the set_rate PLL
> > op. The real problem is that the prepare op in this driver uses a
> > private struct member to stash away the vco frequency so that it can
> > call the set_rate op directly during prepare. Once the set_rate op is
> > never called because recalc_rate told us the rate is the same, we don't
> > set this private struct member before the prepare op runs, so we try to
> > call the set_rate function directly with a frequency of 0. This
> > effectively kills the PLL and configures it for a rate that won't work.
> > Calling set_rate from prepare is really quite bad and will confuse any
> > downstream clks about what the rate actually is of their parent. Fixing
> > that will be a rather large change though so we leave that to later.
> >
> > For now, let's stash away the rate we calculate during recalc so that
> > the prepare op knows what frequency to set, instead of 0. This way
> > things keep working and the display can enable the PLL properly. In the
> > future, we should remove that code from the prepare op so that it
> > doesn't even try to call the set rate function.
> >
> > Cc: Dmitry Baryshkov
> > Cc: Abhinav Kumar
> > Fixes: 001d8dc33875 ("drm/msm/dsi: remove temp data from global pll
> > structure")
> > Signed-off-by: Stephen Boyd
>
> Thank you for the lengthy explanation. May I suggest another solution:
> - Apply
> https://lore.kernel.org/linux-arm-msm/010101750064e17e-3db0087e-fc37-494d-aac9-2c2b9b0a7c5b-000...@us-west-2.amazonses.com/
>
> - And make save_state for 7nm and 10nm cache vco freq (like 14nm does).
>
> What do you think?
>
Maybe that can be done for the next merge window? I'd like to get the
smallest possible patch in as a fix for this cycle given that the Fixes
tag is a recent regression introduced during the most recent merge
window.
I honestly have no idea what's going on with the clk driver in these
files but from the clk framework perspective there are bigger problems
than caching the vco freq properly. As I stated in the commit text
above, calling set_rate from prepare is plain bad. That should stop.
>From my quick glance, the patch you mention looks like another
workaround instead of a proper fix. Why would we need to save the
registers at boot and then snap them back into place on enable? Maybe we
shouldn't reset the phy after registering the clks? Instead register the
clks after the phy is reset so recalc_rate can accurately calculate the
frequency. I suppose that would break continuous splash screen though
where you want the PLL to stay running the entire boot? But then
issuing a reset would break that, wouldn't it? As you can see I'm pretty
confused about how this is all supposed to work.
Note: my problem isn't about recovering what boot sets, it's mostly
exposing incorrect usage of the clk framework in this driver because it
relies on this chain of events:
1) recalc rate calculates something different than what is
set via clk_set_rate()
2) clk_set_rate() is called with the different rate
3) clk_prepare() is called to actually enable the PLL and wait for it
to start
If clk_prepare() was called before clk_set_rate(), which is totally
valid, then it should similarly fail and think the rate is 0 and the PLL
won't lock. Does implementing save_state fix that? If so, it seems like
we have two pieces of code working around each other, maybe for
suspend/resume purposes.
I admit this patch I'm proposing is another workaround, but at least it
makes things work again without going off and adding a bunch of register
save/restore logic.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
Re: [Freedreno] [PATCH] drm/msm/dsi: Stash away calculated vco frequency on recalc
Hi Stephen,
On 08/06/2021 22:55, Stephen Boyd wrote:
A problem was reported on CoachZ devices where the display wouldn't come
up, or it would be distorted. It turns out that the PLL code here wasn't
getting called once dsi_pll_10nm_vco_recalc_rate() started returning the
same exact frequency, down to the Hz, that the bootloader was setting
instead of 0 when the clk was registered with the clk framework.
After commit 001d8dc33875 ("drm/msm/dsi: remove temp data from global
pll structure") we use a hardcoded value for the parent clk frequency,
i.e. VCO_REF_CLK_RATE, and we also hardcode the value for FRAC_BITS,
instead of getting it from the config structure. This combination of
changes to the recalc function allows us to properly calculate the
frequency of the PLL regardless of whether or not the PLL has been
clk_prepare()d or clk_set_rate()d. That's a good improvement.
Unfortunately, this means that now we won't call down into the PLL clk
driver when we call clk_set_rate() because the frequency calculated in
the framework matches the frequency that is set in hardware. If the rate
is the same as what we want it should be OK to not call the set_rate PLL
op. The real problem is that the prepare op in this driver uses a
private struct member to stash away the vco frequency so that it can
call the set_rate op directly during prepare. Once the set_rate op is
never called because recalc_rate told us the rate is the same, we don't
set this private struct member before the prepare op runs, so we try to
call the set_rate function directly with a frequency of 0. This
effectively kills the PLL and configures it for a rate that won't work.
Calling set_rate from prepare is really quite bad and will confuse any
downstream clks about what the rate actually is of their parent. Fixing
that will be a rather large change though so we leave that to later.
For now, let's stash away the rate we calculate during recalc so that
the prepare op knows what frequency to set, instead of 0. This way
things keep working and the display can enable the PLL properly. In the
future, we should remove that code from the prepare op so that it
doesn't even try to call the set rate function.
Cc: Dmitry Baryshkov
Cc: Abhinav Kumar
Fixes: 001d8dc33875 ("drm/msm/dsi: remove temp data from global pll structure")
Signed-off-by: Stephen Boyd
Thank you for the lengthy explanation. May I suggest another solution:
- Apply
https://lore.kernel.org/linux-arm-msm/010101750064e17e-3db0087e-fc37-494d-aac9-2c2b9b0a7c5b-000...@us-west-2.amazonses.com/
- And make save_state for 7nm and 10nm cache vco freq (like 14nm does).
What do you think?
---
I didn't update the 14nm file as the caching logic looks different. But
between the 7nm and 10nm files it looks practically the same.
drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 1 +
drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 34bc93548fcf..657778889d35 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -432,6 +432,7 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct
clk_hw *hw,
pll_freq += div_u64(tmp64, multiplier);
vco_rate = pll_freq;
+ pll_10nm->vco_current_rate = vco_rate;
DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
pll_10nm->phy->id, (unsigned long)vco_rate, dec, frac);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index e76ce40a12ab..6f96fbac8282 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -460,6 +460,7 @@ static unsigned long dsi_pll_7nm_vco_recalc_rate(struct
clk_hw *hw,
pll_freq += div_u64(tmp64, multiplier);
vco_rate = pll_freq;
+ pll_7nm->vco_current_rate = vco_rate;
DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
pll_7nm->phy->id, (unsigned long)vco_rate, dec, frac);
base-commit: 8124c8a6b35386f73523d27eacb71b5364a68c4c
--
With best wishes
Dmitry
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v4 1/4] drm/panel-simple: Add basic DPCD backlight support
Lyude, On Tue, Jun 1, 2021 at 3:20 PM Lyude Paul wrote: > > oh-looks like my patches just got reviewed, so hopefully I should get a chance > to get a look at this in the next day or two :) I'm going to assume that means that you don't need extra eyes on your backlight patches. If you do, please shout and I'll try to find some cycles for it. I've always got more things to do than there are hours in the day, but many folks from the DRM community have helped me out with numerous reviews over the last year or two and I'm happy to pay some of that back by giving reviews if it'll help move things forward. :-) -Doug ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 2/3] dt-bindings: msm: dsi: document phy-type property for 7nm dsi phy
Hi Jonathan,
Thank you for the patch.
On Tue, Jun 08, 2021 at 03:53:28PM -0400, Jonathan Marek wrote:
> Document a new phy-type property which will be used to determine whether
> the phy should operate in D-PHY or C-PHY mode.
>
> Signed-off-by: Jonathan Marek
> ---
> .../devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 4
> include/dt-bindings/phy/phy.h | 2 ++
> 2 files changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
> b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
> index bf16b1c65e10..d447b517ea19 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
> @@ -34,6 +34,10 @@ properties:
> description: |
>Connected to VDD_A_DSI_PLL_0P9 pin (or VDDA_DSI{0,1}_PLL_0P9 for
> sm8150)
>
> + phy-type:
> +description: |
> + D-PHY (default) or C-PHY mode: PHY_TYPE_DSI_DPHY or PHY_TYPE_DSI_CPHY
> +
> required:
>- compatible
>- reg
> diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
> index 887a31b250a8..b978dac16bb8 100644
> --- a/include/dt-bindings/phy/phy.h
> +++ b/include/dt-bindings/phy/phy.h
> @@ -20,5 +20,7 @@
> #define PHY_TYPE_XPCS7
> #define PHY_TYPE_SGMII 8
> #define PHY_TYPE_QSGMII 9
> +#define PHY_TYPE_DSI_DPHY10
> +#define PHY_TYPE_DSI_CPHY11
The MIPI D-PHY and C-PHY apply to both DSI and CSI. I'd name this
PHY_TYPE_DPHY and PHY_TYPE_CPHY, or possibly, PHY_TYPE_MIPI_DPHY and
PHY_TYPE_MIPI_CPHY, without a DSI prefix. With this addressed and the DT
bindings updated accordingly,
Reviewed-by: Laurent Pinchart
(You may be asked to split this in two patches though, not sure)
> #endif /* _DT_BINDINGS_PHY */
--
Regards,
Laurent Pinchart
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/4] drm/msm/dp: Simplify the mvid/nvid calculation
On Fri 28 May 18:11 CDT 2021, abhin...@codeaurora.org wrote: > Hi Bjorn > > On 2021-05-10 21:20, Bjorn Andersson wrote: > > In the search for causes to timing issues seen during implementation of > > eDP support for SC8180x a fair amount of time was spent concluding why > > the calculated mvid/nvid values where wrong. > > > > The overall conclusion is that the ratio of MVID/NVID describes, and > > should match, the ratio between the pixel and link clock. > > > > Downstream this calculation reads the M and N values off the pixel clock > > straight from DISP_CC and are then adjusted based on knowledge of how > > the link and vco_div (parent of the pixel clock) are derrived from the > > common VCO. > > > > While upstreaming, and then extracting the PHY driver, the resulting > > function performs the following steps: > > > > 1) Adjust the passed link rate based on the VCO divider used in the PHY > >driver, and multiply this by 10 based on the link rate divider. > > 2) Pick reasonable choices of M and N, by calculating the ratio between > >this new clock and the pixel clock. > > 3) Subtract M from N and flip the bits, to match the encoding of the N > >register in DISP_CC. > > 4) Flip the bits of N and add M, to get the value of N back. > > 5) Multiply M with 5, per the documentation. > > 6) Scale the values such that N is close to 0x8000 (or larger) > > 7) Multply M with 2 or 3 depending on the link rate of HBR2 or HBR3. > > > > Presumably step 3) was added to provide step 4) with expected input, so > > the two cancel each other out. The factor of 10 from step 1) goes into > > the denominator and is partially cancelled by the 5 in the numerator in > > step 5), resulting in step 7) simply cancelling out step 1). > > > > Both the multiplication of M with 5 and N with 2 or 3 is coming because of > the > ratio between the vco clk and the link clk. > So we could have 2.7, 5.4 or 8.1 Gbps link clks and the factor of 2 or 3 > gets added because hbr2 is 2 * hbr and hbr3 is 3 * hbr. > Thanks for explaining the origin of these numbers, I had quite a difficult time figuring out where the "magic" came from. > Your summary is pretty much right otherwise. Let me add some more points > here: > > 1) Originally we removed reading the M_VID and N_VID from the DISPCC regs > because > of previous upstream comments that we can potentially just recalculate > whatever the clk driver is programming > by using rational_best_approximation > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/clk/qcom/clk-rcg2.c#L1160 > > Not having to read from DISPCC register is also useful because we dont have > to maintain the register offset > of the M_VID and N_VID which keeps changing across chipsets. > Right, so downstream we do all the math and then we scale the denominator by 2x or 3x to compensate for the fact that we didn't account for the division as the clock left the PLL. As this was reworked upstream for some reason this compensation was retained, so the denominator would always be 2x or 3x to large for HBR2 and HBR3. So the way this was solved was to divide by 2x or 3x before calculating the ratio. > However we discussed this again after viewing this patch. So the clk driver > always operates on the vco clk > and calculates the pixel clk from it and sets the M_VID and N_VID based on > that. > In terms of accuracy, the best way is still to re-use the M_VID and N_VID > which the clk driver sets because > the pixel clock was generated based on that and that is the actual pixel > clock we are going to get. > > So even before this change we lost some accuracy because the pixel clock we > are giving here to recalculate > the M_VID and N_VID is a theoretical value. Although for most values of > pixel clk, theoretical and actual > should match. There could be corner cases of pixel clock where its a bit > different. Hence ideally, re-using the M_VID > and N_VID which the clk driver set would have been the best but not having > to hard-code M_VID and N_VID offsets > was a good enough reason to not go back to that again. > > Now, coming to this change. Here its trying to again re-calculate the M_VID > and N_VID by using the same > API which the clk driver uses but uses link clk and pixel clk as the > parameters Vs the clk driver uses > vco clk and actual pixel clock to calculate this. > > So even though this cleanup eliminates the adjustments we need to make to > account for the VCO clk to link clk ratio, > it also could bring additional difference between what was actually set by > the clk driver and what we are calculating > here because clk driver used vco clk as the input vs here we use link clk > after this change. > There might be some pixel clock rates of some resolutions where this > difference could be risky. > > Hence the overall conclusion here was to keep using vco clk as the input to > rational_best_approximation > and not make more changes to this. > So what you're saying is that the
[Freedreno] [PATCH v3 3/3] drm/msm/dsi: support CPHY mode for 7nm pll/phy
Add the required changes to support 7nm pll/phy in CPHY mode.
This adds a "qcom,dsi-phy-cphy-mode" property for the PHY node to enable
the CPHY mode.
Signed-off-by: Jonathan Marek
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi.xml.h | 2 +
drivers/gpu/drm/msm/dsi/dsi_host.c| 34 -
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 49
drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 3 +
drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 145 +++---
5 files changed, 186 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index b8e9e608abfc..a59a9bd3f5d1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -621,6 +621,8 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
return ((val) << DSI_VERSION_MAJOR__SHIFT) & DSI_VERSION_MAJOR__MASK;
}
+#define REG_DSI_CPHY_MODE_CTRL 0x02d4
+
#define REG_DSI_PHY_PLL_CTRL_0 0x0200
#define DSI_PHY_PLL_CTRL_0_ENABLE 0x0001
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 809997f870f6..262d6d3b9c4b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -27,6 +27,7 @@
#include "dsi_cfg.h"
#include "msm_kms.h"
#include "msm_gem.h"
+#include "phy/dsi_phy.h"
#define DSI_RESET_TOGGLE_DELAY_MS 20
@@ -170,6 +171,9 @@ struct msm_dsi_host {
int dlane_swap;
int num_data_lanes;
+ /* from phy DT */
+ bool cphy_mode;
+
u32 dma_cmd_ctrl_restore;
bool registered;
@@ -513,6 +517,7 @@ int msm_dsi_runtime_resume(struct device *dev)
int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
{
+ u32 byte_intf_rate;
int ret;
DBG("Set clk rates: pclk=%d, byteclk=%d",
@@ -532,8 +537,13 @@ int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
}
if (msm_host->byte_intf_clk) {
- ret = clk_set_rate(msm_host->byte_intf_clk,
- msm_host->byte_clk_rate / 2);
+ /* For CPHY, byte_intf_clk is same as byte_clk */
+ if (msm_host->cphy_mode)
+ byte_intf_rate = msm_host->byte_clk_rate;
+ else
+ byte_intf_rate = msm_host->byte_clk_rate / 2;
+
+ ret = clk_set_rate(msm_host->byte_intf_clk, byte_intf_rate);
if (ret) {
pr_err("%s: Failed to set rate byte intf clk, %d\n",
__func__, ret);
@@ -721,7 +731,11 @@ static void dsi_calc_pclk(struct msm_dsi_host *msm_host,
bool is_dual_dsi)
lanes = 1;
}
- do_div(pclk_bpp, (8 * lanes));
+ /* CPHY "byte_clk" is in units of 16 bits */
+ if (msm_host->cphy_mode)
+ do_div(pclk_bpp, (16 * lanes));
+ else
+ do_div(pclk_bpp, (8 * lanes));
msm_host->pixel_clk_rate = pclk_rate;
msm_host->byte_clk_rate = pclk_bpp;
@@ -947,6 +961,9 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host,
bool enable,
data |= DSI_CTRL_ENABLE;
dsi_write(msm_host, REG_DSI_CTRL, data);
+
+ if (msm_host->cphy_mode)
+ dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
}
static void dsi_set_video_dsc(struct msm_dsi_host *msm_host,
@@ -2278,6 +2295,8 @@ int msm_dsi_host_set_src_pll(struct mipi_dsi_host *host,
struct clk *byte_clk_provider, *pixel_clk_provider;
int ret;
+ msm_host->cphy_mode = src_phy->cphy_mode;
+
ret = msm_dsi_phy_get_clk_provider(src_phy,
_clk_provider, _clk_provider);
if (ret) {
@@ -2349,7 +2368,14 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host
*host,
return;
}
- clk_req->bitclk_rate = msm_host->byte_clk_rate * 8;
+ /* CPHY transmits 16 bits over 7 clock cycles
+* "byte_clk" is in units of 16-bits (see dsi_calc_pclk),
+* so multiply by 7 to get the "bitclk rate"
+*/
+ if (msm_host->cphy_mode)
+ clk_req->bitclk_rate = msm_host->byte_clk_rate * 7;
+ else
+ clk_req->bitclk_rate = msm_host->byte_clk_rate * 8;
clk_req->escclk_rate = msm_host->esc_clk_rate;
}
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 6ca6bfd4809b..145331480e74 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include "dsi_phy.h"
@@ -461,6 +462,51 @@ int msm_dsi_dphy_timing_calc_v4(struct msm_dsi_dphy_timing
*timing,
return 0;
}
+int msm_dsi_cphy_timing_calc_v4(struct msm_dsi_dphy_timing *timing,
+ struct msm_dsi_phy_clk_request *clk_req)
+{
+
[Freedreno] [PATCH v3 2/3] dt-bindings: msm: dsi: document phy-type property for 7nm dsi phy
Document a new phy-type property which will be used to determine whether
the phy should operate in D-PHY or C-PHY mode.
Signed-off-by: Jonathan Marek
---
.../devicetree/bindings/display/msm/dsi-phy-7nm.yaml | 4
include/dt-bindings/phy/phy.h | 2 ++
2 files changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
index bf16b1c65e10..d447b517ea19 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
@@ -34,6 +34,10 @@ properties:
description: |
Connected to VDD_A_DSI_PLL_0P9 pin (or VDDA_DSI{0,1}_PLL_0P9 for sm8150)
+ phy-type:
+description: |
+ D-PHY (default) or C-PHY mode: PHY_TYPE_DSI_DPHY or PHY_TYPE_DSI_CPHY
+
required:
- compatible
- reg
diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
index 887a31b250a8..b978dac16bb8 100644
--- a/include/dt-bindings/phy/phy.h
+++ b/include/dt-bindings/phy/phy.h
@@ -20,5 +20,7 @@
#define PHY_TYPE_XPCS 7
#define PHY_TYPE_SGMII 8
#define PHY_TYPE_QSGMII9
+#define PHY_TYPE_DSI_DPHY 10
+#define PHY_TYPE_DSI_CPHY 11
#endif /* _DT_BINDINGS_PHY */
--
2.26.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 1/3] dt-bindings: msm: dsi: add missing 7nm bindings
These got lost when going from .txt to .yaml bindings, add them back.
Signed-off-by: Jonathan Marek
---
.../bindings/display/msm/dsi-phy-7nm.yaml | 66 +++
1 file changed, 66 insertions(+)
create mode 100644
Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
new file mode 100644
index ..bf16b1c65e10
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dsi-phy-7nm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display DSI 7nm PHY
+
+maintainers:
+ - Jonathan Marek
+
+allOf:
+ - $ref: dsi-phy-common.yaml#
+
+properties:
+ compatible:
+oneOf:
+ - const: qcom,dsi-phy-7nm
+ - const: qcom,dsi-phy-7nm-8150
+
+ reg:
+items:
+ - description: dsi phy register set
+ - description: dsi phy lane register set
+ - description: dsi pll register set
+
+ reg-names:
+items:
+ - const: dsi_phy
+ - const: dsi_phy_lane
+ - const: dsi_pll
+
+ vdds-supply:
+description: |
+ Connected to VDD_A_DSI_PLL_0P9 pin (or VDDA_DSI{0,1}_PLL_0P9 for sm8150)
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - vdds-supply
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include
+ #include
+
+ dsi-phy@ae94400 {
+ compatible = "qcom,dsi-phy-7nm";
+ reg = <0x0ae94400 0x200>,
+ <0x0ae94600 0x280>,
+ <0x0ae94900 0x260>;;
+ reg-names = "dsi_phy",
+ "dsi_phy_lane",
+ "dsi_pll";
+
+ #clock-cells = <1>;
+ #phy-cells = <0>;
+
+ vdds-supply = <_l5a_0p88>;
+ clocks = < DISP_CC_MDSS_AHB_CLK>,
+ < RPMH_CXO_CLK>;
+ clock-names = "iface", "ref";
+ };
--
2.26.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 0/3] drm/msm/dsi: support CPHY mode for 7nm pll/phy
Add the required changes to support 7nm pll/phy in CPHY mode. This adds a "qcom,dsi-phy-cphy-mode" property for the PHY node to enable the CPHY mode. v2: - rebased on DSI PHY reworks - reworked getting cphy_mode in dsi_host.c - documentation change in separate patch v3: - yaml bindings - changed binding to "phy-mode = ;" Jonathan Marek (3): dt-bindings: msm: dsi: add missing 7nm bindings dt-bindings: msm: dsi: document phy-type property for 7nm dsi phy drm/msm/dsi: support CPHY mode for 7nm pll/phy .../bindings/display/msm/dsi-phy-7nm.yaml | 70 + drivers/gpu/drm/msm/dsi/dsi.xml.h | 2 + drivers/gpu/drm/msm/dsi/dsi_host.c| 34 +++- drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 49 ++ drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 3 + drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 145 -- include/dt-bindings/phy/phy.h | 2 + 7 files changed, 258 insertions(+), 47 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/msm/dsi-phy-7nm.yaml -- 2.26.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm/dsi: Stash away calculated vco frequency on recalc
A problem was reported on CoachZ devices where the display wouldn't come
up, or it would be distorted. It turns out that the PLL code here wasn't
getting called once dsi_pll_10nm_vco_recalc_rate() started returning the
same exact frequency, down to the Hz, that the bootloader was setting
instead of 0 when the clk was registered with the clk framework.
After commit 001d8dc33875 ("drm/msm/dsi: remove temp data from global
pll structure") we use a hardcoded value for the parent clk frequency,
i.e. VCO_REF_CLK_RATE, and we also hardcode the value for FRAC_BITS,
instead of getting it from the config structure. This combination of
changes to the recalc function allows us to properly calculate the
frequency of the PLL regardless of whether or not the PLL has been
clk_prepare()d or clk_set_rate()d. That's a good improvement.
Unfortunately, this means that now we won't call down into the PLL clk
driver when we call clk_set_rate() because the frequency calculated in
the framework matches the frequency that is set in hardware. If the rate
is the same as what we want it should be OK to not call the set_rate PLL
op. The real problem is that the prepare op in this driver uses a
private struct member to stash away the vco frequency so that it can
call the set_rate op directly during prepare. Once the set_rate op is
never called because recalc_rate told us the rate is the same, we don't
set this private struct member before the prepare op runs, so we try to
call the set_rate function directly with a frequency of 0. This
effectively kills the PLL and configures it for a rate that won't work.
Calling set_rate from prepare is really quite bad and will confuse any
downstream clks about what the rate actually is of their parent. Fixing
that will be a rather large change though so we leave that to later.
For now, let's stash away the rate we calculate during recalc so that
the prepare op knows what frequency to set, instead of 0. This way
things keep working and the display can enable the PLL properly. In the
future, we should remove that code from the prepare op so that it
doesn't even try to call the set rate function.
Cc: Dmitry Baryshkov
Cc: Abhinav Kumar
Fixes: 001d8dc33875 ("drm/msm/dsi: remove temp data from global pll structure")
Signed-off-by: Stephen Boyd
---
I didn't update the 14nm file as the caching logic looks different. But
between the 7nm and 10nm files it looks practically the same.
drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 1 +
drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 34bc93548fcf..657778889d35 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -432,6 +432,7 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct
clk_hw *hw,
pll_freq += div_u64(tmp64, multiplier);
vco_rate = pll_freq;
+ pll_10nm->vco_current_rate = vco_rate;
DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
pll_10nm->phy->id, (unsigned long)vco_rate, dec, frac);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index e76ce40a12ab..6f96fbac8282 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -460,6 +460,7 @@ static unsigned long dsi_pll_7nm_vco_recalc_rate(struct
clk_hw *hw,
pll_freq += div_u64(tmp64, multiplier);
vco_rate = pll_freq;
+ pll_7nm->vco_current_rate = vco_rate;
DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
pll_7nm->phy->id, (unsigned long)vco_rate, dec, frac);
base-commit: 8124c8a6b35386f73523d27eacb71b5364a68c4c
--
https://chromeos.dev
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [pull v2] drm/msm: msm-fixes-v5.13-rc6 for v5.13-rc6
Hi Dave & Daniel, A few late fixes for v5.13 The following changes since commit f2f46b878777e0d3f885c7ddad48f477b4dea247: drm/msm/dp: initialize audio_comp when audio starts (2021-05-06 16:26:57 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git msm-fixes-v5.13-rc6 for you to fetch changes up to ce86c239e4d218ae6040bec18e6d19a58edb8b7c: drm/msm/a6xx: avoid shadow NULL reference in failure path (2021-06-08 11:26:45 -0700) Alexey Minnekhanov (1): drm/msm: Init mm_list before accessing it for use_vram path Jonathan Marek (3): drm/msm/a6xx: update/fix CP_PROTECT initialization drm/msm/a6xx: fix incorrectly set uavflagprd_inv field for A650 drm/msm/a6xx: avoid shadow NULL reference in failure path drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 155 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 +- drivers/gpu/drm/msm/msm_gem.c | 7 ++ 3 files changed, 122 insertions(+), 42 deletions(-) ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [pull] drm/msm: drm-msm-fixes-2021-06-08 for v5.13-rc6
sorry, spotted a small typo, I'll resend this shortly On Tue, Jun 8, 2021 at 10:32 AM Rob Clark wrote: > > Hi Dave & Daniel, > > A few late fixes for v5.13 > > The following changes since commit f2f46b878777e0d3f885c7ddad48f477b4dea247: > > drm/msm/dp: initialize audio_comp when audio starts (2021-05-06 > 16:26:57 -0700) > > are available in the Git repository at: > > https://gitlab.freedesktop.org/drm/msm.git drm-msm-fixes-2021-06-08 > > for you to fetch changes up to 924f4cba9e68bf2b4717e7941697db96c6f203be: > > drm/msm/a6xx: avoid shadow NULL reference in failure path > (2021-06-08 10:08:05 -0700) > > > Alexey Minnekhanov (1): > drm/msm: Init mm_list before accessing it for use_vram path > > Jonathan Marek (3): > drm/msm/a6xx: update/fix CP_PROTECT initialization > drm/msm/a6xx: fix incorrectly set uavflagprd_inv field for A650 > drm/msm/a6xx: avoid shadow NULL reference in failure path > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 155 > +- > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 +- > drivers/gpu/drm/msm/msm_gem.c | 7 ++ > 3 files changed, 122 insertions(+), 42 deletions(-) ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 3/5] drm/msm/a6xx: add GMU_CX_GMU_CX_FALNEXT_INTF write for a650
On 6/8/21 1:27 PM, Jonathan Marek wrote:
downstream msm-5.14 kernel added a write to this register, so match that.
Note: this should say msm-5.4 (msm-5.14 is not a thing)
Signed-off-by: Jonathan Marek
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 4 +++-
drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index c1ee02d6371d..0f3390eab55e 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -751,8 +751,10 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu,
unsigned int state)
int ret;
u32 chipid;
- if (adreno_is_a650(adreno_gpu))
+ if (adreno_is_a650(adreno_gpu)) {
+ gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FALNEXT_INTF, 1);
gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FAL_INTF, 1);
+ }
if (state == GMU_WARM_BOOT) {
ret = a6xx_rpmh_start(gmu);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
index 5a43d3090b0c..eeef3d6d89b8 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
@@ -292,6 +292,8 @@ static inline uint32_t A6XX_GMU_GPU_NAP_CTRL_SID(uint32_t
val)
#define REG_A6XX_GPU_GMU_CX_GMU_CX_FAL_INTF 0x50f0
+#define REF_A6XX_GPU_GMU_CX_GMU_CX_FALNEXT_INTF 0x50f1
+
#define REG_A6XX_GPU_GMU_CX_GMU_PWR_COL_CP_MSG
0x5100
#define REG_A6XX_GPU_GMU_CX_GMU_PWR_COL_CP_RESP 0x5101
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 5/5] drm/msm/a6xx: add support for Adreno 660 GPU
Add adreno_is_{a660,a650_family} helpers and convert update existing
adreno_is_a650 usage based on downstream driver's logic (changing into
adreno_is_a650_family or adding adreno_is_a660).
And add the remaining changes required for A660, again based on
the downstream driver: missing GMU allocations, additional register init,
dummy hfi BW table, cp protect list, entry in gpulist table, hwcg table,
updated a6xx_ucode_check_version check.
Signed-off-by: Jonathan Marek
---
drivers/gpu/drm/msm/adreno/a6xx.xml.h | 4 +
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 32 +++-
drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h | 2 +-
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 162 +
drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 33 +
drivers/gpu/drm/msm/adreno/adreno_device.c | 13 ++
drivers/gpu/drm/msm/adreno/adreno_gpu.c| 2 +-
drivers/gpu/drm/msm/adreno/adreno_gpu.h| 13 +-
8 files changed, 224 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx.xml.h
b/drivers/gpu/drm/msm/adreno/a6xx.xml.h
index 920c5e6b8e96..631c36672560 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx.xml.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx.xml.h
@@ -2240,6 +2240,8 @@ static inline uint32_t
A6XX_DBGC_CFG_DBGBUS_BYTEL_1_BYTEL15(uint32_t val)
#define REG_A6XX_DBGC_CFG_DBGBUS_TRACE_BUF20x0630
+#define REG_A6XX_CP_LPAC_PROG_FIFO_SIZE0x0b34
+
#define REG_A6XX_VSC_PERFCTR_VSC_SEL_0 0x0cd8
#define REG_A6XX_VSC_PERFCTR_VSC_SEL_1 0x0cd9
@@ -2340,6 +2342,8 @@ static inline uint32_t
A6XX_UCHE_CLIENT_PF_PERFSEL(uint32_t val)
#define REG_A6XX_UCHE_PERFCTR_UCHE_SEL_11 0x0e27
+#define REG_A6XX_UCHE_CMDQ_CONFIG 0x0e3c
+
#define REG_A6XX_SP_ADDR_MODE_CNTL 0xae01
#define REG_A6XX_SP_NC_MODE_CNTL 0xae02
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 0f3390eab55e..bf95221c64a2 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -519,7 +519,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
if (!pdcptr)
goto err;
- if (adreno_is_a650(adreno_gpu))
+ if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu))
pdc_in_aop = true;
else if (adreno_is_a618(adreno_gpu) || adreno_is_a640(adreno_gpu))
pdc_address_offset = 0x30090;
@@ -549,7 +549,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
gmu_write_rscc(gmu, REG_A6XX_RSCC_PDC_MATCH_VALUE_HI, 0x4514);
/* Load RSC sequencer uCode for sleep and wakeup */
- if (adreno_is_a650(adreno_gpu)) {
+ if (adreno_is_a650_family(adreno_gpu)) {
gmu_write_rscc(gmu, REG_A6XX_RSCC_SEQ_MEM_0_DRV0, 0xeaaae5a0);
gmu_write_rscc(gmu, REG_A6XX_RSCC_SEQ_MEM_0_DRV0 + 1,
0xe1a1ebab);
gmu_write_rscc(gmu, REG_A6XX_RSCC_SEQ_MEM_0_DRV0 + 2,
0xa2e0a581);
@@ -597,7 +597,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_MSGID + 4, 0x10108);
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_ADDR + 4, 0x3);
- if (adreno_is_a618(adreno_gpu) || adreno_is_a650(adreno_gpu))
+ if (adreno_is_a618(adreno_gpu) || adreno_is_a650_family(adreno_gpu))
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x2);
else
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x3);
@@ -698,7 +698,7 @@ static int a6xx_gmu_fw_load(struct a6xx_gmu *gmu)
u32 itcm_base = 0x;
u32 dtcm_base = 0x0004;
- if (adreno_is_a650(adreno_gpu))
+ if (adreno_is_a650_family(adreno_gpu))
dtcm_base = 0x10004000;
if (gmu->legacy) {
@@ -751,7 +751,7 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, unsigned
int state)
int ret;
u32 chipid;
- if (adreno_is_a650(adreno_gpu)) {
+ if (adreno_is_a650_family(adreno_gpu)) {
gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FALNEXT_INTF, 1);
gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FAL_INTF, 1);
}
@@ -1496,12 +1496,28 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
if (ret)
goto err_put_device;
+
+ /* A660 now requires handling "prealloc requests" in GMU firmware
+* For now just hardcode allocations based on the known firmware.
+* note: there is no indication that these correspond to "dummy" or
+* "debug" regions, but this "guess" allows reusing these BOs which
+* are otherwise unused by a660.
+*/
+ gmu->dummy.size = SZ_4K;
+ if (adreno_is_a660(adreno_gpu)) {
+ ret = a6xx_gmu_memory_alloc(gmu,
[Freedreno] [PATCH v3 3/5] drm/msm/a6xx: add GMU_CX_GMU_CX_FALNEXT_INTF write for a650
downstream msm-5.14 kernel added a write to this register, so match that.
Signed-off-by: Jonathan Marek
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 4 +++-
drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index c1ee02d6371d..0f3390eab55e 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -751,8 +751,10 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu,
unsigned int state)
int ret;
u32 chipid;
- if (adreno_is_a650(adreno_gpu))
+ if (adreno_is_a650(adreno_gpu)) {
+ gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FALNEXT_INTF, 1);
gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FAL_INTF, 1);
+ }
if (state == GMU_WARM_BOOT) {
ret = a6xx_rpmh_start(gmu);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
index 5a43d3090b0c..eeef3d6d89b8 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h
@@ -292,6 +292,8 @@ static inline uint32_t A6XX_GMU_GPU_NAP_CTRL_SID(uint32_t
val)
#define REG_A6XX_GPU_GMU_CX_GMU_CX_FAL_INTF0x50f0
+#define REF_A6XX_GPU_GMU_CX_GMU_CX_FALNEXT_INTF0x50f1
+
#define REG_A6XX_GPU_GMU_CX_GMU_PWR_COL_CP_MSG 0x5100
#define REG_A6XX_GPU_GMU_CX_GMU_PWR_COL_CP_RESP
0x5101
--
2.26.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 4/5] drm/msm/a6xx: add missing PC_DBG_ECO_CNTL bit for a640/a650
See downstream's "disable_tseskip" flag. Signed-off-by: Jonathan Marek --- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 853be7651623..bbbf90d86828 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -844,13 +844,15 @@ static int a6xx_hw_init(struct msm_gpu *gpu) /* Setting the mem pool size */ gpu_write(gpu, REG_A6XX_CP_MEM_POOL_SIZE, 128); - /* Setting the primFifo thresholds default values */ + /* Setting the primFifo thresholds default values, +* and vccCacheSkipDis=1 bit (0x200) for A640 and newer + */ if (adreno_is_a650(adreno_gpu)) - gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x0030); + gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00300200); else if (adreno_is_a640(adreno_gpu)) - gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x0020); + gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00200200); else - gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, (0x300 << 11)); + gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x0018); /* Set the AHB default slave response to "ERROR" */ gpu_write(gpu, REG_A6XX_CP_AHB_CNTL, 0x1); -- 2.26.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 2/5] drm/msm/a6xx: use AOP-initialized PDC for a650
SM8250 AOP firmware already sets up PDC registers for us, and it only needs
to be enabled. This path will be used for other newer GPUs.
Signed-off-by: Jonathan Marek
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 3d55e153fa9c..c1ee02d6371d 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -512,19 +512,26 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
struct adreno_gpu *adreno_gpu = _gpu->base;
struct platform_device *pdev = to_platform_device(gmu->dev);
void __iomem *pdcptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc");
- void __iomem *seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
+ void __iomem *seqptr;
uint32_t pdc_address_offset;
+ bool pdc_in_aop = false;
- if (!pdcptr || !seqptr)
+ if (!pdcptr)
goto err;
- if (adreno_is_a618(adreno_gpu) || adreno_is_a640(adreno_gpu))
+ if (adreno_is_a650(adreno_gpu))
+ pdc_in_aop = true;
+ else if (adreno_is_a618(adreno_gpu) || adreno_is_a640(adreno_gpu))
pdc_address_offset = 0x30090;
- else if (adreno_is_a650(adreno_gpu))
- pdc_address_offset = 0x300a0;
else
pdc_address_offset = 0x30080;
+ if (!pdc_in_aop) {
+ seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
+ if (!seqptr)
+ goto err;
+ }
+
/* Disable SDE clock gating */
gmu_write_rscc(gmu, REG_A6XX_GPU_RSCC_RSC_STATUS0_DRV0, BIT(24));
@@ -556,6 +563,9 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
gmu_write_rscc(gmu, REG_A6XX_RSCC_SEQ_MEM_0_DRV0 + 4,
0x0020e8a8);
}
+ if (pdc_in_aop)
+ goto setup_pdc;
+
/* Load PDC sequencer uCode for power up and power down sequence */
pdc_write(seqptr, REG_A6XX_PDC_GPU_SEQ_MEM_0, 0xfebea1e1);
pdc_write(seqptr, REG_A6XX_PDC_GPU_SEQ_MEM_0 + 1, 0xa5a4a3a2);
@@ -596,6 +606,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 8, 0x3);
/* Setup GPU PDC */
+setup_pdc:
pdc_write(pdcptr, REG_A6XX_PDC_GPU_SEQ_START_ADDR, 0);
pdc_write(pdcptr, REG_A6XX_PDC_GPU_ENABLE_PDC, 0x8001);
--
2.26.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 1/5] drm/msm: remove unused icc_path/ocmem_icc_path
These aren't used by anything anymore.
Signed-off-by: Jonathan Marek
Reviewed-by: Akhil P Oommen
---
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 3 ---
drivers/gpu/drm/msm/msm_gpu.h | 9 -
2 files changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 8fd0777f2dc9..009f4c560f16 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -946,7 +946,4 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
pm_runtime_disable(>gpu_pdev->dev);
msm_gpu_cleanup(_gpu->base);
-
- icc_put(gpu->icc_path);
- icc_put(gpu->ocmem_icc_path);
}
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 18baf935e143..c302ab7ffb06 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -118,15 +118,6 @@ struct msm_gpu {
struct clk *ebi1_clk, *core_clk, *rbbmtimer_clk;
uint32_t fast_rate;
- /* The gfx-mem interconnect path that's used by all GPU types. */
- struct icc_path *icc_path;
-
- /*
-* Second interconnect path for some A3xx and all A4xx GPUs to the
-* On Chip MEMory (OCMEM).
-*/
- struct icc_path *ocmem_icc_path;
-
/* Hang and Inactivity Detection:
*/
#define DRM_MSM_INACTIVE_PERIOD 66 /* in ms (roughly four frames) */
--
2.26.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 0/5] drm/msm/a6xx: add support for Adreno 660 GPU
Add support for Adreno 660 to the drm/msm driver. Very similar to A650 on the kernel side. v2: - added AOP PDC path for a650 and use it for a660 too - fix UBWC config for a650 (also affects a660) - add CP_PROTECT update, and corresponding a660 settings in A660 patch v3: - removed patches already picked up by robclark - added patches for missing PC_DBG_ECO_CNTL/GMU_CX_FALNEXT_INTF settings - changed a660_protect to exclude CP_PERFCTR_CP_SEL - removed 635 id from adreno_is_a650_family - squashed a660 patches Jonathan Marek (5): drm/msm: remove unused icc_path/ocmem_icc_path drm/msm/a6xx: use AOP-initialized PDC for a650 drm/msm/a6xx: add GMU_CX_GMU_CX_FALNEXT_INTF write for a650 drm/msm/a6xx: add missing PC_DBG_ECO_CNTL bit for a640/a650 drm/msm/a6xx: add support for Adreno 660 GPU drivers/gpu/drm/msm/adreno/a6xx.xml.h | 4 + drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 53 +-- drivers/gpu/drm/msm/adreno/a6xx_gmu.xml.h | 2 + drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 172 + drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 33 drivers/gpu/drm/msm/adreno/adreno_device.c | 13 ++ drivers/gpu/drm/msm/adreno/adreno_gpu.c| 5 +- drivers/gpu/drm/msm/adreno/adreno_gpu.h| 13 +- drivers/gpu/drm/msm/msm_gpu.h | 9 -- 9 files changed, 248 insertions(+), 56 deletions(-) -- 2.26.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [pull] drm/msm: drm-msm-fixes-2021-06-08 for v5.13-rc6
Hi Dave & Daniel, A few late fixes for v5.13 The following changes since commit f2f46b878777e0d3f885c7ddad48f477b4dea247: drm/msm/dp: initialize audio_comp when audio starts (2021-05-06 16:26:57 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git drm-msm-fixes-2021-06-08 for you to fetch changes up to 924f4cba9e68bf2b4717e7941697db96c6f203be: drm/msm/a6xx: avoid shadow NULL reference in failure path (2021-06-08 10:08:05 -0700) Alexey Minnekhanov (1): drm/msm: Init mm_list before accessing it for use_vram path Jonathan Marek (3): drm/msm/a6xx: update/fix CP_PROTECT initialization drm/msm/a6xx: fix incorrectly set uavflagprd_inv field for A650 drm/msm/a6xx: avoid shadow NULL reference in failure path drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 155 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 +- drivers/gpu/drm/msm/msm_gem.c | 7 ++ 3 files changed, 122 insertions(+), 42 deletions(-) ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC 5/8] lib: add small API for handling register snapshots
On Thu, Jun 3, 2021 at 9:33 AM Dmitry Baryshkov
wrote:
>
> On Thu, 3 Jun 2021 at 17:41, Rob Clark wrote:
> >
> > On Fri, May 28, 2021 at 5:25 PM Dmitry Baryshkov
> > wrote:
> > >
> > > Add small API covering lists of register dumps. Currently this is a part
> > > of MSM DRM driver, but is extracted as it might be usefull to other
> > > drivers too.
> > >
> > > Signed-off-by: Dmitry Baryshkov
> > > ---
> > > include/linux/dump_state.h | 78 ++
> > > lib/Kconfig| 3 ++
> > > lib/Makefile | 1 +
> > > lib/dump_state.c | 51 +
> > > 4 files changed, 133 insertions(+)
> > > create mode 100644 include/linux/dump_state.h
> > > create mode 100644 lib/dump_state.c
> > >
> > [snip]
> > > diff --git a/lib/dump_state.c b/lib/dump_state.c
> > > new file mode 100644
> > > index ..58d88be65c0a
> > > --- /dev/null
> > > +++ b/lib/dump_state.c
> > > @@ -0,0 +1,51 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2021, Linaro Ltd
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +
> > > +void dump_state_free_blocks(struct dump_state *state)
> > > +{
> > > + struct dump_state_block *block, *tmp;
> > > +
> > > + list_for_each_entry_safe(block, tmp, >blocks, node) {
> > > + list_del(>node);
> > > + kfree(block);
> > > + }
> > > +}
> > > +EXPORT_SYMBOL(dump_state_free_blocks);
> >
> > nit, perhaps EXPORT_SYMBOL_GPL()?
>
> I don't really care. What is the current recommendation?
AFAIU it is to default to EXPORT_SYMBOL_GPL() unless there is a good reason..
BR,
-R
> >
> > BR,
> > -R
> >
> > > +
> > > +struct dump_state_block *dump_state_allocate_block_va(void __iomem
> > > *base_addr, size_t len, gfp_t gfp, const char *fmt, va_list args)
> > > +{
> > > + struct dump_state_block *block = kzalloc(sizeof(*block) + len,
> > > gfp);
> > > +
> > > + if (!block)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + vsnprintf(block->name, sizeof(block->name), fmt, args);
> > > +
> > > + INIT_LIST_HEAD(>node);
> > > + block->size = len;
> > > + block->base_addr = base_addr;
> > > +
> > > + return block;
> > > +}
> > > +EXPORT_SYMBOL(dump_state_allocate_block);
> > > +
> > > +struct dump_state_block *dump_state_allocate_block(void __iomem
> > > *base_addr, size_t len, gfp_t gfp, const char *fmt, ...)
> > > +{
> > > + struct dump_state_block *block;
> > > + va_list va;
> > > +
> > > + va_start(va, fmt);
> > > +
> > > + block = dump_state_allocate_block_va(base_addr, len, gfp, fmt,
> > > va);
> > > +
> > > + va_end(va);
> > > +
> > > + return block;
> > > +}
> > > +EXPORT_SYMBOL(dump_state_allocate_block_va);
> > > --
> > > 2.30.2
> > >
>
>
>
> --
> With best wishes
> Dmitry
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 6/8] drm/msm/a6xx: add support for Adreno 660 GPU
On 5/31/21 11:05 AM, Akhil P Oommen wrote:
On 5/13/2021 10:44 PM, Jonathan Marek wrote:
...
@@ -519,7 +519,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
if (!pdcptr)
goto err;
- if (adreno_is_a650(adreno_gpu))
+ if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu))
why not adreno_is_a650_family() here?
Based on downstream, a620 is part of a650_family but does not have
pdc_in_aop flag.
@@ -751,7 +751,7 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu,
unsigned int state)
int ret;
u32 chipid;
We need to program this register here:
gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FALNEXT_INTF, 1);
msm-4.19 does not have this write for a650, but msm-5.4 then adds it.
Will make it a separate change since it affects a650 and not just a660.
- if (adreno_is_a650(adreno_gpu))
+ if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu))
gmu_write(gmu, REG_A6XX_GPU_GMU_CX_GMU_CX_FAL_INTF, 1);
if (state == GMU_WARM_BOOT) {
@@ -1494,12 +1494,28 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu,
struct device_node *node)
if (ret)
goto err_put_device;
+
+ /* A660 now requires handling "prealloc requests" in GMU firmware
+ * For now just hardcode allocations based on the known firmware.
+ * note: there is no indication that these correspond to "dummy" or
+ * "debug" regions, but this "guess" allows reusing these BOs which
+ * are otherwise unused by a660.
+ */
+ gmu->dummy.size = SZ_4K;
+ if (adreno_is_a660(adreno_gpu)) {
+ ret = a6xx_gmu_memory_alloc(gmu, >debug, SZ_4K * 7,
0x6040);
+ if (ret)
+ goto err_memory;
I think we can simply ignore this allocation for a660 because it was
required for an unused feature. Do you see any issue if you ignore this
allocation?
Yes, without it there will be an error:
arm-smmu 3da.iommu: Unhandled context fault: fsr=0x402,
iova=0x6040, fsynr=0x32, cbfrsynra=0x5, cb=2
+
+ gmu->dummy.size = SZ_8K;
+ }
+
/* Allocate memory for the GMU dummy page */
- ret = a6xx_gmu_memory_alloc(gmu, >dummy, SZ_4K, 0x6000);
+ ret = a6xx_gmu_memory_alloc(gmu, >dummy, gmu->dummy.size,
0x6000);
if (ret)
goto err_memory;
- if (adreno_is_a650(adreno_gpu)) {
+ if (adreno_is_a650_family(adreno_gpu)) {
ret = a6xx_gmu_memory_alloc(gmu, >icache,
SZ_16M - SZ_16K, 0x04000);
if (ret)
@@ -885,6 +937,13 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
/* Protect registers from the CP */
a6xx_set_cp_protect(gpu);
+ if (adreno_is_a660(adreno_gpu)) {
+ gpu_write(gpu, REG_A6XX_CP_CHICKEN_DBG, 0x1);
+ gpu_write(gpu, REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL, 0x0);
+ /* Set dualQ + disable afull for A660 GPU but not for A635 */
+ gpu_write(gpu, REG_A6XX_UCHE_CMDQ_CONFIG, 0x66906);
+ }
+
gpu_rmw(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0, (1 << 9));
We need this for a640, a650 and a660.
Will make this a separate patch too, since it affects non-a660 GPUs too.
/* Enable expanded apriv for targets that support it */
if (gpu->hw_apriv) {
gpu_write(gpu, REG_A6XX_CP_APRIV_CNTL,
+/* check for a650, a660, or any derivatives */
+static inline int adreno_is_a650_family(struct adreno_gpu *gpu)
+{
+ return gpu->revn == 650 || gpu->revn == 620 ||
+ gpu->revn == 660 || gpu->revn == 635;
We can remove 635 references throughout since that is not a valid adreno
chipid anymore.
-Akhil
I will remove it for my patch (it can discussed when adding 635 support,
but I think you will need to have a 6xx ID for the GPU)
+}
+
int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t
*value);
const struct firmware *adreno_request_fw(struct adreno_gpu *adreno_gpu,
const char *fwname);
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 2/8] drm/msm/a6xx: use AOP-initialized PDC for a650
On 5/31/21 3:24 AM, Akhil P Oommen wrote:
On 5/13/2021 10:43 PM, Jonathan Marek wrote:
SM8250 AOP firmware already sets up PDC registers for us, and it only
needs
to be enabled. This path will be used for other newer GPUs.
Signed-off-by: Jonathan Marek
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 3d55e153fa9c..c1ee02d6371d 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -512,19 +512,26 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu
*gmu)
struct adreno_gpu *adreno_gpu = _gpu->base;
struct platform_device *pdev = to_platform_device(gmu->dev);
void __iomem *pdcptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc");
- void __iomem *seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
+ void __iomem *seqptr;
uint32_t pdc_address_offset;
+ bool pdc_in_aop = false;
- if (!pdcptr || !seqptr)
+ if (!pdcptr)
goto err;
- if (adreno_is_a618(adreno_gpu) || adreno_is_a640(adreno_gpu))
+ if (adreno_is_a650(adreno_gpu))
+ pdc_in_aop = true;
+ else if (adreno_is_a618(adreno_gpu) || adreno_is_a640(adreno_gpu))
pdc_address_offset = 0x30090;
- else if (adreno_is_a650(adreno_gpu))
- pdc_address_offset = 0x300a0;
else
pdc_address_offset = 0x30080;
+ if (!pdc_in_aop) {
+ seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
+ if (!seqptr)
+ goto err;
+ }
+
/* Disable SDE clock gating */
gmu_write_rscc(gmu, REG_A6XX_GPU_RSCC_RSC_STATUS0_DRV0, BIT(24));
@@ -556,6 +563,9 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
gmu_write_rscc(gmu, REG_A6XX_RSCC_SEQ_MEM_0_DRV0 + 4,
0x0020e8a8);
}
+ if (pdc_in_aop)
+ goto setup_pdc;
+
/* Load PDC sequencer uCode for power up and power down sequence */
pdc_write(seqptr, REG_A6XX_PDC_GPU_SEQ_MEM_0, 0xfebea1e1);
pdc_write(seqptr, REG_A6XX_PDC_GPU_SEQ_MEM_0 + 1, 0xa5a4a3a2);
@@ -596,6 +606,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 8, 0x3);
/* Setup GPU PDC */
+setup_pdc:
pdc_write(pdcptr, REG_A6XX_PDC_GPU_SEQ_START_ADDR, 0);
pdc_write(pdcptr, REG_A6XX_PDC_GPU_ENABLE_PDC, 0x8001);
We can simply swap the order of PDC and rsc programming here and skip
pdc sequence to jump to the rscc programming for a650. This is the order
followed in the downstream driver anyway.
-Akhil.
The order is the same as the msm-4.19 kernel (msm-4.19 is what a650
hardware are using).
Looks like the order was swapped for the msm-5.4 kernel, but if the
order isn't important, I think it is preferable to keep the current
order (to avoid a large diff mainly).
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v4 6/6] drm/msm: devcoredump iommu fault support
On Tue, Jun 01, 2021 at 03:47:25PM -0700, Rob Clark wrote:
> From: Rob Clark
>
> Wire up support to stall the SMMU on iova fault, and collect a devcore-
> dump snapshot for easier debugging of faults.
>
> Currently this is a6xx-only, but mostly only because so far it is the
> only one using adreno-smmu-priv.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 29 +--
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15
> drivers/gpu/drm/msm/msm_gem.h | 1 +
> drivers/gpu/drm/msm/msm_gem_submit.c| 1 +
> drivers/gpu/drm/msm/msm_gpu.c | 48 +
> drivers/gpu/drm/msm/msm_gpu.h | 17 +
> drivers/gpu/drm/msm/msm_gpummu.c| 5 +++
> drivers/gpu/drm/msm/msm_iommu.c | 11 ++
> drivers/gpu/drm/msm/msm_mmu.h | 1 +
> 9 files changed, 126 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 094dc17fd20f..0dcde917e575 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1008,6 +1008,16 @@ static int a6xx_fault_handler(void *arg, unsigned long
> iova, int flags, void *da
> struct msm_gpu *gpu = arg;
> struct adreno_smmu_fault_info *info = data;
> const char *type = "UNKNOWN";
> + const char *block;
> + bool do_devcoredump = info && !READ_ONCE(gpu->crashstate);
> +
> + /*
> + * If we aren't going to be resuming later from fault_worker, then do
> + * it now.
> + */
> + if (!do_devcoredump) {
> + gpu->aspace->mmu->funcs->resume_translation(gpu->aspace->mmu);
> + }
>
> /*
>* Print a default message if we couldn't get the data from the
> @@ -1031,15 +1041,30 @@ static int a6xx_fault_handler(void *arg, unsigned
> long iova, int flags, void *da
> else if (info->fsr & ARM_SMMU_FSR_EF)
> type = "EXTERNAL";
>
> + block = a6xx_fault_block(gpu, info->fsynr1 & 0xff);
> +
> pr_warn_ratelimited("*** gpu fault: ttbr0=%.16llx iova=%.16lx dir=%s
> type=%s source=%s (%u,%u,%u,%u)\n",
> info->ttbr0, iova,
> - flags & IOMMU_FAULT_WRITE ? "WRITE" : "READ", type,
> - a6xx_fault_block(gpu, info->fsynr1 & 0xff),
> + flags & IOMMU_FAULT_WRITE ? "WRITE" : "READ",
> + type, block,
> gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(4)),
> gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(5)),
> gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(6)),
> gpu_read(gpu, REG_A6XX_CP_SCRATCH_REG(7)));
>
> + if (do_devcoredump) {
> + /* Turn off the hangcheck timer to keep it from bothering us */
> + del_timer(>hangcheck_timer);
> +
> + gpu->fault_info.ttbr0 = info->ttbr0;
> + gpu->fault_info.iova = iova;
> + gpu->fault_info.flags = flags;
> + gpu->fault_info.type = type;
> + gpu->fault_info.block = block;
> +
> + kthread_queue_work(gpu->worker, >fault_work);
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index cf897297656f..4e88d4407667 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -684,6 +684,21 @@ void adreno_show(struct msm_gpu *gpu, struct
> msm_gpu_state *state,
> adreno_gpu->info->revn, adreno_gpu->rev.core,
> adreno_gpu->rev.major, adreno_gpu->rev.minor,
> adreno_gpu->rev.patchid);
> + /*
> + * If this is state collected due to iova fault, so fault related info
> + *
> + * TTBR0 would not be zero, so this is a good way to distinguish
> + */
> + if (state->fault_info.ttbr0) {
> + const struct msm_gpu_fault_info *info = >fault_info;
> +
> + drm_puts(p, "fault-info:\n");
> + drm_printf(p, " - ttbr0=%.16llx\n", info->ttbr0);
> + drm_printf(p, " - iova=%.16lx\n", info->iova);
> + drm_printf(p, " - dir=%s\n", info->flags & IOMMU_FAULT_WRITE ?
> "WRITE" : "READ");
> + drm_printf(p, " - type=%s\n", info->type);
> + drm_printf(p, " - source=%s\n", info->block);
> + }
>
> drm_printf(p, "rbbm-status: 0x%08x\n", state->rbbm_status);
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 03e2cc2a2ce1..405f8411e395 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -328,6 +328,7 @@ struct msm_gem_submit {
> struct dma_fence *fence;
> struct msm_gpu_submitqueue *queue;
> struct pid *pid;/* submitting process */
> + bool fault_dumped; /* Limit devcoredump dumping to one per
Re: [Freedreno] [PATCH v4 5/6] drm/msm: Add crashdump support for stalled SMMU
On Tue, Jun 01, 2021 at 03:47:24PM -0700, Rob Clark wrote:
> From: Rob Clark
>
> For collecting devcoredumps with the SMMU stalled after an iova fault,
> we need to skip the parts of the GPU state which are normally collected
> with the hw crashdumper, since with the SMMU stalled the hw would be
> unable to write out the requested state to memory.
On a5xx and a6xx you can query RBBM_STATUS3 bit 24 to see if the IOMMU is
stalled. That could be an alternative option to adding the "stalled"
infrastructure across all targets.
Jordan
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 2 +-
> drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 2 +-
> drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 2 +-
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 5 ++-
> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 +-
> drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 43 -
> drivers/gpu/drm/msm/msm_debugfs.c | 2 +-
> drivers/gpu/drm/msm/msm_gpu.c | 7 ++--
> drivers/gpu/drm/msm/msm_gpu.h | 2 +-
> 9 files changed, 47 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> index bdc989183c64..d2c31fae64fd 100644
> --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> @@ -434,7 +434,7 @@ static void a2xx_dump(struct msm_gpu *gpu)
> adreno_dump(gpu);
> }
>
> -static struct msm_gpu_state *a2xx_gpu_state_get(struct msm_gpu *gpu)
> +static struct msm_gpu_state *a2xx_gpu_state_get(struct msm_gpu *gpu, bool
> stalled)
> {
> struct msm_gpu_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index 4534633fe7cd..b1a6f87d74ef 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -464,7 +464,7 @@ static void a3xx_dump(struct msm_gpu *gpu)
> adreno_dump(gpu);
> }
>
> -static struct msm_gpu_state *a3xx_gpu_state_get(struct msm_gpu *gpu)
> +static struct msm_gpu_state *a3xx_gpu_state_get(struct msm_gpu *gpu, bool
> stalled)
> {
> struct msm_gpu_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index 82bebb40234d..22780a594d6f 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -549,7 +549,7 @@ static const unsigned int a405_registers[] = {
> ~0 /* sentinel */
> };
>
> -static struct msm_gpu_state *a4xx_gpu_state_get(struct msm_gpu *gpu)
> +static struct msm_gpu_state *a4xx_gpu_state_get(struct msm_gpu *gpu, bool
> stalled)
> {
> struct msm_gpu_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index a0eef5d9b89b..2e7714b1a17f 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1519,7 +1519,7 @@ static void a5xx_gpu_state_get_hlsq_regs(struct msm_gpu
> *gpu,
> msm_gem_kernel_put(dumper.bo, gpu->aspace, true);
> }
>
> -static struct msm_gpu_state *a5xx_gpu_state_get(struct msm_gpu *gpu)
> +static struct msm_gpu_state *a5xx_gpu_state_get(struct msm_gpu *gpu, bool
> stalled)
> {
> struct a5xx_gpu_state *a5xx_state = kzalloc(sizeof(*a5xx_state),
> GFP_KERNEL);
> @@ -1536,7 +1536,8 @@ static struct msm_gpu_state *a5xx_gpu_state_get(struct
> msm_gpu *gpu)
> a5xx_state->base.rbbm_status = gpu_read(gpu, REG_A5XX_RBBM_STATUS);
>
> /* Get the HLSQ regs with the help of the crashdumper */
> - a5xx_gpu_state_get_hlsq_regs(gpu, a5xx_state);
> + if (!stalled)
> + a5xx_gpu_state_get_hlsq_regs(gpu, a5xx_state);
>
> a5xx_set_hwcg(gpu, true);
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index ce0610c5256f..e0f06ce4e1a9 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -86,7 +86,7 @@ unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu);
> void a6xx_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> struct drm_printer *p);
>
> -struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu);
> +struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu, bool stalled);
> int a6xx_gpu_state_put(struct msm_gpu_state *state);
>
> #endif /* __A6XX_GPU_H__ */
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index c1699b4f9a89..d0af68a76c4f 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -833,6 +833,21 @@ static void a6xx_get_registers(struct msm_gpu *gpu,
> a6xx_state,
Re: [Freedreno] [PATCH 1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c.
On Fri, 26 Mar 2021 16:13:02 -0700, Eric Anholt wrote: > db820c wants to use the qcom smmu path to get HUPCF set (which keeps > the GPU from wedging and then sometimes wedging the kernel after a > page fault), but it doesn't have separate pagetables support yet in > drm/msm so we can't go all the way to the TTBR1 path. Applied to will (for-joerg/arm-smmu/updates), thanks! [1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c. https://git.kernel.org/will/c/a242f4297cfe [2/2] arm64: dts: msm8996: Mark the GPU's SMMU as an adreno one. https://git.kernel.org/will/c/19c07b91f85d Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RESEND PATCH v4 1/6] iommu/arm-smmu: Add support for driver IOMMU fault handlers
On Wed, Jun 02, 2021 at 09:52:44AM -0700, Rob Clark wrote: > From: Jordan Crouse > > Call report_iommu_fault() to allow upper-level drivers to register their > own fault handlers. > > Signed-off-by: Jordan Crouse > Signed-off-by: Rob Clark > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c > b/drivers/iommu/arm/arm-smmu/arm-smmu.c > index 6f72c4d208ca..b4b32d31fc06 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > @@ -408,6 +408,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void > *dev) > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_device *smmu = smmu_domain->smmu; > int idx = smmu_domain->cfg.cbndx; > + int ret; > > fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR); > if (!(fsr & ARM_SMMU_FSR_FAULT)) > @@ -417,8 +418,12 @@ static irqreturn_t arm_smmu_context_fault(int irq, void > *dev) > iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR); > cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx)); > > - dev_err_ratelimited(smmu->dev, > - "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, > cbfrsynra=0x%x, cb=%d\n", > + ret = report_iommu_fault(domain, NULL, iova, > + fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : > IOMMU_FAULT_READ); > + > + if (ret == -ENOSYS) > + dev_err_ratelimited(smmu->dev, > + "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, > cbfrsynra=0x%x, cb=%d\n", > fsr, iova, fsynr, cbfrsynra, idx); Acked-by: Will Deacon Will ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3] drm/msm/dsi: add continuous clock support for 7nm PHY
On 08/06/2021 04:28, abhin...@codeaurora.org wrote:
On 2021-06-07 16:00, Dmitry Baryshkov wrote:
Unlike previous generations, 7nm PHYs are required to collaborate with
the host for conitnuos clock mode. Add changes neccessary to enable
"the host for continuous clock mode"
Thanks!
continuous clock mode in the 7nm DSI PHYs.
Signed-off-by: Dmitry Baryshkov
---
Changes since v2:
- Really drop msm_dsi_phy_needs_hs_phy_sel()
Changes since v1:
- Remove the need for a separate msm_dsi_phy_needs_hs_phy_sel() call
- Fix setting continuous clock for a dual DSI case.
Maybe I am missing something but I cannot find this part of the change.
What has been fixed for dual DSI?
I have been passing wrong phy for the slave in dual DSI case.
---
drivers/gpu/drm/msm/dsi/dsi.h | 3 ++-
drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 12
drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++--
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 9 +
drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 +
drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 17 +
7 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h
b/drivers/gpu/drm/msm/dsi/dsi.h
index 7abfeab08165..5be458c701d2 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -108,7 +108,7 @@ int msm_dsi_host_enable(struct mipi_dsi_host *host);
int msm_dsi_host_disable(struct mipi_dsi_host *host);
int msm_dsi_host_power_on(struct mipi_dsi_host *host,
struct msm_dsi_phy_shared_timings *phy_shared_timings,
- bool is_dual_dsi);
+ bool is_dual_dsi, struct msm_dsi_phy *phy);
int msm_dsi_host_power_off(struct mipi_dsi_host *host);
int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
const struct drm_display_mode *mode);
@@ -173,6 +173,7 @@ int msm_dsi_phy_get_clk_provider(struct
msm_dsi_phy *phy,
struct clk **byte_clk_provider, struct clk **pixel_clk_provider);
void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy);
int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy);
+bool msm_dsi_phy_set_continuous_clock(struct msm_dsi_phy *phy, bool
enable);
#endif /* __DSI_CONNECTOR_H__ */
diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index 50eb4d1b8fdd..9762af6035e9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -510,6 +510,7 @@ static inline uint32_t
DSI_CLKOUT_TIMING_CTRL_T_CLK_POST(uint32_t val)
#define DSI_LANE_STATUS_DLN0_DIRECTION 0x0001
#define REG_DSI_LANE_CTRL 0x00a8
+#define DSI_LANE_CTRL_HS_REQ_SEL_PHY 0x0100
#define DSI_LANE_CTRL_CLKLN_HS_FORCE_REQUEST 0x1000
#define REG_DSI_LANE_SWAP_CTRL 0x00ac
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 41e1d0f7ab6e..50be935edcad 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -835,7 +835,7 @@ static inline enum dsi_cmd_dst_format
dsi_get_cmd_fmt(
}
static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
- struct msm_dsi_phy_shared_timings *phy_shared_timings)
+ struct msm_dsi_phy_shared_timings *phy_shared_timings,
struct
msm_dsi_phy *phy)
{
u32 flags = msm_host->mode_flags;
enum mipi_dsi_pixel_format mipi_fmt = msm_host->format;
@@ -930,6 +930,10 @@ static void dsi_ctrl_config(struct msm_dsi_host
*msm_host, bool enable,
if (!(flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)) {
lane_ctrl = dsi_read(msm_host, REG_DSI_LANE_CTRL);
+
+ if (msm_dsi_phy_set_continuous_clock(phy, enable))
+ lane_ctrl |= DSI_LANE_CTRL_HS_REQ_SEL_PHY;
+
Not sure how I missed this in the prev patch but for enabling continuous
clock mode for new PHY, you need to clear bit 24
and not set it. If you set it, it goes back to legacy mode ( older
method of continuous clock mode )
Oops. I've most probably missed the inversion in the downstream code at
some point. I'll send v4 later.
dsi_write(msm_host, REG_DSI_LANE_CTRL,
lane_ctrl | DSI_LANE_CTRL_CLKLN_HS_FORCE_REQUEST);
}
@@ -2360,7 +2364,7 @@ static void msm_dsi_sfpb_config(struct
msm_dsi_host *msm_host, bool enable)
int msm_dsi_host_power_on(struct mipi_dsi_host *host,
struct msm_dsi_phy_shared_timings *phy_shared_timings,
- bool is_dual_dsi)
+ bool is_dual_dsi, struct msm_dsi_phy *phy)
{
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
@@ -2400,7 +2404,7 @@ int msm_dsi_host_power_on(struct mipi_dsi_host
*host,
dsi_timing_setup(msm_host, is_dual_dsi);
dsi_sw_reset(msm_host);
- dsi_ctrl_config(msm_host, true, phy_shared_timings);
+ dsi_ctrl_config(msm_host,
Re: [Freedreno] [Mesa-dev] XDC 2021: Registration & Call for Proposals now open!
Kind reminder. Deadline is Sunday, 4 July 2021 :-) Sam On Thu, 2021-05-20 at 10:01 +, Szwichtenberg, Radoslaw wrote: > Hello! > > Registration & Call for Proposals are now open for XDC 2021, which > will > take place on September 15-17, 2021. This year we will repeat as > virtual event. > > https://indico.freedesktop.org/event/1/ > > As usual, the conference is free of charge and open to the general > public. If you plan on attending, please make sure to register as > early > as possible! > > In order to register as attendee, you will therefore need to register > via the XDC website. As XDC moved to a new Indico infrastructure, if > you previously registered on the XDC website, you need to create a > new > account again. > > https://indico.freedesktop.org/event/1/registrations/1/ > > In addition to registration, the CfP is now open for talks, workshops > and demos at XDC 2021. While any serious proposal will be gratefully > considered, topics of interest to X.Org and freedesktop.org > developers > are encouraged. The program focus is on new development, ongoing > challenges and anything else that will spark discussions among > attendees in the hallway track. > > We are open to talks across all layers of the graphics stack, from > the > kernel to desktop environments / graphical applications and about how > to make things better for the developers who build them. Head to the > CfP page to learn more: > > https://indico.freedesktop.org/event/1/abstracts/ > > The deadline for submissions is Sunday, 4 July 2021. > > Last year we modified our Reimbursement Policy to accept speaker > expenses for X.Org virtual events like XDC 2021. Check it out here: > > https://www.x.org/wiki/XorgFoundation/Policies/Reimbursement/ > > If you have any questions, please send me an email to > radoslaw.szwichtenb...@intel.com, adding on CC the X.org board > (board > at foundation.x.org). > > And don't forget, you can follow us on Twitter for all the latest > updates and to stay connected: > > > https://twitter.com/XOrgDevConf > > Best, > > Radek > > P.S: a DNS redirection (xdc2021.x.org) is work in progress. Please > use > the mentioned links for the moment. > > > Radosław Szwichtenberg > - > Intel Technology Poland sp. z o.o. > ul. Slowackiego 173, 80-298 Gdansk > KRS 101882 - NIP 957-07-52-316 > > ___ > mesa-dev mailing list > mesa-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev signature.asc Description: This is a digitally signed message part ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
