Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
From: Abhinav Kumar
Date: Thu Jun 29 2023 - 23:30:41 EST
On 6/29/2023 8:22 PM, Dmitry Baryshkov wrote:
On 30/06/2023 06:07, Abhinav Kumar wrote:
On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
On 29/06/2023 22:29, Abhinav Kumar wrote:
Instead of using a feature bit to decide whether to enable data
compress or not for DSC use-cases, use dpu core's major version
instead.
This will avoid defining feature bits for every bit level details of
registers.
Also, rename the intf's enable_compression() op to program_datapath()
and allow it to accept a struct intf_dpu_datapath_cfg to program
all the bits at once. This can be re-used by widebus later on as
well as it touches the same register.
Two separate commits please, because...
I thought of it but it seemed better together and was the only way I
could think of. Please see below.
Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16
++++++++++++++--
3 files changed, 26 insertions(+), 8 deletions(-)
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..f4e15b4c4cc9 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
@@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
to_dpu_encoder_phys_cmd(phys_enc);
struct dpu_hw_ctl *ctl;
struct dpu_hw_intf_cfg intf_cfg = { 0 };
+ struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
ctl = phys_enc->hw_ctl;
if (!ctl->ops.setup_intf_cfg)
@@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
phys_enc->hw_intf,
phys_enc->hw_pp->idx);
- if (intf_cfg.dsc != 0 &&
phys_enc->hw_intf->ops.enable_compression)
- phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
+ if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version
>= 0x7) {
... because this becomes incorrect. The datapath should be programmed
in all the cases, if there is a corresponding callback. intf_cfg.dsc
should be used as a condition to set datapath_cfg.
The issue is that today we do not have dpu_mdss_cfg as part of
dpu_hw_intf nor _setup_intf_ops because all of those have been dropped
with some rework or cleanup.
Pass dpu_mdss_cfg to dpu_hw_intf_init(). It was removed as a cleanup,
now we can reintroduce it.
Thanks, that will address all these concerns.
I wanted to get agreement before re-introducing it and also make sure
there was no other way.
Ideally even I would like to assign this op only for core_rev >=7 but
that information is no longer available. We would have to start
passing the major and minor versions to _setup_intf_ops() to go with
that approach. So without making all of those changes, the only way I
had was to assign the op unconditionally but call it only for
major_rev >= 7.
Passing core_rev to the op itself so that we can write the register
only for core_rev >=7 is an option but then what if some bits start
becoming usable only after minor rev. then we will have to start
passing major and minor rev to program_datapath too. Again getting
little messy.
I am open to ideas to achieve the goal of assigning this op only for
core_rev >=7 other than what I wrote above.
+ struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
No need for `0' in the init, empty braces would be enough.
ack.
+
+ datapath_cfg.data_compress = true;
+ phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf,
&datapath_cfg);
+ }
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 5b0f6627e29b..85333df08fbc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -513,11 +513,13 @@ static void
dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
}
-static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
+ struct intf_dpu_datapath_cfg *datapath_cfg)
{
u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
- intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+ if (datapath_cfg->data_compress)
+ intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
}
@@ -543,8 +545,7 @@ static void _setup_intf_ops(struct
dpu_hw_intf_ops *ops,
ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
}
- if (cap & BIT(DPU_INTF_DATA_COMPRESS))
- ops->enable_compression = dpu_hw_intf_enable_compression;
+ ops->program_datapath = dpu_hw_intf_program_datapath;
The `core_major_version >= 7' should either be here or in the
callback itself.
Yes, ideally I would like it like that but please see above why I
couldnt do it.
}
struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 99e21c4137f9..f736dca38463 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -48,6 +48,11 @@ struct intf_status {
u32 line_count; /* current line count including
blanking */
};
+struct intf_dpu_datapath_cfg {
+ u8 data_compress; /* enable data compress between dpu and
dsi */
+ /* can be expanded for other programmable bits */
+};
I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
The goal was to keep it generic. Its actually the handshake between
DPU and interface datapath so I chose that name.
Do you have plans of using it for the video mode?
No because we didnt want to touch the video mode path as that was
discussed in the widebus series already.
Ok, I am fine with intf_cmd_mode_cfg in that case.
This is not specific to command mode and intf_cfg is already there so
I chose that one :)
+
/**
* struct dpu_hw_intf_ops : Interface to the interface Hw driver
functions
* Assumption is these functions will be called after clocks are
enabled
@@ -70,7 +75,7 @@ struct intf_status {
* @get_autorefresh: Retrieve autorefresh config from
hardware
* Return: 0 on success, -ETIMEDOUT
on timeout
* @vsync_sel: Select vsync signal for
tear-effect configuration
- * @enable_compression: Enable data compression
+ * @program_datapath: Program the DPU to interface
datapath for relevant chipsets
*/
struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
*/
void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t
encoder_id, u16 vdisplay);
- void (*enable_compression)(struct dpu_hw_intf *intf);
+ /**
+ * Program the DPU to intf datapath by specifying
+ * which of the settings need to be programmed for
+ * use-cases which need DPU-intf handshake such as
+ * widebus, compression etc.
This is not a valid kerneldoc.
hmmm ... ok so just // ?
I referred disable_autorefresh from above and did the same.
+ */
+ void (*program_datapath)(struct dpu_hw_intf *intf,
+ struct intf_dpu_datapath_cfg *datapath_cfg);
};
struct dpu_hw_intf {