[RFC] drm/amd/display: add SI support to AMD DC

Tue Oct 16 14:48:01 UTC 2018

Hi Harry,
in this thread some of the coding changes with open points,
as a checklist/track for review of upcoming (v2) patches.
M.

On Mon, Oct 15, 2018 at 11:06 PM Harry Wentland <harry.wentland at amd.com> wrote:
>
> On 2018-10-14 5:47 p.m., Mauro Rossi wrote:
> > Hi,
> >
> > reporting about some progress made during the weekend,
> > thanks to Sylvain feedback & suggestions.
> >
> > I have rebased and updated the series on top of
> > https://cgit.freedesktop.org/~agd5f/linux/?h=amd-staging-drm-next
> >
> > Here is the amd_dc_si branch:
> > https://github.com/maurossi/linux/tree/amd_dc_si (uploading)
> > NOTE: arch/x86/kernel/tsc.c changes for 4K display modes are not
> > there, as they are not strictly needed for amd-gfx
> >
> > Copying also Harry, Alex, Christian and Mike in order to get some
> > objective and infallible
> > clues/feedbacks about blocking points and about "no care" items.
> >
> > Please, also big things I may have missed.
> > M.
> >
> >> On Mon, Oct 8, 2018 at 11:23 PM <sylvain.bertrand at gmail.com> wrote:
> >>
> >> Sylvain - I did hack a bit your patch set on amd-staging-drm-next to make it go through the
> >> asic init and I managed to get a x11 display with lines kind of garbled, but
> >> you can still understand easily what's on the screen.
> >
> > I forgot to mention that since I'm gorgeously trying AMD DC also on Mullins
> > I have reverted d9fda24 (""drm/amdgpu: Don't default to DC support for
> > Kaveri and older")
> > because on Mullins I can boot with HDMI and HDMI-to-VGA converter
> >
> > I was hoping for AMD DC being re-enabled for Kaveri and older,
> > but I'm available to submit new version of specific patch if required.
> >
>
> I still need to find time to get through your patchset properly. Just a quick note on this. There are Kabini/Kaveri ASICs with VGA connectors in the market, which the DC code doesn't support. If someone writes it we can re-enable it by default.
>
> Either way, you can revert that patch for your tree or use amdgpu.dc=0 as long as you're aware that VGA won't work with amdgpu on such a kernel.
>
> Harry

At the moment I have reverted commit disabling DC for Kaveri, as I'm
using Acer ES1-521 which has HDMI output (with an HDMI-to-VGA
converter)

If it's not too complex, I could give it a try to have Kaveri working
for laptops with VGA output,
but from your judgment how complex is this task and which code paths
need to be touched?

If it is achievable by me, why not?

>
> >> Sylvain - ... The lines may be garbled in your driver code because,
> >> if I recall properly, "line buffer" programing in dce8 is not
> >> the same than in dce6 (look for registers with the "LB" abbreviation). Or some
> >> slight differences in frame buffer tiling.
> >
> > So the problem could be related to some kind of scan line or tiling
> > buffer issue,
> > at the moment the dce_resouces model is grabbed "AS IS" from DCE8
> > registers/masks
> >

I will check visually this evening the x11 line artifacts on HD7750 Cape Verde,
do you have any suggestion on top of Sylvain B. analysis?

> >>
> >> Sylvain - I checked the kernel log, and like you said, I got errors in DM_PPLIB due to an
> >> invalid powerlevel and atombios/vbios table parsing regarding connectors.
> >> general dpm is in amdgpu(no DC) for SI, it means the DCE related dpm part in
> >> current SI amdgpu code path should be "copied" in DC. It is related very
> >> probably to the parsing of VBIOS/ATOMBIOS tables.
> >
> > 10-09 21:10:14.427     0     0 E         :
> > [drm:dm_pp_get_static_clocks [amdgpu]] *ERROR* DM_PPLIB: invalid
> > powerlevel state: 0!
> >
> > NOTE: the error is the result of Powerplay dependency introduced by
> > using AMD DC for SI
> > it's not fatal and it does not seem to affect performance in the Benchmarks
> >
> > DOUBT: I think that it would make sense to set "power level 0" i.e.
> > the "lower state" as safe default,
> > considering that powerplay smu6/hwmgr are not implemented for SI and
> > smu7 CIK functions do not work,
> > the AS-IS dpm is the only available option. (and it seams to be
> > working, looking at the framerates 250-280 in the V1 Vulkan benchmark)
> >
> >
> >
> >> 10-09 21:10:14.427     0     0 W [drm] dce110_link_encoder_construct: Failed to get encoder_cap_info from VBIOS with error code 4!
> >> 10-09 21:10:14.427     0     0 W [drm] dce110_link_encoder_construct: Failed to get encoder_cap_info from VBIOS with error code 4!
> >
> > NOTE: the warning also appears with Tonga and Vega, it is a Warning
> > and does not seem to cause issues, so I would assume there is a
> > default treatment in place,
> > is this related to missing encoder for drm crtc or to other kind of encoder?
> >

Are these Warnings a problem or just Warnings with mid-low severity?

> >> Sylvain - Did add SI handling in some raven firmware loader function.
> >> In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, "load_dmcu_fw"
> >> function augmented with SI chip asic_type.
> >
> > I've merged the change in the (v2) branch
> > https://github.com/maurossi/linux/tree/amd_dc_si
> >
> >> Sylvain - AFAIK, the real thing that you additionally get with DC is freesync. But
> >> freesync is actually going to be interesting only if displays are able to
> >> get their sync range lower bound to 0, and get significant power saving
> >> thanks to this. For the use case of very low display refresh rate I don't even
> >> think displayport or hdmi can do that, and be power friendly (you would have
> >> to retrain the link probably each time you send a framebuffer to the display).
> >
> >
> > If freesync is about reducing the framerate rate for power saving,
> > provided that I've seen it be mentioned the first time for GCN 2nd generation,
> > I'm not expecting freesync as a mandatory capability for the series.
> >
> >> Mauro -- dce60_resources was having too many building errors due to missing DCE6 macros
> >> in order to temporarily overcome the problem dce_8_0_{d,sh_mask}.h headers
> >> were used for the PoC
> >

I also think that using and completing the dce_6_0_{d,sh_mask}.h
headers when possible is the best way,
also to be able to control and keep changes separated from dce80 path,
while for unmatching items between DCE8 and DCE6 the dce60_resources.c
code is worth a review to spot
problems.

There are tenths of CRTCx registers named differently between DCE6 and DCE8.

> > Still to many building errors due to quite different registers naming,
> > pointers to GPU register info (either in GPUopen or by means of
> > listing the DCE6 vs DC8 differences),
> > or keeping the DCE6 is exactly like DCE8 as register changes are
> > apparently not mission critical.
> >
> >> Mauro - dc/irq suffered the same problem dce_8_0_{d,sh_mask}.h headers
> >> were used for the PoC
> >
> > I could not update dc/irq VBI Vertical Blank Interrupt, because i
> > cannot find the corresponding IRQ register in amdgpu/dce6 registers
> > headers/mask
> >
> > -CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK
> > +?seems trivial but who knows what is the corresponding in DCE6?
> >
> > -CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_CLEAR_MASK
> > +?seems trivial but who knows what is the corresponding in DCE6?
> >
> > NOTE: If this is the very basic VBI Vertical Blank Interrupt signal
> > handling, there should be dce6 registers/masks,
> > but some hint/documentation is necessary for me to find them.
> >

The identification of the vblank interrupt register to be used in DCE6
should suffice to use the dce_6_0_{d,sh_mask}.h headers,
this should solve the dc/irq buidling errors.

> >
> >> Mauro - gfx6 may require some ad hoc initialization, skipped for the moment
> >
> > Are there ad hoc tiling settings which are necessary?

This part is way beyond my knowledge, as it involves the OpenGL
implementation and compliance,
may we skip gfx6 specifics or are there changes necessary?

> >
> > The OpenGLES and Vulkan radv apps I've tested:
> >
> > Android CTS dEQP-VK only 85 tests failed over 220'000
> > Toy Zombies Lite
> > Sky Force Reloaded
> > V1 Benchmark Pro
> > GFXbench
> > Antutu 3D
> > Various OpenGLES demos
> >
> > Here I'm planning to perform also dEQP-EGL, dEQP-GLES2, dEQP-GLES3 soon,
> > but feedbacks from developers are very welcome and appreciated
> >
> >> Mauro - Hainan specific code requires review, as some documentation and code paths
> >> seem to point that famility may not have DCE6, please confirm
> >
> > Hainan specifics were removed and are unsupported in the new serie
> > as DCE6 physical module not available in Hainan parts.
> > Unless the virtual_dce modules supports atomit, but I don't think so.
> >

Is it correct that even with virtual DCE block we would not get
atomic, so Hainan is to be skipped?
I forgot to say this before, Hainan is not supported in (v2) patches
and the commit messages for (v2) patches have been updated
accordingly.

> >> Mauro - video decoding blocks code have not been touched
> >
> > UVD and VCE firmwares and code changes for SI were necessary before the series
> > and they are unrelated to AMD DC for SI patches.

For Harry, Alex, do you have plans to add support to UVD, VCE firmware for SI?
In any case they seem unrelated to DC code paths.

> >
> >> Mauro - dc/dce/dce_clock_source.{c,h} may be missing some SI/DCE6 specifics
> >
> > In amd-staging-drm-next dce_clock_source is generic, SI specifics are
> > not necessary anymore.
> >
> >> Sylvain - It _seems_ there is not that much additional work to do in order to make it
> >> properly work.
> >>
> >
> > Ok, let's keep the momentum and continue tackle with x11 display problem
> > and after that I'm runnign piglit no regression with x11 and with wayland too.
> >
> >> Testing on x11,wayland or other ways
> >
> > Any other testing tools worth a run?
> > In case there is some AMD/GPUopen testing tool with unit tests, please
> > let me know
> > Kind regards
> >
> > Mauro
> >