inefficient CLUT read - Githubissues

gregory38 commented 9 years ago

I found that m_mem.m_clut.Read32 is executed a lots of time in the HW renderer. The valid bit is always cleared to false.

The clut is read 2 times by frame on the HW renderer with different setting.

Start of GSTextureCache::LookupSource

GIFRegTEXA plainTEXA;
memset(&plainTEXA, 0, sizeof(GIFRegTEXA));

plainTEXA.AEM = 1;
plainTEXA.TA0 = 0;
plainTEXA.TA1 = 0x80;
m_renderer->m_mem.m_clut.Read32(TEX0, plainTEXA);

After the texture lookup in the rendererHW

if(GSLocalMemory::m_psm[context->TEX0.PSM].pal > 0)
{
    m_mem.m_clut.Read32(context->TEX0, env.TEXA);
}

Here a list of improvement

It misses a memset(&plainTEXA, 0, sizeof(GIFRegTEXA)) otherwise uninitialized data are compared
First block could only be executed if psm.pal > 0
Ultimately plainTEXA must be replaced with normal TEXA. and the 2nd clut read must be removed. I don't understand why we need a special TEXA here. Neither why we do a conversion in the GPU for palette?

gregory38 commented 9 years ago

@sudonim1 @gabest11 if any of you have any info, feel free to share.

gregory38 commented 9 years ago

Hum it seems 16 bits palettes are alpha expanded with the function Expand16 But why 24 bits palettes aren't alpha expanded too?

gabest11 commented 9 years ago

https://code.google.com/p/pcsx2/source/detail?spec=svn5329&r=5329

gabest11 commented 9 years ago

https://code.google.com/p/pcsx2/source/detail?spec=svn5309&r=5309

gabest11 commented 9 years ago

I'm still not sure what the purpose of these changes were.

gabest11 commented 9 years ago

We need to save those comments ASAP.

gregory38 commented 9 years ago

Me too I don't understand the change. Do you know how work AEM on 24 bits for the SW renderer? I don't see any code to do it on GSClut.

gabest11 commented 9 years ago

I don't think there is a palette format other than 16 and 32 bit.

gregory38 commented 9 years ago

Oh yes you're right. I didn't think about this possibility!

gabest11 commented 9 years ago

Saving the comments from googlecode. I have already backed up the svn dump before. If anyone needs it, I'll upload it somewhere.

gregory38 commented 9 years ago

@gigaherz did you do backup the google comment? Or did you only keep the log message in GIT?

gregory38 commented 9 years ago

Hum I think I begin to understand the issue: The clut will be converted to do the alpha expansion (Expand16)

It is potentially done during the upload of the texture (GSTextureCache::Source::Flush) through rtx function pointer.

(mem.*rtx)(off, r, buff, pitch, plainTEXA);

And finally it is done in the shader.

    for (int i = 0; i < 4; i++)
    {
#if ((PS_FMT & ~FMT_PAL) == FMT_24)
        c[i].a = ( (PS_AEM == 0) || any(bvec3(c[i].rgb))  ) ? TA.x : 0.0f;
#elif ((PS_FMT & ~FMT_PAL) == FMT_16)
        c[i].a = c[i].a >= 0.5 ? TA.y : ( (PS_AEM == 0) || any(bvec3(c[i].rgb)) ) ? TA.x : 0.0f;
#endif
    }

gregory38 commented 9 years ago

Hum old code only do AE for pure 24/16 bits RGBA texture. However it seems GSLocalMemory will do it also. It really seems that shader doesn't need to do it.

gregory38 commented 9 years ago

Ok. I get it. AE must be done if the texture was an old target and therefore CPU never has the opportunity to do the conversion. However if the conversion is done on the CPU, we must check the content of TEXA to select a source texture in the cache.

Then I suspect that any fix of the old revision comes from the change of bilinear interpolation. Old code uses the HW unit (faster but less accurate), new code does the interpolation in shader (more often). The condition to select the bilinear interpolation is rather complex but it explains the rise of GPU load. As you might know, some game really requires an accurate interpolation factor.