Re: [PATCH] RFC: sun4i/drm: Swap back U and V channels for DRM_FORMAT_YVU4xx

["Jernej Škrabec" <jernej.skrabec@siol.net>]

Dne torek, 01. september 2020 ob 23:29:29 CEST je Ondřej Jirman napisal(a):
> On Tue, Sep 01, 2020 at 11:30:47PM +0300, Roman Stratiienko wrote:
> > Fixes: e1ef9006663b ("drm/sun4i: Wire in DE2 YUV support")
> > Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> > 
> > ---
> > CC: megous@megous.com
> > CC: jernej.skrabec@gmail.com
> > CC: linux-sunxi@googlegroups.com
> > CC: dri-devel@lists.freedesktop.org
> > CC: linux-arm-kernel@lists.infradead.org
> > CC: linux-kernel@vger.kernel.org
> > 
> > Hi, this patch fixes wrong colors during video playback for me.
> > Implemented ugly for now, please review/suggest how to improve.
> 
> Why do you think the issue is at DRM level? Have you tried displaying a known
> color image via a vi layer, and was it wrong?
> 
> I used DRM with YUV data in the past, and didn't notice any weird colors,
> so I'm a bit skeptical.

There is a bug, I just tested it, but solution is much more simpler:

--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -211,7 +211,7 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
        return 0;
 }
 
-static bool sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format)
+static u32 sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format)
 {
        if (!format->is_yuv)
                return SUN8I_CSC_MODE_OFF;


I made a mistake when I was reworking (simplifying) format handling. I'll send a fix soon.

Thanks for report!

Best regards,
Jernej

> 
> regards,
> 	o.
> 
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c    |  8 +++++++-
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 +-
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c |  2 +-
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 28 +++++++++++++++++++-------
> >  4 files changed, 30 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index dce40c430100..bbbeef44899a 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -31,6 +31,7 @@
> >  struct de2_fmt_info {
> >  	u32	drm_fmt;
> >  	u32	de2_fmt;
> > +	bool	swap_uv;
> >  };
> >  
> >  static bool hw_preconfigured;
> > @@ -219,14 +220,17 @@ static const struct de2_fmt_info de2_formats[] = {
> >  	{
> >  		.drm_fmt = DRM_FORMAT_YVU422,
> >  		.de2_fmt = SUN8I_MIXER_FBFMT_YUV422,
> > +		.swap_uv = true,
> >  	},
> >  	{
> >  		.drm_fmt = DRM_FORMAT_YVU420,
> >  		.de2_fmt = SUN8I_MIXER_FBFMT_YUV420,
> > +		.swap_uv = true,
> >  	},
> >  	{
> >  		.drm_fmt = DRM_FORMAT_YVU411,
> >  		.de2_fmt = SUN8I_MIXER_FBFMT_YUV411,
> > +		.swap_uv = true,
> >  	},
> >  	{
> >  		.drm_fmt = DRM_FORMAT_P010,
> > @@ -238,13 +242,15 @@ static const struct de2_fmt_info de2_formats[] = {
> >  	},
> >  };
> >  
> > -int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format)
> > +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format, bool *swap_uv)
> >  {
> >  	unsigned int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(de2_formats); ++i)
> >  		if (de2_formats[i].drm_fmt == format) {
> >  			*hw_format = de2_formats[i].de2_fmt;
> > +			if (swap_uv)
> > +				*swap_uv = de2_formats[i].swap_uv;
> >  			return 0;
> >  		}
> >  
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > index 79a74bca1ea3..6358ffd251f9 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -207,5 +207,5 @@ sun8i_channel_base(struct sun8i_mixer *mixer, int channel)
> >  		return DE2_CH_BASE + channel * DE2_CH_SIZE;
> >  }
> >  
> > -int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format);
> > +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format, bool *swap_uv);
> >  #endif /* _SUN8I_MIXER_H_ */
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > index a7f21f08ec89..57bbd9f1071c 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > @@ -215,7 +215,7 @@ static int sun8i_ui_layer_update_formats(struct sun8i_mixer *mixer, int channel,
> >  	ch_base = sun8i_channel_base(mixer, channel);
> >  
> >  	fmt = state->fb->format;
> > -	ret = sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt);
> > +	ret = sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt, NULL);
> >  	if (ret || fmt->is_yuv) {
> >  		DRM_DEBUG_DRIVER("Invalid format\n");
> >  		return -EINVAL;
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > index 3553e38ec642..4da51155c4d5 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -313,7 +313,7 @@ static int sun8i_vi_layer_update_formats(struct sun8i_mixer *mixer, int channel,
> >  	ch_base = sun8i_channel_base(mixer, channel);
> >  
> >  	fmt = state->fb->format;
> > -	ret = sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt);
> > +	ret = sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt, NULL);
> >  	if (ret) {
> >  		DRM_DEBUG_DRIVER("Invalid format\n");
> >  		return ret;
> > @@ -368,8 +368,17 @@ static int sun8i_vi_layer_update_buffer(struct sun8i_mixer *mixer, int channel,
> >  	struct drm_gem_cma_object *gem;
> >  	u32 dx, dy, src_x, src_y;
> >  	dma_addr_t paddr;
> > +	bool swap_uv;
> >  	u32 ch_base;
> > -	int i;
> > +	u32 hw_fmt;
> > +	int ret;
> > +	int i, j;
> > +
> > +	ret = sun8i_mixer_drm_format_to_hw(plane->state->fb->format->format, &hw_fmt, &swap_uv);
> > +	if (ret) {
> > +		DRM_DEBUG_DRIVER("Invalid format\n");
> > +		return ret;
> > +	}
> >  
> >  	ch_base = sun8i_channel_base(mixer, channel);
> >  
> > @@ -377,7 +386,12 @@ static int sun8i_vi_layer_update_buffer(struct sun8i_mixer *mixer, int channel,
> >  	src_x = (state->src.x1 >> 16) & ~(format->hsub - 1);
> >  	src_y = (state->src.y1 >> 16) & ~(format->vsub - 1);
> >  
> > -	for (i = 0; i < format->num_planes; i++) {
> > +	for (j = 0; j < format->num_planes; j++) {
> > +		i = j;
> > +		if (swap_uv && i > 0 && format->num_planes == 3) {
> > +			i = j == 1 ? 2 : 1;
> > +		}
> > +
> >  		/* Get the physical address of the buffer in memory */
> >  		gem = drm_fb_cma_get_gem_obj(fb, i);
> >  
> > @@ -400,18 +414,18 @@ static int sun8i_vi_layer_update_buffer(struct sun8i_mixer *mixer, int channel,
> >  
> >  		/* Set the line width */
> >  		DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
> > -				 i + 1, fb->pitches[i]);
> > +				 j + 1, fb->pitches[i]);
> >  		regmap_write(mixer->engine.regs,
> >  			     SUN8I_MIXER_CHAN_VI_LAYER_PITCH(ch_base,
> > -							     overlay, i),
> > +							     overlay, j),
> >  			     fb->pitches[i]);
> >  
> >  		DRM_DEBUG_DRIVER("Setting %d. buffer address to %pad\n",
> > -				 i + 1, &paddr);
> > +				 j + 1, &paddr);
> >  
> >  		regmap_write(mixer->engine.regs,
> >  			     SUN8I_MIXER_CHAN_VI_LAYER_TOP_LADDR(ch_base,
> > -								 overlay, i),
> > +								 overlay, j),
> >  			     lower_32_bits(paddr));
> >  	}
> >  
> > -- 
> > 2.25.1
> > 
>