[PATCH r128] Do not disable gouraud shading for a render op

[Alex Deucher]

On Wed, Nov 27, 2013 at 3:48 PM, Connor Behan <connor.behan at gmail.com> wrote:
> On 26/11/13 09:50 PM, Alex Deucher wrote:
>> On Tue, Nov 26, 2013 at 10:36 PM, Connor Behan
>> <connor.behan at gmail.com> wrote:
>>> [Fixing subject line]
>>>
>>>
>>> On 26/11/13 05:59 AM, Alex Deucher wrote:
>>>> On Fri, Nov 22, 2013 at 3:25 PM, Connor Behan
>>>> <connor.behan at gmail.com> wrote:
>>>>> The EXA Composite hooks should not overwrite any register values
>>>>> expected by DRI. Initial testing of the EXA support revealed that
>>>>> R128_WINDOW_XY_OFFSET is one register where we have to be careful.
>>>>> However, it was mostly tested using glxgears which does not stress the
>>>>> driver very much. Going through the various 3D screensavers one by one
>>>>> reveals a bug where certain models turn green if compositing is enabled.
>>>>>
>>>>> It seems that if we slightly alter the values passed to R128_SETUP_CNTL
>>>>> and R128_PM4_VC_FPU_SETUP, the 3D driver will be happy and compositing
>>>>> will still work. The proper way would be to constantly save and restore
>>>>> register values but this showed poor performance when dragging 3D
>>>>> windows across the screen.
>>>> You could always just reset the value after each draw command as part
>>>> of the command stream.  That said, if the options don't affect the
>>>> drawing for EXA, it should be fine.
>>>>
>>> Before I go back to experimenting with this, do you have a guess about
>>> whether this would degrade performance? With an xfce desktop there are
>>> roughly 10 composite operations per second. Counting the arguments to
>>> BEGIN_RING, it seems that each one currently requires 70 register
>>> writes. At first I thought there was worse performance when saving and
>>> restoring registers because of the extra 12 writes and 6 reads that
>>> would be needed.
>> I don't think you can read registers via the CCE.  Command processors
>> are generally write only.  For my suggestion, you only need two
>> additional dword writes.  Just write R128_SETUP_CNTL and
>> R128_PM4_VC_FPU_SETUP after you send the draw packet (basically at the
>> end of R128CCEComposite()).  Ideally you'd just reset the values of
>> those registers once at the end of the indirect buffer when you submit
>> it to the kernel.  It seems like maybe the 3D driver isn't programming
>> these registers at all.  In general the ddx and mesa shouldn't depend
>> on each other's state at all.
> Thanks for the explanation. From what I can tell, mesa programs the
> registers once when you open a 3D window, but after that it assumes that
> nothing else will be changing the state. Reads appear to work for at
> least one of them. Specifically, I use the value of
> INREG(R128_WINDOW_XY_OFFSET) to shift the co-ordinates... otherwise all
> pixmaps will be drawn in the wrong place.

Mesa (and the ddx) should reprogram all the accel registers when they
get the dri lock.  Otherwise you have no idea what state the other
driver left it in.  INREG() doesn't not read the register via the
ring, it reads the register via MMIO.  Depending on the register, this
may cause a pipeline stall or you may get a stale value if you have
additional commands in the ring that modify that value that haven't
executed yet.  If you are using the CCE, you generally don't want to
mix in MMIO.

Alex

>>
>>> Are old cards like the r128 slow at writing (with CCE) and reading
>>> registers? Or is this mostly limited by the bus speed? And does it
>>> depend on which register?
>> It depends on the register.  Some registers cause a pipeline stall.  I
>> expect these two registers probably do, so you want to minimize the
>> times you set them.  I don't think the CCE supports reads at all.  If
>> render accel works fine with your patch below, that's probably the
>> best bet.
>>
>> Alex
>>
>>>>> Signed-off-by: Connor Behan <connor.behan at gmail.com>
>>>>> ---
>>>>>  src/r128_exa_render.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/src/r128_exa_render.c b/src/r128_exa_render.c
>>>>> index f00536e..cb9f929 100644
>>>>> --- a/src/r128_exa_render.c
>>>>> +++ b/src/r128_exa_render.c
>>>>> @@ -350,7 +350,7 @@ do {                                                        \
>>>>>                     R128_TEX_MAP_ALPHA_IN_TEXTURE |     \
>>>>>                     R128_TEX_CACHE_LINE_SIZE_4QW);      \
>>>>>      OUT_RING_REG(R128_SETUP_CNTL,                      \
>>>>> -                   R128_COLOR_SOLID_COLOR |            \
>>>>> +                   R128_COLOR_GOURAUD |                \
>>>>>                     R128_PRIM_TYPE_TRI |                \
>>>>>                     R128_TEXTURE_ST_MULT_W |            \
>>>>>                     R128_STARTING_VERTEX_1 |            \
>>>>> @@ -358,9 +358,9 @@ do {                                                        \
>>>>>                     R128_SUB_PIX_4BITS);                \
>>>>>      OUT_RING_REG(R128_PM4_VC_FPU_SETUP,                        \
>>>>>                     R128_FRONT_DIR_CCW |                \
>>>>> -                   R128_BACKFACE_CULL |                \
>>>>> +                   R128_BACKFACE_SOLID |               \
>>>>>                     R128_FRONTFACE_SOLID |              \
>>>>> -                   R128_FPU_COLOR_SOLID |              \
>>>>> +                   R128_FPU_COLOR_GOURAUD |            \
>>>>>                     R128_FPU_SUB_PIX_4BITS |            \
>>>>>                     R128_FPU_MODE_3D |                  \
>>>>>                     R128_TRAP_BITS_DISABLE |            \
>>>>> --
>>>>> 1.8.4
>>>>>
>>>>> _______________________________________________
>>>>> xorg-driver-ati mailing list
>>>>> xorg-driver-ati at lists.x.org
>>>>> http://lists.x.org/mailman/listinfo/xorg-driver-ati
>
>