On Fri, Mar 22, 2019 at 01:17:55PM +0800, Yifeng Li wrote:
> Previously, in staging/sm7xxfb (now fbdev/sm712fb), 2D acceleration
> was implemented, but after its submission, a critical bug that causes
> total system hang was discovered, as a stopgap measure, 2D ops was
> completele removed in commit 3af805735a25 ("staging: sm7xx: remove the
> buggy 2D acceleration support") and never implemented again.
>
> It created a massive usability problem - on YeeLoong 8089, a notable
> MIPS platform which uses SM712 - even scrolling a single line of text
> on the console required an unaccelerated screen redraw, running "dmesg"
> typically takes 8-11 seconds, and absurdly, printf(), became a significant
> performance bottleneck that slows down GCC and "make", make the computer
> largely unusable.
>
> So I decided to take a look. Most of the my actual development was done
> in 2014 in a personal out-of-tree driver, I did not mainline it because
> 2D acceleration was not working properly in 24-bit color. I discovered
> the solution in early 2019 and now it's ready to be mainlined.
>
> This commit reimplements the 2D acceleration for sm712fb. Unlike the
> original implementation, which was messy and unnecessarily complicated
> by calling a 2D acceleration wrapper file with many unneeded functions,
> this is a minimum and (relatively) clean implementation. My tests have
> shown that running "dmesg" only takes 0.9 seconds, a performance boost
> of 950%. System hangs did not occur in my tests.
I didnot notice any performace improvement in my system. Infact, I have
never seen the performance problem that you mentioned. But, it will be
good to have the 2D feature back again.
>
<snip>
> + */
> + smtcfb_reset_accel();
> +
> smtc_set_timing(sfb);
> +
> + /*
> + * Currently, 2D acceleration is only supported on SM712 with
> + * little-endian CPUs, it's disabled on Big Endian systems and SM720
> + * chips as a safety measure. Since I don't have monetary or hardware
> + * support from any company or OEMs, I don't have the hardware and
> + * it's completely untested. I should be also to purchase a Big Endian
> + * test platform and add proper support soon. I still have to spend
> + * 200 USD+ to purchase this piece of 1998's hardware, yikes! If you
> + * have a Big-Endian platform with SM7xx available for testing, please
> + * send an E-mail to Tom, thanks!
> + */
comments in the code are usually used to increase the readability, and
I dont think adding this comment adds to the readibility. I also spent
personal money to get these hardwares but that was never added to any
commit message or comment. Please remove this comment.
> +#ifdef __BIG_ENDIAN
> + sfb->accel = false;
> + if (accel)
> + dev_info(&sfb->pdev->dev,
> + "2D acceleration is unsupported on Big Endian.\n");
> +#endif
> + if (!accel) {
> + sfb->accel = false;
> + dev_info(&sfb->pdev->dev,
> + "2D acceleration is disabled by the user.\n");
> + }
> +
> + /* reset 2D engine after a modesetting is mandatory */
> + smtcfb_reset_accel();
If it is a big endian, acceleration is disabled, but you are still
calling smtcfb_reset_accel() which will "enable Zoom Video Port,
2D Drawing Engine and Video Processor". Will that not create any problem
in a big endian system?
> + smtcfb_init_accel(sfb);
> }
>
> static int smtc_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> @@ -1401,6 +1459,316 @@ static struct fb_ops smtcfb_ops = {
> .fb_write = smtcfb_write,
> };
>
> +static int smtcfb_wait(struct smtcfb_info *fb)
> +{
> + int i;
> + u8 reg;
> +
> + smtc_dprr(DPR_DE_CTRL);
> + for (i = 0; i < 10000; i++) {
> + reg = smtc_seqr(SCR_DE_STATUS);
> + if ((reg & SCR_DE_STATUS_MASK) == SCR_DE_ENGINE_IDLE)
> + return 0;
> + udelay(1);
> + }
> + dev_err(&fb->pdev->dev, "2D engine hang detected!\n");
> + return -EBUSY;
> +}
> +
> +static void
> +smtcfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
> +{
> + u32 width = rect->width, height = rect->height;
> + u32 dx = rect->dx, dy = rect->dy;
> + u32 color;
> +
> + struct smtcfb_info *sfb = info->par;
> +
> + if (unlikely(info->state != FBINFO_STATE_RUNNING))
> + return;
Did you measure the performance difference with and without "unlikely"?
Quoting GregKH from https://lkml.org/lkml/2018/11/7/36
"don't do stuff like this unless you can actually measure the
difference".
> +
<snip>
> + * & HIGH with 0xffffffff (all ones, and we have already set
> + * that in smtcfb_init_accel). Since the color of this mono
> + * pattern is controlled by DPR_FG_COLOR, BITBLTing it with
> + * ROP_COPY is effectively a rectfill().
> + */
> + smtc_dprw(DPR_FG_COLOR, color);
> + smtc_dprw(DPR_DST_COORDS, DPR_COORDS(dx, dy));
I will need some more time to verify all these and other registers with
the datasheet and test again.
--
Regards
Sudip
