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

[Harry Wentland]

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

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