| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <55965344.5050502@rock-chips.com>
Date: Fri, 03 Jul 2015 17:17:56 +0800
From: Mark yao <mark.yao@...k-chips.com>
To: Tomasz Figa <tfiga@...omium.org>
CC: dri-devel <dri-devel@...ts.freedesktop.org>,
David Airlie <airlied@...ux.ie>,
Heiko Stuebner <heiko@...ech.de>,
Daniel Kurtz <djkurtz@...omium.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Daniel Vetter <daniel@...ll.ch>,
Rob Clark <robdclark@...il.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
sandy.huang@...k-chips.com, dkm@...k-chips.com, zwl@...k-chips.com,
xw@...k-chips.com
Subject: Re: [PATCH v2 3/5] drm/rockchip: vop: support plane scale
On 2015年07月03日 15:46, Tomasz Figa wrote:
> Hi Mark,
>
> Please see my comments inline.
>
> On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@...k-chips.com> wrote:
>> Win_full support 1/8 to 8 scale down/up engine, support
>> all format scale.
> [snip]
>
>> @@ -351,6 +412,15 @@ static inline void vop_mask_write_relaxed(struct vop *vop, uint32_t offset,
>> }
>> }
>>
>> +static inline int _get_vskiplines(uint32_t srch, uint32_t dsth)
>> +{
>> + if (srch >= (uint32_t)(4 * dsth * MIN_SCL_FT_AFTER_VSKIP))
>> + return 4;
>> + else if (srch >= (uint32_t)(2 * dsth * MIN_SCL_FT_AFTER_VSKIP))
>> + return 2;
>> + return 1;
> How about rewriting the above to:
>
> #define SCL_MAX_VSKIPLINES 4
>
> uint32_t vskiplines;
>
> for (vskiplines = SCL_MAX_VSKIPLINES; vskiplines > 1; vskiplines /= 2)
> if (srch >= vskiplines * dsth * MIN_SCL_FT_AFTER_VSKIP)
> break;
>
> return vskiplines;
>
> nit: I believe it would be better for readability to move this
> function to other scaler related functions below.
> nit: I don't see any existing functions with names starting from _, so
> to keep existing conventions, how about calling them scl_*, e.g.
> scl_get_vskiplines().
OK
>> +}
>> +
>> static enum vop_data_format vop_convert_format(uint32_t format)
>> {
>> switch (format) {
>> @@ -538,6 +608,310 @@ static void vop_disable(struct drm_crtc *crtc)
>> pm_runtime_put(vop->dev);
>> }
>>
>> +static int _vop_cal_yrgb_lb_mode(int width)
>> +{
>> + int lb_mode = LB_RGB_1920X5;
>> +
>> + if (width > 2560)
>> + lb_mode = LB_RGB_3840X2;
>> + else if (width > 1920)
>> + lb_mode = LB_RGB_2560X4;
> It would be more readable to add
>
> else
> lb_mode = LB_RGB_1920X5;
>
> instead of initializing the variable at declaration time.
>
OK
>> +
>> + return lb_mode;
>> +}
>> +
>> +static int _vop_cal_cbcr_lb_mode(int width)
>> +{
>> + int lb_mode = LB_YUV_2560X8;
>> +
>> + if (width > 2560)
>> + lb_mode = LB_RGB_3840X2;
>> + else if (width > 1920)
>> + lb_mode = LB_RGB_2560X4;
>> + else if (width > 1280)
>> + lb_mode = LB_YUV_3840X5;
>> +
>> + return lb_mode;
>> +}
>> +
>> +static void _vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
>> + uint32_t src_w, uint32_t src_h, uint32_t dst_w,
>> + uint32_t dst_h, uint32_t pixel_format)
>> +{
>> + uint16_t yrgb_hor_scl_mode = SCALE_NONE;
>> + uint16_t yrgb_ver_scl_mode = SCALE_NONE;
>> + uint16_t cbr_hor_scl_mode = SCALE_NONE;
>> + uint16_t cbr_ver_scl_mode = SCALE_NONE;
>> + uint16_t yrgb_hsd_mode = SCALE_DOWN_BIL;
>> + uint16_t cbr_hsd_mode = SCALE_DOWN_BIL;
>> + uint16_t yrgb_vsd_mode = SCALE_DOWN_BIL;
>> + uint16_t cbr_vsd_mode = SCALE_DOWN_BIL;
> No code seems to be assigning the 4 variables above. Is some code
> missing or they are simply constants and they (and code checking their
> values) are not necessary?
those value directly write to the vop regs, it's necessary.
>> + uint16_t yrgb_vsu_mode = SCALE_UP_BIL;
>> + uint16_t cbr_vsu_mode = SCALE_UP_BIL;
>> + uint16_t scale_yrgb_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> + uint16_t scale_yrgb_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> + uint16_t scale_cbcr_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> + uint16_t scale_cbcr_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> + int hsub = drm_format_horz_chroma_subsampling(pixel_format);
>> + int vsub = drm_format_vert_chroma_subsampling(pixel_format);
>> + bool is_yuv = is_yuv_support(pixel_format);
>> + uint16_t vsd_yrgb_gt4 = 0;
>> + uint16_t vsd_yrgb_gt2 = 0;
>> + uint16_t vsd_cbr_gt4 = 0;
>> + uint16_t vsd_cbr_gt2 = 0;
>> + uint16_t yrgb_src_w = src_w;
>> + uint16_t yrgb_src_h = src_h;
>> + uint16_t yrgb_dst_w = dst_w;
>> + uint16_t yrgb_dst_h = dst_h;
>> + uint16_t cbcr_src_w;
>> + uint16_t cbcr_src_h;
>> + uint16_t cbcr_dst_w;
>> + uint16_t cbcr_dst_h;
>> + uint32_t vdmult;
>> + uint16_t lb_mode;
> The amount of local variables suggests that this function needs to be
> split into several smaller ones.
>
> By the way, do you need to initialize all of them? GCC will at least
> warn (if not error out) if an unitialized variable is referenced, so
> it's enough to make sure that the code correctly covers all branch
> paths, which is actually better for the code than to rely on default
> value.
Yeah, some value directly write to the vop, some value gcc report the
warn, so initialize them.
>> +
>> + if (((yrgb_dst_w << 3) <= yrgb_src_w) ||
>> + ((yrgb_dst_h << 3) <= yrgb_src_h) ||
> Hmm, if this is enforcing the maximum downscaling factor, then
> wouldn't it be more readable to write like this:
>
> yrgb_src_w >= 8 * yrgb_dst_w
>
> Also is >= correct here? Is the maximum factor less than 8?
yrgb_src_w >= 8 * yrgb_dst_w means we can't scale down exceed 8,
means that (src_w)1920->(dst_w)240 is wrong.
>> + yrgb_dst_w > 3840) {
> I'd suggest moving this to separate if with different error message.
>
>> + DRM_ERROR("yrgb scale exceed 8,src[%dx%d] dst[%dx%d]\n",
>> + yrgb_src_w, yrgb_src_h, yrgb_dst_w, yrgb_dst_h);
> Hmm, maybe "Maximum downscaling factor (8) exceeded" and then for the
> destination width check "Maximum destination width (3840) exceeded"?
OK.
>> + return;
>> + }
>> +
>> + if (yrgb_src_w < yrgb_dst_w)
>> + yrgb_hor_scl_mode = SCALE_UP;
>> + else if (yrgb_src_w > yrgb_dst_w)
>> + yrgb_hor_scl_mode = SCALE_DOWN;
>> + else
>> + yrgb_hor_scl_mode = SCALE_NONE;
> This looks like a good candidate for a helper function:
>
> enum scl_mode scl_get_scl_mode(uint32_t src, uint32_t dst)
> {
> if (src < dst)
> return SCALE_UP;
> else if (src > dst)
> return SCALE_DOWN;
> else
> return SCALE_NONE;
> }
>
OK
>> +
>> + if (yrgb_src_h < yrgb_dst_h)
>> + yrgb_ver_scl_mode = SCALE_UP;
>> + else if (yrgb_src_h > yrgb_dst_h)
>> + yrgb_ver_scl_mode = SCALE_DOWN;
>> + else
>> + yrgb_ver_scl_mode = SCALE_NONE;
> And now the helper function could be used here as well.
>
>> +
>> + if (is_yuv) {
>> + cbcr_src_w = src_w / hsub;
>> + cbcr_src_h = src_h / vsub;
>> + cbcr_dst_w = dst_w;
>> + cbcr_dst_h = dst_h;
>> + if ((cbcr_dst_w << 3) <= cbcr_src_w ||
>> + (cbcr_dst_h << 3) <= cbcr_src_h ||
>> + cbcr_src_w > 3840 ||
>> + cbcr_src_w == 0)
>> + DRM_ERROR("cbcr scale failed,src[%dx%d] dst[%dx%d]\n",
>> + cbcr_src_w, cbcr_src_h,
>> + cbcr_dst_w, cbcr_dst_h);
> I believe you should have those already enforced by Y plane. Also it
> doesn't seem reasonable to ever get 0 src width as an argument for
> this function.
>
>> + if (cbcr_src_w < cbcr_dst_w)
>> + cbr_hor_scl_mode = SCALE_UP;
>> + else if (cbcr_src_w > cbcr_dst_w)
>> + cbr_hor_scl_mode = SCALE_DOWN;
>> +
>> + if (cbcr_src_h < cbcr_dst_h)
>> + cbr_ver_scl_mode = SCALE_UP;
>> + else if (cbcr_src_h > cbcr_dst_h)
>> + cbr_ver_scl_mode = SCALE_DOWN;
> Aren't the scl_modes for CbCr planes always the same as for Y plane?
No, such as src(1920 x 1080) -> dst(1280x800), yuv format is NV12.
so Y plane horizontal and vertical is scale down.
but src_w = 1920 / 2 = 960 < 1280
src_h = 1080 / 2 = 540 < 800.
So Cbcr horizontal and vertical is scale up.
>> + }
>> + /*
>> + * line buffer mode
>> + */
>> + if (is_yuv) {
>> + if (yrgb_hor_scl_mode == SCALE_DOWN && yrgb_dst_w > 2560)
>> + lb_mode = LB_RGB_3840X2;
>> + else if (cbr_hor_scl_mode == SCALE_DOWN)
>> + lb_mode = _vop_cal_cbcr_lb_mode(cbcr_dst_w);
>> + else
>> + lb_mode = _vop_cal_cbcr_lb_mode(cbcr_src_w);
>> + } else {
>> + if (yrgb_hor_scl_mode == SCALE_DOWN)
>> + lb_mode = _vop_cal_yrgb_lb_mode(yrgb_dst_w);
>> + else
>> + lb_mode = _vop_cal_yrgb_lb_mode(yrgb_src_w);
>> + }
> I guess this could be moved into a helper function.
>
>> +
>> + switch (lb_mode) {
>> + case LB_YUV_3840X5:
>> + case LB_YUV_2560X8:
>> + case LB_RGB_1920X5:
>> + case LB_RGB_1280X8:
>> + yrgb_vsu_mode = SCALE_UP_BIC;
>> + cbr_vsu_mode = SCALE_UP_BIC;
> I might be overlooking something, but don't yrgb_vsu_mode and
> cbr_vsu_mode always have the same values?
Hmm, yrgb_vsu_mode and cbr_vsu_mode mean different vop config, OK, I
think it can merge
together.
>> + break;
>> + case LB_RGB_3840X2:
>> + if (yrgb_ver_scl_mode != SCALE_NONE)
>> + DRM_ERROR("ERROR : not allow yrgb ver scale\n");
>> + if (cbr_ver_scl_mode != SCALE_NONE)
>> + DRM_ERROR("ERROR : not allow cbcr ver scale\n");
> Shouldn't these return error? Also it would be nice to make the error
> messages more helpful.
OK.
>> + break;
>> + case LB_RGB_2560X4:
>> + yrgb_vsu_mode = SCALE_UP_BIL;
>> + cbr_vsu_mode = SCALE_UP_BIL;
>> + break;
>> + default:
>> + DRM_ERROR("unsupport lb_mode:%d\n", lb_mode);
>> + break;
>> + }
> Anyway, this whole switch is a candidate for a helper function.
This only use one time, it's ok to be a helper function?
>> + /*
>> + * (1.1)YRGB HOR SCALE FACTOR
>> + */
>> + switch (yrgb_hor_scl_mode) {
>> + case SCALE_UP:
>> + scale_yrgb_x = GET_SCL_FT_BIC(yrgb_src_w, yrgb_dst_w);
>> + break;
>> + case SCALE_DOWN:
>> + switch (yrgb_hsd_mode) {
>> + case SCALE_DOWN_BIL:
>> + scale_yrgb_x = GET_SCL_FT_BILI_DN(yrgb_src_w,
>> + yrgb_dst_w);
>> + break;
>> + case SCALE_DOWN_AVG:
>> + scale_yrgb_x = GET_SCL_FT_AVRG(yrgb_src_w, yrgb_dst_w);
>> + break;
>> + default:
>> + DRM_ERROR("unsupport yrgb_hsd_mode:%d\n",
>> + yrgb_hsd_mode);
> Shouldn't this return an error?
>
>> + break;
>> + }
>> + break;
>> + }
> Ditto.
>
>> +
>> + /*
>> + * (1.2)YRGB VER SCALE FACTOR
>> + */
>> + switch (yrgb_ver_scl_mode) {
>> + case SCALE_UP:
>> + switch (yrgb_vsu_mode) {
>> + case SCALE_UP_BIL:
>> + scale_yrgb_y = GET_SCL_FT_BILI_UP(yrgb_src_h,
>> + yrgb_dst_h);
>> + break;
>> + case SCALE_UP_BIC:
>> + if (yrgb_src_h < 3)
>> + DRM_ERROR("yrgb_src_h should greater than 3\n");
> Shouldn't this return an error?
>
>> + scale_yrgb_y = GET_SCL_FT_BIC(yrgb_src_h, yrgb_dst_h);
>> + break;
>> + default:
>> + DRM_ERROR("unsupport yrgb_vsu_mode:%d\n",
>> + yrgb_vsu_mode);
> Shouldn't this return an error?
>
>> + break;
>> + }
>> + break;
>> + case SCALE_DOWN:
>> + switch (yrgb_vsd_mode) {
>> + case SCALE_DOWN_BIL:
>> + vdmult = _get_vskiplines(yrgb_src_h, yrgb_dst_h);
>> + scale_yrgb_y = GET_SCL_FT_BILI_DN_VSKIP(yrgb_src_h,
>> + yrgb_dst_h,
>> + vdmult);
>> + if (vdmult == 4) {
>> + vsd_yrgb_gt4 = 1;
>> + vsd_yrgb_gt2 = 0;
>> + } else if (vdmult == 2) {
>> + vsd_yrgb_gt4 = 0;
>> + vsd_yrgb_gt2 = 1;
>> + }
> How about something like this:
>
> vsd_yrgb_gt4 = (vdmult == 4);
> vsd_yrgb_gt2 = (vdmult == 2);
But I think it's not easy to read.
>> + break;
>> + case SCALE_DOWN_AVG:
>> + scale_yrgb_y = GET_SCL_FT_AVRG(yrgb_src_h, yrgb_dst_h);
>> + break;
>> + default:
>> + DRM_ERROR("unsupport yrgb_vsd_mode:%d\n",
>> + yrgb_vsd_mode);
> Shouldn't this return an error?
>
>> + break;
>> + }
>> + break;
>> + }
> Another candidate for separate helper function.
>
>> + /*
>> + * (2.1)CBCR HOR SCALE FACTOR
>> + */
>> + switch (cbr_hor_scl_mode) {
>> + case SCALE_UP:
>> + scale_cbcr_x = GET_SCL_FT_BIC(cbcr_src_w, cbcr_dst_w);
>> + break;
>> + case SCALE_DOWN:
>> + switch (cbr_hsd_mode) {
>> + case SCALE_DOWN_BIL:
>> + scale_cbcr_x = GET_SCL_FT_BILI_DN(cbcr_src_w,
>> + cbcr_dst_w);
>> + break;
>> + case SCALE_DOWN_AVG:
>> + scale_cbcr_x = GET_SCL_FT_AVRG(cbcr_src_w, cbcr_dst_w);
>> + break;
>> + default:
>> + DRM_ERROR("unsupport cbr_hsd_mode:%d\n", cbr_hsd_mode);
> Error.
>
>> + break;
>> + }
>> + break;
>> + }
> Isn't this switch exactly the same as for Y plane just with different
> widths used? Also, wouldn't the values for CbCr plane be the same as
> for Y plane?
But Cbcr case is not same as Y plane case, how to merge?
>> +
>> + /*
>> + * (2.2)CBCR VER SCALE FACTOR
>> + */
>> + switch (cbr_ver_scl_mode) {
>> + case SCALE_UP:
>> + switch (cbr_vsu_mode) {
>> + case SCALE_UP_BIL:
>> + scale_cbcr_y = GET_SCL_FT_BILI_UP(cbcr_src_h,
>> + cbcr_dst_h);
>> + break;
>> + case SCALE_UP_BIC:
>> + if (cbcr_src_h < 3)
>> + DRM_ERROR("cbcr_src_h need greater than 3 !\n");
>> + scale_cbcr_y = GET_SCL_FT_BIC(cbcr_src_h, cbcr_dst_h);
>> + break;
>> + default:
>> + DRM_ERROR("unsupport cbr_vsu_mode:%d\n", cbr_vsu_mode);
> Error.
>
>> + break;
>> + }
>> + break;
>> + case SCALE_DOWN:
>> + switch (cbr_vsd_mode) {
>> + case SCALE_DOWN_BIL:
>> + vdmult = _get_vskiplines(cbcr_src_h, cbcr_dst_h);
>> + scale_cbcr_y = GET_SCL_FT_BILI_DN_VSKIP(cbcr_src_h,
>> + cbcr_dst_h,
>> + vdmult);
>> + if (vdmult == 4) {
>> + vsd_cbr_gt4 = 1;
>> + vsd_cbr_gt2 = 0;
>> + } else if (vdmult == 2) {
>> + vsd_cbr_gt4 = 0;
>> + vsd_cbr_gt2 = 1;
>> + }
>> + break;
>> + case SCALE_DOWN_AVG:
>> + scale_cbcr_y = GET_SCL_FT_AVRG(cbcr_src_h, cbcr_dst_h);
>> + break;
>> + default:
>> + DRM_ERROR("unsupport cbr_vsd_mode:%d\n", cbr_vsd_mode);
>> + break;
>> + }
>> + break;
>> + }
> Again, this looks like the values for CbCr would be the same as for Y.
> Are there actually cases when they differ?
Hmm, Actually the cbcr calculations have some different with Y.
>> +
>> + VOP_SCL_SET(vop, win, yrgb_hor_scl_mode, yrgb_hor_scl_mode);
>> + VOP_SCL_SET(vop, win, yrgb_ver_scl_mode, yrgb_ver_scl_mode);
>> + VOP_SCL_SET(vop, win, cbr_hor_scl_mode, cbr_hor_scl_mode);
>> + VOP_SCL_SET(vop, win, cbr_ver_scl_mode, cbr_ver_scl_mode);
>> + VOP_SCL_SET(vop, win, lb_mode, lb_mode);
>> + VOP_SCL_SET(vop, win, yrgb_hsd_mode, yrgb_hsd_mode);
>> + VOP_SCL_SET(vop, win, cbr_hsd_mode, cbr_hsd_mode);
>> + VOP_SCL_SET(vop, win, yrgb_vsd_mode, yrgb_vsd_mode);
>> + VOP_SCL_SET(vop, win, cbr_vsd_mode, cbr_vsd_mode);
>> + VOP_SCL_SET(vop, win, yrgb_vsu_mode, yrgb_vsu_mode);
>> + VOP_SCL_SET(vop, win, cbr_vsu_mode, cbr_vsu_mode);
>> + VOP_SCL_SET(vop, win, scale_yrgb_x, scale_yrgb_x);
>> + VOP_SCL_SET(vop, win, scale_yrgb_y, scale_yrgb_y);
>> + VOP_SCL_SET(vop, win, vsd_yrgb_gt4, vsd_yrgb_gt4);
>> + VOP_SCL_SET(vop, win, vsd_yrgb_gt2, vsd_yrgb_gt2);
>> + VOP_SCL_SET(vop, win, scale_cbcr_x, scale_cbcr_x);
>> + VOP_SCL_SET(vop, win, scale_cbcr_y, scale_cbcr_y);
>> + VOP_SCL_SET(vop, win, vsd_cbr_gt4, vsd_cbr_gt4);
>> + VOP_SCL_SET(vop, win, vsd_cbr_gt2, vsd_cbr_gt2);
> If you split this function into smaller ones, you probably don't want
> to keep all the writes like here in one place, but rather make the
> smaller functions write particular registers after calculating their
> values.
Hmm, I will try to do that.
>> +}
>> +
>> /*
>> * Caller must hold vsync_mutex.
>> */
>> @@ -624,6 +998,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
>> .y2 = crtc->mode.vdisplay,
>> };
>> bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
>> + int min_scale = win->phy->scl ? 0x02000 : DRM_PLANE_HELPER_NO_SCALING;
>> + int max_scale = win->phy->scl ? 0x80000 : DRM_PLANE_HELPER_NO_SCALING;
> Hmm, I wonder if there aren't maybe some helpers to generate fixed
> point values in the kernel in a readable way. If not, maybe something
> like this would do:
>
> #define FRAC_16_16(mult, div) (((mult) << 16) / (div))
>
> and then you would just use
>
> FRAC_16_16(1, 8) and FRAC_16_16(8, 1) for min and max scale respectively.
OK.
>> if (drm_format_num_planes(fb->pixel_format) > 2) {
>> DRM_ERROR("unsupport more than 2 plane format[%08x]\n",
>> @@ -633,8 +1009,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
>>
>> ret = drm_plane_helper_check_update(plane, crtc, fb,
>> &src, &dest, &clip,
>> - DRM_PLANE_HELPER_NO_SCALING,
>> - DRM_PLANE_HELPER_NO_SCALING,
>> + min_scale,
>> + max_scale,
>> can_position, false, &visible);
>> if (ret)
>> return ret;
>> @@ -732,9 +1108,18 @@ static int vop_update_plane_event(struct drm_plane *plane,
>> VOP_WIN_SET(vop, win, uv_vir, uv_vir_stride);
>> VOP_WIN_SET(vop, win, uv_mst, uv_mst);
>> }
>> +
>> + if (win->phy->scl)
>> + _vop_cal_scl_fac(vop, win, actual_w, actual_h,
>> + dest.x2 - dest.x1, dest.y2 - dest.y1,
>> + fb->pixel_format);
> Maybe the function could be named "vop_init_scaler()".
>
OK
>> +
>> val = (actual_h - 1) << 16;
>> val |= (actual_w - 1) & 0xffff;
>> VOP_WIN_SET(vop, win, act_info, val);
>> +
>> + val = (dest.y2 - dest.y1 - 1) << 16;
>> + val |= (dest.x2 - dest.x1 - 1) & 0xffff;
>> VOP_WIN_SET(vop, win, dsp_info, val);
>> val = (dsp_sty - 1) << 16;
>> val |= (dsp_stx - 1) & 0xffff;
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> index 63e9b3a..edacdee 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> @@ -198,4 +198,100 @@ enum factor_mode {
>> ALPHA_SRC_GLOBAL,
>> };
>>
>> +enum scale_mode {
>> + SCALE_NONE = 0x0,
>> + SCALE_UP = 0x1,
>> + SCALE_DOWN = 0x2
>> +};
>> +
>> +enum lb_mode {
>> + LB_YUV_3840X5 = 0x0,
>> + LB_YUV_2560X8 = 0x1,
>> + LB_RGB_3840X2 = 0x2,
>> + LB_RGB_2560X4 = 0x3,
>> + LB_RGB_1920X5 = 0x4,
>> + LB_RGB_1280X8 = 0x5
>> +};
>> +
>> +enum sacle_up_mode {
>> + SCALE_UP_BIL = 0x0,
>> + SCALE_UP_BIC = 0x1
>> +};
>> +
>> +enum scale_down_mode {
>> + SCALE_DOWN_BIL = 0x0,
>> + SCALE_DOWN_AVG = 0x1
>> +};
>> +
>> +#define CUBIC_PRECISE 0
>> +#define CUBIC_SPLINE 1
>> +#define CUBIC_CATROM 2
>> +#define CUBIC_MITCHELL 3
>> +
>> +#define CUBIC_MODE_SELETION CUBIC_PRECISE
>> +
>> +#define SCL_FT_BILI_DN_FIXPOINT_SHIFT 12
>> +#define SCL_FT_BILI_DN_FIXPOINT(x) \
>> + ((INT32)((x)*(1 << SCL_FT_BILI_DN_FIXPOINT_SHIFT)))
> static inline
>
>> +
>> +#define SCL_FT_BILI_UP_FIXPOINT_SHIFT 16
>> +
>> +#define SCL_FT_AVRG_FIXPOINT_SHIFT 16
>> +#define SCL_FT_AVRG_FIXPOINT(x) \
>> + ((INT32)((x) * (1 << SCL_FT_AVRG_FIXPOINT_SHIFT)))
> static inline
>
>> +
>> +#define SCL_FT_BIC_FIXPOINT_SHIFT 16
>> +#define SCL_FT_BIC_FIXPOINT(x) \
>> + ((INT32)((x)*(1 << SCL_FT_BIC_FIXPOINT_SHIFT)))
> static inline
>
>> +
>> +#define SCL_FT_DEFAULT_FIXPOINT_SHIFT 12
>> +#define SCL_FT_VSDBIL_FIXPOINT_SHIFT 12
>> +
>> +#define SCL_CAL(src, dst, shift) \
>> + ((((src) * 2 - 3) << ((shift) - 1)) / ((dst) - 1))
>> +#define GET_SCL_FT_BILI_DN(src, dst) \
>> + SCL_CAL(src, dst, SCL_FT_BILI_DN_FIXPOINT_SHIFT)
>> +#define GET_SCL_FT_BILI_UP(src, dst) \
>> + SCL_CAL(src, dst, SCL_FT_BILI_UP_FIXPOINT_SHIFT)
>> +#define GET_SCL_FT_BIC(src, dst) \
>> + SCL_CAL(src, dst, SCL_FT_BIC_FIXPOINT_SHIFT)
>> +
>> +#define GET_SCL_DN_ACT_HEIGHT(src_h, vdmult) \
>> + (((src_h) + (vdmult) - 1) / (vdmult))
> All of the function macros above: static inline.
>
>> +
>> +#define MIN_SCL_FT_AFTER_VSKIP 1
>> +#define GET_SCL_FT_BILI_DN_VSKIP(src_h, dst_h, vdmult) \
>> + ((GET_SCL_DN_ACT_HEIGHT((src_h), (vdmult)) == (dst_h)) \
>> + ? (GET_SCL_FT_BILI_DN((src_h), (dst_h))/(vdmult)) \
>> + : GET_SCL_FT_BILI_DN(GET_SCL_DN_ACT_HEIGHT((src_h), \
>> + (vdmult)), (dst_h)))
> Static inline.
>
>> +
>> +#define GET_SCL_FT_AVRG(src, dst) \
>> + (((dst) << ((SCL_FT_AVRG_FIXPOINT_SHIFT) + 1)) \
>> + / (2 * (src) - 1))
> Static inline.
>
>> +
>> +#define SCL_COOR_ACC_FIXPOINT_SHIFT 16
>> +#define SCL_COOR_ACC_FIXPOINT_ONE (1 << SCL_COOR_ACC_FIXPOINT_SHIFT)
>> +#define SCL_COOR_ACC_FIXPOINT(x) \
>> + ((INT32)((x)*(1 << SCL_COOR_ACC_FIXPOINT_SHIFT)))
>> +#define SCL_COOR_ACC_FIXPOINT_REVERT(x) \
>> + ((((x) >> (SCL_COOR_ACC_FIXPOINT_SHIFT-1)) + 1) >> 1)
>> +
>> +#define SCL_GET_COOR_ACC_FIXPOINT(scale, shift) \
>> + ((scale) << (SCL_COOR_ACC_FIXPOINT_SHIFT - (shift)))
>> +#define SCL_FILTER_FT_FIXPOINT_SHIFT 8
>> +#define SCL_FILTER_FT_FIXPOINT_ONE (1 << SCL_FILTER_FT_FIXPOINT_SHIFT)
>> +#define SCL_FILTER_FT_FIXPOINT(x) \
>> + ((INT32)((x) * (1 << SCL_FILTER_FT_FIXPOINT_SHIFT)))
>> +#define SCL_FILTER_FT_FIXPOINT_REVERT(x) \
>> + ((((x) >> (SCL_FILTER_FT_FIXPOINT_SHIFT - 1)) + 1) >> 1)
>> +
>> +#define SCL_GET_FILTER_FT_FIXPOINT(ca, shift) \
>> + (((ca) >> ((shift)-SCL_FILTER_FT_FIXPOINT_SHIFT)) & \
>> + (SCL_FILTER_FT_FIXPOINT_ONE - 1))
>> +
>> +#define SCL_OFFSET_FIXPOINT_SHIFT 8
>> +#define SCL_OFFSET_FIXPOINT(x) \
>> + ((INT32)((x) * (1 << SCL_OFFSET_FIXPOINT_SHIFT)))
> All of the function macros above: static inline. This should also let
> you remove the casts.
what the "casts" mean? Why do you thinking that "static inline" is
better than "#define"?
> Best regards,
> Tomasz
>
>
>
--
Ｍark
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists