On Fri, Apr 19, 2024 at 07:32:35PM -0700, Abhinav Kumar wrote:
>
>
> On 4/19/2024 6:26 PM, Dmitry Baryshkov wrote:
> > On Fri, Apr 19, 2024 at 04:43:20PM -0700, Abhinav Kumar wrote:
> > >
> > >
> > > On 3/19/2024 6:21 AM, Dmitry Baryshkov wrote:
> > > > The msm_kms_funcs::check_modified_format() callback is not used by the
> > > > driver. Drop it completely.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 45 -----------------------------
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 15 ----------
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 -
> > > > drivers/gpu/drm/msm/msm_kms.h | 5 ----
> > > > 4 files changed, 66 deletions(-)
> > > >
> > >
> > > I think in this case, I am leaning towards completing the implementation
> > > rather than dropping it as usual.
> > >
> > > It seems its easier to just add the support to call this like the attached
> > > patch?
> >
> > Please don't attach patches to the email. It makes it impossible to
> > respond to them.
> >
>
> I attached it because it was too much to paste over here.
>
> Please review msm_framebuffer_init() in the downstream sources.
>
> The only missing piece I can see is the handling of DRM_MODE_FB_MODIFIERS
> flags.
I checked and I don't like this approach.
With the generic formats database in place, there should be no
driver-specific code that handles formats. Moreover, I think this should
be handled by the generic code in framebuffer_check() if msm driver
implements a proper get_format_info() callback. Please consider sending
a patch that does it. For now I can only consider the function in
question to be a dead code which should be dropped.
>
> I am unable to trace back why this support was not present.
>
> > Anyway, what are we missing with the current codebase? Why wasn't the
> > callback / function used in the first place?
> >
> > >
> > > WDYT?
> > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > > > index e366ab134249..ff0df478c958 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > > > @@ -960,51 +960,6 @@ int dpu_format_populate_layout(
> > > > return ret;
> > > > }
> > > > -int dpu_format_check_modified_format(
> > > > - const struct msm_kms *kms,
> > > > - const struct msm_format *msm_fmt,
> > > > - const struct drm_mode_fb_cmd2 *cmd,
> > > > - struct drm_gem_object **bos)
> > > > -{
> > > > - const struct drm_format_info *info;
> > > > - const struct dpu_format *fmt;
> > > > - struct dpu_hw_fmt_layout layout;
> > > > - uint32_t bos_total_size = 0;
> > > > - int ret, i;
> > > > -
> > > > - if (!msm_fmt || !cmd || !bos) {
> > > > - DRM_ERROR("invalid arguments\n");
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - fmt = to_dpu_format(msm_fmt);
> > > > - info = drm_format_info(fmt->base.pixel_format);
> > > > - if (!info)
> > > > - return -EINVAL;
> > > > -
> > > > - ret = dpu_format_get_plane_sizes(fmt, cmd->width, cmd->height,
> > > > - &layout, cmd->pitches);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > - for (i = 0; i < info->num_planes; i++) {
> > > > - if (!bos[i]) {
> > > > - DRM_ERROR("invalid handle for plane %d\n", i);
> > > > - return -EINVAL;
> > > > - }
> > > > - if ((i == 0) || (bos[i] != bos[0]))
> > > > - bos_total_size += bos[i]->size;
> > > > - }
> > > > -
> > > > - if (bos_total_size < layout.total_size) {
> > > > - DRM_ERROR("buffers total size too small %u expected %u\n",
> > > > - bos_total_size, layout.total_size);
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - return 0;
> > > > -}
> > > > -
> > > > const struct dpu_format *dpu_get_dpu_format_ext(
> > > > const uint32_t format,
> > > > const uint64_t modifier)
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
> > > > index 84b8b3289f18..9442445f1a86 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
> > > > @@ -54,21 +54,6 @@ const struct msm_format *dpu_get_msm_format(
> > > > const uint32_t format,
> > > > const uint64_t modifiers);
> > > > -/**
> > > > - * dpu_format_check_modified_format - validate format and buffers for
> > > > - * dpu non-standard, i.e. modified format
> > > > - * @kms: kms driver
> > > > - * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
> > > > - * @cmd: fb_cmd2 structure user request
> > > > - * @bos: gem buffer object list
> > > > - *
> > > > - * Return: error code on failure, 0 on success
> > > > - */
> > > > -int dpu_format_check_modified_format(
> > > > - const struct msm_kms *kms,
> > > > - const struct msm_format *msm_fmt,
> > > > - const struct drm_mode_fb_cmd2 *cmd,
> > > > - struct drm_gem_object **bos);
> > > > /**
> > > > * dpu_format_populate_layout - populate the given format layout based on
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > index a1f5d7c4ab91..7257ac4020d8 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > @@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
> > > > .complete_commit = dpu_kms_complete_commit,
> > > > .enable_vblank = dpu_kms_enable_vblank,
> > > > .disable_vblank = dpu_kms_disable_vblank,
> > > > - .check_modified_format = dpu_format_check_modified_format,
> > > > .get_format = dpu_get_msm_format,
> > > > .destroy = dpu_kms_destroy,
> > > > .snapshot = dpu_kms_mdp_snapshot,
> > > > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > > > index 0641f6111b93..b794ed918b56 100644
> > > > --- a/drivers/gpu/drm/msm/msm_kms.h
> > > > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > > > @@ -96,11 +96,6 @@ struct msm_kms_funcs {
> > > > const struct msm_format *(*get_format)(struct msm_kms *kms,
> > > > const uint32_t format,
> > > > const uint64_t modifiers);
> > > > - /* do format checking on format modified through fb_cmd2 modifiers */
> > > > - int (*check_modified_format)(const struct msm_kms *kms,
> > > > - const struct msm_format *msm_fmt,
> > > > - const struct drm_mode_fb_cmd2 *cmd,
> > > > - struct drm_gem_object **bos);
> > > > /* misc: */
> > > > long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
> > > >
> >
--
With best wishes
Dmitry
