Re: [igt-dev] [PATCH i-g-t 3/3] lib/intel_mmio: Remove global igt_global_mmio

[Katarzyna Dec <katarzyna.dec@intel.com>]

On Thu, Apr 04, 2019 at 04:27:01PM +0100, Mrzyglod, Daniel T wrote:
> On Thu, 2019-04-04 at 16:47 +0200, Katarzyna Dec wrote:
> > On Thu, Apr 04, 2019 at 03:18:05PM +0200, Daniel Mrzyglod wrote:
> > > In future tests there will be multiple PCI devices run at once.
> > > It's good for them to have different mmio space.
> > > 
> > > Signed-off-by: Daniel Mrzyglod <daniel.t.mrzyglod@intel.com>
> > > ---
> <CUT>
> > > diff --git a/lib/intel_iosf.c b/lib/intel_iosf.c
> > > index 3b5a1370..52a885f5 100644
> > > --- a/lib/intel_iosf.c
> > > +++ b/lib/intel_iosf.c
> > > @@ -19,8 +19,8 @@
> > >  /* Private register write, double-word addressing, non-posted */
> > >  #define SB_CRWRDA_NP   0x07
> > >  
> > > -static int vlv_sideband_rw(uint32_t port, uint8_t opcode, uint32_t
> > > addr,
> > > -			   uint32_t *val)
> > > +static int vlv_sideband_rw(struct mmio_data *mmio_data, uint32_t
> > > port,
> > > +			   uint8_t opcode, uint32_t addr, uint32_t
> > > *val)
> > As I undestand VLV is quite an old gen, do we need to add mmio_data
> > here? Or not
> > adding it breaks some compatibility?
> > 
> > nit: patch is almost 3000 lines of different changes, please divide
> > it in
> > smaller chunks. It is hard to read so large changes and yet we need
> > to
> > understand what is going on :/
> > Kasia
> 
> The problem is that igt_global_mmio is using by VLV also and almost all
> functions in intel_io.h
> 
> Design that was chosen was perfect for single device for now there are
> many challenges for us and that there will be in many future scenarios:
> - For example we will run competition card and ours - run some 	scenari
> os
> - Run our integrated and our future PCI card
> - Put few different models PCI cards to limit CI cost.
> - other more real scenarios
> 
> When I moved igt_global_mmio there was a need to update everything
> that  was using that global pointer. 
> 
> There were few Tools that required more changes that were using
> INREG/OUTREG directly.... i think intel_audio.c was a leader in diff :>
> 
> It's used by tools/tests/benchmarks others... but if i divide this
> patch it will probably not compile... so it will not meet acceptance
> criteria.
> 
> There is a need for v2 so i will try to adress those problems. 
> 
> Daniel

Maybe you can have 1 patch that is adding new functionality, few another with
appling the changes and last one that is removing old one? With this approach
everything should compile. And will be more review friendly.
Change is quite big and it would be nice to have a decent review to avoid
implementing bugs.
Kasia
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev