Re: [PATCH v2] arm64: dts: qcom: msm8998: enable adreno_smmu by default

[Jeffrey Hugo]

On 5/15/2024 8:27 AM, Marc Gonzalez wrote:

15 qcom platform DTSI files define an adreno_smmu node.
msm8998 is the only one with adreno_smmu disabled by default.

There's no reason why this SMMU should be disabled by default,
it doesn't need any further configuration.

Bring msm8998 in line with the 14 other platforms.

This fixes GPU init failing with ENODEV:
msm_dpu c901000.display-controller: failed to load adreno gpu
msm_dpu c901000.display-controller: failed to bind 500.gpu (ops a3xx_ops): 
-19

Fixes: 87cd46d68aeac8 ("Configure Adreno GPU and related IOMMU")
Signed-off-by: Marc Gonzalez 
Reviewed-by: Bryan O'Donoghue 


Reviewed-by: Jeffrey Hugo 

Re: [Freedreno] [PATCH 2/3] drm/msm/dpu1: Add MSM8998 to hw catalog

[Jeffrey Hugo]

On Wed, Sep 8, 2021 at 2:26 AM Dmitry Baryshkov
 wrote:
>
> Hi,
>
> On Tue, 7 Sept 2021 at 22:13, Jeffrey Hugo  wrote:
> >
> > On Wed, Sep 1, 2021 at 12:11 PM AngeloGioacchino Del Regno
> >  wrote:
> > >
> > > Bringup functionality for MSM8998 in the DPU, driver which is mostly
> > > the same as SDM845 (just a few variations).
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno 
> > > 
> >
> > I don't seem to see a cover letter for this series.
> >
> > Eh, there are a fair number of differences between the MDSS versions
> > for 8998 and 845.
> >
> > Probably a bigger question, why extend the DPU driver for 8998, when
> > the MDP5 driver already supports it[1]?  The MDP/DPU split is pretty
> > dumb, but I don't see a valid reason for both drivers supporting the
> > same target/display revision.  IMO, if you want this support in DPU,
> > remove it from MDP5.
> >
> > [1] 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.14=d6c7b2284b14c66a268a448a7a8d54f585d38785
>
> I don't think that we should enforce such requirements. Having support
> both in MDP5 and DPU would allow one to compare those two drivers,
> performance, features, etc.
> It might be that all MDP5-supported hardware would be also supported
> by DPU, thus allowing us to remove the former driver. But until that
> time I'd suggest leaving support in place.

Well, then you have a host of problems to solve.

Lets ignore the code duplication for a minute and assume we've gone
with this grand experiment.  Two drivers enter, one leaves the victor.

How are the clients supposed to pick which driver to use in the mean
time?  We already have one DT binding for 8998 (which the MDP5 driver
services).  This series proposes a second.  If we go forward with what
you propose, we'll have two bindings for the same hardware, which IMO
doesn't make sense in the context of DT, and the reason for that is to
select which driver is "better".  Driver selection is not supposed to
be tied to DT like this.

So, some boards think MDP5 is better, and some boards think DPU is
better.  At some point, we decide one of the drivers is the clear
winner (lets assume DPU).  Then what happens to the existing DTs that
were using the MDP5 description?  Are they really compatible with DPU?

>From a DT perspective, there should be one description, but then how
do you pick which driver to load?  Both can't bind on the single
description, and while you could argue that the users should build one
driver or the other, but not both (thus picking which one at build
time), that doesn't work for distros that want to build both drivers
so that they can support all platforms with a single build (per arch).

>From where I sit, your position starts with a good idea, but isn't
fully thought out and leads to problems.

If there is some reason why DPU is better for 8998, please enumerate
it.  Does DPU support some config that MDP5 doesn't, which is valuable
to you?  I'm ok with ripping out the MDP5 support, the reason I didn't
go with DPU was that the DPU driver was clearly written only for 845
at the time, and needed significant rework to "downgrade" to an
earlier hardware.  However, the "reason" DPU exists separate from MDP5
is the claim that the MDP hardware underwent a significant
rearchitecture, and thus it was too cumbersome to extend MDP5.  While
I disagree with the premise, that "rearch" started with 8998.

Re: [Freedreno] [PATCH 2/3] drm/msm/dpu1: Add MSM8998 to hw catalog

[Jeffrey Hugo]

On Wed, Sep 1, 2021 at 12:11 PM AngeloGioacchino Del Regno
 wrote:
>
> Bringup functionality for MSM8998 in the DPU, driver which is mostly
> the same as SDM845 (just a few variations).
>
> Signed-off-by: AngeloGioacchino Del Regno 
> 

I don't seem to see a cover letter for this series.

Eh, there are a fair number of differences between the MDSS versions
for 8998 and 845.

Probably a bigger question, why extend the DPU driver for 8998, when
the MDP5 driver already supports it[1]?  The MDP/DPU split is pretty
dumb, but I don't see a valid reason for both drivers supporting the
same target/display revision.  IMO, if you want this support in DPU,
remove it from MDP5.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.14=d6c7b2284b14c66a268a448a7a8d54f585d38785

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Jeffrey Hugo]

On Tue, May 25, 2021 at 11:46 PM Vinod Koul  wrote:
>
> Hello Jeff,
>
> On 21-05-21, 08:09, Jeffrey Hugo wrote:
> > On Fri, May 21, 2021 at 6:50 AM Vinod Koul  wrote:
> > >
> > > Display Stream Compression (DSC) compresses the display stream in host 
> > > which
> > > is later decoded by panel. This series enables this for Qualcomm msm 
> > > driver.
> > > This was tested on Google Pixel3 phone which use LGE SW43408 panel.
> > >
> > > The changes include adding DT properties for DSC then hardware blocks 
> > > support
> > > required in DPU1 driver and support in encoder. We also add support in DSI
> > > and introduce required topology changes.
> > >
> > > In order for panel to set the DSC parameters we add dsc in drm_panel and 
> > > set
> > > it from the msm driver.
> > >
> > > Complete changes which enable this for Pixel3 along with panel driver (not
> > > part of this series) and DT changes can be found at:
> > > git.linaro.org/people/vinod.koul/kernel.git pixel/dsc_rfc
> > >
> > > Comments welcome!
> >
> > This feels backwards to me.  I've only skimmed this series, and the DT
> > changes didn't come through for me, so perhaps I have an incomplete
> > view.
>
> Not sure why, I see it on lore:
> https://lore.kernel.org/dri-devel/20210521124946.3617862-3-vk...@kernel.org/
>
> > DSC is not MSM specific.  There is a standard for it.  Yet it looks
> > like everything is implemented in a MSM specific way, and then pushed
> > to the panel.  So, every vendor needs to implement their vendor
> > specific way to get the DSC info, and then push it to the panel?
> > Seems wrong, given there is an actual standard for this feature.
>
> I have added slice and bpp info in the DT here under the host and then
> pass the generic struct drm_dsc_config to panel which allows panel to
> write the pps cmd
>
> Nothing above is MSM specific.. It can very well work with non MSM
> controllers too.

I disagree.

The DT bindings you defined (thanks for the direct link) are MSM
specific.  I'm not talking (yet) about the properties you defined, but
purely from the stand point that you defined the binding within the
scope of the MSM dsi binding.  No other vendor can use those bindings.
Of course, if we look at the properties themselves, they are prefixed
with "qcom", which is vendor specific.

So, purely on the face of it, this is MSM specific.

Assuming we want a DT solution for DSC, I think it should be something
like Documentation/devicetree/bindings/clock/clock-bindings.txt (the
first example that comes to mind), which is a non-vendor specific
generic set of properties that each vendor/device specific binding can
inherit.  Panel has similar things.

Specific to the properties, I don't much like that you duplicate BPP,
which is already associated with the panel (although perhaps not in
the scope of DT).  What if the panel and your DSC bindings disagree?
Also, I guess I need to ask, have you read the DSC spec?  Last I
looked, there were something like 3 dozen properties that could be
configured.  You have five in your proposed binding.  To me, this is
not a generic DSC solution, this is MSM specific (and frankly I don't
think this supports all the configuration the MSM hardware can do,
either).

I'm surprised Rob Herring didn't have more to say on this.

> I didn't envision DSC to be a specific thing, most of
> the patches here are hardware enabling ones for DSC bits for MSM
> hardware.
>
> > Additionally, we define panel properties (resolution, BPP, etc) at the
> > panel, and have the display drivers pull it from the panel.  However,
> > for DSC, you do the reverse (define it in the display driver, and push
> > it to the panel).  If the argument is that DSC properties can be
> > dynamic, well, so can resolution.  Every panel for MSM MTPs supports
> > multiple resolutions, yet we define that with the panel in Linux.
>
> I dont have an answer for that right now, to start with yes the
> properties are in host but I am okay to discuss this and put wherever we
> feel is most correct thing.  I somehow dont like that we should pull
> from panel DT and program host with that. Here using struct
> drm_dsc_config allows me to configure panel based on resolution passed

I somewhat agree that pulling from the panel and programing the host
based on that is an odd solution, but we have it currently.  Have a
look at Documentation/devicetree/bindings/display/panel in particular
panel-timing.  All of that ends up informing the mdss programing
anyways (particularly the dsi and its phy).  So my problem is that we
currently have a solution that seems to just need to be extended, and
inst

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Jeffrey Hugo]

On Fri, May 21, 2021 at 6:50 AM Vinod Koul  wrote:
>
> Display Stream Compression (DSC) compresses the display stream in host which
> is later decoded by panel. This series enables this for Qualcomm msm driver.
> This was tested on Google Pixel3 phone which use LGE SW43408 panel.
>
> The changes include adding DT properties for DSC then hardware blocks support
> required in DPU1 driver and support in encoder. We also add support in DSI
> and introduce required topology changes.
>
> In order for panel to set the DSC parameters we add dsc in drm_panel and set
> it from the msm driver.
>
> Complete changes which enable this for Pixel3 along with panel driver (not
> part of this series) and DT changes can be found at:
> git.linaro.org/people/vinod.koul/kernel.git pixel/dsc_rfc
>
> Comments welcome!

This feels backwards to me.  I've only skimmed this series, and the DT
changes didn't come through for me, so perhaps I have an incomplete
view.

DSC is not MSM specific.  There is a standard for it.  Yet it looks
like everything is implemented in a MSM specific way, and then pushed
to the panel.  So, every vendor needs to implement their vendor
specific way to get the DSC info, and then push it to the panel?
Seems wrong, given there is an actual standard for this feature.

Additionally, we define panel properties (resolution, BPP, etc) at the
panel, and have the display drivers pull it from the panel.  However,
for DSC, you do the reverse (define it in the display driver, and push
it to the panel).  If the argument is that DSC properties can be
dynamic, well, so can resolution.  Every panel for MSM MTPs supports
multiple resolutions, yet we define that with the panel in Linux.

Finally, I haven't seen the DT bits, but I'm concerned about using DT
for this.  It inherently excludes ACPI systems.  You appear to have
sdm845 support in this series, but what about ACPI boot on the Lenovo
C630 for example?  Or any of the 8cx laptops?  We don't read the panel
resolution, etc from DT, so why the DSC?

I'm glad that work is being done to add DSC to Linux, it's something I
struggled with when working on the 8998 mtp, and I realize this is a
bit of a drive-by review.  However, it seems like there should be a
better way.

>
> Vinod Koul (13):
>   drm/dsc: Add dsc pps header init function
>   dt-bindings: msm/dsi: Document Display Stream Compression (DSC)
> parameters
>   drm/msm/dsi: add support for dsc data
>   drm/msm/disp/dpu1: Add support for DSC
>   drm/msm/disp/dpu1: Add support for DSC in pingpong block
>   drm/msm/disp/dpu1: Add DSC support in RM
>   drm/msm/disp/dpu1: Add DSC for SDM845 to hw_catalog
>   drm/msm/disp/dpu1: Add DSC support in hw_ctl
>   drm/msm/disp/dpu1: Don't use DSC with mode_3d
>   drm/msm/disp/dpu1: Add support for DSC in encoder
>   drm/msm/disp/dpu1: Add support for DSC in topology
>   drm/msm/dsi: Add support for DSC configuration
>   drm/msm/dsi: Pass DSC params to drm_panel
>
>  .../devicetree/bindings/display/msm/dsi.txt   |  15 +
>  drivers/gpu/drm/drm_dsc.c |  11 +
>  drivers/gpu/drm/msm/Makefile  |   1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 204 +++-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  11 +
>  .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |   2 +
>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c|  22 ++
>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h|  26 ++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c|  12 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h|   2 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 221 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h|  79 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h   |  13 +
>  .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c   |  32 ++
>  .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h   |  14 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   |   1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c|  32 ++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h|   1 +
>  drivers/gpu/drm/msm/dsi/dsi.xml.h |  10 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c| 293 +-
>  drivers/gpu/drm/msm/msm_drv.h |  32 ++
>  include/drm/drm_dsc.h |  16 +
>  include/drm/drm_panel.h   |   7 +
>  23 files changed, 1043 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>
> --
> 2.26.3
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 0/9] drm/msm: Avoid possible infinite probe deferral and speed booting

[Jeffrey Hugo]

On Mon, Jul 13, 2020 at 5:50 PM Doug Anderson  wrote:
>
> Hi,
>
> On Mon, Jul 13, 2020 at 1:25 PM Rob Herring  wrote:
> >
> > On Mon, Jul 13, 2020 at 9:08 AM Doug Anderson  wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Jul 13, 2020 at 7:11 AM Rob Herring  wrote:
> > > >
> > > > On Fri, Jul 10, 2020 at 5:02 PM Douglas Anderson 
> > > >  wrote:
> > > > >
> > > > > I found that if I ever had a little mistake in my kernel config,
> > > > > or device tree, or graphics driver that my system would sit in a loop
> > > > > at bootup trying again and again and again.  An example log was:
> > > >
> > > > Why do we care about optimizing the error case?
> > >
> > > It actually results in a _fully_ infinite loop.  That is: if anything
> > > small causes a component of DRM to fail to probe then the whole system
> > > doesn't boot because it just loops trying to probe over and over
> > > again.  The messages I put in the commit message are printed over and
> > > over and over again.
> >
> > Sounds like a bug as that's not what should happen.
> >
> > If you defer during boot (initcalls), then you'll be on the deferred
> > list until late_initcall and everything is retried. After
> > late_initcall, only devices getting added should trigger probing. But
> > maybe the adding and then removing a device is causing a re-trigger.
>
> Right, I'm nearly certain that the adding and then removing is causing
> a re-trigger.  I believe the loop would happen for any case where we
> have a probe function that:
>
> 1. Adds devices.
> 2. After adding devices it decides that it needs to defer.
> 3. Removes the devices it added.
> 4. Return -EPROBE_DEFER from its probe function.
>
> Specifically from what I know about how -EPROBE_DEFER works I'm not
> sure how it wouldn't cause an infinite loop in that case.
>
> Perhaps the missing part of my explanation, though, is why it never
> gets out of this infinite loop.  In my case I purposely made the
> bridge chip "ti-sn65dsi86.c" return an error (-EINVAL) in its probe
> every time.  Obviously I wasn't going to get a display up like this,
> but I just wanted to not loop forever at bootup.  I tracked down
> exactly why we get an - EPROBE_DEFER over and over in this case.
>
> You can see it in msm_dsi_host_register().  If some components haven't
> shown up when that function runs it will _always_ return
> -EPROBE_DEFER.
>
> In my case, since I caused the bridge to fail to probe, those
> components will _never_ show up.  That means that
> msm_dsi_host_register() will _always_ return -EPROBE_DEFER.
>
> I haven't dug through all the DRM code enough, but it doesn't
> necessarily seem like the wrong behavior.  If the bridge driver or a
> panel was a module then (presumably) they could show up later and so
> it should be OK for it to defer, right?
>
> So with all that, it doesn't really feel like this is a bug so much as
> it's an unsupported use case.  The current deferral logic simply can't
> handle the case we're throwing at it.  You cannot return -EPROBE_DEFER
> if your probe function adds devices each time through the probe
> function.
>
> Assuming all the above makes sense, that means we're stuck with:
>
> a) This patch series, which makes us not add devices.
>
> b) Some other patch series which rearchitects the MSM graphics stack
> to not return -EPROBE_DEFER in this case.

This isn't a MSM specific issue.  This is an issue with how the DSI
interface works, and how software is structured in Linux.  I would
expect that pretty much any DSI host in the kernel would have some
version of this issue.

The problem is that DSI is not "hot pluggable", so to give the DRM
stack the info it needs, we need both the DSI controller (aka the MSM
graphics stack in your case), and the thing it connects to (in your
case, the TI bridge, normally the actual panel) because the DRM stack
expects that if init completes, it has certain information
(resolution, etc), and some of that information is in the DSI
controller, and some of it is on the DSI device.

>
> c) Smarten up the deferral system to somehow detect this loop.  I'm
> really not sure how to do this.  You'd have to somehow know that you
> keep adding the same devices over and over again and they didn't get
> us out of the deferral loop last time and so you should eventually
> give up.
>
>
> > > > >   msm ae0.mdss: bound ae01000.mdp (ops 0xffe596e951f8)
> > > > >   msm_dsi ae94000.dsi: ae94000.dsi supply gdsc not found, using dummy 
> > > > > regulator
> > > > >   msm_dsi_manager_register: failed to register mipi dsi host for DSI 0
> > > > >   [drm:ti_sn_bridge_probe] *ERROR* could not find any panel node
> > > > >   ...
> > > > >
> > > > > I finally tracked it down where this was happening:
> > > > >   - msm_pdev_probe() is called.
> > > > >   - msm_pdev_probe() registers drivers.  Registering drivers kicks
> > > > > off processing of probe deferrals.
> > > > >   - component_master_add_with_match() could return -EPROBE_DEFER.
> > > > > making 

Re: [Freedreno] [PATCH 0/9] drm/msm: Avoid possible infinite probe deferral and speed booting

[Jeffrey Hugo]

On Mon, Jul 13, 2020 at 8:11 AM Rob Herring  wrote:
>
> On Fri, Jul 10, 2020 at 5:02 PM Douglas Anderson  
> wrote:
> >
> > I found that if I ever had a little mistake in my kernel config,
> > or device tree, or graphics driver that my system would sit in a loop
> > at bootup trying again and again and again.  An example log was:
>
> Why do we care about optimizing the error case?
>
> >   msm ae0.mdss: bound ae01000.mdp (ops 0xffe596e951f8)
> >   msm_dsi ae94000.dsi: ae94000.dsi supply gdsc not found, using dummy 
> > regulator
> >   msm_dsi_manager_register: failed to register mipi dsi host for DSI 0
> >   [drm:ti_sn_bridge_probe] *ERROR* could not find any panel node
> >   ...
> >
> > I finally tracked it down where this was happening:
> >   - msm_pdev_probe() is called.
> >   - msm_pdev_probe() registers drivers.  Registering drivers kicks
> > off processing of probe deferrals.
> >   - component_master_add_with_match() could return -EPROBE_DEFER.
> > making msm_pdev_probe() return -EPROBE_DEFER.
> >   - When msm_pdev_probe() returned the processing of probe deferrals
> > happens.
> >   - Loop back to the start.
> >
> > It looks like we can fix this by marking "mdss" as a "simple-bus".
> > I have no idea if people consider this the right thing to do or a
> > hack.  Hopefully it's the right thing to do.  :-)
>
> It's a simple test. Do the child devices have any dependency on the
> parent to probe and/or function? If so, not a simple-bus.
>
> > Once I do this I notice that my boot gets marginally faster (you
> > don't need to probe the sub devices over and over) and also if I
>
> Can you quantify that?
>
> Have you run with devlinks enabled. You need a command line option to
> enable. That too should reduce deferred probes.
>
> > have a problem it doesn't loop forever (on my system it still
> > gets upset about some stuck clocks in that case, but at least I
> > can boot up).
>
> Deferred probe only runs when a device is added, so it's not like it
> is continually running.

But it is.  I've hit this as well, but haven't attempted a fix.

So we have a parent device, with several sub devices.  The parent
device probes which causes the sub devices to probe.  One of the sub
devices successfully probes, and another fails with EPROBE_DEFER.
This both caused the probe defer framework to immediately schedule
processing the probe defer queue, and also cause all of the chile
devices and the parent device to be removed to probe defer later.
Since the system state doesn't change (one of the sub devices actually
requires an independent other device to have probed), the system ends
up an an infinite probe defer loop.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v6 0/5] Add support for DisplayPort driver on

[Jeffrey Hugo]

On Mon, Jun 15, 2020 at 4:51 PM  wrote:
>
> On 2020-06-12 16:26, Stephen Boyd wrote:
>
> Thanks for reviews Stephen.
>
> > Quoting Tanmay Shah (2020-06-11 18:50:25)
> >> These patches add support for Display-Port driver on SnapDragon
> >> hardware. It adds
> >> DP driver and DP PLL driver files along with the needed device-tree
> >> bindings.
> >>
> >> The block diagram of DP driver is shown below:
> >>
> >>
> >>  +-+
> >>  |DRM FRAMEWORK|
> >>  +--+--+
> >> |
> >>+v+
> >>| DP DRM  |
> >>+++
> >> |
> >>+v+
> >>  ++|   DP+--++--+
> >>  ++---+| DISPLAY |+---+  |  |
> >>  |++-+-+-+|  |  |
> >>  ||  | |  |  |  |
> >>  ||  | |  |  |  |
> >>  ||  | |  |  |  |
> >>  vv  v v  v  v  v
> >>  +--+ +--+ +---+ ++ ++ +---+ +-+
> >>  |  DP  | |  DP  | |DP | | DP | | DP | |DP | | DP  |
> >>  |PARSER| | HPD  | |AUX| |LINK| |CTRL| |PHY| |POWER|
> >>  +--+---+ +---+--+ +---+ ++ +--+-+ +-+-+ +-+
> >> |  | |
> >>  +--v---+ +v-v+
> >>  |DEVICE| |  DP   |
> >>  | TREE | |CATALOG|
> >>  +--+ +---+---+
> >>   |
> >>   +---v+
> >>   |CTRL/PHY|
> >>   |   HW   |
> >>   ++
> >>
> >
> > I've never seen a block diagram for a driver before...
> >
> It is here for v5. https://patchwork.freedesktop.org/series/74312/

I think Stephen is nitpicking your wording, and you seem to not be
understanding his comment.  I'm sorry if I am mistaken.

The "DP driver" would seem to refer to the linux software driver you
are proposing patches for, however this diagram looks like a hardware
diagram of the various hardware blocks that the Linux driver code (the
"DP driver") is expected to interact with.  I believe you should
re-word "The block diagram of DP driver is shown below:" to be more
specific of what you are describing with your figure.  IE your words
say this is a block diagram of the software, when it looks like it is
a block diagram of the hardware.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drivers: gpu: drm: Add MDP5 configuration for MSM8x36 and its derivatives, such as MSM8939.

[Jeffrey Hugo]

On Mon, May 11, 2020 at 3:03 PM Konrad Dybcio  wrote:
>
> >Is the "| 0" really adding value here?
>
> As far as I can see, it is present in every other config.

Ah, I forgot about that.

Nothing to see here.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drivers: gpu: drm: Add MDP5 configuration for MSM8x36 and its derivatives, such as MSM8939.

[Jeffrey Hugo]

On Fri, May 1, 2020 at 2:52 PM Konrad Dybcio  wrote:
>
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 70 
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> index e3c4c250238b7..1c7de7d6870cf 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> @@ -342,6 +342,75 @@ static const struct mdp5_cfg_hw msm8x16_config = {
> .max_clk = 32000,
>  };
>
> +static const struct mdp5_cfg_hw msm8x36_config = {
> +   .name = "msm8x36",
> +   .mdp = {
> +   .count = 1,
> +   .base = { 0x0 },
> +   .caps = MDP_CAP_SMP |
> +   0,

Is the "| 0" really adding value here?
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v3 1/2] dt-bindings: display: msm: Convert GMU bindings to YAML

[Jeffrey Hugo]

On Tue, Mar 3, 2020 at 8:43 AM Jordan Crouse  wrote:
>
> On Mon, Mar 02, 2020 at 09:49:06PM +0100, Sam Ravnborg wrote:
> > Hi Jordan.
> >
> > On Mon, Mar 02, 2020 at 11:23:43AM -0700, Jordan Crouse wrote:
> > > Convert display/msm/gmu.txt to display/msm/gmu.yaml and remove the old
> > > text bindings.
> > >
> > > Signed-off-by: Jordan Crouse 
> > > ---
> > >
> > >  .../devicetree/bindings/display/msm/gmu.txt| 116 
> > > ---
> > > -
> > > -Required properties:
> > > -- compatible: "qcom,adreno-gmu-XYZ.W", "qcom,adreno-gmu"
> > > -for example: "qcom,adreno-gmu-630.2", "qcom,adreno-gmu"
> > > -  Note that you need to list the less specific "qcom,adreno-gmu"
> > > -  for generic matches and the more specific identifier to identify
> > > -  the specific device.
> > > -- reg: Physical base address and length of the GMU registers.
> > > -- reg-names: Matching names for the register regions
> > > -  * "gmu"
> > > -  * "gmu_pdc"
> > > -  * "gmu_pdc_seg"
> > > -- interrupts: The interrupt signals from the GMU.
> > > -- interrupt-names: Matching names for the interrupts
> > > -  * "hfi"
> > > -  * "gmu"
> > > -- clocks: phandles to the device clocks
> > > -- clock-names: Matching names for the clocks
> > > -   * "gmu"
> > > -   * "cxo"
> > > -   * "axi"
> > > -   * "mnoc"
> > The new binding - and arch/arm64/boot/dts/qcom/sdm845.dtsi agrees that
> > "mnoc" is wrong.
> >
> > > -- power-domains: should be:
> > > -   <_gpucc GPU_CX_GDSC>
> > > -   <_gpucc GPU_GX_GDSC>
> > > -- power-domain-names: Matching names for the power domains
> > > -- iommus: phandle to the adreno iommu
> > > -- operating-points-v2: phandle to the OPP operating points
> > > -
> > > -Optional properties:
> > > -- sram: phandle to the On Chip Memory (OCMEM) that's present on some 
> > > Snapdragon
> > > -SoCs. See Documentation/devicetree/bindings/sram/qcom,ocmem.yaml.
> > This property is not included in the new binding.
>
> Yeah, that guy shouldn't be here. I'm not sure how it got there in the first
> place but I'll update the commit log. Thanks for the poke.

I thought this was something Brian M added for older targets (A4XX?).
Perhaps he should chime in?

>
> Jordan
>
> > Everything else looked fine to me.
> > With sram added - or expalined in commit why it is dropped:
> > Acked-by: Sam Ravnborg 
> >
> >   Sam
> > ___
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly

[Jeffrey Hugo]

On Tue, Feb 11, 2020 at 5:28 PM Rob Clark  wrote:
>
> On Tue, Feb 11, 2020 at 7:59 AM Jeffrey Hugo  wrote:
> >
> > On Tue, Feb 11, 2020 at 8:44 AM Rob Clark  wrote:
> > >
> > > On Mon, Feb 10, 2020 at 9:58 PM  wrote:
> > > >
> > > > On 2020-02-07 19:40, Jeffrey Hugo wrote:
> > > > > On Fri, Feb 7, 2020 at 5:38 AM  wrote:
> > > > >>
> > > > >> On 2020-02-06 20:29, Jeffrey Hugo wrote:
> > > > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P 
> > > > >> > 
> > > > >> > wrote:
> > > > >> >>
> > > > >> >> For a given byte clock, if VCO recalc value is exactly same as
> > > > >> >> vco set rate value, vco_set_rate does not get called assuming
> > > > >> >> VCO is already set to required value. But Due to GDSC toggle,
> > > > >> >> VCO values are erased in the HW. To make sure VCO is programmed
> > > > >> >> correctly, we forcefully call set_rate from vco_prepare.
> > > > >> >
> > > > >> > Is this specific to certain SoCs? I don't think I've observed this.
> > > > >>
> > > > >> As far as Qualcomm SOCs are concerned, since pll is analog and the
> > > > >> value
> > > > >> is directly read from hardware if we get recalc value same as set 
> > > > >> rate
> > > > >> value, the vco_set_rate will not be invoked. We checked in our idp
> > > > >> device which has the same SOC but it works there since the rates are
> > > > >> different.
> > > > >
> > > > > This doesn't seem to be an answer to my question.  What Qualcomm SoCs
> > > > > does this issue apply to?  Everything implementing the 10nm pll?  One
> > > > > specific SoC?  I don't believe I've seen this on MSM8998, nor SDM845,
> > > > > so I'm interested to know what is the actual impact here.  I don't see
> > > > > an "IDP" SoC in the IP catalog, so I really have no idea what you are
> > > > > referring to.
> > > >
> > > >
> > > > This is not 10nm specific. It is applicable for other nms also.
> > > > Its specific to the frequency being set. If vco_recalc returns the same
> > > > value as being set by vco_set_rate,
> > > > vco_set_rate will not be invoked second time onwards.
> > > >
> > > > For example: Lets take below devices:
> > > >
> > > > Cheza is based on SDM845 which is 10nm only.
> > > > Clk frequency:206016
> > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000
> > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947
> > > >
> > > > Trogdor is based on sc7180 which is also 10nm.
> > > > Clk frequency:69300
> > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=166320
> > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 166320
> > > >
> > > > In same trogdor device, we slightly changed the clock frequency and the
> > > > values actually differ which will not cause any issue.
> > > > Clk frequency:69310
> > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=166344
> > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941
> > >
> > >
> > > tbh, loosing state when power is off is kind of the behavior that I'd
> > > expect.  It kinda makes me wonder if things are not getting powered
> > > off all the way on some SoCs?
> > >
> > > jhugo, are you worried that this patch will cause problems on other
> > > users of the 10nm pll?
> >
> > Essentially yes.  Conceptually it doesn't seem like this change should
> > cause any harm, however -
> >
> > This sounds like we are trying to work around the clk framework, which
> > seems wrong.  It feels like we should be able to set a clk flag for
> > this and make the framework deal with it.
> >
> > Also, this fix is 10nm specific, yet this issue affects all
> > implementations?  Seems like this should perhaps be in common code so
> > that we don't need to play whack-a-mole by fixing every implementation
> > piecemeal.
> >
> > Finally, the PLLs are notorious for not taking a configuration unless
> > they are running.  I admit, I haven't looked at this patch in detail
> > to determine if that is the c

Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly

[Jeffrey Hugo]

On Tue, Feb 11, 2020 at 8:44 AM Rob Clark  wrote:
>
> On Mon, Feb 10, 2020 at 9:58 PM  wrote:
> >
> > On 2020-02-07 19:40, Jeffrey Hugo wrote:
> > > On Fri, Feb 7, 2020 at 5:38 AM  wrote:
> > >>
> > >> On 2020-02-06 20:29, Jeffrey Hugo wrote:
> > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P 
> > >> > wrote:
> > >> >>
> > >> >> For a given byte clock, if VCO recalc value is exactly same as
> > >> >> vco set rate value, vco_set_rate does not get called assuming
> > >> >> VCO is already set to required value. But Due to GDSC toggle,
> > >> >> VCO values are erased in the HW. To make sure VCO is programmed
> > >> >> correctly, we forcefully call set_rate from vco_prepare.
> > >> >
> > >> > Is this specific to certain SoCs? I don't think I've observed this.
> > >>
> > >> As far as Qualcomm SOCs are concerned, since pll is analog and the
> > >> value
> > >> is directly read from hardware if we get recalc value same as set rate
> > >> value, the vco_set_rate will not be invoked. We checked in our idp
> > >> device which has the same SOC but it works there since the rates are
> > >> different.
> > >
> > > This doesn't seem to be an answer to my question.  What Qualcomm SoCs
> > > does this issue apply to?  Everything implementing the 10nm pll?  One
> > > specific SoC?  I don't believe I've seen this on MSM8998, nor SDM845,
> > > so I'm interested to know what is the actual impact here.  I don't see
> > > an "IDP" SoC in the IP catalog, so I really have no idea what you are
> > > referring to.
> >
> >
> > This is not 10nm specific. It is applicable for other nms also.
> > Its specific to the frequency being set. If vco_recalc returns the same
> > value as being set by vco_set_rate,
> > vco_set_rate will not be invoked second time onwards.
> >
> > For example: Lets take below devices:
> >
> > Cheza is based on SDM845 which is 10nm only.
> > Clk frequency:206016
> > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000
> > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947
> >
> > Trogdor is based on sc7180 which is also 10nm.
> > Clk frequency:69300
> > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=166320
> > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 166320
> >
> > In same trogdor device, we slightly changed the clock frequency and the
> > values actually differ which will not cause any issue.
> > Clk frequency:69310
> > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=166344
> > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941
>
>
> tbh, loosing state when power is off is kind of the behavior that I'd
> expect.  It kinda makes me wonder if things are not getting powered
> off all the way on some SoCs?
>
> jhugo, are you worried that this patch will cause problems on other
> users of the 10nm pll?

Essentially yes.  Conceptually it doesn't seem like this change should
cause any harm, however -

This sounds like we are trying to work around the clk framework, which
seems wrong.  It feels like we should be able to set a clk flag for
this and make the framework deal with it.

Also, this fix is 10nm specific, yet this issue affects all
implementations?  Seems like this should perhaps be in common code so
that we don't need to play whack-a-mole by fixing every implementation
piecemeal.

Finally, the PLLs are notorious for not taking a configuration unless
they are running.  I admit, I haven't looked at this patch in detail
to determine if that is the case here, but there doesn't seem to be
any indication from the commit test or a comment that doing so is
actually valid in all cases.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly

[Jeffrey Hugo]

On Fri, Feb 7, 2020 at 5:38 AM  wrote:
>
> On 2020-02-06 20:29, Jeffrey Hugo wrote:
> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P 
> > wrote:
> >>
> >> For a given byte clock, if VCO recalc value is exactly same as
> >> vco set rate value, vco_set_rate does not get called assuming
> >> VCO is already set to required value. But Due to GDSC toggle,
> >> VCO values are erased in the HW. To make sure VCO is programmed
> >> correctly, we forcefully call set_rate from vco_prepare.
> >
> > Is this specific to certain SoCs? I don't think I've observed this.
>
> As far as Qualcomm SOCs are concerned, since pll is analog and the value
> is directly read from hardware if we get recalc value same as set rate
> value, the vco_set_rate will not be invoked. We checked in our idp
> device which has the same SOC but it works there since the rates are
> different.

This doesn't seem to be an answer to my question.  What Qualcomm SoCs
does this issue apply to?  Everything implementing the 10nm pll?  One
specific SoC?  I don't believe I've seen this on MSM8998, nor SDM845,
so I'm interested to know what is the actual impact here.  I don't see
an "IDP" SoC in the IP catalog, so I really have no idea what you are
referring to.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly

[Jeffrey Hugo]

On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P  wrote:
>
> For a given byte clock, if VCO recalc value is exactly same as
> vco set rate value, vco_set_rate does not get called assuming
> VCO is already set to required value. But Due to GDSC toggle,
> VCO values are erased in the HW. To make sure VCO is programmed
> correctly, we forcefully call set_rate from vco_prepare.

Is this specific to certain SoCs? I don't think I've observed this.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [v1] drm/msm/dsi: save pll state before dsi host is powered off

[Jeffrey Hugo]

On Thu, Feb 6, 2020 at 1:52 AM Harigovindan P  wrote:
>
> Save pll state before dsi host is powered off. Without this change
> some register values gets resetted.

The phy driver already does this.  Why is the current implementation
insufficient?

>
> Signed-off-by: Harigovindan P 
> ---
>
> Changes in v1:
> - Saving pll state before dsi host is powered off.
> - Removed calling of save state in post_disable since everything
> would be resetted and it would save only resetted values.

Removed from post_disable?  Thats not what I see in the change since
you are adding code to dsi_mgr_bridge_post_disable()
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [v1] dt-bindings: msm:disp: update dsi and dpu bindings

[Jeffrey Hugo]

On Tue, Feb 4, 2020 at 7:15 AM Harigovindan P  wrote:
>
> Updating bindings of dsi and dpu by adding and removing certain
> properties.
>
> Signed-off-by: Harigovindan P 
> ---
>
> Changes in v1:
> - Adding "ahb" clock as a required property.
> - Adding "bus", "rot", "lut" as optional properties for sc7180 device.
> - Removing properties from dsi bindings that are unused.
> - Removing power-domain property since DSI is the child node of MDSS
>   and it will inherit supply from its parent.
>
>  Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 5 -
>  2 files changed, 7 insertions(+), 5 deletions(-)
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
> b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index af95586..61d659a 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -8,13 +8,10 @@ Required properties:
>  - reg-names: The names of register regions. The following regions are 
> required:
>* "dsi_ctrl"
>  - interrupts: The interrupt signal from the DSI block.
> -- power-domains: Should be < MDSS_GDSC>.
>  - clocks: Phandles to device clocks.
>  - clock-names: the following clocks are required:
> -  * "mdp_core"
>* "iface"
>* "bus"
> -  * "core_mmss"

Why do you think these are unused?  I see them used in the driver, and
as far as I can tell these get routed to the hardware, therefore they
should be described in DT.

>* "byte"
>* "pixel"
>* "core"
> @@ -156,7 +153,6 @@ Example:
> "core",
> "core_mmss",
> "iface",
> -   "mdp_core",
> "pixel";
> clocks =
> < MDSS_AXI_CLK>,
> @@ -164,7 +160,6 @@ Example:
> < MDSS_ESC0_CLK>,
> < MMSS_MISC_AHB_CLK>,
> < MDSS_AHB_CLK>,
> -   < MDSS_MDP_CLK>,
> < MDSS_PCLK0_CLK>;
>
> assigned-clocks =
> --
> 2.7.4
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [v1] drm/msm: update LANE_CTRL register value from default value

[Jeffrey Hugo]

On Mon, Dec 23, 2019 at 3:19 AM Harigovindan P  wrote:
>
> Updating REG_DSI_LANE_CTRL register value by reading default
> register value and writing it back using bitwise OR with
> DSI_LANE_CTRL_CLKLN_HS_FORCE_REQUEST. This works for all panels.

Why?
You explain what the code does, which I can tell from reading the
code.  The commit text should tell me why this change is necessary.
Why would I care if this change is in my tree or not?  What feature
does it provide or what issue does it fix?

>
> Signed-off-by: Harigovindan P 
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e6289a3..d3c5233 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -816,7 +816,7 @@ static void dsi_ctrl_config(struct msm_dsi_host 
> *msm_host, bool enable,
> u32 flags = msm_host->mode_flags;
> enum mipi_dsi_pixel_format mipi_fmt = msm_host->format;
> const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
> -   u32 data = 0;
> +   u32 data = 0, lane_ctrl = 0;
>
> if (!enable) {
> dsi_write(msm_host, REG_DSI_CTRL, 0);
> @@ -904,9 +904,11 @@ static void dsi_ctrl_config(struct msm_dsi_host 
> *msm_host, bool enable,
> dsi_write(msm_host, REG_DSI_LANE_SWAP_CTRL,
>   DSI_LANE_SWAP_CTRL_DLN_SWAP_SEL(msm_host->dlane_swap));
>
> -   if (!(flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> +   if (!(flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)) {
> +   lane_ctrl = dsi_read(msm_host, REG_DSI_LANE_CTRL);
> dsi_write(msm_host, REG_DSI_LANE_CTRL,
> -   DSI_LANE_CTRL_CLKLN_HS_FORCE_REQUEST);
> +   lane_ctrl | DSI_LANE_CTRL_CLKLN_HS_FORCE_REQUEST);
> +   }
>
> data |= DSI_CTRL_ENABLE;
>
> --
> 2.7.4
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/mdp5: enable autocommit

[Jeffrey Hugo]

On Thu, Dec 12, 2019 at 7:40 PM Brian Masney  wrote:
>
> Hi Jeffrey,
>
> On Tue, Dec 03, 2019 at 07:18:31AM -0700, Jeffrey Hugo wrote:
> > On Mon, Dec 2, 2019 at 6:40 PM Brian Masney  wrote:
> > > On Wed, Nov 13, 2019 at 06:23:34AM -0500, Brian Masney wrote:
> > > > On Tue, Nov 12, 2019 at 08:38:27AM -0700, Jeffrey Hugo wrote:
> > > > > On Tue, Nov 12, 2019 at 3:49 AM Brian Masney  
> > > > > wrote:
> > > > > >
> > > > > > Since the introduction of commit 2d99ced787e3 ("drm/msm: async 
> > > > > > commit
> > > > > > support"), command-mode panels began throwing the following errors:
> > > > > >
> > > > > > msm fd90.mdss: pp done time out, lm=0
> > > > > >
> > > > > > Let's fix this by enabling the autorefresh feature that's available 
> > > > > > in
> > > > > > the MDP starting at version 1.0. This will cause the MDP to
> > > > > > automatically send a frame to the panel every time the panel invokes
> > > > > > the TE signal, which will trigger the PP_DONE IRQ. This requires not
> > > > > > sending a START signal for command-mode panels.
> > > > > >
> > > > > > This fixes the error and gives us a counter for command-mode panels 
> > > > > > that
> > > > > > we can use to implement async commit support for the MDP5 in a 
> > > > > > follow up
> > > > > > patch.
> > > > > >
> > > > > > Signed-off-by: Brian Masney 
> > > > > > Suggested-by: Jeffrey Hugo 
> > > > > > ---
> > > > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 15 ++-
> > > > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c  |  9 +
> > > > > >  2 files changed, 15 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
> > > > > > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > > > > > index 05cc04f729d6..539348cb6331 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > > > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > > > > > @@ -456,6 +456,7 @@ static void mdp5_crtc_atomic_enable(struct 
> > > > > > drm_crtc *crtc,
> > > > > >  {
> > > > > > struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
> > > > > > struct mdp5_crtc_state *mdp5_cstate = 
> > > > > > to_mdp5_crtc_state(crtc->state);
> > > > > > +   struct mdp5_pipeline *pipeline = _cstate->pipeline;
> > > > > > struct mdp5_kms *mdp5_kms = get_kms(crtc);
> > > > > > struct device *dev = _kms->pdev->dev;
> > > > > >
> > > > > > @@ -493,9 +494,21 @@ static void mdp5_crtc_atomic_enable(struct 
> > > > > > drm_crtc *crtc,
> > > > > >
> > > > > > mdp_irq_register(_kms->base, _crtc->err);
> > > > > >
> > > > > > -   if (mdp5_cstate->cmd_mode)
> > > > > > +   if (mdp5_cstate->cmd_mode) {
> > > > > > mdp_irq_register(_kms->base, 
> > > > > > _crtc->pp_done);
> > > > > >
> > > > > > +   /*
> > > > > > +* Enable autorefresh so we get regular ping/pong 
> > > > > > IRQs.
> > > > > > +* - Bit 31 is the enable bit
> > > > > > +* - Bits 0-15 represent the frame count, 
> > > > > > specifically how many
> > > > > > +*   TE events before the MDP sends a frame.
> > > > > > +*/
> > > > > > +   mdp5_write(mdp5_kms,
> > > > > > +  
> > > > > > REG_MDP5_PP_AUTOREFRESH_CONFIG(pipeline->mixer->pp),
> > > > > > +  BIT(31) | BIT(0));
> > > > > > +   crtc_flush_all(crtc);
> > > > > > +   }
> > > > > > +
> > > > > > mdp5_crtc->enabled = true;
> > > > > >  }
> > > > > >
> > > > > > diff --git a/drivers/g

Re: [Freedreno] [DPU PATCH v3 1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon 845

[Jeffrey Hugo]

On Mon, Dec 2, 2019 at 6:48 AM Chandan Uddaraju  wrote:
>
> Add bindings for Snapdragon 845 DisplayPort and
> display-port PLL driver.
>
> Changes in V2:
> Provide details about sel-gpio
>
> Signed-off-by: Chandan Uddaraju 
> ---
>  .../devicetree/bindings/display/msm/dp.txt | 249 
> +
>  .../devicetree/bindings/display/msm/dpu.txt|  16 +-
>  2 files changed, 261 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/msm/dp.txt
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp.txt 
> b/Documentation/devicetree/bindings/display/msm/dp.txt
> new file mode 100644
> index 000..38be36d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp.txt
> @@ -0,0 +1,249 @@
> +Qualcomm Technologies, Inc.
> +DP is the master Display Port device which supports DP host controllers that 
> are compatible with VESA Display Port interface specification.
> +DP Controller: Required properties:
> +- compatible:   Should be "qcom,dp-display".
> +- reg:  Base address and length of DP hardware's memory 
> mapped regions.
> +- cell-index:   Specifies the controller instance.
> +- reg-names:A list of strings that name the list of regs.
> +   "dp_ahb" - DP controller memory region.
> +   "dp_aux" - DP AUX memory region.
> +   "dp_link" - DP link layer memory region.
> +   "dp_p0" - DP pixel clock domain memory region.
> +   "dp_phy" - DP PHY memory region.
> +   "dp_ln_tx0" - USB3 DP PHY combo TX-0 lane memory 
> region.
> +   "dp_ln_tx1" - USB3 DP PHY combo TX-1 lane memory 
> region.
> +   "dp_mmss_cc" - Display Clock Control memory region.
> +   "qfprom_physical" - QFPROM Phys memory region.
> +   "dp_pll" - USB3 DP combo PLL memory region.
> +   "usb3_dp_com" - USB3 DP PHY combo memory region.
> +   "hdcp_physical" - DP HDCP memory region.
> +- interrupt-parent phandle to the interrupt parent device node.
> +- interrupts:  The interrupt signal from the DP block.
> +- clocks:   Clocks required for Display Port operation. See [1] 
> for details on clock bindings.
> +- clock-names:  Names of the clocks corresponding to handles. 
> Following clocks are required:
> +   "core_aux_clk", 
> "core_usb_ref_clk_src","core_usb_ref_clk", "core_usb_cfg_ahb_clk",
> +   "core_usb_pipe_clk", "ctrl_link_clk", 
> "ctrl_link_iface_clk", "ctrl_crypto_clk",
> +   "ctrl_pixel_clk", "pixel_clk_rcg", "pixel_parent".
> +- pll-node:phandle to DP PLL node.
> +- vdda-1p2-supply: phandle to vdda 1.2V regulator node.
> +- vdda-0p9-supply: phandle to vdda 0.9V regulator node.
> +- qcom,aux-cfg0-settings:  Specifies the DP AUX configuration 0 
> settings. The first
> +   entry in this array corresponds to 
> the register offset
> +   within DP AUX, while the remaining 
> entries indicate the
> +   programmable values.
> +- qcom,aux-cfg1-settings:  Specifies the DP AUX configuration 1 
> settings. The first
> +   entry in this array corresponds to 
> the register offset
> +   within DP AUX, while the remaining 
> entries indicate the
> +   programmable values.
> +- qcom,aux-cfg2-settings:  Specifies the DP AUX configuration 2 
> settings. The first
> +   entry in this array corresponds to 
> the register offset
> +   within DP AUX, while the remaining 
> entries indicate the
> +   programmable values.
> +- qcom,aux-cfg3-settings:  Specifies the DP AUX configuration 3 
> settings. The first
> +   entry in this array corresponds to 
> the register offset
> +   within DP AUX, while the remaining 
> entries indicate the
> +   programmable values.
> +- qcom,aux-cfg4-settings:  Specifies the DP AUX configuration 4 
> settings. The first
> +   entry in this array corresponds to 
> the register offset
> +   within DP AUX, while the remaining 
> entries indicate the
> +   programmable values.
> +- qcom,aux-cfg5-settings:  Specifies the DP AUX configuration 5 
> settings. The first
> +   entry in this array corresponds to 
> 

Re: [Freedreno] [PATCH] drm/msm/mdp5: enable autocommit

[Jeffrey Hugo]

On Mon, Dec 2, 2019 at 6:40 PM Brian Masney  wrote:
>
> Hi Jeffrey,
>
> On Wed, Nov 13, 2019 at 06:23:34AM -0500, Brian Masney wrote:
> > On Tue, Nov 12, 2019 at 08:38:27AM -0700, Jeffrey Hugo wrote:
> > > On Tue, Nov 12, 2019 at 3:49 AM Brian Masney  
> > > wrote:
> > > >
> > > > Since the introduction of commit 2d99ced787e3 ("drm/msm: async commit
> > > > support"), command-mode panels began throwing the following errors:
> > > >
> > > > msm fd90.mdss: pp done time out, lm=0
> > > >
> > > > Let's fix this by enabling the autorefresh feature that's available in
> > > > the MDP starting at version 1.0. This will cause the MDP to
> > > > automatically send a frame to the panel every time the panel invokes
> > > > the TE signal, which will trigger the PP_DONE IRQ. This requires not
> > > > sending a START signal for command-mode panels.
> > > >
> > > > This fixes the error and gives us a counter for command-mode panels that
> > > > we can use to implement async commit support for the MDP5 in a follow up
> > > > patch.
> > > >
> > > > Signed-off-by: Brian Masney 
> > > > Suggested-by: Jeffrey Hugo 
> > > > ---
> > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 15 ++-
> > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c  |  9 +
> > > >  2 files changed, 15 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
> > > > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > > > index 05cc04f729d6..539348cb6331 100644
> > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > > > @@ -456,6 +456,7 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc 
> > > > *crtc,
> > > >  {
> > > > struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
> > > > struct mdp5_crtc_state *mdp5_cstate = 
> > > > to_mdp5_crtc_state(crtc->state);
> > > > +   struct mdp5_pipeline *pipeline = _cstate->pipeline;
> > > > struct mdp5_kms *mdp5_kms = get_kms(crtc);
> > > > struct device *dev = _kms->pdev->dev;
> > > >
> > > > @@ -493,9 +494,21 @@ static void mdp5_crtc_atomic_enable(struct 
> > > > drm_crtc *crtc,
> > > >
> > > > mdp_irq_register(_kms->base, _crtc->err);
> > > >
> > > > -   if (mdp5_cstate->cmd_mode)
> > > > +   if (mdp5_cstate->cmd_mode) {
> > > > mdp_irq_register(_kms->base, _crtc->pp_done);
> > > >
> > > > +   /*
> > > > +* Enable autorefresh so we get regular ping/pong IRQs.
> > > > +* - Bit 31 is the enable bit
> > > > +* - Bits 0-15 represent the frame count, specifically 
> > > > how many
> > > > +*   TE events before the MDP sends a frame.
> > > > +*/
> > > > +   mdp5_write(mdp5_kms,
> > > > +  
> > > > REG_MDP5_PP_AUTOREFRESH_CONFIG(pipeline->mixer->pp),
> > > > +  BIT(31) | BIT(0));
> > > > +   crtc_flush_all(crtc);
> > > > +   }
> > > > +
> > > > mdp5_crtc->enabled = true;
> > > >  }
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c 
> > > > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> > > > index 030279d7b64b..aee295abada3 100644
> > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> > > > @@ -187,14 +187,7 @@ static bool start_signal_needed(struct mdp5_ctl 
> > > > *ctl,
> > > > if (!ctl->encoder_enabled)
> > > > return false;
> > > >
> > > > -   switch (intf->type) {
> > > > -   case INTF_WB:
> > > > -   return true;
> > > > -   case INTF_DSI:
> > > > -   return intf->mode == MDP5_INTF_DSI_MODE_COMMAND;
> > > > -   default:
> > > > -   return false;
> > > > -   }
> > > > +   return intf->type == INTF_WB;
> >

Re: [Freedreno] [PATCH v1 2/2] drm/msm: add DSI config changes to support DSI version

[Jeffrey Hugo]

On Thu, Nov 14, 2019 at 10:47 AM Rob Clark  wrote:
>
> On Thu, Nov 14, 2019 at 2:16 AM Harigovindan P  
> wrote:
> >
> > Add DSI config changes to support DSI version.
> >
> > Signed-off-by: Harigovindan P 
>
> Reviewed-by: Rob Clark 

Can we fix the subject/commit text for this to indicate what DSI
version and/or SoC this is for?  This is a SoC enablement patch, but
at first I thought this was some bug fix or something.

>
> For patch 1/2 with the panel driver, probably best to split that out
> into a different patch(set), since panel drivers are merged into
> drm-next via a different tree
>
> BR,
> -R
>
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_cfg.c | 21 +
> >  drivers/gpu/drm/msm/dsi/dsi_cfg.h |  1 +
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c 
> > b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> > index b7b7c1a..d2c4592 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> > @@ -133,6 +133,10 @@ static const char * const dsi_sdm845_bus_clk_names[] = 
> > {
> > "iface", "bus",
> >  };
> >
> > +static const char * const dsi_sc7180_bus_clk_names[] = {
> > +"iface", "bus",
> > +};
> > +
> >  static const struct msm_dsi_config sdm845_dsi_cfg = {
> > .io_offset = DSI_6G_REG_SHIFT,
> > .reg_cfg = {
> > @@ -147,6 +151,20 @@ static const struct msm_dsi_config sdm845_dsi_cfg = {
> > .num_dsi = 2,
> >  };
> >
> > +static const struct msm_dsi_config sc7180_dsi_cfg = {
> > +   .io_offset = DSI_6G_REG_SHIFT,
> > +   .reg_cfg = {
> > +   .num = 1,
> > +   .regs = {
> > +   {"vdda", 21800, 4 },/* 1.2 V */
> > +   },
> > +   },
> > +   .bus_clk_names = dsi_sc7180_bus_clk_names,
> > +   .num_bus_clks = ARRAY_SIZE(dsi_sc7180_bus_clk_names),
> > +   .io_start = { 0xae94000 },
> > +   .num_dsi = 1,
> > +};
> > +
> >  const static struct msm_dsi_host_cfg_ops msm_dsi_v2_host_ops = {
> > .link_clk_enable = dsi_link_clk_enable_v2,
> > .link_clk_disable = dsi_link_clk_disable_v2,
> > @@ -201,6 +219,9 @@ static const struct msm_dsi_cfg_handler 
> > dsi_cfg_handlers[] = {
> > _dsi_cfg, _dsi_6g_v2_host_ops},
> > {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,
> > _dsi_cfg, _dsi_6g_v2_host_ops},
> > +   {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_4_1,
> > +   _dsi_cfg, _dsi_6g_v2_host_ops},
> > +
> >  };
> >
> >  const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h 
> > b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> > index e2b7a7d..9919536 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> > @@ -19,6 +19,7 @@
> >  #define MSM_DSI_6G_VER_MINOR_V1_4_10x10040001
> >  #define MSM_DSI_6G_VER_MINOR_V2_2_00x2000
> >  #define MSM_DSI_6G_VER_MINOR_V2_2_10x20020001
> > +#define MSM_DSI_6G_VER_MINOR_V2_4_10x20040001
> >
> >  #define MSM_DSI_V2_VER_MINOR_8064  0x0
> >
> > --
> > 2.7.4
> >
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/mdp5: enable autocommit

[Jeffrey Hugo]

On Tue, Nov 12, 2019 at 3:49 AM Brian Masney  wrote:
>
> Since the introduction of commit 2d99ced787e3 ("drm/msm: async commit
> support"), command-mode panels began throwing the following errors:
>
> msm fd90.mdss: pp done time out, lm=0
>
> Let's fix this by enabling the autorefresh feature that's available in
> the MDP starting at version 1.0. This will cause the MDP to
> automatically send a frame to the panel every time the panel invokes
> the TE signal, which will trigger the PP_DONE IRQ. This requires not
> sending a START signal for command-mode panels.
>
> This fixes the error and gives us a counter for command-mode panels that
> we can use to implement async commit support for the MDP5 in a follow up
> patch.
>
> Signed-off-by: Brian Masney 
> Suggested-by: Jeffrey Hugo 
> ---
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 15 ++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c  |  9 +
>  2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> index 05cc04f729d6..539348cb6331 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> @@ -456,6 +456,7 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
>  {
> struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
> struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
> +   struct mdp5_pipeline *pipeline = _cstate->pipeline;
> struct mdp5_kms *mdp5_kms = get_kms(crtc);
> struct device *dev = _kms->pdev->dev;
>
> @@ -493,9 +494,21 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc 
> *crtc,
>
> mdp_irq_register(_kms->base, _crtc->err);
>
> -   if (mdp5_cstate->cmd_mode)
> +   if (mdp5_cstate->cmd_mode) {
> mdp_irq_register(_kms->base, _crtc->pp_done);
>
> +   /*
> +* Enable autorefresh so we get regular ping/pong IRQs.
> +* - Bit 31 is the enable bit
> +* - Bits 0-15 represent the frame count, specifically how 
> many
> +*   TE events before the MDP sends a frame.
> +*/
> +   mdp5_write(mdp5_kms,
> +  
> REG_MDP5_PP_AUTOREFRESH_CONFIG(pipeline->mixer->pp),
> +  BIT(31) | BIT(0));
> +   crtc_flush_all(crtc);
> +   }
> +
> mdp5_crtc->enabled = true;
>  }
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> index 030279d7b64b..aee295abada3 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> @@ -187,14 +187,7 @@ static bool start_signal_needed(struct mdp5_ctl *ctl,
> if (!ctl->encoder_enabled)
> return false;
>
> -   switch (intf->type) {
> -   case INTF_WB:
> -   return true;
> -   case INTF_DSI:
> -   return intf->mode == MDP5_INTF_DSI_MODE_COMMAND;
> -   default:
> -   return false;
> -   }
> +   return intf->type == INTF_WB;
>  }

I don't think this fully works.

The whole "flush" thing exists because the configuration is double
buffered.  You write to the flush register to tell the hardware to
pickup the new configuration, but it doesn't do that automatically.
It only picks up the new config on the next "vsync".  When you have a
video mode panel, you have the timing engine running, which drives
that.  With a command mode panel, you have either the start signal, or
the auto refresh to do the same, but you have a bit of a chicken and
egg situation where if you are programming the hardware from scratch,
autorefresh isn't already enabled to then pickup the config to enable
autorefresh. In this case, you'll need a single start to kick
everything off.  However, if say the bootloader already configured
things and has autorefresh running, then you need to not do that start
because you'll overload the DSI like you saw.

Nothing is simple, is it?  :)
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes

[Jeffrey Hugo]

On Mon, Nov 11, 2019 at 4:38 AM Brian Masney  wrote:
>
> On Sun, Nov 10, 2019 at 10:37:33AM -0700, Jeffrey Hugo wrote:
> > On Sun, Nov 10, 2019 at 6:53 AM Brian Masney  wrote:
> > >
> > > On Fri, Nov 08, 2019 at 07:56:25AM -0700, Jeffrey Hugo wrote:
> > > There's a REG_MDP5_PP_AUTOREFRESH_CONFIG() macro upstream here:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/msm/disp/mdp5/mdp5.xml.h#n1383
> > >
> > > I'm not sure what to put in that register but I tried configuring it
> > > with a 1 this way and still have the same issue.
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c 
> > > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> > > index eeef41fcd4e1..6b9acf68fd2c 100644
> > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> > > @@ -80,6 +80,7 @@ static int pingpong_tearcheck_setup(struct drm_encoder 
> > > *encoder,
> > > mdp5_write(mdp5_kms, REG_MDP5_PP_SYNC_THRESH(pp_id),
> > > MDP5_PP_SYNC_THRESH_START(4) |
> > > MDP5_PP_SYNC_THRESH_CONTINUE(4));
> > > +   mdp5_write(mdp5_kms, REG_MDP5_PP_AUTOREFRESH_CONFIG(pp_id), 1);
> > >
> > > return 0;
> > >  }
> >
> > bit 31 is the enable bit (set that to 1), bits 15:0 are the
> > frame_count (how many te events before the MDP sends a frame, I'd
> > recommend set to 1).  Then after its programmed, you'll have to flush
> > the config, and probably use a _START to make sure the flush takes
> > effect.
>
> I think that I initially get autorefresh enabled based on your
> description above since the ping pong IRQs occur much more frequently.
> However pretty quickly the error 'dsi_err_worker: status=c' is shown,
> the contents on the screen shift to the right, and the screen no longer
> updates after that. That error decodes to
> DSI_ERR_STATE_DLN0_PHY | DSI_ERR_STATE_FIFO according to dsi_host.c.
>
> Here's the relevant code that I have so far:

So, Unless I missed it, you haven't disabled using _start when
autorefresh is enabled.  If you are using both at the same time,
you'll overload the DSI and get those kinds of errors.

>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> index eeef41fcd4e1..85a5cfe54ce8 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> @@ -157,6 +157,7 @@ void mdp5_cmd_encoder_enable(struct drm_encoder *encoder)
> struct mdp5_ctl *ctl = mdp5_cmd_enc->ctl;
> struct mdp5_interface *intf = mdp5_cmd_enc->intf;
> struct mdp5_pipeline *pipeline = 
> mdp5_crtc_get_pipeline(encoder->crtc);
> +   struct mdp5_kms *mdp5_kms = get_kms(encoder);;
>
> if (WARN_ON(mdp5_cmd_enc->enabled))
> return;
> @@ -167,6 +168,14 @@ void mdp5_cmd_encoder_enable(struct drm_encoder *encoder)
>
> mdp5_ctl_commit(ctl, pipeline, mdp_ctl_flush_mask_encoder(intf), 
> true);
>
> +   if (intf->type == INTF_DSI &&
> +   intf->mode == MDP5_INTF_DSI_MODE_COMMAND) {
> +   mdp5_write(mdp5_kms,
> +  
> REG_MDP5_PP_AUTOREFRESH_CONFIG(pipeline->mixer->pp),
> +  BIT(31) | BIT(0));
> +   mdp5_crtc_flush_all(encoder->crtc);
> +   }
> +
> mdp5_ctl_set_encoder_state(ctl, pipeline, true);
>
> mdp5_cmd_enc->enabled = true;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> index 05cc04f729d6..369746ebbc42 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> @@ -103,7 +104,7 @@ static u32 crtc_flush(struct drm_crtc *crtc, u32 
> flush_mask)
>   * so that we can safely queue unref to current fb (ie. next
>   * vblank we know hw is done w/ previous scanout_fb).
>   */
> -static u32 crtc_flush_all(struct drm_crtc *crtc)
> +u32 mdp5_crtc_flush_all(struct drm_crtc *crtc)
>  {
> struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
> struct mdp5_hw_mixer *mixer, *r_mixer;
> @@ -734,7 +735,7 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc,
> if (mdp5_cstate->cmd_mode)
> request_pp_done_pending(crtc);
>
> -   mdp5_crtc->flushed_mask = crtc_flush_all(crtc);
> +   mdp5_crtc->flushed_mask = mdp5_crtc_flush_all(crtc);
>
>  

Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes

[Jeffrey Hugo]

On Sun, Nov 10, 2019 at 6:53 AM Brian Masney  wrote:
>
> On Fri, Nov 08, 2019 at 07:56:25AM -0700, Jeffrey Hugo wrote:
> > On Thu, Nov 7, 2019 at 7:03 PM Rob Clark  wrote:
> > >
> > > On Thu, Nov 7, 2019 at 9:40 AM Jeffrey Hugo  
> > > wrote:
> > > >
> > > > On Thu, Nov 7, 2019 at 9:17 AM Rob Clark  wrote:
> > > > >
> > > > > On Thu, Nov 7, 2019 at 3:10 AM Brian Masney  
> > > > > wrote:
> > > > > >
> > > > > > On Wed, Nov 06, 2019 at 08:58:59AM -0800, Rob Clark wrote:
> > > > > > > On Wed, Nov 6, 2019 at 8:47 AM Jeffrey Hugo 
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > On Wed, Nov 6, 2019 at 9:30 AM Rob Clark  
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Nov 6, 2019 at 1:13 AM Brian Masney 
> > > > > > > > >  wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Nov 05, 2019 at 08:23:27AM -0800, Rob Clark wrote:
> > > > > > > > > > > On Tue, Nov 5, 2019 at 2:08 AM Brian Masney 
> > > > > > > > > > >  wrote:
> > > > > > > > > > > > The 'pp done time out' errors go away if I revert the 
> > > > > > > > > > > > following three
> > > > > > > > > > > > commits:
> > > > > > > > > > > >
> > > > > > > > > > > > cd6d923167b1 ("drm/msm/dpu: async commit support")
> > > > > > > > > > > > d934a712c5e6 ("drm/msm: add atomic traces")
> > > > > > > > > > > > 2d99ced787e3 ("drm/msm: async commit support")
> > > > > > > > > > > >
> > > > > > > > > > > > I reverted the first one to fix a compiler error, and 
> > > > > > > > > > > > the second one so
> > > > > > > > > > > > that the last patch can be reverted without any merge 
> > > > > > > > > > > > conflicts.
> > > > > > > > > > > >
> > > > > > > > > > > > I see that crtc_flush() calls mdp5_ctl_commit(). I 
> > > > > > > > > > > > tried to use
> > > > > > > > > > > > crtc_flush_all() in mdp5_flush_commit() and the 
> > > > > > > > > > > > contents of the frame
> > > > > > > > > > > > buffer dance around the screen like its out of sync. I 
> > > > > > > > > > > > renamed
> > > > > > > > > > > > crtc_flush_all() to mdp5_crtc_flush_all() and removed 
> > > > > > > > > > > > the static
> > > > > > > > > > > > declaration. Here's the relevant part of what I tried:
> > > > > > > > > > > >
> > > > > > > > > > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > > > @@ -171,7 +171,15 @@ static void 
> > > > > > > > > > > > mdp5_prepare_commit(struct msm_kms *kms, struct 
> > > > > > > > > > > > drm_atomic_state *st
> > > > > > > > > > > >
> > > > > > > > > > > >  static void mdp5_flush_commit(struct msm_kms *kms, 
> > > > > > > > > > > > unsigned crtc_mask)
> > > > > > > > > > > >  {
> > > > > > > > > > > > -   /* TODO */
> > > > > > > > > > > > +   struct mdp5_kms *mdp5_kms = 
> > > > > > > > > > > > to_mdp5_kms(to_mdp_kms(kms));
> > > > > > > > > > > > +   struct drm_crtc *crtc;
> > > > > > > > > > > > +
> > > > > > > > > > > > +   for_each_crtc_mask(mdp5_kms->dev, crtc, 
> > > > > > > > > > > > crtc_mask) {
> > > > > > > >

Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()

[Jeffrey Hugo]

On Sat, Nov 9, 2019 at 5:02 AM Stephan Gerhold  wrote:
>
> On Fri, Nov 08, 2019 at 08:47:08PM -0700, Jeffrey Hugo wrote:
> > On Fri, Nov 8, 2019 at 4:47 PM Stephan Gerhold  wrote:
> > >
> > > On Fri, Nov 08, 2019 at 03:12:28PM -0700, Jeffrey Hugo wrote:
> > > > On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold  
> > > > wrote:
> > > > >
> > > > > At the moment, the MSM DSI driver calls drm_panel_enable() rather 
> > > > > early
> > > > > from the DSI bridge pre_enable() function. At this point, the encoder
> > > > > (e.g. MDP5) is not enabled, so we have not started transmitting
> > > > > video data.
> > > > >
> > > > > However, the drm_panel_funcs documentation states that enable()
> > > > > should be called on the panel *after* video data is being transmitted:
> > > > >
> > > > >   The .prepare() function is typically called before the display 
> > > > > controller
> > > > >   starts to transmit video data. [...] After the display controller 
> > > > > has
> > > > >   started transmitting video data, it's safe to call the .enable() 
> > > > > function.
> > > > >   This will typically enable the backlight to make the image on 
> > > > > screen visible.
> > > > >
> > > > > Calling drm_panel_enable() too early causes problems for some panels:
> > > > > The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) 
> > > > > (APQ8016)
> > > > > uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> > > > > backlight/brightness of the screen. The enable sequence is therefore:
> > > > >
> > > > >   drm_panel_enable()
> > > > > drm_panel_funcs.enable():
> > > > >   backlight_enable()
> > > > > backlight_ops.update_status():
> > > > >   mipi_dsi_dcs_set_display_brightness(dsi, 
> > > > > bl->props.brightness);
> > > > >
> > > > > The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> > > > > command if it is sent too early. This prevents setting the initial 
> > > > > brightness,
> > > > > causing the display to be enabled with minimum brightness instead.
> > > > > Adding various delays in the panel initialization code does not result
> > > > > in any difference.
> > > > >
> > > > > On the other hand, moving drm_panel_enable() to 
> > > > > dsi_mgr_bridge_enable()
> > > > > fixes the problem, indicating that the panel requires the video stream
> > > > > to be active before the brightness command is accepted.
> > > > >
> > > > > Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> > > > > delay calling it until video data is being transmitted.
> > > > >
> > > > > Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar 
> > > > > reasons.
> > > > > (This is not strictly required for the panel affected above...)
> > > > >
> > > > > Tested-by: Jasper Korten 
> > > > > Signed-off-by: Stephan Gerhold 
> > > > > ---
> > > > > Since this is a core change I thought it would be better to send this
> > > > > early. I believe Jasper still wants to finish some other changes 
> > > > > before
> > > > > submitting the initial device tree for the Samsung Galaxy Tab A 9.7 
> > > > > (2015). ;)
> > > > >
> > > > > I also tested it on msm8916-samsung-a5u-eur, its display is working 
> > > > > fine
> > > > > with or without this patch.
> > > >
> > > > Nack, please.  I was curious so I threw this on the Lenovo Miix 630
> > > > laptop.  I don't get a display back with this patch.  I'll try to
> > > > figure out why, but currently I can't get into the machine.
> > >
> > > Thanks for testing the patch! Let's try to figure that out...
> > >
> > > I'm a bit confused, but this might be because I'm not very familiar with
> > > the MSM8998 laptops. It does not seem to have display in mainline yet,
> > > so do you have a link to all the patches you are using at the moment?
> >
> > The mdp5 support is there.  Some of the dependencies have dragged out.
> > I'd have t

Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()

[Jeffrey Hugo]

On Fri, Nov 8, 2019 at 4:47 PM Stephan Gerhold  wrote:
>
> On Fri, Nov 08, 2019 at 03:12:28PM -0700, Jeffrey Hugo wrote:
> > On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold  wrote:
> > >
> > > At the moment, the MSM DSI driver calls drm_panel_enable() rather early
> > > from the DSI bridge pre_enable() function. At this point, the encoder
> > > (e.g. MDP5) is not enabled, so we have not started transmitting
> > > video data.
> > >
> > > However, the drm_panel_funcs documentation states that enable()
> > > should be called on the panel *after* video data is being transmitted:
> > >
> > >   The .prepare() function is typically called before the display 
> > > controller
> > >   starts to transmit video data. [...] After the display controller has
> > >   started transmitting video data, it's safe to call the .enable() 
> > > function.
> > >   This will typically enable the backlight to make the image on screen 
> > > visible.
> > >
> > > Calling drm_panel_enable() too early causes problems for some panels:
> > > The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
> > > uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> > > backlight/brightness of the screen. The enable sequence is therefore:
> > >
> > >   drm_panel_enable()
> > > drm_panel_funcs.enable():
> > >   backlight_enable()
> > > backlight_ops.update_status():
> > >   mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> > >
> > > The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> > > command if it is sent too early. This prevents setting the initial 
> > > brightness,
> > > causing the display to be enabled with minimum brightness instead.
> > > Adding various delays in the panel initialization code does not result
> > > in any difference.
> > >
> > > On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
> > > fixes the problem, indicating that the panel requires the video stream
> > > to be active before the brightness command is accepted.
> > >
> > > Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> > > delay calling it until video data is being transmitted.
> > >
> > > Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
> > > (This is not strictly required for the panel affected above...)
> > >
> > > Tested-by: Jasper Korten 
> > > Signed-off-by: Stephan Gerhold 
> > > ---
> > > Since this is a core change I thought it would be better to send this
> > > early. I believe Jasper still wants to finish some other changes before
> > > submitting the initial device tree for the Samsung Galaxy Tab A 9.7 
> > > (2015). ;)
> > >
> > > I also tested it on msm8916-samsung-a5u-eur, its display is working fine
> > > with or without this patch.
> >
> > Nack, please.  I was curious so I threw this on the Lenovo Miix 630
> > laptop.  I don't get a display back with this patch.  I'll try to
> > figure out why, but currently I can't get into the machine.
>
> Thanks for testing the patch! Let's try to figure that out...
>
> I'm a bit confused, but this might be because I'm not very familiar with
> the MSM8998 laptops. It does not seem to have display in mainline yet,
> so do you have a link to all the patches you are using at the moment?

The mdp5 support is there.  Some of the dependencies have dragged out.
I'd have to make sense of my development tree as to what is relevant.
>
> Judging from the patches I was able to find, the Lenovo Miix 630 is
> using a DSI to eDP bridge.
> Isn't the panel managed by the bridge driver in that case?

It uses the TI SN65 bridge.

>
> struct msm_dsi contains:
> /*
>  * panel/external_bridge connected to dsi bridge output, only one of 
> the
>  * two can be valid at a time
>  */
> struct drm_panel *panel;
> struct drm_bridge *external_bridge;
>
> So if you have "external_bridge" set in your case, "panel" should be NULL.
> I have only moved code that uses msm_dsi->panel, so my patch really
> shouldn't make any difference for you.
>
> Am I confusing something here?

I don't think panel is null in my case.  I need to trace a few things
through to be sure.

Taking a quick look at the datasheet for the bridge, I suspect that
operations are occurring in the enable() phase of the bridge, that
need to occur before video data is transmitted.  Based on your
analysis in the commit message, I suspect these operations need to be
moved to pre_enable().

I'm hoping to gather more data this weekend, which will hopefully
identify what we need to do to move this forward without causing
regressions.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/dsi: Delay drm_panel_enable() until dsi_mgr_bridge_enable()

[Jeffrey Hugo]

On Fri, Nov 8, 2019 at 2:29 PM Stephan Gerhold  wrote:
>
> At the moment, the MSM DSI driver calls drm_panel_enable() rather early
> from the DSI bridge pre_enable() function. At this point, the encoder
> (e.g. MDP5) is not enabled, so we have not started transmitting
> video data.
>
> However, the drm_panel_funcs documentation states that enable()
> should be called on the panel *after* video data is being transmitted:
>
>   The .prepare() function is typically called before the display controller
>   starts to transmit video data. [...] After the display controller has
>   started transmitting video data, it's safe to call the .enable() function.
>   This will typically enable the backlight to make the image on screen 
> visible.
>
> Calling drm_panel_enable() too early causes problems for some panels:
> The TFT LCD panel used in the Samsung Galaxy Tab A 9.7 (2015) (APQ8016)
> uses the MIPI_DCS_SET_DISPLAY_BRIGHTNESS command to control
> backlight/brightness of the screen. The enable sequence is therefore:
>
>   drm_panel_enable()
> drm_panel_funcs.enable():
>   backlight_enable()
> backlight_ops.update_status():
>   mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
>
> The panel seems to silently ignore the MIPI_DCS_SET_DISPLAY_BRIGHTNESS
> command if it is sent too early. This prevents setting the initial brightness,
> causing the display to be enabled with minimum brightness instead.
> Adding various delays in the panel initialization code does not result
> in any difference.
>
> On the other hand, moving drm_panel_enable() to dsi_mgr_bridge_enable()
> fixes the problem, indicating that the panel requires the video stream
> to be active before the brightness command is accepted.
>
> Therefore: Move drm_panel_enable() to dsi_mgr_bridge_enable() to
> delay calling it until video data is being transmitted.
>
> Move drm_panel_disable() to dsi_mgr_bridge_disable() for similar reasons.
> (This is not strictly required for the panel affected above...)
>
> Tested-by: Jasper Korten 
> Signed-off-by: Stephan Gerhold 
> ---
> Since this is a core change I thought it would be better to send this
> early. I believe Jasper still wants to finish some other changes before
> submitting the initial device tree for the Samsung Galaxy Tab A 9.7 (2015). ;)
>
> I also tested it on msm8916-samsung-a5u-eur, its display is working fine
> with or without this patch.

Nack, please.  I was curious so I threw this on the Lenovo Miix 630
laptop.  I don't get a display back with this patch.  I'll try to
figure out why, but currently I can't get into the machine.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes

[Jeffrey Hugo]

On Thu, Nov 7, 2019 at 7:03 PM Rob Clark  wrote:
>
> On Thu, Nov 7, 2019 at 9:40 AM Jeffrey Hugo  wrote:
> >
> > On Thu, Nov 7, 2019 at 9:17 AM Rob Clark  wrote:
> > >
> > > On Thu, Nov 7, 2019 at 3:10 AM Brian Masney  wrote:
> > > >
> > > > On Wed, Nov 06, 2019 at 08:58:59AM -0800, Rob Clark wrote:
> > > > > On Wed, Nov 6, 2019 at 8:47 AM Jeffrey Hugo 
> > > > >  wrote:
> > > > > >
> > > > > > On Wed, Nov 6, 2019 at 9:30 AM Rob Clark  
> > > > > > wrote:
> > > > > > >
> > > > > > > On Wed, Nov 6, 2019 at 1:13 AM Brian Masney 
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > On Tue, Nov 05, 2019 at 08:23:27AM -0800, Rob Clark wrote:
> > > > > > > > > On Tue, Nov 5, 2019 at 2:08 AM Brian Masney 
> > > > > > > > >  wrote:
> > > > > > > > > > The 'pp done time out' errors go away if I revert the 
> > > > > > > > > > following three
> > > > > > > > > > commits:
> > > > > > > > > >
> > > > > > > > > > cd6d923167b1 ("drm/msm/dpu: async commit support")
> > > > > > > > > > d934a712c5e6 ("drm/msm: add atomic traces")
> > > > > > > > > > 2d99ced787e3 ("drm/msm: async commit support")
> > > > > > > > > >
> > > > > > > > > > I reverted the first one to fix a compiler error, and the 
> > > > > > > > > > second one so
> > > > > > > > > > that the last patch can be reverted without any merge 
> > > > > > > > > > conflicts.
> > > > > > > > > >
> > > > > > > > > > I see that crtc_flush() calls mdp5_ctl_commit(). I tried to 
> > > > > > > > > > use
> > > > > > > > > > crtc_flush_all() in mdp5_flush_commit() and the contents of 
> > > > > > > > > > the frame
> > > > > > > > > > buffer dance around the screen like its out of sync. I 
> > > > > > > > > > renamed
> > > > > > > > > > crtc_flush_all() to mdp5_crtc_flush_all() and removed the 
> > > > > > > > > > static
> > > > > > > > > > declaration. Here's the relevant part of what I tried:
> > > > > > > > > >
> > > > > > > > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > @@ -171,7 +171,15 @@ static void mdp5_prepare_commit(struct 
> > > > > > > > > > msm_kms *kms, struct drm_atomic_state *st
> > > > > > > > > >
> > > > > > > > > >  static void mdp5_flush_commit(struct msm_kms *kms, 
> > > > > > > > > > unsigned crtc_mask)
> > > > > > > > > >  {
> > > > > > > > > > -   /* TODO */
> > > > > > > > > > +   struct mdp5_kms *mdp5_kms = 
> > > > > > > > > > to_mdp5_kms(to_mdp_kms(kms));
> > > > > > > > > > +   struct drm_crtc *crtc;
> > > > > > > > > > +
> > > > > > > > > > +   for_each_crtc_mask(mdp5_kms->dev, crtc, crtc_mask) {
> > > > > > > > > > +   if (!crtc->state->active)
> > > > > > > > > > +   continue;
> > > > > > > > > > +
> > > > > > > > > > +   mdp5_crtc_flush_all(crtc);
> > > > > > > > > > +   }
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > Any tips would be appreciated.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > I think this is along the lines of what we need to enable 
> > > > > > > > > async commit
> > > > > > > > > for mdp5 (but also removing the flush from the ato

Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes

[Jeffrey Hugo]

On Thu, Nov 7, 2019 at 9:17 AM Rob Clark  wrote:
>
> On Thu, Nov 7, 2019 at 3:10 AM Brian Masney  wrote:
> >
> > On Wed, Nov 06, 2019 at 08:58:59AM -0800, Rob Clark wrote:
> > > On Wed, Nov 6, 2019 at 8:47 AM Jeffrey Hugo  
> > > wrote:
> > > >
> > > > On Wed, Nov 6, 2019 at 9:30 AM Rob Clark  wrote:
> > > > >
> > > > > On Wed, Nov 6, 2019 at 1:13 AM Brian Masney  
> > > > > wrote:
> > > > > >
> > > > > > On Tue, Nov 05, 2019 at 08:23:27AM -0800, Rob Clark wrote:
> > > > > > > On Tue, Nov 5, 2019 at 2:08 AM Brian Masney 
> > > > > > >  wrote:
> > > > > > > > The 'pp done time out' errors go away if I revert the following 
> > > > > > > > three
> > > > > > > > commits:
> > > > > > > >
> > > > > > > > cd6d923167b1 ("drm/msm/dpu: async commit support")
> > > > > > > > d934a712c5e6 ("drm/msm: add atomic traces")
> > > > > > > > 2d99ced787e3 ("drm/msm: async commit support")
> > > > > > > >
> > > > > > > > I reverted the first one to fix a compiler error, and the 
> > > > > > > > second one so
> > > > > > > > that the last patch can be reverted without any merge conflicts.
> > > > > > > >
> > > > > > > > I see that crtc_flush() calls mdp5_ctl_commit(). I tried to use
> > > > > > > > crtc_flush_all() in mdp5_flush_commit() and the contents of the 
> > > > > > > > frame
> > > > > > > > buffer dance around the screen like its out of sync. I renamed
> > > > > > > > crtc_flush_all() to mdp5_crtc_flush_all() and removed the static
> > > > > > > > declaration. Here's the relevant part of what I tried:
> > > > > > > >
> > > > > > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > @@ -171,7 +171,15 @@ static void mdp5_prepare_commit(struct 
> > > > > > > > msm_kms *kms, struct drm_atomic_state *st
> > > > > > > >
> > > > > > > >  static void mdp5_flush_commit(struct msm_kms *kms, unsigned 
> > > > > > > > crtc_mask)
> > > > > > > >  {
> > > > > > > > -   /* TODO */
> > > > > > > > +   struct mdp5_kms *mdp5_kms = 
> > > > > > > > to_mdp5_kms(to_mdp_kms(kms));
> > > > > > > > +   struct drm_crtc *crtc;
> > > > > > > > +
> > > > > > > > +   for_each_crtc_mask(mdp5_kms->dev, crtc, crtc_mask) {
> > > > > > > > +   if (!crtc->state->active)
> > > > > > > > +   continue;
> > > > > > > > +
> > > > > > > > +   mdp5_crtc_flush_all(crtc);
> > > > > > > > +   }
> > > > > > > >  }
> > > > > > > >
> > > > > > > > Any tips would be appreciated.
> > > > > > >
> > > > > > >
> > > > > > > I think this is along the lines of what we need to enable async 
> > > > > > > commit
> > > > > > > for mdp5 (but also removing the flush from the atomic-commit 
> > > > > > > path)..
> > > > > > > the principle behind the async commit is to do all the atomic 
> > > > > > > state
> > > > > > > commit normally, but defer writing the flush bits.  This way, if 
> > > > > > > you
> > > > > > > get another async update before the next vblank, you just apply it
> > > > > > > immediately instead of waiting for vblank.
> > > > > > >
> > > > > > > But I guess you are on a command mode panel, if I remember?  
> > > > > > > Which is
> > > > > > > a case I didn't have a way to test.  And I'm not entirely about 
> > > > > > > how
> > > > > > > kms_funcs->vsync_time() should be implemented for cmd mode panels.
> > > > > >
> > > > > &g

Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes

[Jeffrey Hugo]

On Wed, Nov 6, 2019 at 9:30 AM Rob Clark  wrote:
>
> On Wed, Nov 6, 2019 at 1:13 AM Brian Masney  wrote:
> >
> > On Tue, Nov 05, 2019 at 08:23:27AM -0800, Rob Clark wrote:
> > > On Tue, Nov 5, 2019 at 2:08 AM Brian Masney  wrote:
> > > > The 'pp done time out' errors go away if I revert the following three
> > > > commits:
> > > >
> > > > cd6d923167b1 ("drm/msm/dpu: async commit support")
> > > > d934a712c5e6 ("drm/msm: add atomic traces")
> > > > 2d99ced787e3 ("drm/msm: async commit support")
> > > >
> > > > I reverted the first one to fix a compiler error, and the second one so
> > > > that the last patch can be reverted without any merge conflicts.
> > > >
> > > > I see that crtc_flush() calls mdp5_ctl_commit(). I tried to use
> > > > crtc_flush_all() in mdp5_flush_commit() and the contents of the frame
> > > > buffer dance around the screen like its out of sync. I renamed
> > > > crtc_flush_all() to mdp5_crtc_flush_all() and removed the static
> > > > declaration. Here's the relevant part of what I tried:
> > > >
> > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > @@ -171,7 +171,15 @@ static void mdp5_prepare_commit(struct msm_kms 
> > > > *kms, struct drm_atomic_state *st
> > > >
> > > >  static void mdp5_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> > > >  {
> > > > -   /* TODO */
> > > > +   struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> > > > +   struct drm_crtc *crtc;
> > > > +
> > > > +   for_each_crtc_mask(mdp5_kms->dev, crtc, crtc_mask) {
> > > > +   if (!crtc->state->active)
> > > > +   continue;
> > > > +
> > > > +   mdp5_crtc_flush_all(crtc);
> > > > +   }
> > > >  }
> > > >
> > > > Any tips would be appreciated.
> > >
> > >
> > > I think this is along the lines of what we need to enable async commit
> > > for mdp5 (but also removing the flush from the atomic-commit path)..
> > > the principle behind the async commit is to do all the atomic state
> > > commit normally, but defer writing the flush bits.  This way, if you
> > > get another async update before the next vblank, you just apply it
> > > immediately instead of waiting for vblank.
> > >
> > > But I guess you are on a command mode panel, if I remember?  Which is
> > > a case I didn't have a way to test.  And I'm not entirely about how
> > > kms_funcs->vsync_time() should be implemented for cmd mode panels.
> >
> > Yes, this is a command-mode panel and there's no hardware frame counter
> > available. The key to getting the display working on this phone was this
> > patch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2bab52af6fe68c43b327a57e5ce5fc10eefdfadf
> >
> > > That all said, I think we should first fix what is broken, before
> > > worrying about extending async commit support to mdp5.. which
> > > shouldn't hit the async==true path, due to not implementing
> > > kms_funcs->vsync_time().
> > >
> > > What I think is going on is that, in the cmd mode case,
> > > mdp5_wait_flush() (indirectly) calls mdp5_crtc_wait_for_pp_done(),
> > > which waits for a pp-done irq regardless of whether there is a flush
> > > in progress.  Since there is no flush pending, the irq never comes.
> > > But the expectation is that kms_funcs->wait_flush() returns
> > > immediately if there is nothing to wait for.
> >
> > I don't think that's happening in this case. I added some pr_info()
> > statements to request_pp_done_pending() and mdp5_crtc_pp_done_irq().
> > Here's the first two sets of messages that appear in dmesg:
> >
> > [   14.018907] msm fd90.mdss: pp done time out, lm=0
> > [   14.018993] request_pp_done_pending: HERE
> > [   14.074208] mdp5_crtc_pp_done_irq: HERE
> > [   14.074368] Console: switching to colour frame buffer device 135x120
> > [   14.138938] msm fd90.mdss: pp done time out, lm=0
> > [   14.139021] request_pp_done_pending: HERE
> > [   14.158097] mdp5_crtc_pp_done_irq: HERE
> >
> > The messages go on like this with the same pattern.
> >
> > I tried two different changes:
> >
> > 1) I moved the request_pp_done_pending() and corresponding if statement
> >from mdp5_crtc_atomic_flush() and into mdp5_crtc_atomic_begin().
> >
> > 2) I increased the timeout in wait_for_completion_timeout() by several
> >increments; all the way to 5 seconds.
>
> increasing the timeout won't help, because the pp-done irq has already
> come at the point where we wait for it..
>
> maybe the easy thing is just add mdp5_crtc->needs_pp, set to true
> before requesting, and false when we get the irq.. and then
> mdp5_crtc_wait_for_pp_done() just returns if needs_pp==false..

On the otherhand, what about trying to make command mode panels
resemble video mode panels slightly?  Video mode panels have a vsync
counter in hardware, which is missing from command mode - however it
seems like the driver/drm framework would prefer such a counter.
Would it be a 

Re: [Freedreno] [PATCH RESEND] drm/msm/adreno: Do not print error on "qcom, gpu-pwrlevels" absence

[Jeffrey Hugo]

On Tue, Oct 15, 2019 at 7:40 AM Fabio Estevam  wrote:
>
> Booting the adreno driver on a imx53 board leads to the following
> error message:
>
> adreno 3000.gpu: [drm:adreno_gpu_init] *ERROR* Could not find the GPU 
> powerlevels
>
> As the "qcom,gpu-pwrlevels" property is optional and never present on
> i.MX5, turn the message into debug level instead.
>
> Signed-off-by: Fabio Estevam 

Seems reasonable.  Reviewed-by: Jeffrey Hugo 

> ---
>
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 0783e4b5486a..5d7bdb4c83cc 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -826,7 +826,7 @@ static int adreno_get_legacy_pwrlevels(struct device *dev)
>
> node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
> if (!node) {
> -   DRM_DEV_ERROR(dev, "Could not find the GPU powerlevels\n");
> +   DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
> return -ENXIO;
> }
>
> --
> 2.17.1
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH v2] drm/msm/dsi: Implement reset correctly

[Jeffrey Hugo]

On msm8998, vblank timeouts are observed because the DSI controller is not
reset properly, which ends up stalling the MDP.  This is because the reset
logic is not correct per the hardware documentation.

The documentation states that after asserting reset, software should wait
some time (no indication of how long), or poll the status register until it
returns 0 before deasserting reset.

wmb() is insufficient for this purpose since it just ensures ordering, not
timing between writes.  Since asserting and deasserting reset occurs on the
same register, ordering is already guaranteed by the architecture, making
the wmb extraneous.

Since we would define a timeout for polling the status register to avoid a
possible infinite loop, lets just use a static delay of 20 ms, since 16.666
ms is the time available to process one frame at 60 fps.

Fixes: a689554ba6ed (drm/msm: Initial add DSI connector support)
Signed-off-by: Jeffrey Hugo 
Reviewed-by: Sean Paul 
---

v2:
-make a DEFINE for the delay

 drivers/gpu/drm/msm/dsi/dsi_host.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 663ff9f4fac9..9a81705301c6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -26,6 +26,8 @@
 #include "dsi_cfg.h"
 #include "msm_kms.h"
 
+#define RESET_DELAY 20
+
 static int dsi_get_version(const void __iomem *base, u32 *major, u32 *minor)
 {
u32 ver;
@@ -986,7 +988,7 @@ static void dsi_sw_reset(struct msm_dsi_host *msm_host)
wmb(); /* clocks need to be enabled before reset */
 
dsi_write(msm_host, REG_DSI_RESET, 1);
-   wmb(); /* make sure reset happen */
+   msleep(RESET_DELAY); /* make sure reset happen */
dsi_write(msm_host, REG_DSI_RESET, 0);
 }
 
@@ -1396,7 +1398,7 @@ static void dsi_sw_reset_restore(struct msm_dsi_host 
*msm_host)
 
/* dsi controller can only be reset while clocks are running */
dsi_write(msm_host, REG_DSI_RESET, 1);
-   wmb();  /* make sure reset happen */
+   msleep(RESET_DELAY);/* make sure reset happen */
dsi_write(msm_host, REG_DSI_RESET, 0);
wmb();  /* controller out of reset */
dsi_write(msm_host, REG_DSI_CTRL, data0);
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/dsi: Implement reset correctly

[Jeffrey Hugo]

On Thu, Oct 10, 2019 at 12:49 PM Sean Paul  wrote:
>
> On Thu, Oct 10, 2019 at 2:45 PM Sean Paul  wrote:
> >
> > On Wed, Oct 09, 2019 at 02:34:54PM -0700, Jeffrey Hugo wrote:
> > > On msm8998, vblank timeouts are observed because the DSI controller is not
> > > reset properly, which ends up stalling the MDP.  This is because the reset
> > > logic is not correct per the hardware documentation.
> > >
> > > The documentation states that after asserting reset, software should wait
> > > some time (no indication of how long), or poll the status register until 
> > > it
> > > returns 0 before deasserting reset.
> > >
> > > wmb() is insufficient for this purpose since it just ensures ordering, not
> > > timing between writes.  Since asserting and deasserting reset occurs on 
> > > the
> > > same register, ordering is already guaranteed by the architecture, making
> > > the wmb extraneous.
> > >
> > > Since we would define a timeout for polling the status register to avoid a
> > > possible infinite loop, lets just use a static delay of 20 ms, since 
> > > 16.666
> > > ms is the time available to process one frame at 60 fps.
> > >
> > > Fixes: a689554ba6ed (drm/msm: Initial add DSI connector support)
> > > Signed-off-by: Jeffrey Hugo 
> > > ---
> > >
> > > Rob et al, is it possible for this to go into a 5.4-rc?
>
> Sorry, I missed this on the first go-around, I'm Ok with this getting
> into 5.4. Rob, if you're Ok with this, I can send it through -misc
> unless you're planning an msm-fixes PR.
>
> > >
> > >  drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> > > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > index 663ff9f4fac9..68ded9b4735d 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > @@ -986,7 +986,7 @@ static void dsi_sw_reset(struct msm_dsi_host 
> > > *msm_host)
> > >   wmb(); /* clocks need to be enabled before reset */
> > >
> > >   dsi_write(msm_host, REG_DSI_RESET, 1);
> > > - wmb(); /* make sure reset happen */
> > > + msleep(20); /* make sure reset happen */
> >
> > Could you please pull this out into a #define used for both in case we 
> > decide to
> > tweak it? I don't want these 2 values to drift.
> >

Oh, yeah.  That's a really good point.  Will fix.

>
> oh yeah, and with that fixed,
>
> Reviewed-by: Sean Paul 

Thanks.

>
> > Thanks,
> > Sean
> >
> > >   dsi_write(msm_host, REG_DSI_RESET, 0);
> > >  }
> > >
> > > @@ -1396,7 +1396,7 @@ static void dsi_sw_reset_restore(struct 
> > > msm_dsi_host *msm_host)
> > >
> > >   /* dsi controller can only be reset while clocks are running */
> > >   dsi_write(msm_host, REG_DSI_RESET, 1);
> > > - wmb();  /* make sure reset happen */
> > > + msleep(20); /* make sure reset happen */
> > >   dsi_write(msm_host, REG_DSI_RESET, 0);
> > >   wmb();  /* controller out of reset */
> > >   dsi_write(msm_host, REG_DSI_CTRL, data0);
> > > --
> > > 2.17.1
> > >
> >
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH] drm/msm/dsi: Implement reset correctly

[Jeffrey Hugo]

On msm8998, vblank timeouts are observed because the DSI controller is not
reset properly, which ends up stalling the MDP.  This is because the reset
logic is not correct per the hardware documentation.

The documentation states that after asserting reset, software should wait
some time (no indication of how long), or poll the status register until it
returns 0 before deasserting reset.

wmb() is insufficient for this purpose since it just ensures ordering, not
timing between writes.  Since asserting and deasserting reset occurs on the
same register, ordering is already guaranteed by the architecture, making
the wmb extraneous.

Since we would define a timeout for polling the status register to avoid a
possible infinite loop, lets just use a static delay of 20 ms, since 16.666
ms is the time available to process one frame at 60 fps.

Fixes: a689554ba6ed (drm/msm: Initial add DSI connector support)
Signed-off-by: Jeffrey Hugo 
---

Rob et al, is it possible for this to go into a 5.4-rc?

 drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 663ff9f4fac9..68ded9b4735d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -986,7 +986,7 @@ static void dsi_sw_reset(struct msm_dsi_host *msm_host)
wmb(); /* clocks need to be enabled before reset */
 
dsi_write(msm_host, REG_DSI_RESET, 1);
-   wmb(); /* make sure reset happen */
+   msleep(20); /* make sure reset happen */
dsi_write(msm_host, REG_DSI_RESET, 0);
 }
 
@@ -1396,7 +1396,7 @@ static void dsi_sw_reset_restore(struct msm_dsi_host 
*msm_host)
 
/* dsi controller can only be reset while clocks are running */
dsi_write(msm_host, REG_DSI_RESET, 1);
-   wmb();  /* make sure reset happen */
+   msleep(20); /* make sure reset happen */
dsi_write(msm_host, REG_DSI_RESET, 0);
wmb();  /* controller out of reset */
dsi_write(msm_host, REG_DSI_CTRL, data0);
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 1/5] drm/msm/mdp5: Add optional TBU and TBU_RT clocks

[Jeffrey Hugo]

On Sun, Sep 22, 2019 at 8:16 AM  wrote:
>
> From: "Angelo G. Del Regno" 
>
> Some SoCs, like MSM8956/8976 (and APQ variants), do feature these
> clocks and we need to enable them in order to get the hardware to
> properly work.
>
> Signed-off-by: Angelo G. Del Regno 

I don't see these clocks documented in the mdp5 DT bindings document.
They need to be added in the DT first.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/dsi: Move static keyword to the front of declarations

[Jeffrey Hugo]

On Wed, Sep 4, 2019 at 3:15 PM Krzysztof Wilczynski  wrote:
>
> Move the static keyword to the front of declarations
> of msm_dsi_v2_host_ops, msm_dsi_6g_host_ops and
> msm_dsi_6g_v2_host_ops, and resolve the following
> compiler warnings that can be seen when building
> with warnings enabled (W=1):
>
> drivers/gpu/drm/msm/dsi/dsi_cfg.c:150:1: warning:
>   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
>
> drivers/gpu/drm/msm/dsi/dsi_cfg.c:161:1: warning:
>   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
>
> drivers/gpu/drm/msm/dsi/dsi_cfg.c:172:1: warning:
>   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
>
> Signed-off-by: Krzysztof Wilczynski 

Seems fine to me.

Reviewed-by: Jeffrey Hugo 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH v2] drm/msm/mdp5: Find correct node for creating gem address space

[Jeffrey Hugo]

Creating the msm gem address space requires a reference to the dev where
the iommu is located.  The driver currently assumes this is the same as
the platform device, which breaks when the iommu is outside of the
platform device (ie in the parent).  Default to using the platform device,
but check to see if that has an iommu reference, and if not, use the parent
device instead.  This should handle all the various iommu designs for
mdp5 supported systems.

Signed-off-by: Jeffrey Hugo 
---

v2: It turns out there isn't a universal way to get the iommu device, so 
check to see if its in the current node or parent

 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 4a60f5fca6b0..02dc7d426cb0 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -663,6 +663,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
struct msm_kms *kms;
struct msm_gem_address_space *aspace;
int irq, i, ret;
+   struct device *iommu_dev;
 
/* priv->kms would have been populated by the MDP5 driver */
kms = priv->kms;
@@ -702,7 +703,11 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
mdelay(16);
 
if (config->platform.iommu) {
-   aspace = msm_gem_address_space_create(>dev,
+   iommu_dev = >dev;
+   if (!iommu_dev->iommu_fwspec)
+   iommu_dev = iommu_dev->parent;
+
+   aspace = msm_gem_address_space_create(iommu_dev,
config->platform.iommu, "mdp5");
if (IS_ERR(aspace)) {
ret = PTR_ERR(aspace);
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/mdp5: Use eariler mixers when possible

[Jeffrey Hugo]

On Mon, Jul 1, 2019 at 1:55 PM Rob Clark  wrote:
>
> On Mon, Jul 1, 2019 at 10:41 AM Jeffrey Hugo  wrote:
> >
> > When assigning a mixer, we will iterate through the entire list looking for
> > a suitable match.  This results in selecting the last match.  We should
> > stop at the first match, since lower numbered mixers will typically have
> > more capabilities, and are likely to be what the bootloader used, if we
> > are looking to reuse the bootloader config in future.
>
> I think for matching bootloader config, we need to read it out of the
> hw and do it the hard way, rather than making assumptions.
>
> For picking hwpipe for a plane, I made an effort to pick the available
> hwpipe w/ the *least* capabilities that fit the requirements (ie.
> scaling/yuv/etc) in order to leave the more capable hwpipe available
> for future use.  Not sure if same approach would make sense for
> mixers?  But not sure if picking something that bootloader probably
> also would have picked is a great argument.
>
> I do have some (now ancient) code from db410/mdp5 to read out he hw
> state.. I *think* that might have post-dated dynamically picking
> mixers.  (The older mdp5 on db410c also had to deal with figuring out
> SMP block config, iirc.. thankfully newer mdp5 doesn't have to deal
> with that.)

So I ripped this out and retested as I was looking back at it.  Things
still work.  I probably saw a need for this in the middle of my
hacking, but its clearly not needed anymore so lets drop it for now.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

[Jeffrey Hugo]

On Wed, Jul 3, 2019 at 6:25 AM Rob Clark  wrote:
>
> On Tue, Jul 2, 2019 at 9:08 PM Bjorn Andersson
>  wrote:
> >
> > On Mon 01 Jul 10:39 PDT 2019, Jeffrey Hugo wrote:
> >
> > > Creating the msm gem address space requires a reference to the dev where
> > > the iommu is located.  The driver currently assumes this is the same as
> > > the platform device, which breaks when the iommu is outside of the
> > > platform device.  Use the drm_device instead, which happens to always have
> > > a reference to the proper device.
> > >
> > > Signed-off-by: Jeffrey Hugo 
> >
> > Sorry, but on db820c this patch results in:
> >
> > [   64.803263] msm_mdp 901000.mdp: [drm:mdp5_kms_init [msm]] *ERROR* failed 
> > to attach iommu: -19
> >
> > Followed by 3 oopses as we're trying to fail the initialization.
>
> yeah, that is kinda what I suspected would happen.  I guess to deal
> with how things are hooked up on 8998, perhaps the best thing is to
> first try >dev, and then if that fails try dev->dev

Thanks for the test feedback Bjorn.  Its unfortunate this solution
didn't work as I expected.  I'll give Rob's suggestion a shot and spin
another version.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: make sure we have panel or bridge earlier

[Jeffrey Hugo]

On Tue, Jul 2, 2019 at 2:30 PM Jeffrey Hugo  wrote:
>
> On Sun, Jun 30, 2019 at 7:16 AM Rob Clark  wrote:
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1824,6 +1824,20 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
> > goto fail;
> > }
> >
> > +   /*
> > +* Make sure we have panel or bridge early, before we start
> > +* touching the hw.  If bootloader enabled the display, we
> > +* want to be sure to keep it running until the bridge/panel
> > +* is probed and we are all ready to go.  Otherwise we'll
> > +* kill the display and then -EPROBE_DEFER
> > +*/
> > +   if (IS_ERR(of_drm_find_panel(msm_host->device_node)) &&
> > +   !of_drm_find_bridge(msm_host->device_node)) {
> > +   pr_err("%s: no panel or bridge yet\n", __func__);
>
> pr_err() doesn't seem right for a probe defer condition.  pr_dbg?
>
> > +   return -EPROBE_DEFER;
> > +   }
> > +
> > +
>
> Tested-by: Jeffrey Hugo 
> Reviewed-by: Jeffrey Hugo 

Actually, I'm sorry, I'm now NACKing this.

Turns out this prevents the panel/bridge from ever probing if its a
child node of the dsi device, since mipi_dsi_host_register() is never
called.

This probably works for you on the c630 because the bridge hangs off
the i2c bus.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: make sure we have panel or bridge earlier

[Jeffrey Hugo]

On Sun, Jun 30, 2019 at 7:16 AM Rob Clark  wrote:
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1824,6 +1824,20 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
> goto fail;
> }
>
> +   /*
> +* Make sure we have panel or bridge early, before we start
> +* touching the hw.  If bootloader enabled the display, we
> +* want to be sure to keep it running until the bridge/panel
> +* is probed and we are all ready to go.  Otherwise we'll
> +* kill the display and then -EPROBE_DEFER
> +*/
> +   if (IS_ERR(of_drm_find_panel(msm_host->device_node)) &&
> +   !of_drm_find_bridge(msm_host->device_node)) {
> +   pr_err("%s: no panel or bridge yet\n", __func__);

pr_err() doesn't seem right for a probe defer condition.  pr_dbg?

> +   return -EPROBE_DEFER;
> +   }
> +
> +

Tested-by: Jeffrey Hugo 
Reviewed-by: Jeffrey Hugo 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

[Jeffrey Hugo]

On Mon, Jul 1, 2019 at 1:45 PM Rob Clark  wrote:
>
> On Mon, Jul 1, 2019 at 10:39 AM Jeffrey Hugo  wrote:
> >
> > Creating the msm gem address space requires a reference to the dev where
> > the iommu is located.  The driver currently assumes this is the same as
> > the platform device, which breaks when the iommu is outside of the
> > platform device.  Use the drm_device instead, which happens to always have
> > a reference to the proper device.
> >
> > Signed-off-by: Jeffrey Hugo 
> > ---
> >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
> > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index 4a60f5fca6b0..1347a5223918 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -702,7 +702,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> > mdelay(16);
> >
> > if (config->platform.iommu) {
> > -   aspace = msm_gem_address_space_create(>dev,
> > +   aspace = msm_gem_address_space_create(dev->dev,
> > config->platform.iommu, "mdp5");
>
> hmm, do you have a tree somewhere with your dt files?  This makes me
> think the display iommu is hooked up somewhere differently compared
> to, say, msm8916.dtsi

I'll post something somewhere and forward it to you.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 4/5] drm/msm/dsi: get the clocks into OFF state at init

[Jeffrey Hugo]

On 7/1/2019 12:58 PM, Rob Clark wrote:

On Mon, Jul 1, 2019 at 11:37 AM Jeffrey Hugo  wrote:


On 6/30/2019 9:01 AM, Rob Clark wrote:

From: Rob Clark 

Do an extra enable/disable cycle at init, to get the clks into disabled
state in case bootloader left them enabled.

In case they were already enabled, the clk_prepare_enable() has no real
effect, other than getting the enable_count/prepare_count into the right
state so that we can disable clocks in the correct order.  This way we
avoid having stuck clocks when we later want to do a modeset and set the
clock rates.

Signed-off-by: Rob Clark 
---
   drivers/gpu/drm/msm/dsi/dsi_host.c | 18 +++---
   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c |  1 +
   2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c 
b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
index aabab6311043..d0172d8db882 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
@@ -354,6 +354,7 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm 
*pll)
   if (rc)
   pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
  pll->id, status);
+rc = 0; // HACK, this will fail if PLL already running..


Umm, why?  Is this intentional?



I need to sort out a proper solution for this.. but PLL lock will fail
if the clk is already running (which, in that case, is fine since it
is already running and locked), which will cause the clk_enable to
fail..

I guess there is some way that I can check that clk is already running
and skip this check..



I'm sorry, but this makes no sense to me.  What clock are we talking 
about here?


If the pll is locked, the the lock check should just drop through.  If 
the pll cannot lock, you have an issue.  I'm confused as to how any of 
the downstream clocks can actually be running if the pll isn't locked.


I feel like we are not yet on the same page about what situation you 
seem to be in.  Can you describe in exacting detail?


___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 5/5] drm/bridge: ti-sn65dsi86: support booloader enabled display

[Jeffrey Hugo]

On 6/30/2019 9:01 AM, Rob Clark wrote:

From: Rob Clark 

Request the enable gpio ASIS to avoid disabling bridge during probe, if
already enabled.  And if already enabled, defer enabling runpm until
attach to avoid cutting off the power to the bridge.

Once we get to attach, we know panel and drm driver are probed
successfully, so at this point it i s safe to enable runpm and reset the


is?


bridge.  If we do it earlier, we kill efifb (in the case that panel or
drm driver do not probe successfully, giving the user no way to see what
is going on.


Where should the missing ")" be?


___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 3/5] drm/msm/dsi: split clk rate setting and enable

[Jeffrey Hugo]

On Sun, Jun 30, 2019 at 9:03 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> Prep work for the following patch.
>
> Signed-off-by: Rob Clark 

Reviewed-by: Jeffrey Hugo 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 2/5] genpd/gdsc: inherit display powerdomain from bootloader

[Jeffrey Hugo]

On Sun, Jun 30, 2019 at 9:02 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> Mark power domains that may be enabled by bootloader, and which should
> not be disabled until a driver takes them over.
>
> This keeps efifb alive until the real driver can be probed.  In a distro
> kernel, the driver will most likely built as a module, and not probed
> until we get to userspace (after late_initcall)
>
> Signed-off-by: Rob Clark 

Reviewed-by: Jeffrey Hugo 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 1/5] clk: inherit clocks enabled by bootloader

[Jeffrey Hugo]

On Sun, Jun 30, 2019 at 9:02 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> The goal here is to support inheriting a display setup by bootloader,
> although there may also be some non-display related use-cases.
>
> Rough idea is to add a flag for clks and power domains that might
> already be enabled when kernel starts, and which should not be
> disabled at late_initcall if the kernel thinks they are "unused".
>
> If bootloader is enabling display, and kernel is using efifb before
> real display driver is loaded (potentially from kernel module after
> userspace starts, in a typical distro kernel), we don't want to kill
> the clocks and power domains that are used by the display before
> userspace starts.
>
> Signed-off-by: Rob Clark 

Seems sane to me.  I'm curious what Stephen Boyd thinks.
I'll try to give it a spin on one of the 835 laptops.

Reviewed-by: Jeffrey Hugo 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH] drm/msm/mdp5: Add msm8998 support

[Jeffrey Hugo]

Add support for MDP5 version v3.0 found on msm8998.

Signed-off-by: Jeffrey Hugo 
---

8998 support could probably be MDP5 or DPU.  This MDP5 support works,
but may not support all of the features that 8998 supports.  However,
DPU seems to only support 845 (MDP v4.0) with fundamental assumptions
about the base level a features supported, some of which 8998 does not
infact support, so DPU would likely need significant re-writes to
support 8998.  I'm not sure the effort is worth it since MDP5 needs so
little effort to support 8998.

 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 132 ++-
 1 file changed, 128 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index dd1daf0e305a..fb4762cec4f1 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -630,7 +630,115 @@ const struct mdp5_cfg_hw msm8917_config = {
.max_clk = 32000,
 };
 
-static const struct mdp5_cfg_handler cfg_handlers[] = {
+const struct mdp5_cfg_hw msm8998_config = {
+   .name = "msm8998",
+   .mdp = {
+   .count = 1,
+   .caps = MDP_CAP_DSC |
+   MDP_CAP_CDM |
+   MDP_CAP_SRC_SPLIT |
+   0,
+   },
+   .ctl = {
+   .count = 5,
+   .base = { 0x01000, 0x01200, 0x01400, 0x01600, 0x01800 },
+   .flush_hw_mask = 0xf7ff,
+   },
+   .pipe_vig = {
+   .count = 4,
+   .base = { 0x04000, 0x06000, 0x08000, 0x0a000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_SCALE  |
+   MDP_PIPE_CAP_CSC|
+   MDP_PIPE_CAP_DECIMATION |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   0,
+   },
+   .pipe_rgb = {
+   .count = 4,
+   .base = { 0x14000, 0x16000, 0x18000, 0x1a000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_SCALE  |
+   MDP_PIPE_CAP_DECIMATION |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   0,
+   },
+   .pipe_dma = {
+   .count = 2, /* driver supports max of 2 currently */
+   .base = { 0x24000, 0x26000, 0x28000, 0x2a000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   0,
+   },
+   .pipe_cursor = {
+   .count = 2,
+   .base = { 0x34000, 0x36000 },
+   .caps = MDP_PIPE_CAP_HFLIP  |
+   MDP_PIPE_CAP_VFLIP  |
+   MDP_PIPE_CAP_SW_PIX_EXT |
+   MDP_PIPE_CAP_CURSOR |
+   0,
+   },
+
+   .lm = {
+   .count = 6,
+   .base = { 0x44000, 0x45000, 0x46000, 0x47000, 0x48000, 0x49000 
},
+   .instances = {
+   { .id = 0, .pp = 0, .dspp = 0,
+ .caps = MDP_LM_CAP_DISPLAY |
+ MDP_LM_CAP_PAIR, },
+   { .id = 1, .pp = 1, .dspp = 1,
+ .caps = MDP_LM_CAP_DISPLAY, },
+   { .id = 2, .pp = 2, .dspp = -1,
+ .caps = MDP_LM_CAP_DISPLAY |
+ MDP_LM_CAP_PAIR, },
+   { .id = 3, .pp = -1, .dspp = -1,
+ .caps = MDP_LM_CAP_WB, },
+   { .id = 4, .pp = -1, .dspp = -1,
+ .caps = MDP_LM_CAP_WB, },
+   { .id = 5, .pp = 3, .dspp = -1,
+ .caps = MDP_LM_CAP_DISPLAY, },
+},
+   .nb_stages = 8,
+   .max_width = 2560,
+   .max_height = 0x,
+   },
+   .dspp = {
+   .count = 2,
+   .base = { 0x54000, 0x56000 },
+   },
+   .ad = {
+   .count = 3,
+   .base = { 0x78000, 0x78800, 0x79000 },
+   },
+   .pp = {
+   .count = 4,
+   .base = { 0x7, 0x70800, 0x71000, 0x71800 },
+   },
+   .cdm = {
+   .count = 1,
+   .base = { 0x79200 },
+   },
+   .dsc = {
+   .count = 2,
+   .base = { 0x8, 0x80400 },
+   },
+   .intf = {
+   .base = { 0x6a000, 0x6a800, 0x6b000, 0x6b800, 0x6c000 },
+   .connect = {
+   [0] = INTF_eDP,
+   [1] = INTF_DSI,
+   [2] 

[Freedreno] [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

[Jeffrey Hugo]

Creating the msm gem address space requires a reference to the dev where
the iommu is located.  The driver currently assumes this is the same as
the platform device, which breaks when the iommu is outside of the
platform device.  Use the drm_device instead, which happens to always have
a reference to the proper device.

Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 4a60f5fca6b0..1347a5223918 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -702,7 +702,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
mdelay(16);
 
if (config->platform.iommu) {
-   aspace = msm_gem_address_space_create(>dev,
+   aspace = msm_gem_address_space_create(dev->dev,
config->platform.iommu, "mdp5");
if (IS_ERR(aspace)) {
ret = PTR_ERR(aspace);
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 1/4] dt-bindings: chosen: document panel-id binding

[Jeffrey Hugo]

On 7/1/2019 8:03 AM, Rob Herring wrote:

On Sun, Jun 30, 2019 at 2:36 PM Rob Clark  wrote:


From: Rob Clark 

The panel-id property in chosen can be used to communicate which panel,
of multiple possibilities, is installed.

Signed-off-by: Rob Clark 
---
  Documentation/devicetree/bindings/chosen.txt | 69 
  1 file changed, 69 insertions(+)


I need to update this file to say it's moved to the schema repository...

But I don't think that will matter...


diff --git a/Documentation/devicetree/bindings/chosen.txt 
b/Documentation/devicetree/bindings/chosen.txt
index 45e79172a646..d502e6489b8b 100644
--- a/Documentation/devicetree/bindings/chosen.txt
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -68,6 +68,75 @@ on PowerPC "stdout" if "stdout-path" is not found.  However, 
the
  "linux,stdout-path" and "stdout" properties are deprecated. New platforms
  should only use the "stdout-path" property.

+panel-id
+
+
+For devices that have multiple possible display panels (multi-sourcing the
+display panels is common on laptops, phones, tablets), this allows the
+bootloader to communicate which panel is installed, e.g.


How does the bootloader figure out which panel? Why can't the kernel
do the same thing?


Its platform specific.  In the devices that Rob Clark seems interested
in, there are multiple mechanisms in place - read a gpio, enable the
DSI and send a specific command to the panel controller asking for its
panel id, or read some efuses.

The efuses may not be accessible by Linux.

The DSI solution is problematic because it causes a chicken and egg
situation where linux needs the DT to probe the DSI driver to query
the panel, in order to edit the DT to probe DSI/panel.

In the systems Rob Clark is interested in, the FW already provides a
specific EFI variable with the panel id encoded in it for HLOS to use
(although this is broken on some of the devices), but this is a
specific vendor's solution.

The FW/bootloader has probably already figured out the panel details
and brought up the display for a boot splash, bios menu, etc.  I'm not
sure it makes a lot of sense to define N mechanisms for linux to
figure out the same across every platform/vendor.




+
+/ {
+   chosen {
+   panel-id = <0xc4>;
+   };
+
+   ivo_panel {
+   compatible = "ivo,m133nwf4-r0";
+   power-supply = <_3v3>;
+   no-hpd;
+
+   ports {
+   port {
+   ivo_panel_in_edp: endpoint {
+   remote-endpoint = <_out_ivo>;
+   };
+   };
+   };
+   };
+
+   boe_panel {
+   compatible = "boe,nv133fhm-n61";


Both panels are going to probe. So the bootloader needs to disable the
not populated panel setting 'status' (or delete the node). If you do
that, do you even need 'panel-id'?


+   power-supply = <_3v3>;
+   no-hpd;
+
+   ports {
+   port {
+   boe_panel_in_edp: endpoint {
+   remote-endpoint = <_out_boe>;
+   };
+   };
+   };
+   };
+
+   display_or_bridge_device {
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   ...
+
+   port@0 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0>;
+
+   endpoint@c4 {
+   reg = <0xc4>;


What does this number represent? It is supposed to be defined by the
display_or_bridge_device, not a specific panel.


Its the specific FW/bootloader defined panel id, that matches the
above defined panel-id property.



We also need to consider how the DSI case with panels as children of
the DSI controller would work and how this would work with multiple
displays each having multiple panel options.

Rob




--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.

Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH] drm/msm: Transition console to msm framebuffer

[Jeffrey Hugo]

If booting a device using EFI, efifb will likely come up and claim the
console.  When the msm display stack finally comes up, we want the
console to move over to the msm fb, so add support to kick out any
firmware based framebuffers to accomplish the console transition.

Suggested-by: Rob Clark 
Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/msm_fbdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 2429d5e6ce9f..e3836c7725a6 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -169,6 +169,9 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
if (ret)
goto fini;
 
+   /* the fw fb could be anywhere in memory */
+   drm_fb_helper_remove_conflicting_framebuffers(NULL, "msm", false);
+
ret = drm_fb_helper_initial_config(helper, 32);
if (ret)
goto fini;
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH] drm: msm: Fix add_gpu_components

[Jeffrey Hugo]

add_gpu_components() adds found GPU nodes from the DT to the match list,
regardless of the status of the nodes.  This is a problem, because if the
nodes are disabled, they should not be on the match list because they will
not be matched.  This prevents display from initing if a GPU node is
defined, but it's status is disabled.

Fix this by checking the node's status before adding it to the match list.

Fixes: dc3ea265b856 ("drm/msm: Drop the gpu binding")
Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/msm_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ab64ab470de7..4aeb84f1d874 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1279,7 +1279,8 @@ static int add_gpu_components(struct device *dev,
if (!np)
return 0;
 
-   drm_of_component_match_add(dev, matchptr, compare_of, np);
+   if (of_device_is_available(np))
+   drm_of_component_match_add(dev, matchptr, compare_of, np);
 
of_node_put(np);
 
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 5/5 v3] drm/msm/mdp5: Use the interconnect API

[Jeffrey Hugo]

On Tue, Jun 18, 2019 at 4:10 PM Rob Clark  wrote:
>
> From: Georgi Djakov 
>
> The interconnect API provides an interface for consumer drivers to
> express their bandwidth needs in the SoC. This data is aggregated
> and the on-chip interconnect hardware is configured to the most
> appropriate power/performance profile.
>
> Use the API to configure the interconnects and request bandwidth
> between DDR and the display hardware (MDP port(s) and rotator
> downscaler).
>
> v2: update the path names to be consistent with dpu, handle the NULL
> path case, updated commit msg from Georgi.
> v3: split out icc setup into it's own function, and rework logic
> slightly so no interconnect paths is not fatal.
>
> Signed-off-by: Georgi Djakov 
> Signed-off-by: Rob Clark 

Looks good to me.
Reviewed-By: Jeffrey Hugo 
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 5/5] drm/msm/mdp5: Use the interconnect API

[Jeffrey Hugo]

On Tue, Jun 18, 2019 at 2:25 PM Rob Clark  wrote:
>
> From: Georgi Djakov 
>
> The interconnect API provides an interface for consumer drivers to
> express their bandwidth needs in the SoC. This data is aggregated
> and the on-chip interconnect hardware is configured to the most
> appropriate power/performance profile.
>
> Use the API to configure the interconnects and request bandwidth
> between DDR and the display hardware (MDP port(s) and rotator
> downscaler).
>
> v2: update the path names to be consistent with dpu, handle the NULL
> path case, updated commit msg from Georgi.
>
> Signed-off-by: Georgi Djakov 
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 97179bec8902..eeac429acf40 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -16,6 +16,7 @@
>   * this program.  If not, see .
>   */
>
> +#include 
>  #include 
>
>  #include "msm_drv.h"
> @@ -1050,6 +1051,19 @@ static const struct component_ops mdp5_ops = {
>
>  static int mdp5_dev_probe(struct platform_device *pdev)
>  {
> +   struct icc_path *path0 = of_icc_get(>dev, "mdp0-mem");
> +   struct icc_path *path1 = of_icc_get(>dev, "mdp1-mem");
> +   struct icc_path *path_rot = of_icc_get(>dev, "rotator-mem");
> +
> +   if (IS_ERR_OR_NULL(path0))
> +   return PTR_ERR_OR_ZERO(path0);

Umm, am I misunderstanding something?  It seems like of_icc_get()
returns NULL if the property doesn't exist.  Won't this be backwards
incompatible?  Existing DTs won't specify the property, and I don't
believe the property is supported on all targets.  Seems like we'll
break things by not calling the below component_add() if the
interconnect is not supported, specified, or the interconnect driver
is not compiled.

> +   icc_set_bw(path0, 0, MBps_to_icc(6400));
> +
> +   if (!IS_ERR_OR_NULL(path1))
> +   icc_set_bw(path1, 0, MBps_to_icc(6400));
> +   if (!IS_ERR_OR_NULL(path_rot))
> +   icc_set_bw(path_rot, 0, MBps_to_icc(6400));
> +
> DBG("");
> return component_add(>dev, _ops);
>  }
> --
> 2.20.1
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH v3] drm/msm/adreno: Add A540 support

[Jeffrey Hugo]

The A540 is a derivative of the A530, and is found in the MSM8998 SoC.

Signed-off-by: Jeffrey Hugo 
---

v3:
-Adjusted MERCIU for A540 for best performance.

 drivers/gpu/drm/msm/adreno/a5xx.xml.h  | 28 
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 26 +++-
 drivers/gpu/drm/msm/adreno/a5xx_power.c| 76 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c | 18 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.h|  6 ++
 5 files changed, 137 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx.xml.h 
b/drivers/gpu/drm/msm/adreno/a5xx.xml.h
index cf4fe14ddd6e..4a61d4e72c98 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx.xml.h
+++ b/drivers/gpu/drm/msm/adreno/a5xx.xml.h
@@ -8,19 +8,19 @@ This file was generated by the rules-ng-ng headergen tool in 
this git repository
 git clone https://github.com/freedreno/envytools.git
 
 The rules-ng-ng source files this header was generated from are:
-- /home/robclark/src/envytools/rnndb/adreno.xml   (501 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/freedreno_copyright.xml  (   1572 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/adreno/a2xx.xml  (  42463 bytes, 
from 2018-11-19 13:44:03)
-- /home/robclark/src/envytools/rnndb/adreno/adreno_common.xml (  14201 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/adreno_pm4.xml(  43052 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/a3xx.xml  (  83840 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/adreno/a4xx.xml  ( 112086 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/adreno/a5xx.xml  ( 147240 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/a6xx.xml  ( 140790 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/a6xx_gmu.xml  (  10431 bytes, 
from 2018-09-14 13:03:07)
-- /home/robclark/src/envytools/rnndb/adreno/ocmem.xml (   1773 bytes, 
from 2018-07-03 19:37:13)
-
-Copyright (C) 2013-2018 by the following authors:
+- /home/ubuntu/envytools/envytools/rnndb/./adreno.xml (501 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/freedreno_copyright.xml  (   1572 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a2xx.xml  (  79608 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/adreno_common.xml (  14239 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/adreno_pm4.xml(  43155 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a3xx.xml  (  83840 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a4xx.xml  ( 112086 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a5xx.xml  ( 147291 
bytes, from 2019-05-29 14:51:41)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a6xx.xml  ( 148461 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a6xx_gmu.xml  (  10431 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/ocmem.xml (   1773 
bytes, from 2019-05-29 01:28:15)
+
+Copyright (C) 2013-2019 by the following authors:
 - Rob Clark  (robclark)
 - Ilia Mirkin  (imirkin)
 
@@ -2148,6 +2148,8 @@ static inline uint32_t A5XX_VSC_RESOLVE_CNTL_Y(uint32_t 
val)
 
 #define REG_A5XX_HLSQ_TIMEOUT_THRESHOLD_1  0x0e01
 
+#define REG_A5XX_HLSQ_DBG_ECO_CNTL 0x0e04
+
 #define REG_A5XX_HLSQ_ADDR_MODE_CNTL   0x0e05
 
 #define REG_A5XX_HLSQ_MODE_CNTL
0x0e06
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index e5fcefa49f19..556402f18908 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -318,12 +318,18 @@ static const struct {
 
 void a5xx_set_hwcg(struct msm_gpu *gpu, bool state)
 {
+   struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
unsigned int i;
 
for (i = 0; i < ARRAY_SIZE(a5xx_hwcg); i++)
gpu_write(gpu, a5xx_hwcg[i].offset,
state ? a5xx_hwcg[i].value : 0);
 
+   if (adreno_is_a540(adreno_gpu)) {
+   gpu_write(gpu, REG_A5XX_RBBM_CLOCK_DELAY_GPMU, state ? 
0x0770 : 0);
+   gpu_write(gpu, REG_A5XX_RBBM_CLOCK_HYST_GPMU, state ? 
0x0004 : 0);
+   }
+
gpu_write(gpu, REG_A5XX_RBBM_CLOCK_CNTL, state ? 0xAAA8AA00 : 0);
gpu_write(gpu, REG_A5XX_RBBM_ISDB_CNT, state ? 0x182 : 0x180);
 }
@@ -507,6 +513,9 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 
gpu_write(gpu, REG_A5XX_VBIF_ROUND_ROBIN_QOS_ARB, 0x0

Re: [Freedreno] [PATCH] drm/msm/adreno: Ensure that the zap shader region is big enough

[Jeffrey Hugo]

On Fri, May 31, 2019 at 4:09 PM Jordan Crouse  wrote:
>
> Before loading the zap shader we should ensure that the reserved memory
> region is big enough to hold the loaded file.
>
> Signed-off-by: Jordan Crouse 

Reviewed-by: Jeffrey Hugo 

> ---
>
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 6f7f411..3db8e49 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -67,7 +67,6 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const 
> char *fwname,
> return ret;
>
> mem_phys = r.start;
> -   mem_size = resource_size();
>
> /* Request the MDT file for the firmware */
> fw = adreno_request_fw(to_adreno_gpu(gpu), fwname);
> @@ -83,6 +82,13 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const 
> char *fwname,
> goto out;
> }
>
> +   if (mem_size > resource_size()) {
> +   DRM_DEV_ERROR(dev,
> +   "memory region is too small to load the MDT\n");
> +   ret = -E2BIG;
> +   goto out;
> +   }
> +
> /* Allocate memory for the firmware image */
> mem_region = memremap(mem_phys, mem_size,  MEMREMAP_WC);
> if (!mem_region) {
> --
> 2.7.4
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v3 2/6] drm/msm: add support for per-CRTC max_vblank_count on mdp5

[Jeffrey Hugo]

On Fri, May 31, 2019 at 3:46 AM Brian Masney  wrote:
>
> The mdp5 drm/kms driver currently does not work on command-mode DSI
> panels due to 'vblank wait timed out' errors. This causes a latency
> of seconds, or tens of seconds in some cases, before content is shown
> on the panel. This hardware does not have the something that we can use
> as a frame counter available when running in command mode, so we need to
> fall back to using timestamps by setting the max_vblank_count to zero.
> This can be done on a per-CRTC basis, so the convert mdp5 to use
> drm_crtc_set_max_vblank_count().
>
> This change was tested on a LG Nexus 5 (hammerhead) phone.
>
> Signed-off-by: Brian Masney 
> Suggested-by: Jeffrey Hugo 

Reviewed-by: Jeffrey Hugo 

> ---
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 16 +++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  |  2 +-
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> index c3751c95b452..6fde1097844f 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> @@ -450,6 +450,18 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc 
> *crtc,
> mdp5_crtc->enabled = false;
>  }
>
> +static void mdp5_crtc_vblank_on(struct drm_crtc *crtc)
> +{
> +   struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
> +   struct mdp5_interface *intf = mdp5_cstate->pipeline.intf;
> +   u32 count;
> +
> +   count = intf->mode == MDP5_INTF_DSI_MODE_COMMAND ? 0 : 0x;
> +   drm_crtc_set_max_vblank_count(crtc, count);
> +
> +   drm_crtc_vblank_on(crtc);
> +}
> +
>  static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
> struct drm_crtc_state *old_state)
>  {
> @@ -486,7 +498,7 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
> }
>
> /* Restore vblank irq handling after power is enabled */
> -   drm_crtc_vblank_on(crtc);
> +   mdp5_crtc_vblank_on(crtc);
>
> mdp5_crtc_mode_set_nofb(crtc);
>
> @@ -1039,6 +1051,8 @@ static void mdp5_crtc_reset(struct drm_crtc *crtc)
> mdp5_crtc_destroy_state(crtc, crtc->state);
>
> __drm_atomic_helper_crtc_reset(crtc, _cstate->base);
> +
> +   drm_crtc_vblank_reset(crtc);
>  }
>
>  static const struct drm_crtc_funcs mdp5_crtc_funcs = {
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 97179bec8902..fcb0b0455abe 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -750,7 +750,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> dev->driver->get_vblank_timestamp = 
> drm_calc_vbltimestamp_from_scanoutpos;
> dev->driver->get_scanout_position = mdp5_get_scanoutpos;
> dev->driver->get_vblank_counter = mdp5_get_vblank_counter;
> -   dev->max_vblank_count = 0x;
> +   dev->max_vblank_count = 0; /* max_vblank_count is set on each CRTC */
> dev->vblank_disable_immediate = true;
>
> return kms;
> --
> 2.20.1
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH 4/4] drm/msm/dsi: Add support for MSM8998 DSI controller

[Jeffrey Hugo]

The DSI controller on the MSM8998 SoC is a 6G v2.0.0 controller which is
very similar to the v2.0.1 of SDM845.

Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 21 +
 drivers/gpu/drm/msm/dsi/dsi_cfg.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c 
b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index dcdfb1bb54f9..7dd17b59c69d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -118,6 +118,25 @@ static const struct msm_dsi_config msm8996_dsi_cfg = {
.num_dsi = 2,
 };
 
+static const char * const dsi_msm8998_bus_clk_names[] = {
+   "iface", "bus", "core",
+};
+
+static const struct msm_dsi_config msm8998_dsi_cfg = {
+   .io_offset = DSI_6G_REG_SHIFT,
+   .reg_cfg = {
+   .num = 2,
+   .regs = {
+   {"vdd", 367000, 16 },   /* 0.9 V */
+   {"vdda", 62800, 2 },/* 1.2 V */
+   },
+   },
+   .bus_clk_names = dsi_msm8998_bus_clk_names,
+   .num_bus_clks = ARRAY_SIZE(dsi_msm8998_bus_clk_names),
+   .io_start = { 0xc994000, 0xc996000 },
+   .num_dsi = 2,
+};
+
 static const char * const dsi_sdm845_bus_clk_names[] = {
"iface", "bus",
 };
@@ -186,6 +205,8 @@ static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] 
= {
_dsi_cfg, _dsi_6g_host_ops},
{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_4_1,
_dsi_cfg, _dsi_6g_host_ops},
+   {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_0,
+   _dsi_cfg, _dsi_6g_v2_host_ops},
{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,
_dsi_cfg, _dsi_6g_v2_host_ops},
 };
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h 
b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
index 16c50790..4f63b57b19dc 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
@@ -25,6 +25,7 @@
 #define MSM_DSI_6G_VER_MINOR_V1_3  0x1003
 #define MSM_DSI_6G_VER_MINOR_V1_3_10x10030001
 #define MSM_DSI_6G_VER_MINOR_V1_4_10x10040001
+#define MSM_DSI_6G_VER_MINOR_V2_2_00x2000
 #define MSM_DSI_6G_VER_MINOR_V2_2_10x20020001
 
 #define MSM_DSI_V2_VER_MINOR_8064  0x0
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH 2/4] drm/msm/dsi: Add support for MSM8998 10nm dsi phy

[Jeffrey Hugo]

The MSM8998 dsi phy is 10nm v3.0.0 like SDM845, however there appear to
be minor differences such as the address space location.

Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c  |  2 ++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h  |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 18 ++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 1760483b247e..fda73749fcc0 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -507,6 +507,8 @@ static const struct of_device_id dsi_phy_dt_match[] = {
 #ifdef CONFIG_DRM_MSM_DSI_10NM_PHY
{ .compatible = "qcom,dsi-phy-10nm",
  .data = _phy_10nm_cfgs },
+   { .compatible = "qcom,dsi-phy-10nm-8998",
+ .data = _phy_10nm_8998_cfgs },
 #endif
{}
 };
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index a24ab80994a3..7161beb23b03 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -49,6 +49,7 @@ extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs;
 extern const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs;
+extern const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs;
 
 struct msm_dsi_dphy_timing {
u32 clk_pre;
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 44959e79ce28..b1e7dbc69fa6 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -221,3 +221,21 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
.io_start = { 0xae94400, 0xae96400 },
.num_dsi_phy = 2,
 };
+
+const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
+   .type = MSM_DSI_PHY_10NM,
+   .src_pll_truthtable = { {false, false}, {true, false} },
+   .reg_cfg = {
+   .num = 1,
+   .regs = {
+   {"vdds", 36000, 32},
+   },
+   },
+   .ops = {
+   .enable = dsi_10nm_phy_enable,
+   .disable = dsi_10nm_phy_disable,
+   .init = dsi_10nm_phy_init,
+   },
+   .io_start = { 0xc994400, 0xc996400 },
+   .num_dsi_phy = 2,
+};
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH 3/4] drm/msm/dsi: Add old timings quirk for 10nm phy

[Jeffrey Hugo]

The v3.0.0 10nm phy has two different implementations between MSM8998 and
SDM845, which require different timings calculations.  Unfortunately, the
hardware designers did not choose to revise the version to account for this
delta so implement a quirk instead.

Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h  |  4 
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 12 +---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index 7161beb23b03..3c51df1aa2ee 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -21,6 +21,9 @@
 #define dsi_phy_read(offset) msm_readl((offset))
 #define dsi_phy_write(offset, data) msm_writel((data), (offset))
 
+/* v3.0.0 10nm implementation that requires the old timings settings */
+#define V3_0_0_10NM_OLD_TIMINGS_QUIRK  BIT(0)
+
 struct msm_dsi_phy_ops {
int (*init) (struct msm_dsi_phy *phy);
int (*enable)(struct msm_dsi_phy *phy, int src_pll_id,
@@ -41,6 +44,7 @@ struct msm_dsi_phy_cfg {
bool src_pll_truthtable[DSI_MAX][DSI_MAX];
const resource_size_t io_start[DSI_MAX];
const int num_dsi_phy;
+   const int quirks;
 };
 
 extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs;
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 b1e7dbc69fa6..eb28937f4b34 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -42,6 +42,9 @@ static void dsi_phy_hw_v3_0_lane_settings(struct msm_dsi_phy 
*phy)
u8 tx_dctrl[] = { 0x00, 0x00, 0x00, 0x04, 0x01 };
void __iomem *lane_base = phy->lane_base;
 
+   if (phy->cfg->quirks & V3_0_0_10NM_OLD_TIMINGS_QUIRK)
+   tx_dctrl[3] = 0x02;
+
/* Strength ctrl settings */
for (i = 0; i < 5; i++) {
dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_LPTX_STR_CTRL(i),
@@ -74,9 +77,11 @@ static void dsi_phy_hw_v3_0_lane_settings(struct msm_dsi_phy 
*phy)
  tx_dctrl[i]);
}
 
-   /* Toggle BIT 0 to release freeze I/0 */
-   dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 0x05);
-   dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 0x04);
+   if (!phy->cfg->quirks & V3_0_0_10NM_OLD_TIMINGS_QUIRK) {
+   /* Toggle BIT 0 to release freeze I/0 */
+   dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 
0x05);
+   dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(3), 
0x04);
+   }
 }
 
 static int dsi_10nm_phy_enable(struct msm_dsi_phy *phy, int src_pll_id,
@@ -238,4 +243,5 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
},
.io_start = { 0xc994400, 0xc996400 },
.num_dsi_phy = 2,
+   .quirks = V3_0_0_10NM_OLD_TIMINGS_QUIRK,
 };
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH 1/4] dt-bindings: msm/dsi: Add 10nm phy for msm8998 compatible

[Jeffrey Hugo]

The DSI phy on MSM8998 is a 10nm design like SDM845, however it has some
slightly different quirks which need to be handled by drivers.  Provide
a separate compatible to assist in handling the specifics.

Signed-off-by: Jeffrey Hugo 
---
 Documentation/devicetree/bindings/display/msm/dsi.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
b/Documentation/devicetree/bindings/display/msm/dsi.txt
index 9ae946942720..af95586c898f 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -88,6 +88,7 @@ Required properties:
   * "qcom,dsi-phy-28nm-8960"
   * "qcom,dsi-phy-14nm"
   * "qcom,dsi-phy-10nm"
+  * "qcom,dsi-phy-10nm-8998"
 - reg: Physical base address and length of the registers of PLL, PHY. Some
   revisions require the PHY regulator base address, whereas others require the
   PHY lane base address. See below for each PHY revision.
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH 0/4] MSM8998 DSI support

[Jeffrey Hugo]

Enabling DSI support for the MSM8998 SoC is another step to getting end
to end display going.  This will allow the SoC to drive panels that are
integraded on the device (ie not a HDMI port), but won't do much until
we have the display processor feeding the DSI blocks with lines to
scanout.

Jeffrey Hugo (4):
  dt-bindings: msm/dsi: Add 10nm phy for msm8998 compatible
  drm/msm/dsi: Add support for MSM8998 10nm dsi phy
  drm/msm/dsi: Add old timings quirk for 10nm phy
  drm/msm/dsi: Add support for MSM8998 DSI controller

 .../devicetree/bindings/display/msm/dsi.txt   |  1 +
 drivers/gpu/drm/msm/dsi/dsi_cfg.c | 21 +
 drivers/gpu/drm/msm/dsi/dsi_cfg.h |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c |  2 ++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h |  5 
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c| 30 +--
 6 files changed, 57 insertions(+), 3 deletions(-)

-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH RFC v2 0/6] ARM: qcom: initial Nexus 5 display support

[Jeffrey Hugo]

On 5/29/2019 1:30 PM, Brian Masney wrote:

On Wed, May 29, 2019 at 08:41:31AM -0600, Jeffrey Hugo wrote:

On Wed, May 29, 2019 at 4:28 AM Brian Masney  wrote:


On Tue, May 28, 2019 at 08:53:49PM -0600, Jeffrey Hugo wrote:

On Tue, May 28, 2019 at 8:46 PM Brian Masney  wrote:


On Tue, May 28, 2019 at 07:42:19PM -0600, Jeffrey Hugo wrote:

Do you know if the nexus 5 has a video or command mode panel?  There
is some glitchyness with vblanks and command mode panels.


Its in command mode. I know this because I see two 'pp done time out'
messages, even on 4.17. Based on my understanding, the ping pong code is
only applicable for command mode panels.


Actually, the ping pong element exists in both modes, but 'pp done
time out' is a good indicator that it is command mode.

Are you also seeing vblank timeouts?


Yes, here's a snippet of the first one.

[2.556014] WARNING: CPU: 0 PID: 5 at 
drivers/gpu/drm/drm_atomic_helper.c:1429 
drm_atomic_helper_wait_for_vblanks.part.1+0x288/0x290
[2.556020] [CRTC:49:crtc-0] vblank wait timed out
[2.556023] Modules linked in:
[2.556034] CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 
5.2.0-rc1-00178-g72c3c1fd5f86-dirty #426
[2.556038] Hardware name: Generic DT based system
[2.556056] Workqueue: events deferred_probe_work_func
...


Do you have busybox?

Can you run -
sudo busybox devmem 0xFD900614
sudo busybox devmem 0xFD900714
sudo busybox devmem 0xFD900814
sudo busybox devmem 0xFD900914
sudo busybox devmem 0xFD900A14


# busybox devmem 0xFD900614
0x00020020


Ok, so CTL_0 path, command mode, ping pong 0, with the output going to DSI 1.

Next one please:

busybox devmem 0xFD912D30


It's 0x on mainline and 4.17. I used the following script to
dump the entire mdp5 memory region and attached the dump from 4.17 and
5.2rc1.



ok, 0 means autorefresh is not on.  Which is fine.  My next guess
would be the vblank code checking the hardware vblank counter, which
doesn't exist.
In video mode, there is a frame counter which increments, which can be
used as the vblank counter.  Unfortunately, that hardware isn't active
in command mode, and there isn't an equivalent.

So, the vblank code is going to read the register, and look for an
update, which will never happen, thus it will timeout.  There is a
backup path which uses timestamps (no hardware), which you can
activate with a quick hack - make max_vblank_count = 0 at the
following line
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c#L753


That fixed the issue!


Awesome.  I'm glad it was something simple.



I previously observed that mdp5_get_vblank_counter, specifically
mdp5_encoder_get_framecount, would always return 0.

What's the best way to fix this in mainline? Set that to zero if any
of the interface modes is MDP5_INTF_DSI_MODE_COMMAND?



Short version, yes.  Long version:

I still have that hack in my tree and haven't come back to formulating
a proper fix yet.  Feel free to run with it.

Thinking about it briefly, we could do two things.  We could fake a
hardware counter by just increment an int every time the vblank irq is
processed, but that seems clunky.  Otherwise, we could force a
fallback onto the timestamp solution, which seems less invasive.

In theory, we could service multiple displays, with different
properties (ie a combination of command and video mode).  The hack
then, is not good, because it would break video mode (at-least we
wouldn't be using the register when we could).  It would be great if
the use of the hardware register could be done per display.

Luckily, it looks like someone just made that possible -
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/drm_vblank.c?h=v5.2-rc2=ed20151a7699bb2c77eba3610199789a126940c4

--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.

Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH v2 2/2] drm/msm/adreno: Add A540 support

[Jeffrey Hugo]

The A540 is a derivative of the A530, and is found in the MSM8998 SoC.

Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/adreno/a5xx.xml.h  | 28 
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 21 ++
 drivers/gpu/drm/msm/adreno/a5xx_power.c| 76 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c | 18 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.h|  6 ++
 5 files changed, 133 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx.xml.h 
b/drivers/gpu/drm/msm/adreno/a5xx.xml.h
index cf4fe14ddd6e..4a61d4e72c98 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx.xml.h
+++ b/drivers/gpu/drm/msm/adreno/a5xx.xml.h
@@ -8,19 +8,19 @@ This file was generated by the rules-ng-ng headergen tool in 
this git repository
 git clone https://github.com/freedreno/envytools.git
 
 The rules-ng-ng source files this header was generated from are:
-- /home/robclark/src/envytools/rnndb/adreno.xml   (501 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/freedreno_copyright.xml  (   1572 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/adreno/a2xx.xml  (  42463 bytes, 
from 2018-11-19 13:44:03)
-- /home/robclark/src/envytools/rnndb/adreno/adreno_common.xml (  14201 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/adreno_pm4.xml(  43052 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/a3xx.xml  (  83840 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/adreno/a4xx.xml  ( 112086 bytes, 
from 2018-07-03 19:37:13)
-- /home/robclark/src/envytools/rnndb/adreno/a5xx.xml  ( 147240 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/a6xx.xml  ( 140790 bytes, 
from 2018-12-02 17:29:54)
-- /home/robclark/src/envytools/rnndb/adreno/a6xx_gmu.xml  (  10431 bytes, 
from 2018-09-14 13:03:07)
-- /home/robclark/src/envytools/rnndb/adreno/ocmem.xml (   1773 bytes, 
from 2018-07-03 19:37:13)
-
-Copyright (C) 2013-2018 by the following authors:
+- /home/ubuntu/envytools/envytools/rnndb/./adreno.xml (501 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/freedreno_copyright.xml  (   1572 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a2xx.xml  (  79608 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/adreno_common.xml (  14239 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/adreno_pm4.xml(  43155 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a3xx.xml  (  83840 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a4xx.xml  ( 112086 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a5xx.xml  ( 147291 
bytes, from 2019-05-29 14:51:41)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a6xx.xml  ( 148461 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/a6xx_gmu.xml  (  10431 
bytes, from 2019-05-29 01:28:15)
+- /home/ubuntu/envytools/envytools/rnndb/adreno/ocmem.xml (   1773 
bytes, from 2019-05-29 01:28:15)
+
+Copyright (C) 2013-2019 by the following authors:
 - Rob Clark  (robclark)
 - Ilia Mirkin  (imirkin)
 
@@ -2148,6 +2148,8 @@ static inline uint32_t A5XX_VSC_RESOLVE_CNTL_Y(uint32_t 
val)
 
 #define REG_A5XX_HLSQ_TIMEOUT_THRESHOLD_1  0x0e01
 
+#define REG_A5XX_HLSQ_DBG_ECO_CNTL 0x0e04
+
 #define REG_A5XX_HLSQ_ADDR_MODE_CNTL   0x0e05
 
 #define REG_A5XX_HLSQ_MODE_CNTL
0x0e06
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index e5fcefa49f19..13652863103f 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -318,12 +318,18 @@ static const struct {
 
 void a5xx_set_hwcg(struct msm_gpu *gpu, bool state)
 {
+   struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
unsigned int i;
 
for (i = 0; i < ARRAY_SIZE(a5xx_hwcg); i++)
gpu_write(gpu, a5xx_hwcg[i].offset,
state ? a5xx_hwcg[i].value : 0);
 
+   if (adreno_is_a540(adreno_gpu)) {
+   gpu_write(gpu, REG_A5XX_RBBM_CLOCK_DELAY_GPMU, state ? 
0x0770 : 0);
+   gpu_write(gpu, REG_A5XX_RBBM_CLOCK_HYST_GPMU, state ? 
0x0004 : 0);
+   }
+
gpu_write(gpu, REG_A5XX_RBBM_CLOCK_CNTL, state ? 0xAAA8AA00 : 0);
gpu_write(gpu, REG_A5XX_RBBM_ISDB_CNT, state ? 0x182 : 0x180);
 }
@@ -507,6 +513,9 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 
gpu_write(gpu, REG_A5XX_VBIF_ROUND_ROBIN_QOS_ARB, 0x0003);
 
+   if (adreno_is_a540(adreno_gpu))
+   gpu_write(

[Freedreno] [PATCH v2 1/2] a5xx: Define HLSQ_DBG_ECO_CNTL

[Jeffrey Hugo]

---
 rnndb/adreno/a5xx.xml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rnndb/adreno/a5xx.xml b/rnndb/adreno/a5xx.xml
index ae654eeb..16203512 100644
--- a/rnndb/adreno/a5xx.xml
+++ b/rnndb/adreno/a5xx.xml
@@ -1523,6 +1523,7 @@ xsi:schemaLocation="http://nouveau.freedesktop.org/ 
rules-ng.xsd">
 


+   

 

-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH v2 0/2] Adreno A540 support

[Jeffrey Hugo]

Adreno driver support for the A540 found in the MSM8998 SoC

v2:
-Removed extra RBBM write
-Corrected added RBBM writes to allow for hwcg disable
-Patch to add REG_A5XX_HLSQ_DBG_ECO_CNTL to envytools
-Regenerated a5xx header file with updated envytools
-Used REG_A5XX_HLSQ_DBG_ECO_CNTL in code
-Stripped extranious magic number comment

Jeffrey Hugo (1):
  drm/msm/adreno: Add A540 support

 drivers/gpu/drm/msm/adreno/a5xx.xml.h  | 28 
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 21 ++
 drivers/gpu/drm/msm/adreno/a5xx_power.c| 76 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c | 18 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.h|  6 ++
 5 files changed, 133 insertions(+), 16 deletions(-)

-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH RFC v2 0/6] ARM: qcom: initial Nexus 5 display support

[Jeffrey Hugo]

On Tue, May 28, 2019 at 8:15 PM Rob Clark  wrote:
>
> On Tue, May 28, 2019 at 6:17 PM Brian Masney  wrote:
> >
> > On Tue, May 28, 2019 at 03:46:14PM +0200, Linus Walleij wrote:
> > > On Thu, May 9, 2019 at 4:04 AM Brian Masney  wrote:
> > >
> > > > Here is a patch series that adds initial display support for the LG
> > > > Nexus 5 (hammerhead) phone. It's not fully working so that's why some
> > > > of these patches are RFC until we can get it fully working.
> > > >
> > > > The phones boots into terminal mode, however there is a several second
> > > > (or more) delay when writing to tty1 compared to when the changes are
> > > > actually shown on the screen. The following errors are in dmesg:
> > >
> > > I tested to apply patches 2-6 and got the console up on the phone as well.
> > > I see the same timouts, and I also notice the update is slow in the
> > > display, as if the DSI panel was running in low power (LP) mode.
> > >
> > > Was booting this to do some other work, but happy to see the progress!
> >
> > Thanks!
> >
> > I've had three people email me off list regarding the display working on
> > 4.17 before the msm kms/drm driver was converted to the DRM atomic API so
> > this email is to get some more information out publicly.
> >
> > I pushed up a branch to my github with 15 patches applied against 4.17
> > that has a working display:
> >
> > https://github.com/masneyb/linux/commits/display-works-4.17
> >
> > It's in low speed mode but its usable. The first 10 patches are in
> > mainline now and the last 5 are in essence this patch series with the
> > exception of 'drm/atomic+msm: add helper to implement legacy dirtyfb'.
> > There's a slightly different version of that patch in mainline now.
> >
> > I'm planning to work on the msm8974 interconnect support once some of
> > the outstanding interconnect patches for the msm kms/drm driver arrive
> > in mainline. I'd really like to understand why the display works on
> > 4.17 with those patches though. I assume that it's related to the
> > vblank events not working properly? Let me preface this with I'm a
> > total DRM newbie, but it looked like the pre-DRM-atomic driver wasn't
> > looking for these events in the atomic commits before the migration?
> > See commit 70db18dca4e0 ("drm/msm: Remove msm_commit/worker, use atomic
> > helper commit"), specifically the drm_atomic_helper_wait_for_vblanks()
> > call that was added.
>
> interconnect probably good to get going anyways (and I need to find
> some time to respin those mdp5/dpu patches) but I guess not related to
> what you see (ie. I'd expect interconnect issue would trigger
> underflow irq's)..
>
> I'm not entirely sure why atomic would break things but cmd mode
> panels aren't especially well tested.  I can't find it now but there
> was a thread (or IRC discussion?) that intf2vblank() should be
> returning MDP5_IRQ_PING_PONG__DONE instead of
> MDP5_IRQ_PING_PONG__RD_PTR, which seems likely and possibly related
> to vblank timing issues..

That was an irc discussion, and I've changed my mind on that.

My big issue ended up being that autorefresh was enabled (which
basically turns the command panel into a pseudo video mode panel),
which appears incompatible with using the start kick.  If FW is
configuring things in autorefresh mode, the driver likely doesn't
know, and will make a mess of things by using the start kick.

Disabling autorefresh would make the driver work as is, but fbcon
wouldn't work well (you'd need to do a start kick per frame, which
doesn't seem to happen).  Removing the start kick and using the
autorefresh feature seemed better in my testing, but I haven't cleaned
up my code tree to send something up.

However, lets see how the hardware is actually configured in Brian's case.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH RFC v2 0/6] ARM: qcom: initial Nexus 5 display support

[Jeffrey Hugo]

On Tue, May 28, 2019 at 7:37 PM Brian Masney  wrote:
>
> On Tue, May 28, 2019 at 07:32:14PM -0600, Jeffrey Hugo wrote:
> > On Tue, May 28, 2019 at 7:17 PM Brian Masney  wrote:
> > >
> > > On Tue, May 28, 2019 at 03:46:14PM +0200, Linus Walleij wrote:
> > > > On Thu, May 9, 2019 at 4:04 AM Brian Masney  
> > > > wrote:
> > > >
> > > > > Here is a patch series that adds initial display support for the LG
> > > > > Nexus 5 (hammerhead) phone. It's not fully working so that's why some
> > > > > of these patches are RFC until we can get it fully working.
> > > > >
> > > > > The phones boots into terminal mode, however there is a several second
> > > > > (or more) delay when writing to tty1 compared to when the changes are
> > > > > actually shown on the screen. The following errors are in dmesg:
> > > >
> > > > I tested to apply patches 2-6 and got the console up on the phone as 
> > > > well.
> > > > I see the same timouts, and I also notice the update is slow in the
> > > > display, as if the DSI panel was running in low power (LP) mode.
> > > >
> > > > Was booting this to do some other work, but happy to see the progress!
> > >
> > > Thanks!
> > >
> > > I've had three people email me off list regarding the display working on
> > > 4.17 before the msm kms/drm driver was converted to the DRM atomic API so
> > > this email is to get some more information out publicly.
> > >
> > > I pushed up a branch to my github with 15 patches applied against 4.17
> > > that has a working display:
> > >
> > > https://github.com/masneyb/linux/commits/display-works-4.17
> > >
> > > It's in low speed mode but its usable. The first 10 patches are in
> > > mainline now and the last 5 are in essence this patch series with the
> > > exception of 'drm/atomic+msm: add helper to implement legacy dirtyfb'.
> > > There's a slightly different version of that patch in mainline now.
> > >
> > > I'm planning to work on the msm8974 interconnect support once some of
> > > the outstanding interconnect patches for the msm kms/drm driver arrive
> > > in mainline. I'd really like to understand why the display works on
> > > 4.17 with those patches though. I assume that it's related to the
> > > vblank events not working properly? Let me preface this with I'm a
> > > total DRM newbie, but it looked like the pre-DRM-atomic driver wasn't
> > > looking for these events in the atomic commits before the migration?
> > > See commit 70db18dca4e0 ("drm/msm: Remove msm_commit/worker, use atomic
> > > helper commit"), specifically the drm_atomic_helper_wait_for_vblanks()
> > > call that was added.
> >
> > Do you know if the nexus 5 has a video or command mode panel?  There
> > is some glitchyness with vblanks and command mode panels.
>
> Its in command mode. I know this because I see two 'pp done time out'
> messages, even on 4.17. Based on my understanding, the ping pong code is
> only applicable for command mode panels.

Actually, the ping pong element exists in both modes, but 'pp done
time out' is a good indicator that it is command mode.

Are you also seeing vblank timeouts?

Do you have busybox?

Can you run -
sudo busybox devmem 0xFD900614
sudo busybox devmem 0xFD900714
sudo busybox devmem 0xFD900814
sudo busybox devmem 0xFD900914
sudo busybox devmem 0xFD900A14
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH RFC v2 0/6] ARM: qcom: initial Nexus 5 display support

[Jeffrey Hugo]

On Tue, May 28, 2019 at 7:17 PM Brian Masney  wrote:
>
> On Tue, May 28, 2019 at 03:46:14PM +0200, Linus Walleij wrote:
> > On Thu, May 9, 2019 at 4:04 AM Brian Masney  wrote:
> >
> > > Here is a patch series that adds initial display support for the LG
> > > Nexus 5 (hammerhead) phone. It's not fully working so that's why some
> > > of these patches are RFC until we can get it fully working.
> > >
> > > The phones boots into terminal mode, however there is a several second
> > > (or more) delay when writing to tty1 compared to when the changes are
> > > actually shown on the screen. The following errors are in dmesg:
> >
> > I tested to apply patches 2-6 and got the console up on the phone as well.
> > I see the same timouts, and I also notice the update is slow in the
> > display, as if the DSI panel was running in low power (LP) mode.
> >
> > Was booting this to do some other work, but happy to see the progress!
>
> Thanks!
>
> I've had three people email me off list regarding the display working on
> 4.17 before the msm kms/drm driver was converted to the DRM atomic API so
> this email is to get some more information out publicly.
>
> I pushed up a branch to my github with 15 patches applied against 4.17
> that has a working display:
>
> https://github.com/masneyb/linux/commits/display-works-4.17
>
> It's in low speed mode but its usable. The first 10 patches are in
> mainline now and the last 5 are in essence this patch series with the
> exception of 'drm/atomic+msm: add helper to implement legacy dirtyfb'.
> There's a slightly different version of that patch in mainline now.
>
> I'm planning to work on the msm8974 interconnect support once some of
> the outstanding interconnect patches for the msm kms/drm driver arrive
> in mainline. I'd really like to understand why the display works on
> 4.17 with those patches though. I assume that it's related to the
> vblank events not working properly? Let me preface this with I'm a
> total DRM newbie, but it looked like the pre-DRM-atomic driver wasn't
> looking for these events in the atomic commits before the migration?
> See commit 70db18dca4e0 ("drm/msm: Remove msm_commit/worker, use atomic
> helper commit"), specifically the drm_atomic_helper_wait_for_vblanks()
> call that was added.

Do you know if the nexus 5 has a video or command mode panel?  There
is some glitchyness with vblanks and command mode panels.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/adreno: Add A540 support

[Jeffrey Hugo]

On Tue, May 28, 2019 at 2:45 PM Jordan Crouse  wrote:
>
> On Tue, May 28, 2019 at 10:06:12AM -0700, Jeffrey Hugo wrote:
> > The A540 is a derivative of the A530, and is found in the MSM8998 SoC.
> >
> > Signed-off-by: Jeffrey Hugo 
> > ---
> >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 22 +++
> >  drivers/gpu/drm/msm/adreno/a5xx_power.c| 76 +-
> >  drivers/gpu/drm/msm/adreno/adreno_device.c | 18 +
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.h|  6 ++
> >  4 files changed, 119 insertions(+), 3 deletions(-)
>
> This is awesome.  I'm glad we can finally check this off the list.

Thanks for the quick review.

>
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
> > b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > index e5fcefa49f19..7ca8fde22fd8 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > @@ -318,12 +318,19 @@ static const struct {
> >
> >  void a5xx_set_hwcg(struct msm_gpu *gpu, bool state)
> >  {
> > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >   unsigned int i;
> >
> >   for (i = 0; i < ARRAY_SIZE(a5xx_hwcg); i++)
> >   gpu_write(gpu, a5xx_hwcg[i].offset,
> >   state ? a5xx_hwcg[i].value : 0);
> >
> > + if (adreno_is_a540(adreno_gpu)) {
> > + gpu_write(gpu, REG_A5XX_RBBM_CLOCK_HYST_GPMU, 0x0222);
>
> I don't think this one is needed - we can just go with the one two lines 
> below.

I'll try without it, and see.

>
> > + gpu_write(gpu, REG_A5XX_RBBM_CLOCK_DELAY_GPMU, 0x0770);
> > + gpu_write(gpu, REG_A5XX_RBBM_CLOCK_HYST_GPMU, 0x0004);
>
> Both of these need the state ?  : 0x0 hack to turn off HWCG if 
> requested

I see.  Will do.

>
> > + }
> > +
> >   gpu_write(gpu, REG_A5XX_RBBM_CLOCK_CNTL, state ? 0xAAA8AA00 : 0);
> >   gpu_write(gpu, REG_A5XX_RBBM_ISDB_CNT, state ? 0x182 : 0x180);
> >  }
> > @@ -507,6 +514,9 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >
> >   gpu_write(gpu, REG_A5XX_VBIF_ROUND_ROBIN_QOS_ARB, 0x0003);
> >
> > + if (adreno_is_a540(adreno_gpu))
> > + gpu_write(gpu, REG_A5XX_VBIF_GATE_OFF_WRREQ_EN, 0x0009);
> > +
> >   /* Make all blocks contribute to the GPU BUSY perf counter */
> >   gpu_write(gpu, REG_A5XX_RBBM_PERFCTR_GPU_BUSY_MASKED, 0x);
> >
> > @@ -592,6 +602,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >   /* Set the highest bank bit */
> >   gpu_write(gpu, REG_A5XX_TPL1_MODE_CNTL, 2 << 7);
> >   gpu_write(gpu, REG_A5XX_RB_MODE_CNTL, 2 << 1);
> > + if (adreno_is_a540(adreno_gpu))
> > + gpu_write(gpu, REG_A5XX_UCHE_DBG_ECO_CNTL_2, 2);
> >
> >   /* Protect registers from the CP */
> >   gpu_write(gpu, REG_A5XX_CP_PROTECT_CNTL, 0x0007);
> > @@ -642,6 +654,16 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >   REG_A5XX_RBBM_SECVID_TSB_TRUSTED_BASE_HI, 0x);
> >   gpu_write(gpu, REG_A5XX_RBBM_SECVID_TSB_TRUSTED_SIZE, 0x);
> >
> > + /*
> > +  * VPC corner case with local memory load kill leads to corrupt
> > +  * internal state. Normal Disable does not work for all a5x chips.
> > +  * So do the following setting to disable it.
> > +  */
> > + if (adreno_gpu->info->quirks & ADRENO_QUIRK_LMLOADKILL_DISABLE) {
>
> This is a5xx only quirk but it applies to multiple 5xx targets so I think its
> reasonable to identify it as a quirk and use it as such to make it easier on
> future code porters.
>
> > + gpu_rmw(gpu, REG_A5XX_VPC_DBG_ECO_CNTL, 0, BIT(23));
> > + gpu_rmw(gpu, 0xE04, BIT(18), 0); 
> > /*REG_A5XX_HLSQ_DBG_ECO_CNTL*/
>
> You should define and use REG_A5XX_HLSQ_DBG_ECO_CNTL symbolically. We can help
> you do the envytools dance if you need.

Thanks for the offline explanation.  Will do

>
> > + }
> > +
> >   ret = adreno_hw_init(gpu);
> >   if (ret)
> >   return ret;
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c 
> > b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > index 70e65c94e525..5cb325c6eb8f 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > @@ -32,6 +32,18 @@
> >  #define AGC_POWER_CONFIG_PRODUCTION_ID 1
> >  #define AGC_INIT_MSG_VALUE 0xBABEFACE
> >
> > +/* AGC_LM_CONFIG (A540+) */
> > +#define AGC_LM_

[Freedreno] [PATCH RESEND] drm/msm/mdp5: Fix mdp5_cfg_init error return

[Jeffrey Hugo]

If mdp5_cfg_init fails because of an unknown major version, a null pointer
dereference occurs.  This is because the caller of init expects error
pointers, but init returns NULL on error.  Fix this by returning the
expected values on error.

Fixes: 2e362e1772b8 (drm/msm/mdp5: introduce mdp5_cfg module)
Signed-off-by: Jeffrey Hugo 
Reviewed-by: Bjorn Andersson 
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index ea8f7d7daf7f..52e23780fce1 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -721,7 +721,7 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms 
*mdp5_kms,
if (cfg_handler)
mdp5_cfg_destroy(cfg_handler);
 
-   return NULL;
+   return ERR_PTR(ret);
 }
 
 static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH] drm/msm/mdp5: Fix mdp5_cfg_init error return

[Jeffrey Hugo]

If mdp5_cfg_init fails because of an unknown major version, a null pointer
dereference occurs.  This is because the caller of init expects error
pointers, but init returns NULL on error.  Fix this by returning the
expected values on error.

Fixes: 2e362e1772b8 (drm/msm/mdp5: introduce mdp5_cfg module)
Signed-off-by: Jeffrey Hugo 
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index ea8f7d7daf7f..52e23780fce1 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -721,7 +721,7 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms 
*mdp5_kms,
if (cfg_handler)
mdp5_cfg_destroy(cfg_handler);
 
-   return NULL;
+   return ERR_PTR(ret);
 }
 
 static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
-- 
2.17.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm: Fix incorrect struct size for memory allocation

[Jeffrey Hugo]

On 2/21/2019 2:54 PM, Jordan Crouse wrote:

The allocation for the clock bulk data does a classic sizeof(pointer)
instead of sizeof(struct) so the array ends up incorrectly sized
for the clock data.

Cc: sta...@vger.kernel.org
Fixes: 8e54eea ("drm/msm: Add a helper function to parse clock names")
Signed-off-by: Jordan Crouse 
---

  drivers/gpu/drm/msm/msm_drv.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 906b2bb..31e1481 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -96,7 +96,7 @@ int msm_clk_bulk_get(struct device *dev, struct clk_bulk_data 
**bulk)
if (count < 1)
return 0;
  
-	local = devm_kcalloc(dev, sizeof(struct clk_bulk_data *),

+   local = devm_kcalloc(dev, sizeof(struct clk_bulk_data),
count, GFP_KERNEL);
if (!local)
return -ENOMEM;



Isn't msm_clk_bulk_get a duplication of devm_clk_bulk_get_all() ? 
Surely it would be better to just use that instead?


--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.

Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno