Thread (16 messages) 16 messages, 3 authors, 2020-01-02

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

flat view

From: Jernej Škrabec <hidden>
Date: 2019-12-29 13:02:41
Also in: dri-devel, lkml 

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 [off-list ref] 
wrote:
quoted
Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko

napisal(a):
quoted
Hello Jernej,

Thank you for review.

On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec
[off-list ref]
wrote:
quoted
quoted
Hi!

Dne sobota, 28. december 2019 ob 21:28:17 CET je

roman.stratiienko@globallogic.com napisal(a):
quoted
From: Roman Stratiienko <redacted>

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 <redacted>
---

 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().
quoted
+     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
quoted
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so
everyone would understand where this number comes from.
Will do.
quoted
quoted
+
+             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);
quoted
+                     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);
quoted
+                     j++;
+             }
+     }
+
+     regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_PIPE_CTL(base),
quoted
+                        SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
pipe_ctl);
quoted
+
+     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

quoted
quoted
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

quoted
quoted
quoted
      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.

quoted
Best regards,
Jernej

quoted
      /*
      
       * 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,

quoted
-                               unsigned int old_zpos)
+                               int overlay, bool enable,
unsigned int zpos)

quoted
 {

-     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),

quoted
-
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),

quoted
-                                0);
-
-             regmap_update_bits(mixer->engine.regs,
-
SUN8I_MIXER_BLEND_ROUTE(bld_base),

quoted
-
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),

quoted
-                                0);
-     }
-
-     if (enable) {
-             val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
-
-             regmap_update_bits(mixer->engine.regs,
-
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),

quoted
-                                val, val);
-
-             val = channel <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);

quoted
-
-             regmap_update_bits(mixer->engine.regs,
-
SUN8I_MIXER_BLEND_ROUTE(bld_base),

quoted
-
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),

quoted
-                                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,

quoted
-                           old_zpos);
+     sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
false,
0);

quoted
 }
 
 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);

quoted
+                                   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,

quoted
-                               unsigned int old_zpos)
+                               int overlay, bool enable,
unsigned int zpos)

quoted
 {
 
      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),

quoted
-
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),

quoted
-                                0);
-
-             regmap_update_bits(mixer->engine.regs,
-
SUN8I_MIXER_BLEND_ROUTE(bld_base),

quoted
-
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),

quoted
-                                0);
-     }
-
-     if (enable) {
-             val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
-
-             regmap_update_bits(mixer->engine.regs,
-
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),

quoted
-                                val, val);
-
-             val = channel <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);

quoted
-
-             regmap_update_bits(mixer->engine.regs,
-
SUN8I_MIXER_BLEND_ROUTE(bld_base),

quoted
-
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),

quoted
-                                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,

quoted
-                           old_zpos);
+     sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
false,
0);

quoted
 }
 
 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);

quoted
+                                   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
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help