Bug in drawpixel(..)?

[AJJ]

In the drawpixel(..) function in gdisp.c starting at line 127 there is the following section:

Quote

    // Next best is cursor based streaming
    #if GDISP_HARDWARE_DRAWPIXEL != TRUE && GDISP_HARDWARE_STREAM_POS && GDISP_HARDWARE_STREAM_WRITE
        #if GDISP_HARDWARE_STREAM_POS == HARDWARE_AUTODETECT
            if (gvmt(g)->writepos)
        #endif
        {
            if (!(g->flags & GDISP_FLG_SCRSTREAM))
                setglobalwindow(g);
            gdisp_lld_write_pos(g);
            gdisp_lld_write_color(g);
            return;
        }
    #endif

It seems to me that this code is missing calls to acquire and release the bus around the gdisp_lld_write_xxx(..) functions.  The setglobalwindow(..) does call gdisp_lld_write_start(..), which acquires the bus, but setglobalwindow(..) is not always called.  And of course there isn't anything to release the bus.

Edited June 23, 2017 by AJJ

[AJJ]

After some more looking at the code I think I understand that the GDISP_FLG_SCRSTREAM flag is meant to indicate that the bus is being held open and eventually autoflush(..) is supposed to release the bus.  However, in my code I'm somehow getting into the drawpixel(..) function without owning the bus but the GDISP_FLG_SCRSTREAM is already set.  So let me dig some more and I'll post an update once I've figured it out.

[AJJ]

In the function hline_clip(..) in gdisp.c, the following code is executed when GDISP_HARDWARE_FILLS is defined:

    // This is an optimization for the point case. It is only worthwhile however if we
    // have hardware fills or if we support both hardware pixel drawing and hardware streaming
    #if GDISP_HARDWARE_FILLS || (GDISP_HARDWARE_DRAWPIXEL && GDISP_HARDWARE_STREAM_WRITE)
        // Is this a point
        if (g->p.x == g->p.x1) {
            drawpixel(g);
            return;
        }
    #endif

    // Best is hardware accelerated area fill
    #if GDISP_HARDWARE_FILLS
        #if GDISP_HARDWARE_FILLS == HARDWARE_AUTODETECT
            if (gvmt(g)->fill)
        #endif
        {
            g->p.cx = g->p.x1 - g->p.x + 1;
            g->p.cy = 1;
            gdisp_lld_fill_area(g);
            return;
        }
    #endif

I think the problem that I see is that drawpixel(..) is aware of the GDISP_FLG_SCRSTREAM flag, acquires the bus only if this flag is not set, and does not release the bus.  However, the SSD2119 version of gdisp_lld_fill_area(..) (as well as many of the others that I checked) does not seem to be aware of this GDISP_FLG_SCRSTREAM flag, always acquires the bus and always releases the bus.

So if this hline_clip(..) function is being called in a loop, as it is when drawing text, then depending on the order of single pixels vs. areas drawn we can mismanage the bus ownership.  For instance, if we draw a single pixel and then an area, we will try to acquire the bus twice in a row, then perform a single release.  Then if we draw another single pixel the GDISP_FLG_SCRSTREAM flag is already set and we attempt to access the bus without a first acquiring it.

Have I misunderstood the intent of the acquire_bus(..) and release_bus(..) functions and they are supposed to implement reference counting?

[inmarket]

The GDISP_FLG_SCRSTREAM flag has nothing to do with the current bus state. This flag is used to indicate that the current controller window covers the entire display area. It is an optimization that is only possible when the controller supports a position set operation. The flag improves the efficiency of setting pixels where no vertical wrap is required.

Note also that it is perfectly fine to aquire the bus multiple times (or arguably even release it multiple times). It is the first aquire and first release that are effective. 

For a streaming driver the cycle is start/write.../stop. For a streaming  driver with pos capability it is start/(pos/write).../stop.

In both cases start and stop are typically used to obtain and release the bus.

When you add fillarea into the mix things become a lot more complex. The fillarea is defined as being independent of the stream cursor window. If a driver maintains both streaming and fillarea it needs to be able to perform the fillarea without affecting the stream operation if any is in progress or to atleast reset the cursor window to its original state (and by implication the bus).

Looking at the SSD2119 driver, the fillarea you are talking about is only activated in the DMA enabled situation. DMA is completely incompatible with a shared bus unless custom interrupt handlers are written to release the bus at the end of the dma operation and thus aquire/release are meaningless in that scenario. 

If you are finding writes on an otherwise unaquired bus this is likely therefore to be a driver bug. The simple solution in the short term is to simply turn off the accelerated fillarea (or the dma acceleration) in the gdisp_lld_config.h file for the driver.

Looking at the few drivers that use streaming with a fillarea acceleration, it appears that the assumption of the cursor window being retained is actually not normally happenning. I have therefore made a change to the gdisp that should fix that for those few drivers. The changes will be in the repository soon and should also help with your aquire/release ordering (although it will not fix drivers that prematurely release the bus on a dma operation).

[inmarket]

The updates are now in the repo.

[AJJ]

I am finally updating uGFX from the version that I had downloaded 2 years ago.  Per the previous comments I see that you added the following snippet of code to the fillarea(..) and vline_clip(..) functions just prior to calling gdisp_lld_fill_area(..):

            #if GDISP_HARDWARE_STREAM_POS && GDISP_HARDWARE_STREAM_WRITE
                if ((g->flags & GDISP_FLG_SCRSTREAM)) {
                    gdisp_lld_write_stop(g);
                    g->flags &= ~GDISP_FLG_SCRSTREAM;
                }
            #endif

I think that this same piece of code also needs to be added to the hline_clip(..) function prior to calling gdisp_lld_fill_area(..).  Without this code in hline_clip(..) my code locks up because it tries to access the bus without re-acquiring after gdisp_lld_fill_area(..) releases it.

There are a couple of other calls to gdisp_lld_fill_area(..) such as in gdispGClear(..), but I'm not calling these functions in my own code so I'm not sure if this same problem exists there.

Thanks,
AJ