From: "Jernej Škrabec" <jernej.skrabec@siol.net>
To: Roman Stratiienko <roman.stratiienko@globallogic.com>
Cc: Maxime Ripard <mripard@kernel.org>,
dri-devel@lists.freedesktop.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic
Date: Sun, 29 Dec 2019 14:02:33 +0100 [thread overview]
Message-ID: <2015568.Icojqenx9y@jernej-laptop> (raw)
In-Reply-To: <CAODwZ7srrrbDk=kKjg2-amVtGzNsqqZ72JopHijtNPD=-EzjgA@mail.gmail.com>
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@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@siol.net>
> >
> > wrote:
> > > > Hi!
> > > >
> > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > >
> > > > roman.stratiienko@globallogic.com napisal(a):
> > > > > From: Roman Stratiienko <roman.stratiienko@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@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?
Best regards,
Jernej
>
> > > Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
> >
> > Yes, at least I understand following commit message in such manner:
> > https://cgit.freedesktop.org/drm/drm-misc/commit/?
> > id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
> >
> > Best regards,
> > Jernej
> >
> > > > > regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > > > >
> > > > > SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > > > >
> > > > > }
> > > > >
> > > > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > > > > struct
> > > > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > > >
> > > > > mixer->engine.node = dev->of_node;
> > > > >
> > > > > + mixer->channel_zpos[0] = -1;
> > > > > + mixer->channel_zpos[1] = -1;
> > > > > + mixer->channel_zpos[2] = -1;
> > > > > + mixer->channel_zpos[3] = -1;
> > > > > + mixer->channel_zpos[4] = -1;
> > > > > +
> > > >
> > > > for loop would be better, especially using proposed macro.
> > >
> > > I'll put it into already existent for-loop below.
> > >
> > > > Best regards,
> > > > Jernej
> > > >
> > > > > /*
> > > > >
> > > > > * While this function can fail, we shouldn't do anything
> > > > > * if this happens. Some early DE2 DT entries don't provide
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index
> > > > > 915479cc3077..9c2ff87923d8
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > > >
> > > > > struct clk *bus_clk;
> > > > > struct clk *mod_clk;
> > > > >
> > > > > +
> > > > > + /* -1 means that layer is disabled */
> > > > > + int channel_zpos[5];
> > > > >
> > > > > };
> > > > >
> > > > > static inline struct sun8i_mixer *
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > > > 893076716070..23c2f4b68c89
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > @@ -24,12 +24,10 @@
> > > > >
> > > > > #include "sun8i_ui_scaler.h"
> > > > >
> > > > > static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int
> > > > > channel,
> > > > >
> > > > > - int overlay, bool enable,
> > > >
> > > > unsigned int zpos,
> > > >
> > > > > - unsigned int old_zpos)
> > > > > + int overlay, bool enable,
> > > >
> > > > unsigned int zpos)
> > > >
> > > > > {
> > > > >
> > > > > - u32 val, bld_base, ch_base;
> > > > > + u32 val, ch_base;
> > > > >
> > > > > - bld_base = sun8i_blender_base(mixer);
> > > > >
> > > > > ch_base = sun8i_channel_base(mixer, channel);
> > > > >
> > > > > DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > > >
> > > > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> > > > > overlay),
> > > > >
> > > > > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
> > > > >
> > > > > - if (!enable || zpos != old_zpos) {
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> > > >
> > > > > - 0);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> > > >
> > > > > - 0);
> > > > > - }
> > > > > -
> > > > > - if (enable) {
> > > > > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > - val, val);
> > > > > -
> > > > > - val = channel <<
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> > > >
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> > > >
> > > > > - val);
> > > > > - }
> > > > > + mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > >
> > > > > }
> > > > >
> > > > > static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > int
> > > > >
> > > > > channel, @@ -235,11 +208,9 @@ static void
> > > > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > >
> > > > > {
> > > > >
> > > > > struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > - sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > >
> > > > false, 0,
> > > >
> > > > > - old_zpos);
> > > > > + sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > >
> > > > 0);
> > > >
> > > > > }
> > > > >
> > > > > static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > > >
> > > > > @@ -247,12 +218,11 @@ static void
> > > > > sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > >
> > > > > struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > > unsigned int zpos = plane->state->normalized_zpos;
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > if (!plane->state->visible) {
> > > > >
> > > > > sun8i_ui_layer_enable(mixer, layer->channel,
> > > > >
> > > > > - layer->overlay, false, 0,
> > > >
> > > > old_zpos);
> > > >
> > > > > + layer->overlay, false, 0);
> > > > >
> > > > > return;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *plane, sun8i_ui_layer_update_buffer(mixer,
> > > > > layer->channel,
> > > > >
> > > > > layer->overlay, plane);
> > > > >
> > > > > sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > >
> > > > > - true, zpos, old_zpos);
> > > > > + true, zpos);
> > > > >
> > > > > }
> > > > >
> > > > > static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs =
> > > > > {
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > > > 42d445d23773..97cbc98bf781
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > @@ -17,8 +17,7 @@
> > > > >
> > > > > #include "sun8i_vi_scaler.h"
> > > > >
> > > > > static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int
> > > > > channel,
> > > > >
> > > > > - int overlay, bool enable,
> > > >
> > > > unsigned int zpos,
> > > >
> > > > > - unsigned int old_zpos)
> > > > > + int overlay, bool enable,
> > > >
> > > > unsigned int zpos)
> > > >
> > > > > {
> > > > >
> > > > > u32 val, bld_base, ch_base;
> > > > >
> > > > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > > > > overlay),
> > > > >
> > > > > SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
> > > > >
> > > > > - if (!enable || zpos != old_zpos) {
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> > > >
> > > > > - 0);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> > > >
> > > > > - 0);
> > > > > - }
> > > > > -
> > > > > - if (enable) {
> > > > > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > - val, val);
> > > > > -
> > > > > - val = channel <<
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> > > >
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> > > >
> > > > > - val);
> > > > > - }
> > > > > + mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > >
> > > > > }
> > > > >
> > > > > static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > int
> > > > >
> > > > > channel, @@ -350,11 +324,9 @@ static void
> > > > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > >
> > > > > {
> > > > >
> > > > > struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > - sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > >
> > > > false, 0,
> > > >
> > > > > - old_zpos);
> > > > > + sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > >
> > > > 0);
> > > >
> > > > > }
> > > > >
> > > > > static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > > >
> > > > > @@ -362,12 +334,11 @@ static void
> > > > > sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > >
> > > > > struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > > unsigned int zpos = plane->state->normalized_zpos;
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > if (!plane->state->visible) {
> > > > >
> > > > > sun8i_vi_layer_enable(mixer, layer->channel,
> > > > >
> > > > > - layer->overlay, false, 0,
> > > >
> > > > old_zpos);
> > > >
> > > > > + layer->overlay, false, 0);
> > > > >
> > > > > return;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *plane, sun8i_vi_layer_update_buffer(mixer,
> > > > > layer->channel,
> > > > >
> > > > > layer->overlay, plane);
> > > > >
> > > > > sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > >
> > > > > - true, zpos, old_zpos);
> > > > > + true, zpos);
> > > > >
> > > > > }
> > > > >
> > > > > static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs =
> > > > > {
WARNING: multiple messages have this Message-ID (diff)
From: "Jernej Škrabec" <jernej.skrabec@siol.net>
To: Roman Stratiienko <roman.stratiienko@globallogic.com>
Cc: linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org,
Maxime Ripard <mripard@kernel.org>,
linux-kernel@vger.kernel.org
Subject: Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic
Date: Sun, 29 Dec 2019 14:02:33 +0100 [thread overview]
Message-ID: <2015568.Icojqenx9y@jernej-laptop> (raw)
In-Reply-To: <CAODwZ7srrrbDk=kKjg2-amVtGzNsqqZ72JopHijtNPD=-EzjgA@mail.gmail.com>
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@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@siol.net>
> >
> > wrote:
> > > > Hi!
> > > >
> > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > >
> > > > roman.stratiienko@globallogic.com napisal(a):
> > > > > From: Roman Stratiienko <roman.stratiienko@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@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?
Best regards,
Jernej
>
> > > Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
> >
> > Yes, at least I understand following commit message in such manner:
> > https://cgit.freedesktop.org/drm/drm-misc/commit/?
> > id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
> >
> > Best regards,
> > Jernej
> >
> > > > > regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > > > >
> > > > > SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > > > >
> > > > > }
> > > > >
> > > > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > > > > struct
> > > > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > > >
> > > > > mixer->engine.node = dev->of_node;
> > > > >
> > > > > + mixer->channel_zpos[0] = -1;
> > > > > + mixer->channel_zpos[1] = -1;
> > > > > + mixer->channel_zpos[2] = -1;
> > > > > + mixer->channel_zpos[3] = -1;
> > > > > + mixer->channel_zpos[4] = -1;
> > > > > +
> > > >
> > > > for loop would be better, especially using proposed macro.
> > >
> > > I'll put it into already existent for-loop below.
> > >
> > > > Best regards,
> > > > Jernej
> > > >
> > > > > /*
> > > > >
> > > > > * While this function can fail, we shouldn't do anything
> > > > > * if this happens. Some early DE2 DT entries don't provide
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index
> > > > > 915479cc3077..9c2ff87923d8
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > > >
> > > > > struct clk *bus_clk;
> > > > > struct clk *mod_clk;
> > > > >
> > > > > +
> > > > > + /* -1 means that layer is disabled */
> > > > > + int channel_zpos[5];
> > > > >
> > > > > };
> > > > >
> > > > > static inline struct sun8i_mixer *
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > > > 893076716070..23c2f4b68c89
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > @@ -24,12 +24,10 @@
> > > > >
> > > > > #include "sun8i_ui_scaler.h"
> > > > >
> > > > > static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int
> > > > > channel,
> > > > >
> > > > > - int overlay, bool enable,
> > > >
> > > > unsigned int zpos,
> > > >
> > > > > - unsigned int old_zpos)
> > > > > + int overlay, bool enable,
> > > >
> > > > unsigned int zpos)
> > > >
> > > > > {
> > > > >
> > > > > - u32 val, bld_base, ch_base;
> > > > > + u32 val, ch_base;
> > > > >
> > > > > - bld_base = sun8i_blender_base(mixer);
> > > > >
> > > > > ch_base = sun8i_channel_base(mixer, channel);
> > > > >
> > > > > DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > > >
> > > > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> > > > > overlay),
> > > > >
> > > > > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
> > > > >
> > > > > - if (!enable || zpos != old_zpos) {
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> > > >
> > > > > - 0);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> > > >
> > > > > - 0);
> > > > > - }
> > > > > -
> > > > > - if (enable) {
> > > > > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > - val, val);
> > > > > -
> > > > > - val = channel <<
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> > > >
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> > > >
> > > > > - val);
> > > > > - }
> > > > > + mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > >
> > > > > }
> > > > >
> > > > > static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > int
> > > > >
> > > > > channel, @@ -235,11 +208,9 @@ static void
> > > > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > >
> > > > > {
> > > > >
> > > > > struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > - sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > >
> > > > false, 0,
> > > >
> > > > > - old_zpos);
> > > > > + sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > >
> > > > 0);
> > > >
> > > > > }
> > > > >
> > > > > static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > > >
> > > > > @@ -247,12 +218,11 @@ static void
> > > > > sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > >
> > > > > struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > > unsigned int zpos = plane->state->normalized_zpos;
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > if (!plane->state->visible) {
> > > > >
> > > > > sun8i_ui_layer_enable(mixer, layer->channel,
> > > > >
> > > > > - layer->overlay, false, 0,
> > > >
> > > > old_zpos);
> > > >
> > > > > + layer->overlay, false, 0);
> > > > >
> > > > > return;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *plane, sun8i_ui_layer_update_buffer(mixer,
> > > > > layer->channel,
> > > > >
> > > > > layer->overlay, plane);
> > > > >
> > > > > sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > >
> > > > > - true, zpos, old_zpos);
> > > > > + true, zpos);
> > > > >
> > > > > }
> > > > >
> > > > > static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs =
> > > > > {
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > > > 42d445d23773..97cbc98bf781
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > @@ -17,8 +17,7 @@
> > > > >
> > > > > #include "sun8i_vi_scaler.h"
> > > > >
> > > > > static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int
> > > > > channel,
> > > > >
> > > > > - int overlay, bool enable,
> > > >
> > > > unsigned int zpos,
> > > >
> > > > > - unsigned int old_zpos)
> > > > > + int overlay, bool enable,
> > > >
> > > > unsigned int zpos)
> > > >
> > > > > {
> > > > >
> > > > > u32 val, bld_base, ch_base;
> > > > >
> > > > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > > > > overlay),
> > > > >
> > > > > SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
> > > > >
> > > > > - if (!enable || zpos != old_zpos) {
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> > > >
> > > > > - 0);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> > > >
> > > > > - 0);
> > > > > - }
> > > > > -
> > > > > - if (enable) {
> > > > > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > - val, val);
> > > > > -
> > > > > - val = channel <<
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> > > >
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> > > >
> > > > > - val);
> > > > > - }
> > > > > + mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > >
> > > > > }
> > > > >
> > > > > static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > int
> > > > >
> > > > > channel, @@ -350,11 +324,9 @@ static void
> > > > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > >
> > > > > {
> > > > >
> > > > > struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > - sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > >
> > > > false, 0,
> > > >
> > > > > - old_zpos);
> > > > > + sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > >
> > > > 0);
> > > >
> > > > > }
> > > > >
> > > > > static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > > >
> > > > > @@ -362,12 +334,11 @@ static void
> > > > > sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > >
> > > > > struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > > unsigned int zpos = plane->state->normalized_zpos;
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > if (!plane->state->visible) {
> > > > >
> > > > > sun8i_vi_layer_enable(mixer, layer->channel,
> > > > >
> > > > > - layer->overlay, false, 0,
> > > >
> > > > old_zpos);
> > > >
> > > > > + layer->overlay, false, 0);
> > > > >
> > > > > return;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *plane, sun8i_vi_layer_update_buffer(mixer,
> > > > > layer->channel,
> > > > >
> > > > > layer->overlay, plane);
> > > > >
> > > > > sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > >
> > > > > - true, zpos, old_zpos);
> > > > > + true, zpos);
> > > > >
> > > > > }
> > > > >
> > > > > static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs =
> > > > > {
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: "Jernej Škrabec" <jernej.skrabec@siol.net>
To: Roman Stratiienko <roman.stratiienko@globallogic.com>
Cc: linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic
Date: Sun, 29 Dec 2019 14:02:33 +0100 [thread overview]
Message-ID: <2015568.Icojqenx9y@jernej-laptop> (raw)
In-Reply-To: <CAODwZ7srrrbDk=kKjg2-amVtGzNsqqZ72JopHijtNPD=-EzjgA@mail.gmail.com>
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@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@siol.net>
> >
> > wrote:
> > > > Hi!
> > > >
> > > > Dne sobota, 28. december 2019 ob 21:28:17 CET je
> > > >
> > > > roman.stratiienko@globallogic.com napisal(a):
> > > > > From: Roman Stratiienko <roman.stratiienko@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@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?
Best regards,
Jernej
>
> > > Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
> >
> > Yes, at least I understand following commit message in such manner:
> > https://cgit.freedesktop.org/drm/drm-misc/commit/?
> > id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
> >
> > Best regards,
> > Jernej
> >
> > > > > regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > > > >
> > > > > SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > > > >
> > > > > }
> > > > >
> > > > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > > > > struct
> > > > > device *master, mixer->engine.ops = &sun8i_engine_ops;
> > > > >
> > > > > mixer->engine.node = dev->of_node;
> > > > >
> > > > > + mixer->channel_zpos[0] = -1;
> > > > > + mixer->channel_zpos[1] = -1;
> > > > > + mixer->channel_zpos[2] = -1;
> > > > > + mixer->channel_zpos[3] = -1;
> > > > > + mixer->channel_zpos[4] = -1;
> > > > > +
> > > >
> > > > for loop would be better, especially using proposed macro.
> > >
> > > I'll put it into already existent for-loop below.
> > >
> > > > Best regards,
> > > > Jernej
> > > >
> > > > > /*
> > > > >
> > > > > * While this function can fail, we shouldn't do anything
> > > > > * if this happens. Some early DE2 DT entries don't provide
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index
> > > > > 915479cc3077..9c2ff87923d8
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > > > @@ -178,6 +178,9 @@ struct sun8i_mixer {
> > > > >
> > > > > struct clk *bus_clk;
> > > > > struct clk *mod_clk;
> > > > >
> > > > > +
> > > > > + /* -1 means that layer is disabled */
> > > > > + int channel_zpos[5];
> > > > >
> > > > > };
> > > > >
> > > > > static inline struct sun8i_mixer *
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index
> > > > > 893076716070..23c2f4b68c89
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > > > @@ -24,12 +24,10 @@
> > > > >
> > > > > #include "sun8i_ui_scaler.h"
> > > > >
> > > > > static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int
> > > > > channel,
> > > > >
> > > > > - int overlay, bool enable,
> > > >
> > > > unsigned int zpos,
> > > >
> > > > > - unsigned int old_zpos)
> > > > > + int overlay, bool enable,
> > > >
> > > > unsigned int zpos)
> > > >
> > > > > {
> > > > >
> > > > > - u32 val, bld_base, ch_base;
> > > > > + u32 val, ch_base;
> > > > >
> > > > > - bld_base = sun8i_blender_base(mixer);
> > > > >
> > > > > ch_base = sun8i_channel_base(mixer, channel);
> > > > >
> > > > > DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
> > > > >
> > > > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> > > > > overlay),
> > > > >
> > > > > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
> > > > >
> > > > > - if (!enable || zpos != old_zpos) {
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> > > >
> > > > > - 0);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> > > >
> > > > > - 0);
> > > > > - }
> > > > > -
> > > > > - if (enable) {
> > > > > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > - val, val);
> > > > > -
> > > > > - val = channel <<
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> > > >
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> > > >
> > > > > - val);
> > > > > - }
> > > > > + mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > >
> > > > > }
> > > > >
> > > > > static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > int
> > > > >
> > > > > channel, @@ -235,11 +208,9 @@ static void
> > > > > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > >
> > > > > {
> > > > >
> > > > > struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > - sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > >
> > > > false, 0,
> > > >
> > > > > - old_zpos);
> > > > > + sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > >
> > > > 0);
> > > >
> > > > > }
> > > > >
> > > > > static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > > >
> > > > > @@ -247,12 +218,11 @@ static void
> > > > > sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > >
> > > > > struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> > > > > unsigned int zpos = plane->state->normalized_zpos;
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > if (!plane->state->visible) {
> > > > >
> > > > > sun8i_ui_layer_enable(mixer, layer->channel,
> > > > >
> > > > > - layer->overlay, false, 0,
> > > >
> > > > old_zpos);
> > > >
> > > > > + layer->overlay, false, 0);
> > > > >
> > > > > return;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct
> > > > > drm_plane *plane, sun8i_ui_layer_update_buffer(mixer,
> > > > > layer->channel,
> > > > >
> > > > > layer->overlay, plane);
> > > > >
> > > > > sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> > > > >
> > > > > - true, zpos, old_zpos);
> > > > > + true, zpos);
> > > > >
> > > > > }
> > > > >
> > > > > static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs =
> > > > > {
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index
> > > > > 42d445d23773..97cbc98bf781
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > @@ -17,8 +17,7 @@
> > > > >
> > > > > #include "sun8i_vi_scaler.h"
> > > > >
> > > > > static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int
> > > > > channel,
> > > > >
> > > > > - int overlay, bool enable,
> > > >
> > > > unsigned int zpos,
> > > >
> > > > > - unsigned int old_zpos)
> > > > > + int overlay, bool enable,
> > > >
> > > > unsigned int zpos)
> > > >
> > > > > {
> > > > >
> > > > > u32 val, bld_base, ch_base;
> > > > >
> > > > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct
> > > > > sun8i_mixer
> > > > > *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > > > > overlay),
> > > > >
> > > > > SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
> > > > >
> > > > > - if (!enable || zpos != old_zpos) {
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> > > >
> > > > > - 0);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> > > >
> > > > > - 0);
> > > > > - }
> > > > > -
> > > > > - if (enable) {
> > > > > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> > > >
> > > > > - val, val);
> > > > > -
> > > > > - val = channel <<
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> > > >
> > > > > -
> > > > > - regmap_update_bits(mixer->engine.regs,
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE(bld_base),
> > > >
> > > > > -
> > > >
> > > > SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> > > >
> > > > > - val);
> > > > > - }
> > > > > + mixer->channel_zpos[channel] = enable ? zpos : -1;
> > > > >
> > > > > }
> > > > >
> > > > > static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer,
> > > > > int
> > > > >
> > > > > channel, @@ -350,11 +324,9 @@ static void
> > > > > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct
> > > > > drm_plane_state *old_state)
> > > > >
> > > > > {
> > > > >
> > > > > struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > - sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > >
> > > > false, 0,
> > > >
> > > > > - old_zpos);
> > > > > + sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > > false,
> > > >
> > > > 0);
> > > >
> > > > > }
> > > > >
> > > > > static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > > >
> > > > > @@ -362,12 +334,11 @@ static void
> > > > > sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *plane, {
> > > > >
> > > > > struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> > > > > unsigned int zpos = plane->state->normalized_zpos;
> > > > >
> > > > > - unsigned int old_zpos = old_state->normalized_zpos;
> > > > >
> > > > > struct sun8i_mixer *mixer = layer->mixer;
> > > > >
> > > > > if (!plane->state->visible) {
> > > > >
> > > > > sun8i_vi_layer_enable(mixer, layer->channel,
> > > > >
> > > > > - layer->overlay, false, 0,
> > > >
> > > > old_zpos);
> > > >
> > > > > + layer->overlay, false, 0);
> > > > >
> > > > > return;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct
> > > > > drm_plane *plane, sun8i_vi_layer_update_buffer(mixer,
> > > > > layer->channel,
> > > > >
> > > > > layer->overlay, plane);
> > > > >
> > > > > sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> > > > >
> > > > > - true, zpos, old_zpos);
> > > > > + true, zpos);
> > > > >
> > > > > }
> > > > >
> > > > > static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs =
> > > > > {
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-12-29 13:02 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-28 20:28 roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-28 20:28 ` [RFC 1/4] drm/sun4i: Wait for previous mixing process finish before committing next roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-29 9:11 ` Jernej Škrabec
2019-12-29 9:11 ` Jernej Škrabec
2019-12-29 9:11 ` Jernej Škrabec
2019-12-28 20:28 ` [RFC 2/4] drm/sun4i: Use CRTC size instead of PRIMARY plane size as mixer frame roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-29 9:20 ` Jernej Škrabec
2019-12-29 9:20 ` Jernej Škrabec
2019-12-29 9:20 ` Jernej Škrabec
2019-12-28 20:28 ` [RFC 3/4] drm/sun4i: Reimplement plane z position setting logic roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-29 9:40 ` Jernej Škrabec
2019-12-29 9:40 ` Jernej Škrabec
2019-12-29 9:40 ` Jernej Škrabec
2019-12-29 12:08 ` Roman Stratiienko
2019-12-29 12:08 ` Roman Stratiienko
2019-12-29 12:08 ` Roman Stratiienko
2019-12-29 12:18 ` Jernej Škrabec
2019-12-29 12:18 ` Jernej Škrabec
2019-12-29 12:18 ` Jernej Škrabec
2019-12-29 12:47 ` Roman Stratiienko
2019-12-29 12:47 ` Roman Stratiienko
2019-12-29 12:47 ` Roman Stratiienko
2019-12-29 13:02 ` Jernej Škrabec [this message]
2019-12-29 13:02 ` Jernej Škrabec
2019-12-29 13:02 ` Jernej Škrabec
2019-12-29 13:13 ` Roman Stratiienko
2019-12-29 13:13 ` Roman Stratiienko
2019-12-29 13:13 ` Roman Stratiienko
2019-12-29 13:33 ` Jernej Škrabec
2019-12-29 13:33 ` Jernej Škrabec
2019-12-29 13:33 ` Jernej Škrabec
2020-01-02 9:49 ` Maxime Ripard
2020-01-02 9:49 ` Maxime Ripard
2020-01-02 9:49 ` Maxime Ripard
2019-12-28 20:28 ` [RFC 4/4] drm/sun4i: Update mixer's internal registers after initialization roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-28 20:28 ` roman.stratiienko
2019-12-29 9:25 ` Jernej Škrabec
2019-12-29 9:25 ` Jernej Škrabec
2019-12-29 9:25 ` Jernej Škrabec
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2015568.Icojqenx9y@jernej-laptop \
--to=jernej.skrabec@siol.net \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mripard@kernel.org \
--cc=roman.stratiienko@globallogic.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.