Re: [Freedreno] [PATCH 2/3] drm/msm/dpu: Set DATABUS_WIDEN on command mode encoders

From: Abhinav Kumar
Date: Thu Jun 29 2023 - 13:26:52 EST




On 6/22/2023 4:37 PM, Abhinav Kumar wrote:


On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote:
On 23/06/2023 01:37, Abhinav Kumar wrote:


On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote:
On 22/06/2023 02:01, Abhinav Kumar wrote:


On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
On 21/06/2023 18:17, Marijn Suijten wrote:
On 2023-06-20 14:38:34, Jessica Zhang wrote:
<snip>
+    if (phys_enc->hw_intf->ops.enable_widebus)
+ phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);

No. Please provide a single function which takes necessary
configuration, including compression and wide_bus_enable.


There are two ways to look at this. Your point is coming from the
perspective that its programming the same register but just a different
bit. But that will also make it a bit confusing.

My point is to have a high-level function that configures the INTF for
the CMD mode. This way it can take a structure with necessary
configuration bits.

Hi Dmitry,

After discussing this approach with Abhinav, we still have a few
questions about it:

Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
rest are reserved with no plans of being programmed in the future). Does
this still justify the use of a struct to pass in the necessary
configuration?

No.  The point Dmitry is making is **not** about this concidentally
using the same register, but about adding a common codepath to enable
compression on this hw_intf (regardless of the registers it needs to
touch).

Actually to setup INTF for CMD stream (which is equal to setting up compression at this point).


Yes it should be setup intf for cmd and not enable compression.

Widebus and compression are different features and we should be able to control them independently.

We just enable them together for DSI. So a separation is necessary.

But I am still not totally convinced we even need to go down the path for having an op called setup_intf_cmd() which takes in a struct like

struct dpu_cmd_intf_cfg {
     bool data_compress;
     bool widebus_en;
};

As we have agreed that we will not touch the video mode timing engine path, it leaves us with only two bits.

And like I said, its not that these two bits always go together. We want to be able to control them independently which means that its not necessary both bits program the same register one by one. We might just end up programming one of them if we just use widebus.

Thats why I am still leaning on keeping this approach.

I do not like the idea of having small functions being called between modules. So, yes there will a config of two booleans, but it is preferable (and more scalable) compared to separate callbacks.


I definitely agree with the scalable part and I even cross checked that the number of usable bitfields of this register is going up from one chipset to the other although once again that depends on whether we will use those features.

For that reason I am not opposed to the struct idea.

But there is also another pattern i am seeing which worries me. Usable bitfields sometimes even reduce. For those cases, if we go with a pre-defined struct it ends up with redundant members as those bitfields go away.

With the function op based approach, we just call the op if the feature bit / core revision.

So I wanted to check once more about the fact that we should consider not just expansion but also reduction.

As we have to support all generations, there is no actual reduction. Newer SoCs do not have particular feature/bit, but older ones do. By having multiple optional ops we just move this knowledge from ops->complex_callback() to _setup_block_ops(). But more importantly the caller gets more complicated. Instead of just calling ops->set_me_up(), it has to check all the optional callbacks and call the one by one.


Alright, I am thinking that perhaps because this register is kind of unique that its actually controlling a specific setting in the datapath, downstream also has separate ops for this.

But thats fine, we can go ahead with the struct based approach.


As data_compress has already landed, let me introduced the struct along with the core_revision based approach in the core_revision series and this series will expand that struct to include widebus too.