Re: [Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes
On Fri, 6 Jan 2023 at 22:57, Jessica Zhang wrote: > 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 > >> --- > >> 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.c > >> index 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_ABGR, 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.c > >> index 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_FLAGBIT(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_ABGR); > > > > 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,
Re: [Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes
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 --- 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.c index 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_ABGR, 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.c index 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_ABGR); 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, 0xFF, 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-
Re: [Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes
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 --- 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.c index 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_ABGR, 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.c index 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_ABGR); 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. 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, 0xFF, 0xFF); - else if (pdpu->color_fill & DPU
[Freedreno] [RFC PATCH v3 3/3] drm/msm/dpu: Use color_fill property for DPU planes
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 --- 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.c index 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_ABGR, 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.c index 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_ABGR); 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, 0xFF, 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_fil