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

Tue Oct 16 12:20:15 UTC 2018

Hi Harry,
thanks a lot for response,

On Mon, Oct 15, 2018 at 11:19 PM Harry Wentland <harry.wentland at amd.com> wrote:
>
> On 2018-10-15 5:06 p.m., Harry Wentland 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
> >>
>
>
> What updates do you have to your original series?
>
> If there are substantial updates can you send a v2 of your series?

The new v2 series is rebased on amd-staging-drm-next with following benefits:

- dce_clocks chipset families cases are not required anymore
- dcmu fw load for DCN1 has some SI cases added
- it is ready to be applied in amd-staging-drm-next

Not much changes compared to v1, but I'm about to submit v2 very soon.

>
> If not I'll go through the v1. The branch is great for review but email patches are easier for commenting.

This morning I've launched a build on v2  series, on
amd-staging-drm-next branch rebased on top of 4.19rc8
just to test it builds fine.

I'm sending the v2 series this evening and review may proceed on v2
amd-staging-drm-next context.
Mauro

>
>
> >> 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
> >
> >>> 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
> >>
>
> We should probably update the DCE6 headers and use those. Sometimes register addresses change in subtle ways and cause problems later on that are hard to spot.
>
> >>>
> >>> 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)
> >>
>
> I wouldn't worry about this too much for now. DC really just wants powerplay. Might make sense to just silence the error print if things seem to work otherwise.
>
> Great work in getting things up and running with DCE6 and thanks for sending patches to get this work upstreamed.
>
> Harry
>
> >>
> >>
> >>> 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
> >>
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >