On 1/4/2023 6:16 PM, Dmitry Baryshkov wrote:
On 05/01/2023 01:40, Jessica Zhang wrote:Initialize and use the color_fill properties for planes in DPU driver. In addition, relax framebuffer requirements within atomic commit path and add checks for NULL framebuffers. Finally, drop DPU_PLANE_COLOR_FILL_FLAG as it's unused. Changes since V2: - Fixed dropped 'const' warning - Dropped use of solid_fill_format - Switched to using drm_plane_solid_fill_enabled helper method - Added helper to convert color fill to BGR888 (Rob) - Added support for solid fill on planes of varying sizes - Removed DPU_PLANE_COLOR_FILL_FLAG Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 65 ++++++++++++++--------- 2 files changed, 49 insertions(+), 25 deletions(-)diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.cindex 13ce321283ff..0695b70ea1b7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c@@ -409,6 +409,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,struct drm_plane_state *state; struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); struct dpu_plane_state *pstate = NULL; + const struct msm_format *fmt; struct dpu_format *format; struct dpu_hw_ctl *ctl = mixer->lm_ctl;@@ -441,7 +442,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,sspp_idx - SSPP_VIG0, state->fb ? state->fb->base.id : -1); - format = to_dpu_format(msm_framebuffer_format(pstate->base.fb)); + if (pstate->base.fb) + fmt = msm_framebuffer_format(pstate->base.fb); + else + fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base, + DRM_FORMAT_ABGR8888, 0); + + format = to_dpu_format(fmt); if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable) bg_alpha_enable = true;diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.cindex 86719020afe2..51a7507373f7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -44,7 +44,6 @@ #define DPU_NAME_SIZE 12 -#define DPU_PLANE_COLOR_FILL_FLAG BIT(31) #define DPU_ZPOS_MAX 255 /* multirect rect index */ @@ -105,7 +104,6 @@ struct dpu_plane { enum dpu_sspp pipe; struct dpu_hw_pipe *pipe_hw; - uint32_t color_fill; bool is_error; bool is_rt_pipe; const struct dpu_mdss_cfg *catalog;@@ -678,6 +676,17 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,&scaler3_cfg); }+static uint32_t _dpu_plane_get_fill_color(struct drm_solid_fill solid_fill)+{ + uint32_t ret = 0; + + ret |= ((uint8_t) solid_fill.b) << 16; + ret |= ((uint8_t) solid_fill.g) << 8; + ret |= ((uint8_t) solid_fill.r); + + return ret; +} + /** * _dpu_plane_color_fill - enables color fill on plane * @pdpu: Pointer to DPU plane object@@ -1001,12 +1010,17 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,dst = drm_plane_state_dest(new_plane_state); - fb_rect.x2 = new_plane_state->fb->width; - fb_rect.y2 = new_plane_state->fb->height; + if (new_plane_state->fb) { + fb_rect.x2 = new_plane_state->fb->width; + fb_rect.y2 = new_plane_state->fb->height; + } max_linewidth = pdpu->catalog->caps->max_linewidth; - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); + if (new_plane_state->fb)+ fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));+ else + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);I think this should be more explicit: if (solid_fill) fmt = dpu_get_dpu_format(...) else fmt = to_dpu_format(msm_framebuffer_format(...). And in the _dpu_crtc_blend_setup_mixer() too.
Hi Dmitry, Noted.
Maybe the code can be extracted to a helper.min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;@@ -1018,7 +1032,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,return -EINVAL; /* check src bounds */ - } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {+ } else if (new_plane_state->fb && !dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", DRM_RECT_ARG(&src)); return -E2BIG; @@ -1086,9 +1100,10 @@ void dpu_plane_flush(struct drm_plane *plane) if (pdpu->is_error) /* force white frame with 100% alpha pipe output on error */ _dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF); - else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)+ else if (!(plane->state->fb) && drm_plane_solid_fill_enabled(plane->state))/* force 100% alpha */ - _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);+ _dpu_plane_color_fill(pdpu, _dpu_plane_get_fill_color(plane->state->solid_fill),+ 0xFF);I'd push alpha into _dpu_plane_get_fill_color(). Then adding alpha support would be more transparent.
Acked.
else if (pdpu->pipe_hw && pdpu->pipe_hw->ops.setup_csc) {const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(plane->state->fb)); const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, fmt); @@ -1127,23 +1142,30 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)struct drm_crtc *crtc = state->crtc; struct drm_framebuffer *fb = state->fb; bool is_rt_pipe, update_qos_remap; - const struct dpu_format *fmt = - to_dpu_format(msm_framebuffer_format(fb)); + const struct dpu_format *fmt; struct dpu_hw_pipe_cfg pipe_cfg; - memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); - - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb); - pstate->pending = true; is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT); _dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);- DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT - ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),- crtc->base.id, DRM_RECT_ARG(&state->dst), - (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt)); + /* override for color fill */ + if (!fb && drm_plane_solid_fill_enabled(plane->state)) { + /* skip remaining processing on color fill */ + return; + } + + memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); + + fmt = to_dpu_format(msm_framebuffer_format(fb)); + _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);This change deserves a comment somewhere (in the commit message?). Beforehand the driver tried to set the scanout/layout for the COLOR_FILL case. You have changed this.
Sounds good. Thanks, Jessica Zhang
+ + if (fb)+ DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT + ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),+ crtc->base.id, DRM_RECT_ARG(&state->dst),+ (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));pipe_cfg.src_rect = state->src;@@ -1155,12 +1177,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)pipe_cfg.dst_rect = state->dst; - /* override for color fill */ - if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) { - /* skip remaining processing on color fill */ - return; - } - if (pdpu->pipe_hw->ops.setup_rects) { pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw, &pipe_cfg,@@ -1511,6 +1527,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,DPU_ERROR("failed to install zpos property, rc = %d\n", ret); drm_plane_create_alpha_property(plane); + drm_plane_create_solid_fill_property(plane); drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PIXEL_NONE) | BIT(DRM_MODE_BLEND_PREMULTI) |-- With best wishes Dmitry
