On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote:
On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/26/2024 4:39 PM, Paloma Arellano wrote:
On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
On 25/01/2024 21:38, Paloma Arellano wrote:
Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
Signed-off-by: Paloma Arellano <quic_parellan@xxxxxxxxxxx>
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 +--
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 31 ++++++++++---------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 993f263433314..37ac385727c3b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -153,6 +153,7 @@ enum dpu_intr_idx {
* @hw_intf: Hardware interface to the intf registers
* @hw_wb: Hardware interface to the wb registers
* @hw_cdm: Hardware interface to the CDM registers
+ * @cdm_cfg: CDM block config needed to store WB/DP block's CDM
configuration
Please realign the description.
Ack
* @dpu_kms: Pointer to the dpu_kms top level
* @cached_mode: DRM mode cached at mode_set time, acted on in
enable
* @vblank_ctl_lock: Vblank ctl mutex lock to protect
vblank_refcount
@@ -183,6 +184,7 @@ struct dpu_encoder_phys {
struct dpu_hw_intf *hw_intf;
struct dpu_hw_wb *hw_wb;
struct dpu_hw_cdm *hw_cdm;
+ struct dpu_hw_cdm_cfg cdm_cfg;
It might be slightly better to move it after all the pointers, so
after the dpu_kms.
Ack
struct dpu_kms *dpu_kms;
struct drm_display_mode cached_mode;
struct mutex vblank_ctl_lock;
@@ -213,7 +215,6 @@ static inline int
dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
* @wbirq_refcount: Reference count of writeback interrupt
* @wb_done_timeout_cnt: number of wb done irq timeout errors
* @wb_cfg: writeback block config to store fb related details
- * @cdm_cfg: cdm block config needed to store writeback block's CDM
configuration
* @wb_conn: backpointer to writeback connector
* @wb_job: backpointer to current writeback job
* @dest: dpu buffer layout for current writeback output buffer
@@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
atomic_t wbirq_refcount;
int wb_done_timeout_cnt;
struct dpu_hw_wb_cfg wb_cfg;
- struct dpu_hw_cdm_cfg cdm_cfg;
struct drm_writeback_connector *wb_conn;
struct drm_writeback_job *wb_job;
struct dpu_hw_fmt_layout dest;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 4cd2d9e3131a4..072fc6950e496 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -269,28 +269,21 @@ static void
dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
* This API does not handle
DPU_CHROMA_H1V2.
* @phys_enc:Pointer to physical encoder
*/
-static void dpu_encoder_helper_phys_setup_cdm(struct
dpu_encoder_phys *phys_enc)
+static void dpu_encoder_helper_phys_setup_cdm(struct
dpu_encoder_phys *phys_enc,
+ const struct dpu_format *dpu_fmt,
+ u32 output_type)
{
struct dpu_hw_cdm *hw_cdm;
struct dpu_hw_cdm_cfg *cdm_cfg;
struct dpu_hw_pingpong *hw_pp;
- struct dpu_encoder_phys_wb *wb_enc;
- const struct msm_format *format;
- const struct dpu_format *dpu_fmt;
- struct drm_writeback_job *wb_job;
int ret;
if (!phys_enc)
return;
- wb_enc = to_dpu_encoder_phys_wb(phys_enc);
- cdm_cfg = &wb_enc->cdm_cfg;
+ cdm_cfg = &phys_enc->cdm_cfg;
hw_pp = phys_enc->hw_pp;
hw_cdm = phys_enc->hw_cdm;
- wb_job = wb_enc->wb_job;
-
- format = msm_framebuffer_format(wb_enc->wb_job->fb);
- dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
wb_job->fb->modifier);
if (!hw_cdm)
return;
@@ -306,10 +299,10 @@ static void
dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
- cdm_cfg->output_width = wb_job->fb->width;
- cdm_cfg->output_height = wb_job->fb->height;
+ cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
+ cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
This is a semantic change. Instead of passing the FB size, this passes
the mode dimensions. They are not guaranteed to be the same,
especially for the WB case.
The WB job is storing the output FB of WB. I cannot think of a use-case
where this cannot match the current mode programmed to the WB encoder.
Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
plane can scale the contents but here thats not the case. Here its the
output FB of WB.
Is it a part of WB uAPI, to have the FB dimensions equal to mode
dimensions? Or is it just our current limitation? I can easily imagine
WB outputting data to a part of the FB (just like we can clip FB using
plane's clip rectangle).
This boils down to a question, whether CDM should be setup in terms of
actual output date or the physical memory buffer parameters. I suspect
the former is the case (which makes this change correct). But it
either should be described in the commit message or (even better)
split to a separate commit.
I would say its a combination of both today.
The way I would look at it is even if WB crops a certain section of FB,
that will not change the FB size. FB size of WB should match the rest of
the DRM pipeline (the mode programmed to the CRTC/encoder). If WB
decides to write to only a small section of FB (cropping), then we need
another WB property like CROP_ROI so that we can program the WB to only
write to a small section of the programmed FB. So in some sense, there
is no such support in DRM UAPI today. Hence the FB of WB is the full
mode of the WB.
I'd say, CROP_ROI can refer to cropping of the image source (esp. in
the cloned output case). For writing to the part of the FB there can
be DST_X/_Y/_W/_H properties. But this becomes off-topic.
CDM is before WB so follows the rest of the pipeline that is whatever
the data feeding it was programmed to.
Yes. So the change is correct, but it should be split or documented
properly. I prefer the first option.
Ok just to clarify you prefer below part of the change to be moved to
its own commit right?
- cdm_cfg->output_width = wb_job->fb->width;
- cdm_cfg->output_height = wb_job->fb->height;
+ cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
+ cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
If so, ack.
