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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 6/16/2023 5:35 PM, Dmitry Baryshkov wrote:
On 17/06/2023 00:54, Marijn Suijten wrote:
On 2023-06-16 14:18:39, Abhinav Kumar wrote:


On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote:
On 14/06/2023 04:57, Jessica Zhang wrote:
Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
databus-widen mode datapath.

Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 +++
   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 12 ++++++++++++
   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  3 +++
   3 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index b856c6286c85..124ba96bebda 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
       if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression) phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
+
+    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?

In addition, it seems that video mode does all its INTF_CONFIG2 configuration separately in dpu_hw_intf_setup_timing_engine(). If we have a generic set_intf_config2() op, it might be good to have it as part of a larger cleanup where we have both video and command mode use the generic op. What are your thoughts on this?

Thanks,

Jessica Zhang



So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool
data_compress)

Now for the caller who wants to just enable widebus this will be

intf_cfg2_xxx(....., true, false)

if we want to do both

intf_cfg2_xxx(...., true, true)

the last one where we want to do just data_compress(highly unlikely and
not recommended)

intf_cfg2_xxx(...., false, true)

Now someone looking at the code will have to go and find out what each
bool is.

Whereas with separate ops, its kind of implicit.

That's why you never pass bools as function argument (edge-case if it is
the only argument, and its meaning becomes clear from the function
name).  Use enumerations anywhere else.

- Marijn


For that reason, I dont think this patch is bad at all.

--
With best wishes
Dmitry




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux