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 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.


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 DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux