On Thu, 12 Dec 2024 at 04:59, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > > Having I/O regions inside a msm_dp_catalog_private() results in extra > > layers of one-line wrappers for accessing the data. Move I/O region base > > and size to the globally visible struct msm_dp_catalog. > > > > Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > --- > > drivers/gpu/drm/msm/dp/dp_catalog.c | 457 +++++++++++++++--------------------- > > drivers/gpu/drm/msm/dp/dp_catalog.h | 12 + > > 2 files changed, 197 insertions(+), 272 deletions(-) > > > > > Fundamentally, the whole point of catalog was that it needs to be the > only place where we want to access the registers. Thats how this really > started. > > This pre-dates my time with the DP driver but as I understand thats what > it was for. > > Basically separating out the logical abstraction vs actual register writes . > > If there are hardware sequence differences within the controller reset > OR any other register offsets which moved around, catalog should have > been able to absorb it without that spilling over to all the layers. > > So for example, if we call dp_ctrl_reset() --> ctrl->catalog->reset_ctrl() > > Then the reset_ctrl op of the catalog should manage any controller > version differences within the reset sequence. The problem is that the register-level writes are usually not the best abstraction. So, instead of designing the code around dp_catalog I'd prefer to see actual hw / programming changes first. > > We do not use or have catalog ops today so it looks redundant as we just > call the dp_catalog APIs directly but that was really the intention. > > Another reason which was behind this split but not applicable to current > upstream driver is that the AUX is part of the PHY driver in upstream > but in downstream, that remains a part of catalog and as we know the AUX > component keeps changing with chipsets especially the settings. That was > the reason of keeping catalog separate and the only place which should > deal with registers and not the entire DP driver. > > The second point seems not applicable to this driver but first point > still is. I do admit there is re-direction like ctrl->catalog > instead of just writing it within dp_ctrl itself but the redirection was > only because ctrl layers were not really meant to deal with the register > programming. So for example, now with patch 7 of this series every > register being written to i exposed in dp_ctrl.c and likewise for other > files. That seems unnecessary. Because if we do end up with some > variants which need separate registers written, then we will now have to > end up touching every file as opposed to only touching dp_catalog. Yes. I think that it's a bonus, not a problem. We end up touching the files that are actually changed, so we see what is happening. Quite frequently register changes are paired with the functionality changes. For example (a very simple and dumb one), when designing code around dp_catalog you ended up adding separate _p1 handlers. Doing that from the data source point of view demands adding a stream_id param. In the DPU driver we also have version-related conditionals in the HW modules rather than pushing all data access to dpu_hw_catalog.c & counterparts. I think it's better to make DP driver reflect DPU rather than keeping a separate wrapper for no particular reason (note, DPU has hardware abstractions, but on a block level, not on a register level). -- With best wishes Dmitry
