On Thu, 12 May 2022 at 19:22, Zhang, Dingchen (David) <Dingchen.Zhang@xxxxxxx> 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@xxxxxxxx> > Sent: Thursday, May 12, 2022 7:22 AM > To: Alex Deucher <alexdeucher@xxxxxxxxx> > Cc: Zhang, Dingchen (David) <Dingchen.Zhang@xxxxxxx>; amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Wang, Chao-kai (Stylon) <Stylon.Wang@xxxxxxx>; Li, Sun peng (Leo) <Sunpeng.Li@xxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>; Zhuo, Qingqing (Lillian) <Qingqing.Zhuo@xxxxxxx>; Siqueira, Rodrigo <Rodrigo.Siqueira@xxxxxxx>; Li, Roman <Roman.Li@xxxxxxx>; Chiu, Solomon <Solomon.Chiu@xxxxxxx>; Zuo, Jerry <Jerry.Zuo@xxxxxxx>; Pillai, Aurabindo <Aurabindo.Pillai@xxxxxxx>; Lin, Wayne <Wayne.Lin@xxxxxxx>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@xxxxxxx>; Gutierrez, Agustin <Agustin.Gutierrez@xxxxxxx>; Kotarac, Pavle <Pavle.Kotarac@xxxxxxx> > 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@xxxxxxxxx> wrote: > > > > On Tue, May 10, 2022 at 4:45 PM David Zhang <dingchen.zhang@xxxxxxx> 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&data=05%7C01%7Cdingchen.zhang%40amd.com%7Cbf7f256980c04124f60808da3409b3d4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637879513542024968%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tlr9ThR7DPE%2B8wv9e3n7Ud63Ju9%2FRrka4OdK1KRgeWI%3D&reserved=0 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
