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

[Mauro Rossi]

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.

> 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

>
> 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?

> 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

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.


>Mauro - gfx6 may require some ad hoc initialization, skipped for the moment

Are there ad hoc tiling settings which are 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.

> 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.

>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