Re: [PATCH 09/10] drm/rockchip: Implement drm plane->ctm property.

[Sean Paul <seanpaul@chromium.org>]

On Thu, Feb 15, 2018 at 12:32:59AM -0500, Daniele Castagna wrote:
> Validate drm PLANE_CTM matrix and map it to YUV2YUV registers.
> 
> Change-Id: Ib4fe49558c6266bf0c310af121d625cd7b2cedf6

Missing Signed-off-by

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 +++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index ea43ab797f555..8c8118c3db308 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -656,6 +656,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>  	struct drm_framebuffer *fb = state->fb;
>  	struct vop_win *vop_win = to_vop_win(plane);
>  	const struct vop_win_data *win = vop_win->data;
> +	int i;
>  	int ret;
>  	struct drm_rect clip;
>  	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
> @@ -697,6 +698,25 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
>  		return -EINVAL;
>  	}
>  
> +	if (state->ctm) {
> +		struct drm_color_ctm* color_ctm = (struct drm_color_ctm*)state->ctm->data;
> +		if (state->ctm->length != sizeof(struct drm_color_ctm)) {
> +			DRM_ERROR("Invalid PLANE_CTM blob size.\n");
> +			return -EINVAL;
> +		}
> +
> +		for (i = 0; i < 9; i++) {

s/9/ARRAY_SIZE(color_ctm->matrix)/

> +			/*
> +			 * YUV2YUV R2R registers have a signed fixed point S2.10 format.
> +			 * The input values, that are in signed fixed point S31.32 format,
> +			 * can be converted only if the first 30 MSBs are all 1s or 0s.
> +			 */
> +			uint32_t msbs = (uint32_t) (color_ctm->matrix[i] >> 34);

The behavior of a negative value shift is implementation-defined, so probably
best not to use it.

You could do:

uint64_t mask = GENMASK_ULL(63, 34);
uint64_t msbs = (uint64_t)color_ctm->matrix[i] & mask;

if (msbs == 0 || msbs == mask)

> +			if (msbs != ~0u && msbs != 0)
> +				    return -EOVERFLOW;

Indent is off here.

> +	      }
> +	}
> +
>  	return 0;
>  }
>  
> @@ -816,6 +836,31 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>  		}
>  	}
>  
> +	if (!win_index) {
> +		VOP_YUV2YUV_SET(vop, win0_r2r_en, !!state->ctm);
> +	} else if (win_index == 1) {
> +		VOP_YUV2YUV_SET(vop, win1_r2r_en, !!state->ctm);
> +	} else if (win_index == 2) {
> +		VOP_YUV2YUV_SET(vop, win2_r2r_en, !!state->ctm);
> +	}
> +	if (state->ctm) {
> +		struct drm_color_ctm* color_ctm = (struct drm_color_ctm*)state->ctm->data;
> +                /*

Indent is messed up here too

> +		 * Convert matrix values from fixed point S31.32 to S2.10, by discarding
> +		 * the lowest 22 bits.
> +		 */
> +		for (i = 0; i < 9; i++) {
> +			uint32_t value = (color_ctm->matrix[i] >> 22) & 0x1FFF;

Same comment regarding the shift here, best to cast and mask before shifting.

> +			if (!win_index) {
> +				VOP_YUV2YUV_SET(vop, win0_r2r_coefficients[i], value);
> +			 } else if (win_index == 1){
> +				VOP_YUV2YUV_SET(vop, win1_r2r_coefficients[i], value);
> +			 } else if (win_index == 2) {
> +				VOP_YUV2YUV_SET(vop, win2_r2r_coefficients[i], value);
> +			 }
> +		}
> +	}
> +
>  	if (win->phy->scl)
>  		scl_vop_cal_scl_fac(vop, win, actual_w, actual_h,
>  				    drm_rect_width(dest), drm_rect_height(dest),
> -- 
> 2.16.1.291.g4437f3f132-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel