Message ID | 20240625-dpu-mode-config-width-v5-8-501d984d634f@linaro.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | drm/msm/dpu: be more friendly to X.org | expand |
On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote: > The struct dpu_hw_fmt_layout defines hardware data layout (addresses, > sizes and pitches. Drop format field from this structure as it's not a Missing closing brace ")" here? > part of the data layout. > Its a bit subjective IMO whether you consider format as part of hardware data layout or not. Registers do have format bitfields too so I am somewhat unsure if this change is really needed. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 31 +++++++--------------- > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 23 ++++++++-------- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 -- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 4 +-- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 3 ++- > 5 files changed, 25 insertions(+), 38 deletions(-) > <Snip> > @@ -318,15 +318,10 @@ static void dpu_encoder_phys_wb_setup( > { > struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; > struct drm_display_mode mode = phys_enc->cached_mode; > - struct drm_framebuffer *fb = NULL; > struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc); > - struct drm_writeback_job *wb_job; > const struct msm_format *format; > - const struct msm_format *dpu_fmt; > > - wb_job = wb_enc->wb_job; > format = msm_framebuffer_format(wb_enc->wb_job->fb); > - dpu_fmt = mdp_get_format(&phys_enc->dpu_kms->base, format->pixel_format, wb_job->fb->modifier); > This is interesting. I wonder why I just didnt use format directly that time itself :) Maybe I was thinking that mdp_get_format() will also match the modifiers and return the corresponding msm_format. > DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", > hw_wb->idx - WB_0, mode.name, > @@ -338,9 +333,9 @@ static void dpu_encoder_phys_wb_setup( > > dpu_encoder_phys_wb_set_qos(phys_enc); > > - dpu_encoder_phys_wb_setup_fb(phys_enc, fb); > + dpu_encoder_phys_wb_setup_fb(phys_enc, format); > > - dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB); > + dpu_encoder_helper_phys_setup_cdm(phys_enc, format, CDM_CDWN_OUTPUT_WB); > > dpu_encoder_phys_wb_setup_ctl(phys_enc); > } > @@ -584,14 +579,6 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc > > format = msm_framebuffer_format(job->fb); > > - wb_cfg->dest.format = mdp_get_format(&phys_enc->dpu_kms->base, > - format->pixel_format, job->fb->modifier); > - if (!wb_cfg->dest.format) { > - /* this error should be detected during atomic_check */ > - DPU_ERROR("failed to get format %p4cc\n", &format->pixel_format); > - return; > - } > - > ret = dpu_format_populate_layout(aspace, job->fb, &wb_cfg->dest); > if (ret) { > DPU_DEBUG("failed to populate layout %d\n", ret);
On Wed, 17 Jul 2024 at 23:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote: > > The struct dpu_hw_fmt_layout defines hardware data layout (addresses, > > sizes and pitches. Drop format field from this structure as it's not a > Missing closing brace ")" here? > > > part of the data layout. > > > > Its a bit subjective IMO whether you consider format as part of hardware > data layout or not. Registers do have format bitfields too so I am > somewhat unsure if this change is really needed. It's not a part of the data buffer layout (num_planes, sizes, pitches and offsets) - the items that are defined by struct dpu_hw_fmt_layout. As such there is no need to store it in the structure. When necessary we can always get it from the framebuffer itself. > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 31 +++++++--------------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 23 ++++++++-------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 -- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 4 +-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 3 ++- > > 5 files changed, 25 insertions(+), 38 deletions(-) > > > > <Snip> > > > @@ -318,15 +318,10 @@ static void dpu_encoder_phys_wb_setup( > > { > > struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; > > struct drm_display_mode mode = phys_enc->cached_mode; > > - struct drm_framebuffer *fb = NULL; > > struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc); > > - struct drm_writeback_job *wb_job; > > const struct msm_format *format; > > - const struct msm_format *dpu_fmt; > > > > - wb_job = wb_enc->wb_job; > > format = msm_framebuffer_format(wb_enc->wb_job->fb); > > - dpu_fmt = mdp_get_format(&phys_enc->dpu_kms->base, format->pixel_format, wb_job->fb->modifier); > > > > This is interesting. I wonder why I just didnt use format directly that > time itself :) > > Maybe I was thinking that mdp_get_format() will also match the modifiers > and return the corresponding msm_format. > > > DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", > > hw_wb->idx - WB_0, mode.name, > > @@ -338,9 +333,9 @@ static void dpu_encoder_phys_wb_setup( > > > > dpu_encoder_phys_wb_set_qos(phys_enc); > > > > - dpu_encoder_phys_wb_setup_fb(phys_enc, fb); > > + dpu_encoder_phys_wb_setup_fb(phys_enc, format); > > > > - dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB); > > + dpu_encoder_helper_phys_setup_cdm(phys_enc, format, CDM_CDWN_OUTPUT_WB); > > > > dpu_encoder_phys_wb_setup_ctl(phys_enc); > > } > > @@ -584,14 +579,6 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc > > > > format = msm_framebuffer_format(job->fb); > > > > - wb_cfg->dest.format = mdp_get_format(&phys_enc->dpu_kms->base, > > - format->pixel_format, job->fb->modifier); > > - if (!wb_cfg->dest.format) { > > - /* this error should be detected during atomic_check */ > > - DPU_ERROR("failed to get format %p4cc\n", &format->pixel_format); > > - return; > > - } > > - > > ret = dpu_format_populate_layout(aspace, job->fb, &wb_cfg->dest); > > if (ret) { > > DPU_DEBUG("failed to populate layout %d\n", ret);
On 7/17/2024 3:09 PM, Dmitry Baryshkov wrote: > On Wed, 17 Jul 2024 at 23:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote: >>> The struct dpu_hw_fmt_layout defines hardware data layout (addresses, >>> sizes and pitches. Drop format field from this structure as it's not a >> Missing closing brace ")" here? >> >>> part of the data layout. >>> >> >> Its a bit subjective IMO whether you consider format as part of hardware >> data layout or not. Registers do have format bitfields too so I am >> somewhat unsure if this change is really needed. > > It's not a part of the data buffer layout (num_planes, sizes, pitches > and offsets) - the items that are defined by struct dpu_hw_fmt_layout. > As such there is no need to store it in the structure. When necessary > we can always get it from the framebuffer itself. > Alright, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 31 +++++++--------------- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 23 ++++++++-------- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 -- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 4 +-- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 3 ++- >>> 5 files changed, 25 insertions(+), 38 deletions(-) >>> >> >> <Snip> >> >>> @@ -318,15 +318,10 @@ static void dpu_encoder_phys_wb_setup( >>> { >>> struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; >>> struct drm_display_mode mode = phys_enc->cached_mode; >>> - struct drm_framebuffer *fb = NULL; >>> struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc); >>> - struct drm_writeback_job *wb_job; >>> const struct msm_format *format; >>> - const struct msm_format *dpu_fmt; >>> >>> - wb_job = wb_enc->wb_job; >>> format = msm_framebuffer_format(wb_enc->wb_job->fb); >>> - dpu_fmt = mdp_get_format(&phys_enc->dpu_kms->base, format->pixel_format, wb_job->fb->modifier); >>> >> >> This is interesting. I wonder why I just didnt use format directly that >> time itself :) >> >> Maybe I was thinking that mdp_get_format() will also match the modifiers >> and return the corresponding msm_format. >> >>> DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", >>> hw_wb->idx - WB_0, mode.name, >>> @@ -338,9 +333,9 @@ static void dpu_encoder_phys_wb_setup( >>> >>> dpu_encoder_phys_wb_set_qos(phys_enc); >>> >>> - dpu_encoder_phys_wb_setup_fb(phys_enc, fb); >>> + dpu_encoder_phys_wb_setup_fb(phys_enc, format); >>> >>> - dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB); >>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, format, CDM_CDWN_OUTPUT_WB); >>> >>> dpu_encoder_phys_wb_setup_ctl(phys_enc); >>> } >>> @@ -584,14 +579,6 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc >>> >>> format = msm_framebuffer_format(job->fb); >>> >>> - wb_cfg->dest.format = mdp_get_format(&phys_enc->dpu_kms->base, >>> - format->pixel_format, job->fb->modifier); >>> - if (!wb_cfg->dest.format) { >>> - /* this error should be detected during atomic_check */ >>> - DPU_ERROR("failed to get format %p4cc\n", &format->pixel_format); >>> - return; >>> - } >>> - >>> ret = dpu_format_populate_layout(aspace, job->fb, &wb_cfg->dest); >>> if (ret) { >>> DPU_DEBUG("failed to populate layout %d\n", ret); > > >
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 882c717859ce..c4a16a73bc97 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 @@ -166,10 +166,10 @@ static void dpu_encoder_phys_wb_set_qos(struct dpu_encoder_phys *phys_enc) /** * dpu_encoder_phys_wb_setup_fb - setup output framebuffer * @phys_enc: Pointer to physical encoder - * @fb: Pointer to output framebuffer + * @format: Format of the framebuffer */ static void dpu_encoder_phys_wb_setup_fb(struct dpu_encoder_phys *phys_enc, - struct drm_framebuffer *fb) + const struct msm_format *format) { struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc); struct dpu_hw_wb *hw_wb; @@ -193,12 +193,12 @@ static void dpu_encoder_phys_wb_setup_fb(struct dpu_encoder_phys *phys_enc, hw_wb->ops.setup_roi(hw_wb, wb_cfg); if (hw_wb->ops.setup_outformat) - hw_wb->ops.setup_outformat(hw_wb, wb_cfg); + hw_wb->ops.setup_outformat(hw_wb, wb_cfg, format); if (hw_wb->ops.setup_cdp) { const struct dpu_perf_cfg *perf = phys_enc->dpu_kms->catalog->perf; - hw_wb->ops.setup_cdp(hw_wb, wb_cfg->dest.format, + hw_wb->ops.setup_cdp(hw_wb, format, perf->cdp_cfg[DPU_PERF_CDP_USAGE_NRT].wr_enable); } @@ -318,15 +318,10 @@ static void dpu_encoder_phys_wb_setup( { struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; struct drm_display_mode mode = phys_enc->cached_mode; - struct drm_framebuffer *fb = NULL; struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc); - struct drm_writeback_job *wb_job; const struct msm_format *format; - const struct msm_format *dpu_fmt; - wb_job = wb_enc->wb_job; format = msm_framebuffer_format(wb_enc->wb_job->fb); - dpu_fmt = mdp_get_format(&phys_enc->dpu_kms->base, format->pixel_format, wb_job->fb->modifier); DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", hw_wb->idx - WB_0, mode.name, @@ -338,9 +333,9 @@ static void dpu_encoder_phys_wb_setup( dpu_encoder_phys_wb_set_qos(phys_enc); - dpu_encoder_phys_wb_setup_fb(phys_enc, fb); + dpu_encoder_phys_wb_setup_fb(phys_enc, format); - dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB); + dpu_encoder_helper_phys_setup_cdm(phys_enc, format, CDM_CDWN_OUTPUT_WB); dpu_encoder_phys_wb_setup_ctl(phys_enc); } @@ -584,14 +579,6 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc format = msm_framebuffer_format(job->fb); - wb_cfg->dest.format = mdp_get_format(&phys_enc->dpu_kms->base, - format->pixel_format, job->fb->modifier); - if (!wb_cfg->dest.format) { - /* this error should be detected during atomic_check */ - DPU_ERROR("failed to get format %p4cc\n", &format->pixel_format); - return; - } - ret = dpu_format_populate_layout(aspace, job->fb, &wb_cfg->dest); if (ret) { DPU_DEBUG("failed to populate layout %d\n", ret); @@ -600,10 +587,10 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc wb_cfg->dest.width = job->fb->width; wb_cfg->dest.height = job->fb->height; - wb_cfg->dest.num_planes = wb_cfg->dest.format->num_planes; + wb_cfg->dest.num_planes = format->num_planes; - if ((wb_cfg->dest.format->fetch_type == MDP_PLANE_PLANAR) && - (wb_cfg->dest.format->element[0] == C1_B_Cb)) + if ((format->fetch_type == MDP_PLANE_PLANAR) && + (format->element[0] == C1_B_Cb)) swap(wb_cfg->dest.plane_addr[1], wb_cfg->dest.plane_addr[2]); DPU_DEBUG("[fb_offset:%8.8x,%8.8x,%8.8x,%8.8x]\n", diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c index 8c2dc5b59bb0..46237a1ca6a5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c @@ -104,7 +104,6 @@ static int _dpu_format_get_plane_sizes_ubwc( bool meta = MSM_FORMAT_IS_UBWC(fmt); memset(layout, 0, sizeof(struct dpu_hw_fmt_layout)); - layout->format = fmt; layout->width = width; layout->height = height; layout->num_planes = fmt->num_planes; @@ -116,7 +115,7 @@ static int _dpu_format_get_plane_sizes_ubwc( return -EINVAL; } - if (MSM_FORMAT_IS_YUV(layout->format)) { + if (MSM_FORMAT_IS_YUV(fmt)) { uint32_t y_sclines, uv_sclines; uint32_t y_meta_scanlines = 0; uint32_t uv_meta_scanlines = 0; @@ -182,7 +181,6 @@ static int _dpu_format_get_plane_sizes_linear( int i; memset(layout, 0, sizeof(struct dpu_hw_fmt_layout)); - layout->format = fmt; layout->width = width; layout->height = height; layout->num_planes = fmt->num_planes; @@ -190,8 +188,8 @@ static int _dpu_format_get_plane_sizes_linear( /* Due to memset above, only need to set planes of interest */ if (fmt->fetch_type == MDP_PLANE_INTERLEAVED) { layout->num_planes = 1; - layout->plane_size[0] = width * height * layout->format->bpp; - layout->plane_pitch[0] = width * layout->format->bpp; + layout->plane_size[0] = width * height * fmt->bpp; + layout->plane_pitch[0] = width * fmt->bpp; } else { uint32_t v_subsample, h_subsample; uint32_t chroma_samp; @@ -272,6 +270,7 @@ static int _dpu_format_populate_addrs_ubwc( struct drm_framebuffer *fb, struct dpu_hw_fmt_layout *layout) { + const struct msm_format *fmt; uint32_t base_addr = 0; bool meta; @@ -286,10 +285,11 @@ static int _dpu_format_populate_addrs_ubwc( return -EFAULT; } - meta = MSM_FORMAT_IS_UBWC(layout->format); + fmt = msm_framebuffer_format(fb); + meta = MSM_FORMAT_IS_UBWC(fmt); /* Per-format logic for verifying active planes */ - if (MSM_FORMAT_IS_YUV(layout->format)) { + if (MSM_FORMAT_IS_YUV(fmt)) { /************************************************/ /* UBWC ** */ /* buffer ** DPU PLANE */ @@ -390,6 +390,7 @@ int dpu_format_populate_layout( struct drm_framebuffer *fb, struct dpu_hw_fmt_layout *layout) { + const struct msm_format *fmt; int ret; if (!fb || !layout) { @@ -403,17 +404,17 @@ int dpu_format_populate_layout( return -ERANGE; } - layout->format = msm_framebuffer_format(fb); + fmt = msm_framebuffer_format(fb); /* Populate the plane sizes etc via get_format */ - ret = dpu_format_get_plane_sizes(layout->format, fb->width, fb->height, + ret = dpu_format_get_plane_sizes(fmt, fb->width, fb->height, layout, fb->pitches); if (ret) return ret; /* Populate the addresses given the fb */ - if (MSM_FORMAT_IS_UBWC(layout->format) || - MSM_FORMAT_IS_TILE(layout->format)) + if (MSM_FORMAT_IS_UBWC(fmt) || + MSM_FORMAT_IS_TILE(fmt)) ret = _dpu_format_populate_addrs_ubwc(aspace, fb, layout); else ret = _dpu_format_populate_addrs_linear(aspace, fb, layout); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h index a2eff36a2224..f8806a4d317b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h @@ -293,7 +293,6 @@ enum dpu_3d_blend_mode { /** * struct dpu_hw_fmt_layout - format information of the source pixel data - * @format: pixel format parameters * @num_planes: number of planes (including meta data planes) * @width: image width * @height: image height @@ -303,7 +302,6 @@ enum dpu_3d_blend_mode { * @plane_pitch: pitch of each plane */ struct dpu_hw_fmt_layout { - const struct msm_format *format; uint32_t num_planes; uint32_t width; uint32_t height; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c index 93ff01c889b5..f39db534697d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c @@ -64,10 +64,10 @@ static void dpu_hw_wb_setup_outaddress(struct dpu_hw_wb *ctx, } static void dpu_hw_wb_setup_format(struct dpu_hw_wb *ctx, - struct dpu_hw_wb_cfg *data) + struct dpu_hw_wb_cfg *data, + const struct msm_format *fmt) { struct dpu_hw_blk_reg_map *c = &ctx->hw; - const struct msm_format *fmt = data->dest.format; u32 dst_format, pattern, ystride0, ystride1, outsize, chroma_samp; u32 write_config = 0; u32 opmode = 0; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h index 37497473e16c..b240a4f7b33a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h @@ -37,7 +37,8 @@ struct dpu_hw_wb_ops { struct dpu_hw_wb_cfg *wb); void (*setup_outformat)(struct dpu_hw_wb *ctx, - struct dpu_hw_wb_cfg *wb); + struct dpu_hw_wb_cfg *wb, + const struct msm_format *fmt); void (*setup_roi)(struct dpu_hw_wb *ctx, struct dpu_hw_wb_cfg *wb);
The struct dpu_hw_fmt_layout defines hardware data layout (addresses, sizes and pitches. Drop format field from this structure as it's not a part of the data layout. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 31 +++++++--------------- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 23 ++++++++-------- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 4 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 3 ++- 5 files changed, 25 insertions(+), 38 deletions(-)