Re: [PATCH v2 06/27] drm/msm/dpu: move pipe_hw to dpu_plane_state
On 30/01/2023 23:51, Abhinav Kumar wrote:
On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
In preparation to adding fully virtualized planes, move struct
dpu_hw_sspp instance from struct dpu_plane to struct dpu_plane_state, as
it will become a part of state (allocated during atomic check) rather
than part of a plane (allocated during boot).
I was thinking about a couple of things about this patch:
1) Since we are moving away from using "pipe" and using "sspp", perhaps
we can rename pipe_hw to hw_sspp in the below struct
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
@@ -35,6 +35,8 @@ struct dpu_plane_state {
uint32_t multirect_mode;
bool pending;
+ struct dpu_hw_pipe *hw_sspp;
+
Ack
u64 plane_fetch_bw;
u64 plane_clk;
};
2) I still dont see any comment as promised in v1 about why we are doing
this in dpu_plane_reset().
https://patchwork.freedesktop.org/patch/473155/?series=99909=1#comment_875365
I think what we need to mention is that the dpu_plane_reset() is the one
which allocates the plane state today and hence pipe_hw can only be
assigned there.
Ack
Rest LGTM.
--
With best wishes
Dmitry
Re: [PATCH v2 06/27] drm/msm/dpu: move pipe_hw to dpu_plane_state
On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
In preparation to adding fully virtualized planes, move struct
dpu_hw_sspp instance from struct dpu_plane to struct dpu_plane_state, as
it will become a part of state (allocated during atomic check) rather
than part of a plane (allocated during boot).
I was thinking about a couple of things about this patch:
1) Since we are moving away from using "pipe" and using "sspp", perhaps
we can rename pipe_hw to hw_sspp in the below struct
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
@@ -35,6 +35,8 @@ struct dpu_plane_state {
uint32_t multirect_mode;
bool pending;
+ struct dpu_hw_pipe *hw_sspp;
+
u64 plane_fetch_bw;
u64 plane_clk;
};
2) I still dont see any comment as promised in v1 about why we are doing
this in dpu_plane_reset().
https://patchwork.freedesktop.org/patch/473155/?series=99909=1#comment_875365
I think what we need to mention is that the dpu_plane_reset() is the one
which allocates the plane state today and hence pipe_hw can only be
assigned there.
Rest LGTM.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 102 --
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 +
2 files changed, 57 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 43fb8e00ada6..7ba954c7b3e0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -104,7 +104,6 @@ struct dpu_plane {
enum dpu_sspp pipe;
- struct dpu_hw_sspp *pipe_hw;
uint32_t color_fill;
bool is_error;
bool is_rt_pipe;
@@ -279,6 +278,7 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
struct drm_framebuffer *fb, struct dpu_hw_pipe_cfg *pipe_cfg)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
const struct dpu_format *fmt = NULL;
u64 qos_lut;
u32 total_fl = 0, lut_usage;
@@ -310,7 +310,7 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
fmt ? (char *)>base.pixel_format : NULL,
pdpu->is_rt_pipe, total_fl, qos_lut);
- pdpu->pipe_hw->ops.setup_creq_lut(pdpu->pipe_hw, qos_lut);
+ pstate->pipe_hw->ops.setup_creq_lut(pstate->pipe_hw, qos_lut);
}
/**
@@ -322,6 +322,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane
*plane,
struct drm_framebuffer *fb)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
const struct dpu_format *fmt = NULL;
u32 danger_lut, safe_lut;
@@ -361,7 +362,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
danger_lut,
safe_lut);
- pdpu->pipe_hw->ops.setup_danger_safe_lut(pdpu->pipe_hw,
+ pstate->pipe_hw->ops.setup_danger_safe_lut(pstate->pipe_hw,
danger_lut, safe_lut);
}
@@ -375,14 +376,15 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
bool enable, u32 flags)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
memset(_qos_cfg, 0, sizeof(pipe_qos_cfg));
if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
- pipe_qos_cfg.creq_vblank =
pdpu->pipe_hw->cap->sblk->creq_vblank;
+ pipe_qos_cfg.creq_vblank =
pstate->pipe_hw->cap->sblk->creq_vblank;
pipe_qos_cfg.danger_vblank =
- pdpu->pipe_hw->cap->sblk->danger_vblank;
+ pstate->pipe_hw->cap->sblk->danger_vblank;
pipe_qos_cfg.vblank_en = enable;
}
@@ -408,7 +410,7 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
pipe_qos_cfg.danger_vblank,
pdpu->is_rt_pipe);
- pdpu->pipe_hw->ops.setup_qos_ctrl(pdpu->pipe_hw,
+ pstate->pipe_hw->ops.setup_qos_ctrl(pstate->pipe_hw,
_qos_cfg);
}
@@ -422,18 +424,19 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
struct drm_crtc *crtc, struct dpu_hw_pipe_cfg *pipe_cfg)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
struct dpu_vbif_set_ot_params ot_params;
struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
memset(_params, 0, sizeof(ot_params));
- ot_params.xin_id = pdpu->pipe_hw->cap->xin_id;
- ot_params.num = pdpu->pipe_hw->idx - SSPP_NONE;
+ ot_params.xin_id = pstate->pipe_hw->cap->xin_id;
+ ot_params.num = pstate->pipe_hw->idx - SSPP_NONE;
[PATCH v2 06/27] drm/msm/dpu: move pipe_hw to dpu_plane_state
In preparation to adding fully virtualized planes, move struct
dpu_hw_sspp instance from struct dpu_plane to struct dpu_plane_state, as
it will become a part of state (allocated during atomic check) rather
than part of a plane (allocated during boot).
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 102 --
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 +
2 files changed, 57 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 43fb8e00ada6..7ba954c7b3e0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -104,7 +104,6 @@ struct dpu_plane {
enum dpu_sspp pipe;
- struct dpu_hw_sspp *pipe_hw;
uint32_t color_fill;
bool is_error;
bool is_rt_pipe;
@@ -279,6 +278,7 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
struct drm_framebuffer *fb, struct dpu_hw_pipe_cfg *pipe_cfg)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
const struct dpu_format *fmt = NULL;
u64 qos_lut;
u32 total_fl = 0, lut_usage;
@@ -310,7 +310,7 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
fmt ? (char *)>base.pixel_format : NULL,
pdpu->is_rt_pipe, total_fl, qos_lut);
- pdpu->pipe_hw->ops.setup_creq_lut(pdpu->pipe_hw, qos_lut);
+ pstate->pipe_hw->ops.setup_creq_lut(pstate->pipe_hw, qos_lut);
}
/**
@@ -322,6 +322,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane
*plane,
struct drm_framebuffer *fb)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
const struct dpu_format *fmt = NULL;
u32 danger_lut, safe_lut;
@@ -361,7 +362,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane
*plane,
danger_lut,
safe_lut);
- pdpu->pipe_hw->ops.setup_danger_safe_lut(pdpu->pipe_hw,
+ pstate->pipe_hw->ops.setup_danger_safe_lut(pstate->pipe_hw,
danger_lut, safe_lut);
}
@@ -375,14 +376,15 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane
*plane,
bool enable, u32 flags)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
memset(_qos_cfg, 0, sizeof(pipe_qos_cfg));
if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
- pipe_qos_cfg.creq_vblank =
pdpu->pipe_hw->cap->sblk->creq_vblank;
+ pipe_qos_cfg.creq_vblank =
pstate->pipe_hw->cap->sblk->creq_vblank;
pipe_qos_cfg.danger_vblank =
- pdpu->pipe_hw->cap->sblk->danger_vblank;
+ pstate->pipe_hw->cap->sblk->danger_vblank;
pipe_qos_cfg.vblank_en = enable;
}
@@ -408,7 +410,7 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
pipe_qos_cfg.danger_vblank,
pdpu->is_rt_pipe);
- pdpu->pipe_hw->ops.setup_qos_ctrl(pdpu->pipe_hw,
+ pstate->pipe_hw->ops.setup_qos_ctrl(pstate->pipe_hw,
_qos_cfg);
}
@@ -422,18 +424,19 @@ static void _dpu_plane_set_ot_limit(struct drm_plane
*plane,
struct drm_crtc *crtc, struct dpu_hw_pipe_cfg *pipe_cfg)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
struct dpu_vbif_set_ot_params ot_params;
struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
memset(_params, 0, sizeof(ot_params));
- ot_params.xin_id = pdpu->pipe_hw->cap->xin_id;
- ot_params.num = pdpu->pipe_hw->idx - SSPP_NONE;
+ ot_params.xin_id = pstate->pipe_hw->cap->xin_id;
+ ot_params.num = pstate->pipe_hw->idx - SSPP_NONE;
ot_params.width = drm_rect_width(_cfg->src_rect);
ot_params.height = drm_rect_height(_cfg->src_rect);
ot_params.is_wfd = !pdpu->is_rt_pipe;
ot_params.frame_rate = drm_mode_vrefresh(>mode);
ot_params.vbif_idx = VBIF_RT;
- ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl;
+ ot_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
ot_params.rd = true;
dpu_vbif_set_ot_limit(dpu_kms, _params);
@@ -446,14 +449,15 @@ static void _dpu_plane_set_ot_limit(struct drm_plane
*plane,
static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
+ struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
struct dpu_vbif_set_qos_params qos_params;
struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
memset(_params, 0,
