On Wed, May 5, 2021 at 11:47 PM <sbillaka@xxxxxxxxxxxxxx> wrote: > > On 2021-05-05 15:31, Dmitry Baryshkov wrote: > > Hi, > > > > On Wed, 5 May 2021 at 11:17, Sankeerth Billakanti > > <sbillaka@xxxxxxxxxxxxxx> wrote: > >> > >> These patches add support for the next generation eDP driver on > >> SnapDragon > >> with dpu support. The existing eDP driver cannot support the new eDP > >> hardware. So, to maintain backward compatibility, the older eDP driver > >> is > >> moved to v200 folder and the new generation eDP driver is added in > >> the v510 folder. > > > > What exactly does this version correspond to? > > I assume that v510 corresponds to sdmshrike/sc8180x. Is it right? > [Sankeerth] This is for sc7280. > > > Is it really so specific, or just v2/v5 would be enough? Not to > > mention that this is the MDP/ version, while other blocks tend to use > > block-specific versions/ids. > [Sankeerth] I can rename it as edp-v1 and edp-v2. Edp v1 is very old > chip and there is considerable HW delta between v1 and v2. So, we want > to separate the driver. We followed similar model for DPU driver where, > MDP4, MDP5 and DPU have separate folders. EDP v1 belongs to MDP4 > generation. Bjorn brought up the idea of just dropping the existing drm/msm/edp.. since the efforts to upstream the platform it worked on (8084?) fizzled out, I don't think there is any device which uses it. But it does sound like edp is a subset of the the newer dp driver, so seems sort of like the better approach would be to add edp support to dp. I believe Bjorn has something based on this approach which is working for sc8280 (although not sure if it is in shape to post patches yet) BR, -R > > > > Also, how much does it differ from the current DP core supported via > > drivers/gpu/drm/msm/dp ? > [Sankeerth] eDP is a native controller like DP but does not have audio, > content protection and interoperability requirement. Upstream already > supports eDP as a new interface driver found here: > drivers/gpu/drm/msm/edp. > I wanted to add the new controller driver as part of that folder. > > > > > First two patches did not make it to the linux-msm, so I can not > > comment on each of the lines. > [Sankeerth] I am also not sure why they did not make it to patchwork. I > will repost them. > > > However just my few cents (other reviewers might disagree though): > > > > - I see little benefit in renaming the folders just for the sake of > > renaming. You can put your code in drivers/gpu/drm/msm/edp-v510, if > > you really insist on that. Note that for all other (even incompatible) > > hardware types we still use single level of folders. > > > > - Also I see that significant parts of code (e.g. AUX, bridge, > > connector, maybe more) are just c&p of old edp code pieces. Please > > share the code instead of duplicating it. > [Sankeerth] It is a baseline driver. As we add more features, it will > considerably deviate a lot. The effort seems to be very high to maintain > the common portion of code as I expect a lot of deviation. > > > > - Please consider updating register definitions in xml form and then > > providing both changed xml files (to mesa project (?)) and generated > > headers into the kernel. > [Sankeerth] I followed what was done in the DP driver at > /drivers/gpu/drm/msm/dp. I need to explore the xml approach to generate > the register definitions. > > > > - Please consider using clk_bulk_* functions instead of using > > dss_module_power. I'm going to send a patchset reworking current users > > to use the generic clk_bulk_* function family. > [Sankeerth] I will explore and rebase after your patch is available. > > > > - In generic, this eDP clock handling seems to match closely DP clocks > > handling (with all the name comparison, etc). Consider moving this to > > a generic piece of code > > > > - PHY seems to be a version of QMP PHY. Please use it, like it was > > done for the DP itself. There is support for combined USB+DP PHYs > > (both v3 and v4), so it should be possible to extend that for eDP. > [Sankeerth] The DP phy is a combophy which supports both usb and dp phy > concurrently, unlike eDP phy which is specific to only the eDP > controller in sc7280. So, I implemented the edp phy sequences in the > same folder. > > > > > >> These are baseline changes with which we can enable display. The new > >> eDP > >> controller can also support additional features such as backlight > >> control, > >> PSR etc. which will be enabled in subsequent patch series. > >> > >> Summary of changes: > >> DPU driver interface to the new eDP v510 display driver. > >> New generation eDP controller and phy driver implementation. > >> A common interface to choose enable the required eDP driver. > >> > >> Sankeerth Billakanti (3): > >> drm/msm/edp: support multiple generations of edp hardware > >> drm/msm/edp: add support for next gen edp > >> drm/msm/disp/dpu1: add support for edp encoder > >> > >> drivers/gpu/drm/msm/Makefile | 19 +- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 33 + > >> drivers/gpu/drm/msm/edp/edp.c | 198 --- > >> drivers/gpu/drm/msm/edp/edp.h | 78 - > >> drivers/gpu/drm/msm/edp/edp.xml.h | 380 ----- > >> drivers/gpu/drm/msm/edp/edp_aux.c | 264 ---- > >> drivers/gpu/drm/msm/edp/edp_bridge.c | 111 -- > >> drivers/gpu/drm/msm/edp/edp_common.c | 38 + > >> drivers/gpu/drm/msm/edp/edp_common.h | 47 + > >> drivers/gpu/drm/msm/edp/edp_connector.c | 132 -- > >> drivers/gpu/drm/msm/edp/edp_ctrl.c | 1375 > >> ------------------ > >> drivers/gpu/drm/msm/edp/edp_phy.c | 98 -- > >> drivers/gpu/drm/msm/edp/v200/edp.xml.h | 380 +++++ > >> drivers/gpu/drm/msm/edp/v200/edp_v200.c | 210 +++ > >> drivers/gpu/drm/msm/edp/v200/edp_v200.h | 70 + > >> drivers/gpu/drm/msm/edp/v200/edp_v200_aux.c | 264 ++++ > >> drivers/gpu/drm/msm/edp/v200/edp_v200_bridge.c | 111 ++ > >> drivers/gpu/drm/msm/edp/v200/edp_v200_connector.c | 132 ++ > >> drivers/gpu/drm/msm/edp/v200/edp_v200_ctrl.c | 1375 > >> ++++++++++++++++++ > >> drivers/gpu/drm/msm/edp/v200/edp_v200_phy.c | 98 ++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510.c | 220 +++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510.h | 151 ++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510_aux.c | 268 ++++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510_bridge.c | 111 ++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510_connector.c | 117 ++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510_ctrl.c | 1583 > >> +++++++++++++++++++++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510_phy.c | 641 +++++++++ > >> drivers/gpu/drm/msm/edp/v510/edp_v510_reg.h | 339 +++++ > >> 29 files changed, 6207 insertions(+), 2643 deletions(-) > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp.c > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp.h > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp.xml.h > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp_aux.c > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp_bridge.c > >> create mode 100644 drivers/gpu/drm/msm/edp/edp_common.c > >> create mode 100644 drivers/gpu/drm/msm/edp/edp_common.h > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp_connector.c > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp_ctrl.c > >> delete mode 100644 drivers/gpu/drm/msm/edp/edp_phy.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp.xml.h > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp_v200.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp_v200.h > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp_v200_aux.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp_v200_bridge.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp_v200_connector.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp_v200_ctrl.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v200/edp_v200_phy.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510.h > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510_aux.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510_bridge.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510_connector.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510_ctrl.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510_phy.c > >> create mode 100644 drivers/gpu/drm/msm/edp/v510/edp_v510_reg.h > >> > >> -- > >> The Qualcomm Innovatin Center, Inc. is a member of the Code Aurora > >> Forum, a Linux Foundation Collaborative Project > >> > > > > > > -- > > With best wishes > > Dmitry
