[Intel-gfx] intel-gpu-tools patches for read/write MMIO

Tue Jan 29 21:01:09 CET 2013

Can you just post them externally to intel-gfx at lists.freedesktop.org?
It's best to use git send-email to do it, that way the changelogs are
preserved and posted to the ml along with the patches.

Not sure if there's a bunch of duplication between the two, but you
could split them up a bit.

I still don't like the idea of silently adding the display offset on
vlv; these are just debug tools and the developer should get the
absolute offset they asked for no matter what.

Jesse

On Tue, 29 Jan 2013 00:16:51 -0800
"Cheah, Vincent Beng Keat" <vincent.beng.keat.cheah at intel.com> wrote:

> Hi 
> 
> Attached refers to two different patches that I have made for Benjamin Windawsky’s branch (bwidawsk_branch.patch) and intel-gpu-tools (master branch - intel-gpu-tools_master.patch). Alternative link: (\\pglvm2008-v03.png.intel.com\automation\binary\Linux\Automation\patches )
> 
> patches: 
> 	•	intel-gpu-tools-1.3_master.patch 
> 		o	To be applied on latest intel-gpu-tools-1.3 (git clone git://anongit.freedesktop.org/xorg/app/intel-gpu-tools ) 
> 		o	The patches added are VLV chipset support + correcting intel_read_reg.c, intel_reg_wirte.c and intel_gtt.c
> 		o	Web link: http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/
> 	•	bwidawsk_branch.patch
> 		o	To be applied on Benjamin Windawsky’s branch (git clone git://people.freedesktop.org/~bwidawsk/intel-gpu-tools -b dump_util
> 		o	The patches added are VLV chipset support + correcting intel_read_reg.c, intel_reg_wirte.c and intel_gtt.c + merge in change(s) from intel-gpu-tools-1.3
> 		o	Web link: http://cgit.freedesktop.org/~bwidawsk/intel-gpu-tools/?h=dump_util
> 
> Could somebody you please help to upstream this? 
> 
> Thanks.
> 
> Best regards, 
> Vincent 
> 
> 
> -----Original Message-----
> From: Ben Widawsky [mailto:benjamin.widawsky at intel.com] 
> Sent: Tuesday, January 15, 2013 2:55 PM
> To: Teres Alexis, Alan Previn
> Cc: Barnes, Jesse; Cheah, Vincent Beng Keat; Vetter, Daniel
> Subject: Re: intel-gpu-tools patches for read/write MMIO
> 
> On Mon, Jan 14, 2013 at 10:42 PM, Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com> wrote:
> > Ben, point us to that infrastructure ur working on - and since ur currently maintaining the intel-gpu-tools, let us know if that framework is still being worked on for VLV support or if someone else is working on adding VLV support in some form into the intel-gpu-tools.
> > Vincent is already starting to work on adding IS_DISPLAY_REG for VLV. Don’t want any overlap - let us know if so.
> >
> 
> I am too lazy to find the mailing list post, but here it is:
> http://cgit.freedesktop.org/~bwidawsk/intel-gpu-tools/log/?h=dump_util
> 
> I made some changed during PO which I probably never pushed. I'd have to look. IMO, this is the way to go though. (see vlv_display.txt)
> 
> > On the intel_reg_read/write should only do what the user asks - I agree with that. But if that function is being re-used by other internal tests like "dump display regs" or something, then an internal function could pass in that value - i.e. the option to explicitly say if its display or not should still be there.
> 
> We don't have the kind of capability you're referring to there. It would be nice to have, but not there yet. Anyway, I agree with you.
> 
> > Also, the option to have a text file define the range sounds excellent 
> > - but should stop the one-off cmd line drive reg read / write - which 
> > I am sure is not being removed by anyone in any branch for any reason 
> > :P
> 
> Yeah, I think Daniel gave up arguing against it, I forget if I was supposed to resubmit the patch. It came up at our London meeting.
> Anyone remember?
> 
> >
> > ...alan
> >
> >
> > -----Original Message-----
> > From: Ben Widawsky [mailto:benjamin.widawsky at intel.com]
> > Sent: Tuesday, January 15, 2013 12:49 PM
> > To: Teres Alexis, Alan Previn
> > Cc: Barnes, Jesse; Cheah, Vincent Beng Keat; Vetter, Daniel
> > Subject: Re: intel-gpu-tools patches for read/write MMIO
> >
> > This is what that infrastructure I worked on was meant to do (where a text file defines the registers you want to read), you know, the one Daniel more or less nak'd ;-) ... intel_reg_read/write shouldn't ever do anything except what the user asked. Personally, I think the dump range never belonged in read/write, but that predated me.
> > intel_reg_dumper is a bit of another story though, see first sentenc.
> >
> > There is no need to work with Daniel directly if you don't want.
> > Simply submit them to the intel-gfx mailing lists. If we have patches that cannot be me public yet, we have an internal list for that which we can point you to (and I am currently maintaining that intel-gpu-tools repository).
> >
> > Anyway, I wasn't directly addressed, so I'll butt out having left my 
> > $.02 :-)
> >
> > On Mon, Jan 14, 2013 at 6:19 PM, Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com> wrote:
> >> Hey Jesse and Daniel,
> >> Looks like our team mate didn't add VLV support into the whole intel_gpu_tools suite, he only added VLV support intel_reg_read and intel_reg_write - where the 0x180000 was hard coded for manual user register reads and register writes.
> >> The other tests would pass or fail depending. For example, intel_reg_dumper.c might fail (in most cases), because its mostly display regs and needs the 0x18000 but intel_gem_blahblah tests would pass because I belive most of them don't touch display regs.
> >> But any tests that want to verify GTT might fail because the gtt mapping was not modded to support VLV.
> >>
> >> Jesse, Daniel,  do u have someone on OTC enabling full support of VLV for intel-gpu-tools??? If not, then  then Vincent has volunteered to enable this and upstream thru Daniel - I will help him add explicit support on test-case by test-case basis as I summarized above.
> >>
> >> For generic reading / writing regs, I would propose an additional param (that is defaulted to zero) that means "is_display_reg" so the user could explicitly request to read or write a register and tell the tool that it IS_DISPLAY or  IS_NOT_DISPLAY. And in other cases, this tool will decide based on the same IS_DISPLAY macro in the kernel driver. (the optional override is important since we have overlapping IRQ and some other registers that have the same offset for both render and display and those cases require explicit mention).
> >>
> >> ...alan
> >>
> >>
> >> -----Original Message-----
> >> From: Teres Alexis, Alan Previn
> >> Sent: Tuesday, January 15, 2013 7:13 AM
> >> To: Barnes, Jesse; Cheah, Vincent Beng Keat
> >> Cc: Vetter, Daniel; Widawsky, Benjamin
> >> Subject: RE: intel-gpu-tools patches for read/write MMIO
> >>
> >> Vincent - lets review this offline - if intel-gpu-tools holds register names and addresses, then we can add that driver IS_VLV_DISPLAY_REG macro into that tool (which handles the optional need to add - or not to add - the 0x180000 offset).
> >> Else we should remove it and just ensure the MMIO BAR ranges can cover the larger range.
> >> ...alan
> >>
> >> -----Original Message-----
> >> From: Barnes, Jesse
> >> Sent: Monday, January 14, 2013 11:38 PM
> >> To: Cheah, Vincent Beng Keat
> >> Cc: Vetter, Daniel; Teres Alexis, Alan Previn; Widawsky, Benjamin
> >> Subject: Re: intel-gpu-tools patches for read/write MMIO
> >>
> >> On Mon, 14 Jan 2013 00:57:15 -0800
> >> "Cheah, Vincent Beng Keat" <vincent.beng.keat.cheah at intel.com> wrote:
> >>
> >>> Hi Daniel.
> >>> Attached refers to the patches  that I have done on intel-gpu-tools-1.3 to read and write MMIO register for VLV platform specific.
> >>>
> >>> Could help me to make this  upstream.
> >>
> >> I don't think this is quite right.  Not all of the regs are above 0x180000, just the display ones.
> >>
> >> Also, I think we should drop the comments about "PO boards" and just call them VLV_D, VLV_M, and VLV_T to match the SKUs we have.
> >>
> >> I don't think we need to add the offset to _read & _write either; those are just bare tools and users can just add the offset themselves.
> >>
> >> But yes, we do have permission to publish this stuff, so you can publish an updated patch to the mailing list.
> >>
> >> Thanks,
> >> Jesse