RE: [PATCH v4 06/15] mei: pxp: support matching with a gfx discrete card

["Winkler, Tomas" <tomas.winkler@intel.com>]

> card
> 
> On Fri, Sep 09, 2022 at 09:21:30AM +0000, Winkler, Tomas wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Sent: Friday, September 09, 2022 09:16
> > > > > To: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com>
> > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > dri-devel@lists.freedesktop.org; Winkler, Tomas
> > > > > <tomas.winkler@intel.com>; Lubart, Vitaly
> > > > > <vitaly.lubart@intel.com>; Teres Alexis, Alan Previn
> > > > > <alan.previn.teres.alexis@intel.com>
> > > > > Subject: Re: [PATCH v4 06/15] mei: pxp: support matching with a
> > > > > gfx discrete card
> > > > >
> > > > > On Thu, Sep 08, 2022 at 05:16:03PM -0700, Daniele Ceraolo Spurio
> wrote:
> > > > > > From: Tomas Winkler <tomas.winkler@intel.com>
> > > > > >
> > > > > > With on-boards graphics card, both i915 and MEI are in the
> > > > > > same device hierarchy with the same parent, while for discrete
> > > > > > gfx card the MEI is its child device.
> > > > > > Adjust the match function for that scenario by matching MEI
> > > > > > parent device with i915.
> > > > > >
> > > > > > V2:
> > > > > >  1. More detailed commit message  2. Check for dev is not null
> > > > > > before it is accessed.
> > > > > >
> > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > > > Signed-off-by: Daniele Ceraolo Spurio
> > > > > > <daniele.ceraolospurio@intel.com>
> > > > > > Cc: Vitaly Lubart <vitaly.lubart@intel.com>
> > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > > > > > ---
> > > > > >  drivers/misc/mei/pxp/mei_pxp.c | 13 ++++++++++---
> > > > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/misc/mei/pxp/mei_pxp.c
> > > > > > b/drivers/misc/mei/pxp/mei_pxp.c index
> > > > > > 17c5d201603f..afc047627800
> > > > > > 100644
> > > > > > --- a/drivers/misc/mei/pxp/mei_pxp.c
> > > > > > +++ b/drivers/misc/mei/pxp/mei_pxp.c
> > > > > > @@ -159,17 +159,24 @@ static int
> > > > > > mei_pxp_component_match(struct
> > > > > device
> > > > > > *dev, int subcomponent,  {
> > > > > >  	struct device *base = data;
> > > > > >
> > > > > > +	if (!dev)
> > > > > > +		return 0;
> > > > >
> > > > > How can that happen?
> > > > >
> > > > > > +
> > > > > >  	if (!dev->driver || strcmp(dev->driver->name, "i915") ||
> > > > >
> > > > > That's crazy to assume, but whatever :(
> > > > Explained here:
> > > > https://lore.kernel.org/all/20220418175932.1809770-2-
> > > wonchung@google.c
> > > > om/
> > >
> > > Still crazy :(
> > >
> > > >
> > > > >
> > > > > >  	    subcomponent != I915_COMPONENT_PXP)
> > > > > >  		return 0;
> > > > > >
> > > > > >  	base = base->parent;
> > > > > > -	if (!base)
> > > > > > +	if (!base) /* mei device */
> > > > >
> > > > > Why does this mean that?
> > > > >
> > > > > Where is that documented?
> > > > >
> > > > > >  		return 0;
> > > > > >
> > > > > > -	base = base->parent;
> > > > > > -	dev = dev->parent;
> > > > > > +	base = base->parent; /* pci device */
> > > > >
> > > > > Again, why is this the case?
> > > > >
> > > > > > +	/* for dgfx */
> > > > > > +	if (base && dev == base)
> > > > > > +		return 1;
> > > > > >
> > > > > > +	/* for pch */
> > > > > > +	dev = dev->parent;
> > > > >
> > > > > You are digging through a random device tree and assuming that
> > > > > you
> > > "know"
> > > > > what the topology is going to be, that feels very very fragile
> > > > > and ripe for problems going forward.
> > > >
> > > > I don't think it is random.
> > >
> > > Today it is one specific way, but how do you know this always will
> > > be this way?
> > >
> > > > > How do you ensure that this really is they way the tree is for
> > > > > ALL
> > > systems?
> > > >
> > > > Yes we take the topology assumption in PCI hierarchy.
> > > > There is a case where both GFX and MEI are in PCH and you cannot
> > > > stick
> > > additional PCI extender or anything else there.
> > > > And case where MEI is child on a standalone graphics card this is
> > > > set in software so topology is not going to change unless we
> > > > rewrite
> > > everything.  Be happy to hear your insights.
> > >
> > > This is ripe to break in the future if someone makes a differently
> > > structured device as there is nothing forcing the current layout to
> > > always be this way by hardware designers.
> > >
> > > The goal of the driver model is to NOT have these types of
> > > hard-coded topology assumptions because, supprise, assumptions like
> > > this have always come back to bite people in the end.
> > >
> > > This is your driver, so that's fine, but really this feels very very
> > > wrong and you will have a hard time guaranteeing that this will
> > > always be this way for the next 20+ years of hardware designs.  So
> > > why not do it properly from the beginning and pass in the correct
> pointers to different places?
> > >
> > > There is a very good reason that the driver model/core does not make
> > > it easy to determine what type of device a 'struct device *' is,
> > > because you shouldn't have to rely on that type of thing ever.  You
> > > are taking it one step further and just assuming that you know what
> > > the type is here, with no real way to ensure that this is the case.
> > >
> > > In short, this feels like a very bad design as it is very fragile.
> >
> > I believe I understand your concern but I would need to invent another
> > addressing scheme to associate hw components that are already
> > addressable by let say PCI hierarchy.  We've changed two subsystems
> > for this work components and aux bus already.  So let's have some fun in
> the future.
> 
> Why are you trying to reach across subsystems in the first place?  Why is that
> needed at all and why doesn't MEI just provide a generic way to do this for
> any bus type, it shouldn't require any specific topology from what I can
> determine.
> 
> What am I missing here?

MEI on PCH is hardware-wise  associated with the on-board GFX.  And MEI on GFX standalone card is  hardware-wise  associated with that GFX card instance.
We need to couple MEI with correct GFX instance. So it is either sibling on PCH case or a parent device in standalone case.

Thanks
Tomas



> 
> thanks,
> 
> greg k-h