[Openchrome-devel] xf86-video-openchrome: src/via_display.c src/via_driver.h

[Luc Verhaegen]

On Fri, Jan 06, 2017 at 10:18:24AM +0000, Kevin Brace wrote:
>  src/via_display.c |  244 ++++++++++++++++++++++++++++++++++++++++++++----------
>  src/via_driver.h  |    3 
>  2 files changed, 203 insertions(+), 44 deletions(-)
> 
> New commits:
> commit f8551e0e07366a70cf6aa8c3801d2965a86e170f
> Author: Kevin Brace <kevinbrace at gmx.com>
> Date:   Fri Jan 6 04:17:32 2017 -0600
> 
>     Preparation for the partial fix of save / restore functions
>     
>     Signed-off-by: Kevin Brace <kevinbrace at gmx.com>
> 
> diff --git a/src/via_display.c b/src/via_display.c
> index a405ce2..9f3920a 100644
> --- a/src/via_display.c
> +++ b/src/via_display.c
> @@ -3342,41 +3342,111 @@ viaIGA2Save(ScrnInfoPtr pScrn)
>      DEBUG(xf86DrvMsg(pScrn->scrnIndex, X_INFO,
>                          "Saving IGA2 registers.\n"));
>  
> -/* Unlock and save extended registers. */
> +    /* Unlock extended registers. */
>      hwp->writeSeq(hwp, 0x10, 0x01);
>  
> -    /* Save LCD control registers (from CR 0x50 to 0x93). */
> -    for (i = 0; i < 68; i++)
> -        Regs->CRTCRegs[i] = hwp->readCrtc(hwp, i + 0x50);
> -
> -    if (pVia->Chipset != VIA_CLE266 && pVia->Chipset != VIA_KM400) {
> -        /* LVDS Channel 2 Function Select 0 / DVI Function Select */
> -        Regs->CR97 = hwp->readCrtc(hwp, 0x97);
> -        /* LVDS Channel 1 Function Select 0 */
> -        Regs->CR99 = hwp->readCrtc(hwp, 0x99);
> -        /* Digital Video Port 1 Function Select 0 */
> -        Regs->CR9B = hwp->readCrtc(hwp, 0x9B);
> -        /* Power Now Control 4 */
> -        Regs->CR9F = hwp->readCrtc(hwp, 0x9F);
> -
> -        /* Horizontal Scaling Initial Value */
> -        Regs->CRA0 = hwp->readCrtc(hwp, 0xA0);
> -        /* Vertical Scaling Initial Value */
> -        Regs->CRA1 = hwp->readCrtc(hwp, 0xA1);
> -        /* Scaling Enable Bit */
> -        Regs->CRA2 = hwp->readCrtc(hwp, 0xA2);
> +    for (i = 0; i < (0x88 - 0x50 + 1); i++) {
> +        Regs->CR[i + 0x50] = hwp->readCrtc(hwp, i + 0x50);
> +    }
> +
> +    for (i = 0; i < (0x92 - 0x8A + 1); i++) {
> +        Regs->CR[i + 0x8A] = hwp->readCrtc(hwp, i + 0x8A);
> +    }
> +
> +    for (i = 0; i < (0xA3 - 0x94 + 1); i++) {
> +        Regs->CR[i + 0x94] = hwp->readCrtc(hwp, i + 0x94);
>      }

This is a mistake, and a very slippery slope you started. I had some 
very good reasons for not doing tables for save/restore.
1) Direct typed registers requires you to think very carefully about 
which registers to restore. They require you to have very acute 
knowledge of the display engine you are working with. Direct type 
register names do not allow you to take shortcuts.
2) Synchronicity with a the real modesetting code. A missed register 
in save/restore will usually also mean a missed register in the proper 
modeset. Direct typed forces you to think about things more in depth and 
makes it more natural to keep both the save/restore as the actual 
modeset in sync.
2) what about timing and ordering? How are you going to deal with that 
with the next abstraction step after you've introduced a table?

The fact that you missed CR6A/6B clearly shows that you missed something 
big in save/restore. What did you do to stop saving restoring these 
registers, which are very ordering critical.

My most recent customer was using register tables both in the capture 
chip setup and for their poor excuse for a bootloader. In 2016. It was 
amazing. Besides the fact that they were poor developers who couldn't 
find or fix anything without JTAG, it also made it impossible for proper 
developers to see the forest through the trees, or to try to use logic 
to figure things out, and it was impossible to try to fix (apart from 
the fact that the poor developers would have to change their idiotic 
ways, which is impossible in itself).

Luc Verhaegen.