On 12/14/2024 2:05 PM, Dmitry Baryshkov wrote:
On Sat, 14 Dec 2024 at 22:53, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
Hi Dmitry
On 12/12/2024 3:09 PM, Dmitry Baryshkov wrote:
On Thu, 12 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 12/12/2024 12:52 AM, Dmitry Baryshkov wrote:
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.
So thats the issue here. If we did end up with registers and sequences
different for controller versions, the ctrl layer was expected to just
call a reset() op for example similar to the DPU example you gave. And
as you rightly noted, the dpu_hw_xxx files only expose the ops based on
version and the upper layers were supposed to just call into the ops
without knowing the register level details. Thats pretty much what
dp_ctrl tried to do here. We did not want to expose all the register
defines in those layers. This series is doing exactly opposite of that.
We don't have the issue up to now, even though we support DP
controllers since SDM845 up to SM8650 and X1E80100. The SDE driver has
v200 vs v420 catalog files for PHY programming, the rest of the
functions are common. So, for me it looks like a preparation for the
imaginary case that didn't come to existence up to now.
So, yes. I want to get rid of extra useless indirection and I want to
expose register sequences in those layers.
Yes because PHY programming is managed in the PHY driver today and does
not go through catalog whereas in SDE driver it does, I do not have any
other concrete example to give you which exists in the current code
where sequence changes across chipset variants for DP controller and
since I certainly cannot discuss how things can evolve moving forward,
as usual, I have to accept it as one of those things which is not used
today. So yes, I guess the register sequencing point changing across
chipset variants, does not have a good example which I can really share.
But exposing register sequences within the same file, I am not too sure
about that. For example, you can take a look at
dp_catalog_panel_config_hdr in the SDE code OR even
msm_dp_catalog_panel_enable_vsc_sdp in the current upstream code. Why
should this entire sequence be exposed to the dp_panel layer?
Why not? The dp_catalog_panel_config_hdr() is a bit tough, we don't
implement similar functions currently. For
msm_dp_catalog_panel_enable_vsc_sdp() this is also a logical sequence:
configure GENERIC0, write the package to GENERIC0, indicate presence
of the VSC SDP. I really don't see why this should go to a separate
file.
We have to plan for hdr for sure. its not an imaginary use-case.
msm_dp_catalog_panel_enable_vsc_sdp() does a bunch of things, packing
the size, programming the SDP and triggering the update. dp_panel does
not need to know all these details.
There are many other examples. Take a look at
msm_dp_catalog_ctrl_mainlink_ctrl(). from the dp_ctl standpoint, all it
needs to know is it needs to program the mainlink_Ctrl() at some point,
it really does not need to know the full sequence of doing that. Thats
the abstraction getting lost with this.
For smaller functions which are one-liners the redirection seems
redundant but when the sequence is bigger like in the examples I gave,
the logical Vs register sequence separation grows. Thats where the
dp_catalog came from.
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.
Not exactly. Why should dp_ctrl really know that some register offset or
some block shift happened for example. It only needs to know when to
reset the hardware and not how. Thats the separation getting broken with
this.
Yes. And I'm removing that separation very intentionally. If one is
looking for AUX programming, they should be looking into dp_aux only,
not dp_aux & dp_catalog. Likewise all audio code should be in
dp_audio. By using dp_catalog we ended up with a very very very bad
abstraction of msm_dp_catalog_audio_get_header() /
msm_dp_catalog_audio_set_header() / enum
msm_dp_catalog_audio_sdp_type. Just because reads & writes should go
through the catalog.
No, I think this is where there is some correction needed. the
get_header() / set_header() was done not because all writes need to go
through catalog but because the audio headers were thought to be written
only one header at a time and we had thought that read-modify-write had
to be done to preserve the bytes. And when we have to do only one header
at a time and because two headers map to one register, catalog had to
end up managing an audio_map. Now, after checking where it came from as
I commented on that patch, this requirement was not a functional one but
was just trying to preserve the pre-silicon validation scripts sequence,
this part of it can be dropped. So no need of get_header() /
set_header() and an audio_map. Now all registers going through catalog
is another thing which we are still discussing here.
You've skipped the msm_dp_catalog_audio_sdp_type enum (which was
explicitly mentioned). It is an abstraction which in my opinion also
isn't required, but it clearly comes from dp_catalog.
msm_dp_catalog_audio_sdp_type was needed only till the map was needed as
it was made with an intention of trying to re-use the sdp_map layout for
different packets. So its still tied to the fact that we needed a map.
After dropping the map, this can be dropped too as you already did.
For dp_panel likewise there is no need to look into some other source
file to follow the register sequences. It can all be contained within
dp_panel.c, helping one to understand the code.
Last, but not least. Code complexity. dp_catalog.c consists of 1340
lines, covering different submodules. It is hard to follow it in this
way.
Its just a question of spreading up the functions all over, not reducing
code complexity. So yes, it reduces the file size of dp_catalog whereas
increases that of others. Code complexity impact due to that is subjective.
The main issue is that dp_catalog now contains unrelated sets of
functions. That's code complexity.
dp_catalog was never meant to be a place where we have related
functions. It was supposed to provide the register space abstraction and
hiding away the details from rest of the layers. Going by what you are
saying even the APIs in dsi_host.c should be related but they are not
because of the same reason. I think this series went too far in terms of
what it was trying to achieve trying to clenaup even useful things.
Audio map removal was a problem tied to the fact that that
read-modify-write behavior was preserved. That was removed after we
identified its background. No issues with that. but beyond that, this is
too much of a rework. I will let other developers chime into this. But I
am not too fond of this one.
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.
I have not checked your comment on that series here but if your concern
This is really a bad cadence. I have provided most of the feedback
almost a week ago.
Yes, was a very tight week trying to enable upstream developers to land
their platforms such as QCS615 by fixing platform specific dpu things
and had the fixes cycle this week too so as a result my own feature took
a bit of a hit this week :(
is stream_id should not be stored in the catalog but just passed, thats
fine, we can change it. stream_id as a param is needed anyway because
the register programming layer needs to know which offset to use. This
series is not mitigating that fact.
No, my concern was that you have been adding separate _p1() functions
which are a duplicate of _p0() counterparts. When one looks at the
dp_catalog.c it is logical: there are two different register areas, so
there are two distinct sets of functions. If one starts looking from
the dp_panel point of view, it's obvious that there should be a single
msm_dp_write_stream() function which accepts stream_id and then
multiplexes it to go to p0 or p1.
Your multiplexing suggestion of adding a msm_dp_read_pn/msm_dp_write_pn
by passing a stream_id can be done even with current dp_catalog intact
as it will help reduce storing the stream_id in the dp_catalog. So its a
valid suggestion and can be implemented even in the current code and not
tied to the fact that register writing is done in dp_catalog or dp_panel.
It can. The point was about the implementation logic, not about the possibility.
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.
The dpu_hw_catalog.c and the dp_catalog.c are not the right files to
compare with each other. dp_catalog.c should be compared with
dpu_hw_xxx.c and as you noted, DPU version dependencies are handled in
those files only and not all over the files like what this series is doing.
Not really. dpu_encoder_phys_cmd_init() checks for the core_major_ver.
Let me see if other files check for the version under the hood.
Well, thats because only cmd mode panel cares about TE. No other files
from what I checked.
I've sent a series which refactors feature bits into core_major_ver.
Now HW revision is being checked inside dpu_encoder_phys_wb.c,
dpu_kms.c and dpu_rm.c. And I didn't refactor SSPP, which would bring
similar checks to dpu_plane.c and possibly dpu_vbif.c
We will evaluate it on its merits / demerits as usual and decide.
Also as you wrote, there are multiple dpu_hw_xxx.c files, each
handling register issues on its own. We don't have a single file which
keeps all such differences in one place.
Thats because of the way the registers are laid our in the SW interface
document aligns nicely with the file split we have in the DPU even when
the first DPU post happened.
But I still dont think its a fair comparison.
If you really had to go deeper into this thought, then even dp_reg.h
should be broken down into smaller headers because the offsets in
dpu_hw_*** files are relevant only to those files but after this change
all DP files must include dp_reg.h even though they will not be using
all of the offsets. Since current code was already doing that, which it
didnt have to as dp_Catalog was the only one writing all registers, this
went unnoticed.
Well... I had a thought about reworking DP into using XML files to
describe registers. It will make it slightly cleaner and
self-documented, but it most likely will be a single file.
It being in a single file is leaning towards the same model we have
right now. If dp_regs.h is remaining one unified file, I dont see why
dp_catalog can stay as well.
Last, but not least, in the DPU driver there are actual differences
between generations, which require different code paths. In the DP
driver there are none.
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).
Thats the issue here. DPU hardware blocks are arranged according to the
sub-blocks both in the software interface document and hence the code
matches it file-by-file. DP registers are grouped by clock domains and
the file separation we have today does not match that anyway. Hence
grouping link registers writes or pixel clock register writes into
dp_ctrl is also not correct that way. Let catalog handle that separation
internally which it already does.
I'd say, dp_panel, dp_audio and dp_link are already pretty
self-contained. I was hoping to look at dp_display vs dp_drm later on,
once the HPD issue gets resolved. Only dp_ctrl is not that logical
from my point of view.
Not from the point of view of the register separation of what belongs
where. Its just a subjective opinion of what belongs where. Different
developers can view it differently.
