Bug 8283 - i915 can't swizzle TEX arguments
Summary: i915 can't swizzle TEX arguments
Status: VERIFIED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i915 (show other bugs)
Version: git
Hardware: x86 (IA32) Linux (All)
: medium normal
Assignee: haihao
QA Contact:
URL:
Whiteboard:
Keywords:
: 10786 14122 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-09-14 17:55 UTC by Frank Richter
Modified: 2009-08-24 12:24 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add swizzle support for TEX FP op. (5.48 KB, patch)
2006-09-14 17:55 UTC, Frank Richter
Details | Splinter Review
Add swizzle support for TEX FP op. (try 2) (8.16 KB, patch)
2006-09-15 11:56 UTC, Frank Richter
Details | Splinter Review
TEXLD swizzle nr 3 (7.07 KB, patch)
2006-09-20 03:42 UTC, Keith Whitwell
Details | Splinter Review

Description Frank Richter 2006-09-14 17:55:06 UTC
i915 doesn't support swizzling TEX arguments. The attached patch solves that by
first creating a table of what R regs are "taken" for each instruction. Then,
when a TEX with swizzle arg is encountered, this information is used to pick a
free R reg, which will take the swizzled argument, so TEX can be invoked w/o
swizzling.
Comment 1 Frank Richter 2006-09-14 17:55:44 UTC
Created attachment 6989 [details] [review]
Add swizzle support for TEX FP op.
Comment 2 Keith Whitwell 2006-09-15 00:33:40 UTC
Firstly, the approach looks like a good one, but a couple of comments:  

    - Please use universal diffs (diff -u) as they're easier for others to read...

    - It looks like you don't seem to cope with the situation where
get_free_rreg() fails, ie. the return value isn't checked.

    - There should already be a 'NumInstructions' field hanging around, so you
shouldn't need to scan the list twice.

    - It would be good to move the calculation of temporaries used into core
mesa somewhere, perhaps taking the regsNeeded array as an argument.  But I don't
think this is a big deal at this point.

Otherwise, it looks good - nice work.
Comment 3 Keith Whitwell 2006-09-15 05:39:40 UTC
Actually I think the calculation of neededRegs is incorrect, consider:

MOV B, 99
..
MOV C, B
TEX X, Y.zyx
MOV B, B
MOV A, B

Working backwards, I think that after MOV B, B the fact that B is still live
will be lost, so regsNeeded at the TEX instruction will be incorrect and B could
be used as a temporary.

Comment 4 Frank Richter 2006-09-15 11:56:09 UTC
Created attachment 6993 [details] [review]
Add swizzle support for TEX FP op. (try 2)

Incorporated feedback from comments. The case in comment #3 should now be
handled properly, I think.
Comment 5 Keith Whitwell 2006-09-20 03:42:30 UTC
Created attachment 7095 [details] [review]
TEXLD swizzle nr 3
Comment 6 Keith Whitwell 2006-09-20 03:43:37 UTC
Does this version work for you?  I've slightly cleaned things up:
  - move calculate into its own function
  - use the existing mesa call for the number of argments of an instruction
  - just pass the bitmask to emit_texld so that code doesn't learn too much
about mesa's fragment program representation.

Keith
Comment 7 Gordon Jin 2007-03-14 19:32:34 UTC
The bug priority was upgraded (P2->high) with the bugzilla configuration change.
I'm Changing the priority back to the normal one.
Sorry for the spam.
Comment 8 Michael Fu 2007-10-28 05:32:04 UTC
any response, Frank?
Comment 9 Michael Fu 2007-12-02 19:28:15 UTC
*** Bug 10786 has been marked as a duplicate of this bug. ***
Comment 10 haihao 2008-01-06 22:05:35 UTC
The latest (In reply to comment #5)
> Created an attachment (id=7095) [details]
> TEXLD swizzle nr 3
> 
This patch works fine for me and I check it into git (3369cd9a6f943365242d7832e69788d4aede9a8f)
Comment 11 Michael Fu 2008-01-18 05:44:42 UTC
*** Bug 14122 has been marked as a duplicate of this bug. ***
Comment 12 WuNian 2008-06-10 02:27:03 UTC
Haihao, the fix is not at mesa_7_0_branch. Please pick it up. Thanks!
Comment 13 haihao 2008-06-10 20:34:01 UTC
(In reply to comment #12)
> Haihao, the fix is not at mesa_7_0_branch. Please pick it up. Thanks!
> 
Done
Comment 14 Jeremie 2008-07-01 07:06:38 UTC
Still doesn't work for me (Using the 7.0.3 mesa driver version bundled with OpenSuSE 11.0)

Also I can't seem to be able to use your "patches" (although I'm about 98% sure I'm doing something wrong - file saved as "patch.cgi", chmoded to 764) Here is the error I'm getting:

./patch.cgi: line 1: ?: command not found
./patch.cgi: line 2: ?: command not found
./patch.cgi: line 3: ?: command not found
./patch.cgi: line 4: ?: command not found
./patch.cgi: line 5: Index:: command not found
./patch.cgi: line 6: ===================================================================: command not found
./patch.cgi: line 7: RCS: command not found
./patch.cgi: line 8: retrieving: command not found
diff: invalid option -- '.'
diff: Try `diff --help' for more information.
./patch.cgi: line 10: ---: command not found
./patch.cgi: line 11: +++: command not found
./patch.cgi: line 12: @@: command not found
./patch.cgi: line 16: +#include: command not found
./patch.cgi: line 20: @@: command not found
./patch.cgi: line 24: syntax error near unexpected token `('
./patch.cgi: line 24: `+#define I915_MAX_INSN          (I915_MAX_TEX_INSN+I915_MAX_ALU_INSN)'

Could someone tell me what I'm doing wrong / suggest another fix / do I have have to file a bug report for the patch (don't think so, though: seems to be my fault...)
Comment 15 haihao 2008-07-01 19:06:47 UTC
This patch has been pushed into mesa_7_0_branch. 
Comment 16 Adam Jackson 2009-08-24 12:24:28 UTC
Mass version move, cvs -> git


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.