Hi!
Sorry that I missed few details in first review. Please take a look below.
Dne nedelja, 29. december 2019 ob 17:28:27 CET je roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
According to DRM documentation the only difference between PRIMARY and OVERLAY plane is that each CRTC must have PRIMARY plane and OVERLAY are optional.
Allow PRIMARY plane to have dimension different from full-screen.
Fixes: 90212fffa4fc ("drm/sun4i: Use values calculated by atomic check")
This fixes tag doesn't seem to be a good choice. First time where code in question was introduced was:
9d75b8c0b999 drm/sun4i: add support for Allwinner DE2 mixers
and it was later moved to sun8i_ui_layer.c in:
5bb5f5dafa1a drm/sun4i: Reorganize UI layer code in DE2
Not sure which one is better. You can also include both.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
v2:
- Split commit in 2 parts
- Add Fixes line to the commit message
drivers/gpu/drm/sun4i/sun8i_mixer.c | 35 ++++++++++++++++++++++++++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 30 ---------------------- 2 files changed, 35 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index d306ad5dc093..5d90a95ff855 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -257,6 +257,40 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32 format) return NULL; }
+static void sun8i_mode_set(struct sunxi_engine *engine,
struct drm_display_mode *mode)+{
- u32 dst_w = mode->crtc_hdisplay;
- u32 dst_h = mode->crtc_vdisplay;
Now that you moved code in separate function, "dst_" prefix doesn't make sense anymore. Plain "width" and "height" work just fine.
- u32 outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
- bool interlaced = false;
No need to initialize above variable. This value is never used.
- u32 val;
- struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
- u32 bld_base = sun8i_blender_base(mixer);
Not extremely important, but can you move above two lines to the top? At least I prefer to have those lines sorted from longest to shortest as much as possible.
Once above comments are addressed, code is: Reviewed-by: Jernej Skrabec jernej.skrabec@siol.net
Best regards, Jernej
- DRM_DEBUG_DRIVER("Mode change, updating global size W: %u H: %u\n",
dst_w, dst_h);- regmap_write(mixer->engine.regs,
SUN8I_MIXER_GLOBAL_SIZE,outsize);- regmap_write(mixer->engine.regs,
SUN8I_MIXER_BLEND_OUTSIZE(bld_base), outsize);- interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
- if (interlaced)
val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;- else
val = 0;- regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_OUTCTL(bld_base),SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,val);- DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
interlaced ? "on" : "off");+}
static void sun8i_mixer_commit(struct sunxi_engine *engine) { struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine); @@ -356,6 +390,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm, static const struct sunxi_engine_ops sun8i_engine_ops = { .commit = sun8i_mixer_commit, .layers_init = sun8i_layers_init,
- .mode_set = sun8i_mode_set,
};
static struct regmap_config sun8i_mixer_regmap_config = { diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index ee7c13d8710f..23c2f4b68c89 100644 --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c @@ -72,36 +72,6 @@ static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel, insize = SUN8I_MIXER_SIZE(src_w, src_h); outsize = SUN8I_MIXER_SIZE(dst_w, dst_h);
- if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
bool interlaced = false;u32 val;DRM_DEBUG_DRIVER("Primary layer, updating global size
W: %u H: %u\n",
dst_w, dst_h);regmap_write(mixer->engine.regs,SUN8I_MIXER_GLOBAL_SIZE,outsize);regmap_write(mixer->engine.regs,SUN8I_MIXER_BLEND_OUTSIZE(bld_base),
outsize);
if (state->crtc)interlaced = state->crtc->state-adjusted_mode.flags
& DRM_MODE_FLAG_INTERLACE;if (interlaced)val = SUN8I_MIXER_BLEND_OUTCTL_INTERLACED;elseval = 0;regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_OUTCTL(bld_base),
SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
val);DRM_DEBUG_DRIVER("Switching display mixer interlaced
mode %s\n",
interlaced ? "on" : "off");- }
- /* Set height and width */ DRM_DEBUG_DRIVER("Layer source offset X: %d Y: %d\n", state->src.x1 >> 16, state->src.y1 >> 16);