Re: [PATCH v2 00/19] DC/DM changes needed for amdgpu PSR-SU

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, 12 May 2022 at 19:22, Zhang, Dingchen (David)
<Dingchen.Zhang@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> Hi Daniel
>
> Thanks for your comments and explanations. I replied in-line and look forward to more discussion.
>
> regards
> David
>
>
> From: Daniel Vetter <daniel@ffwll.ch>
> Sent: Thursday, May 12, 2022 7:22 AM
> To: Alex Deucher <alexdeucher@gmail.com>
> Cc: Zhang, Dingchen (David) <Dingchen.Zhang@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Wang, Chao-kai (Stylon) <Stylon.Wang@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Zhuo, Qingqing (Lillian) <Qingqing.Zhuo@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Li, Roman <Roman.Li@amd.com>; Chiu, Solomon <Solomon.Chiu@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@amd.com>; Gutierrez, Agustin <Agustin.Gutierrez@amd.com>; Kotarac, Pavle <Pavle.Kotarac@amd.com>
> Subject: Re: [PATCH v2 00/19] DC/DM changes needed for amdgpu PSR-SU
>
> On Wed, 11 May 2022 at 17:35, Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Tue, May 10, 2022 at 4:45 PM David Zhang <dingchen.zhang@amd.com> wrote:
> > >
> > > changes in v2:
> > > -----------------------
> > > - set vsc_packet_rev2 for PSR1 which is safer
> > > - add exposure of AMD specific DPCD regs for PSR-SU-RC (rate-control)
> > > - add DC/DM change related to amdgpu PSR-SU-RC
> > >
> > >
> > > David Zhang (18):
> > >   drm/amd/display: align dmub cmd header to latest dmub FW to support
> > >     PSR-SU
> > >   drm/amd/display: feed PSR-SU as psr version to dmub FW
> > >   drm/amd/display: combine dirty rectangles in DMUB FW
> > >   drm/amd/display: update GSP1 generic info packet for PSRSU
> > >   drm/amd/display: revise Start/End SDP data
> > >   drm/amd/display: program PSR2 DPCD Configuration
> > >   drm/amd/display: Passing Y-granularity to dmub fw
> > >   drm/amd/display: Set default value of line_capture_indication
> > >   drm/amd/display: add vline time in micro sec to PSR context
> > >   drm/amd/display: fix system hang when PSR exits
> > >   drm/amd/display: Set PSR level to enable ALPM by default
> > >   drm/amd/display: use HW lock mgr for PSR-SU
> > >   drm/amd/display: PSRSU+DSC WA for specific TCON
> > >   drm/amd/display: add shared helpers to update psr config fields to
> > >     power module
> > >   drm/amd/display: calculate psr config settings in runtime in DM
> > >   drm/amd/display: update cursor position to DMUB FW
> > >   drm/amd/display: expose AMD source specific DPCD for FreeSync PSR
> > >     support
> > >   drm/amd/display: PSR-SU rate control support in DC
> > >
> > > Leo Li (1):
> > >   drm/amd/display: Implement MPO PSR SU
> >
> > A couple of suggestions from Daniel on IRC:
> > 1.  Might be good to extract the "calculate total crtc damage" code
> > from i915 in intel_psr2_sel_fetch_update, stuff that into damage
> > helpers and reuse for i915 and amdgpu
>
> To expand a bit on this. There is currently a helper for total damage,
> but it's at the fb/plane level for drivers which need to upload
> buffers (usb/spi or virtual) drm_atomic_helper_damage_merged(). That
> one probably needs to be renamed to signify it's about the plane, and
> then we need a new drm_atomic_helper_crtc_damage_merged() which
> (extract from i915 code ideally) which computes total crtc damage for
> stuff like psr2/su or the command mode dsi panels (unfortunately none
> of the drivers for android for these panels have been upstreamed yet).
>
> <<<
> Checked the DRM comment for the helper `drm_atomic_helper_damage_merged()` and
> quoted below:
> *****
> Drivers might want to use the helper functions drm_atomic_helper_damage_iter_init()
> and drm_atomic_helper_damage_iter_next() or drm_atomic_helper_damage_merged()
> if the driver can only handle a single damage region at most.
> *****
> Currently for amdgpu, the multiple damage clips combination (merging) is handled in
> DMUB firmware. And the DRM comment shows that the usage of "damage_merged()"
> helper is for the driver which can only handle single damage region at most.
>
> Since AMDGPU is capable of handling multiple damaged clip (in DMUB FW), can I
> understand that the group of helpers of `damage_merged()` in DRM is not mandatory
> but optional?

Ah I didn't see from a quick read that this was possible. How does
this work when the plane is enabled/disabled or resized or moved?
-Daniel

> I also think that the split between dc and kms is a bit funny, I'd put
> only the resulting damage rect into dc_pipe and do the computation of
> that in the drm/kms linux code outside of dc functions (or in the glue
> code for dc), since I'm assuming on windows it's completely different
> approach in how you compute damage. Especially once we have the crtc
> damage helper on linux.
>
> > 2.  The commit message on "drm/amd/display: Implement MPO PSR SU" is a
> > bit funny, since if you use the helpers right you always get damage
> > information, just when it's from userspace that doesn't set explicit
> > damage it's just always the entire plane.
>
> <<<
> The current implementation to mark the entire MPO as dirt RECT is not the final
> version. Our next step is to implement the translation of DRM damaged clips to
> DC regions and pass to let DMUB FW to handle, which is able to handle at most
> 3 damaged regions for each DC surface.
>
>
>
> Yeah so that one was just another reason to use the helpers more in
> amdgpu for this.
> -Daniel
>
> >
> > Alex
> >
> > >
> > >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 142 +++++++++-
> > >  .../drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c |  21 +-
> > >  drivers/gpu/drm/amd/display/dc/core/dc.c      |  54 ++++
> > >  drivers/gpu/drm/amd/display/dc/core/dc_link.c |  47 +++-
> > >  drivers/gpu/drm/amd/display/dc/dc_link.h      |   4 +
> > >  drivers/gpu/drm/amd/display/dc/dc_stream.h    |   5 +
> > >  drivers/gpu/drm/amd/display/dc/dc_types.h     |  23 +-
> > >  .../drm/amd/display/dc/dce/dmub_hw_lock_mgr.c |   2 +
> > >  drivers/gpu/drm/amd/display/dc/dce/dmub_psr.c |  64 +++++
> > >  drivers/gpu/drm/amd/display/dc/dce/dmub_psr.h |   2 +
> > >  .../gpu/drm/amd/display/dc/dcn10/dcn10_hubp.c |   2 +
> > >  .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 131 +++++++++
> > >  .../gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c |   2 +
> > >  .../dc/dcn30/dcn30_dio_stream_encoder.c       |  15 ++
> > >  drivers/gpu/drm/amd/display/dc/inc/hw/hubp.h  |   1 +
> > >  .../drm/amd/display/dc/inc/hw/link_encoder.h  |  21 +-
> > >  .../gpu/drm/amd/display/dmub/inc/dmub_cmd.h   | 250 +++++++++++++++++-
> > >  .../amd/display/include/ddc_service_types.h   |   1 +
> > >  .../display/modules/info_packet/info_packet.c |  29 +-
> > >  .../amd/display/modules/power/power_helpers.c |  84 ++++++
> > >  .../amd/display/modules/power/power_helpers.h |   6 +
> > >  21 files changed, 887 insertions(+), 19 deletions(-)
> > >
> > > --
> > > 2.25.1
> > >
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=05%7C01%7Cdingchen.zhang%40amd.com%7Cbf7f256980c04124f60808da3409b3d4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637879513542024968%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=tlr9ThR7DPE%2B8wv9e3n7Ud63Ju9%2FRrka4OdK1KRgeWI%3D&amp;reserved=0



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch