[RFC 3/4] drm/sun4i: Reimplement plane z position setting logic

[Maxime Ripard]

On Sun, Dec 29, 2019 at 02:02:33PM +0100, Jernej Škrabec wrote:
> Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko
> napisal(a):
> > On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec <jernej.skrabec at siol.net>
> wrote:
> > > Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
> > >
> > > napisal(a):
> > > > Hello Jernej,
> > > >
> > > > Thank you for review.
> > > >
> > > > On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec
> > > > <jernej.skrabec at siol.net>
> > >
> > > wrote:
> > > > > Hi!
> > > > >
> > > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > > >
> > > > > roman.stratiienko at globallogic.com napisal(a):
> > > > > > From: Roman Stratiienko <roman.stratiienko at globallogic.com>
> > > > > >
> > > > > > To set blending channel order register software needs to know state
> > > > > > and
> > > > > > position of each channel, which impossible at plane commit stage.
> > > > > >
> > > > > > Move this procedure to atomic_flush stage, where all necessary
> > > > > > information
> > > > > > is available.
> > > > > >
> > > > > > Signed-off-by: Roman Stratiienko <roman.stratiienko at globallogic.com>
> > > > > > ---
> > > > > >
> > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 47
> > > > > >  +++++++++++++++++++++++++-
> > > > > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  3 ++
> > > > > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++-------------------
> > > > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------
> > > > > >  4 files changed, 60 insertions(+), 71 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index
> > > > > > bb9a665fd053..da84fccf7784
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > > > > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct
> > > > > > sunxi_engine
> > > > > > *engine,
> > > > > >
> > > > > >  static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > > > > >  {
> > > > > >
> > > > > > -     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > > > +     struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > > > > > +     u32 base = sun8i_blender_base(mixer);
> > > > > > +     int i, j;
> > > > > > +     int channel_by_zpos[4] = {-1, -1, -1, -1};
> > > > > > +     u32 route = 0, pipe_ctl = 0;
> > > > > > +
> > > > > > +     DRM_DEBUG_DRIVER("Update blender routing\n");
> > > > >
> > > > > Use drm_dbg().
> > > > >
> > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > >
> > > > > channel_zpos can hold 5 elements which is also theoretical maximum for
> > > > > current HW design. Why do you check only 4 elements?
> > > >
> > > > I'll use plane_cnt as it done in mixer_bind
> > > >
> > > > > It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so
> > > > > everyone would understand where this number comes from.
> > > >
> > > > Will do.
> > > >
> > > > > > +
> > > > > > +             if (zpos >= 0 && zpos < 4)
> > > > > > +                     channel_by_zpos[zpos] = i;
> > > > > > +     }
> > > > > > +
> > > > > > +     j = 0;
> > > > > > +     for (i = 0; i < 4; i++) {
> > > > > > +             int ch = channel_by_zpos[i];
> > > > > > +
> > > > > > +             if (ch >= 0) {
> > > > > > +                     pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > > > > > +                     route |= ch <<
> > > > >
> > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > >
> > > > > > +                     j++;
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > > +     for (i = 0; i < 4 && j < 4; i++) {
> > > > > > +             int zpos = mixer->channel_zpos[i];
> > > > > >
> > > > > > +             if (zpos < 0) {
> > > > > > +                     route |= i <<
> > > > >
> > > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > > > >
> > > > > > +                     j++;
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > > +     regmap_update_bits(mixer->engine.regs,
> > > > >
> > > > > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > > > >
> > > > > > +                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
> > > > >
> > > > > pipe_ctl);
> > > > >
> > > > > > +
> > > > > > +     regmap_write(mixer->engine.regs,
> > > > > > +                  SUN8I_MIXER_BLEND_ROUTE(base), route);
> > > > > > +
> > > > > > +     DRM_DEBUG_DRIVER("Committing changes\n");
> > > > >
> > > > > Use drm_dbg().
> > > >
> > > > According to
> > > > https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3
> > > > beb
> > > > 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 ,
> > > > DRM_DEBUG_DRIVER uses drm_dbg.
> > > > Also, using drm_dbg with category macro would require larger indent,
> > > > making harder to fit in 80 chars limit.
> > >
> > > From what I can see, category is already defined by macro name. Check
> > > here:
> > > https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n46
> > > 5
> > >
> > > So it should be actually shorter.
> >
> > Ah, it something very recent.
> > drm_dbg also require drm_device struct
> > Do you know the best way to extract it from `struct engine`?
>
> I don't think there is a way. I guess we can solve this later. Maxime, any
> thoughts?

There's no way at the moment, but it would make sense to add a pointer
to it.

Maximey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200102/ed5650b8/attachment-0001.sig>