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
