Re: [Intel-gfx] [PATCH v8 1/3] gpu: drm: separate panel orientation property creating and value setting

[Emil Velikov]

On Tue, 15 Feb 2022 at 16:37, Simon Ser  wrote:
>
> On Tuesday, February 15th, 2022 at 15:38, Emil Velikov 
>  wrote:
>
> > On Tue, 15 Feb 2022 at 13:55, Simon Ser  wrote:
> > >
> > > On Tuesday, February 15th, 2022 at 13:04, Emil Velikov 
> > >  wrote:
> > >
> > > > Greetings everyone,
> > > >
> > > > Padron for joining in so late o/
> > > >
> > > > On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang  wrote:
> > > > >
> > > > > drm_dev_register() sets connector->registration_state to
> > > > > DRM_CONNECTOR_REGISTERED and dev->registered to true. If
> > > > > drm_connector_set_panel_orientation() is first called after
> > > > > drm_dev_register(), it will fail several checks and results in 
> > > > > following
> > > > > warning.
> > > > >
> > > > > Add a function to create panel orientation property and set default 
> > > > > value
> > > > > to UNKNOWN, so drivers can call this function to init the property 
> > > > > earlier
> > > > > , and let the panel set the real value later.
> > > > >
> > > >
> > > > The warning illustrates a genuine race condition, where userspace will
> > > > read the old/invalid property value/state. So this patch masks away
> > > > the WARNING without addressing the actual issue.
> > > > Instead can we fix the respective drivers, so that no properties are
> > > > created after drm_dev_register()?
> > > >
> > > > Longer version:
> > > > As we look into drm_dev_register() it's in charge of creating the
> > > > dev/sysfs nodes (et al). Note that connectors cannot disappear at
> > > > runtime.
> > > > For panel orientation, we are creating an immutable connector
> > > > properly, meaning that as soon as drm_dev_register() is called we must
> > > > ensure that the property is available (if applicable) and set to the
> > > > correct value.
> > >
> > > Unfortunately we can't quite do this. To apply the panel orientation 
> > > quirks we
> > > need to grab the EDID of the eDP connector, and this happened too late in 
> > > my
> > > testing.
> > >
> > > What we can do is create the prop early during module load, and update it 
> > > when
> > > we read the EDID (at the place where we create it right now). User-space 
> > > will
> > > receive a hotplug event after the EDID is read, so will be able to pick 
> > > up the
> > > new value if any.
> >
> > Didn't quite get that, are you saying that a GETPROPERTY for the EDID,
> > the ioctl blocks or that we get an empty EDID?
>
> I'm not referring to GETPROPERTY, I'm referring to the driver getting the EDID
> from the sink (here, the eDP panel). In my experimentations with amdgpu I
> noticed that the driver module load finished before the EDID was available to
> the driver. Maybe other drivers behave differently and probe connectors when
> loaded, not sure.
>
I see thanks.

> > The EDID hotplug even thing is neat - sounds like it also signals on
> > panel orientation, correct?
> > On such an event, which properties userspace should be re-fetching -
> > everything or guess randomly?
> >
> > Looking through the documentation, I cannot see a clear answer :-\
>
> User-space should re-fetch *all* properties. In practice some user-space may
> only be fetching some properties, but that should get fixed in user-space.
>
> Also the kernel can indicate that only a single connector changed via the
> "CONNECTOR" uevent prop, or even a single connector property via "PROPERTY".
> See [1] for a user-space implementation. But all of this is purely an optional
> optimization. Re-fetching all properties is a bit slower (especially if some
> drmModeGetConnector calls force-probe connectors) but works perfectly fine.
>
Looking at KDE/kwin (the one I'm running) - doesn't seem like it
handles any of the three "HOTPLUG", "PROPERTY" or "CONNECTOR" uevents
:-\
Skimming through GNOME/mutter - it handles "HOTPLUG", but not the optional ones.

Guess we're in the clear wrt potential races, even though the
documentation on the topic is lacklustre.

> It would be nice to document, if you have the time feel free to send a patch
> and CC danvet, pq and me.
>

Doubt I will have the time in the upcoming weeks, but I'll add it to
my ever-growing TODO list :-P

Thanks
Emil

Re: [Intel-gfx] [PATCH v8 1/3] gpu: drm: separate panel orientation property creating and value setting

[Emil Velikov]

On Tue, 15 Feb 2022 at 13:55, Simon Ser  wrote:
>
> On Tuesday, February 15th, 2022 at 13:04, Emil Velikov 
>  wrote:
>
> > Greetings everyone,
> >
> > Padron for joining in so late o/
> >
> > On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang  wrote:
> > >
> > > drm_dev_register() sets connector->registration_state to
> > > DRM_CONNECTOR_REGISTERED and dev->registered to true. If
> > > drm_connector_set_panel_orientation() is first called after
> > > drm_dev_register(), it will fail several checks and results in following
> > > warning.
> > >
> > > Add a function to create panel orientation property and set default value
> > > to UNKNOWN, so drivers can call this function to init the property earlier
> > > , and let the panel set the real value later.
> > >
> >
> > The warning illustrates a genuine race condition, where userspace will
> > read the old/invalid property value/state. So this patch masks away
> > the WARNING without addressing the actual issue.
> > Instead can we fix the respective drivers, so that no properties are
> > created after drm_dev_register()?
> >
> > Longer version:
> > As we look into drm_dev_register() it's in charge of creating the
> > dev/sysfs nodes (et al). Note that connectors cannot disappear at
> > runtime.
> > For panel orientation, we are creating an immutable connector
> > properly, meaning that as soon as drm_dev_register() is called we must
> > ensure that the property is available (if applicable) and set to the
> > correct value.
>
> Unfortunately we can't quite do this. To apply the panel orientation quirks we
> need to grab the EDID of the eDP connector, and this happened too late in my
> testing.
>
> What we can do is create the prop early during module load, and update it when
> we read the EDID (at the place where we create it right now). User-space will
> receive a hotplug event after the EDID is read, so will be able to pick up the
> new value if any.

Didn't quite get that, are you saying that a GETPROPERTY for the EDID,
the ioctl blocks or that we get an empty EDID?

The EDID hotplug even thing is neat - sounds like it also signals on
panel orientation, correct?
On such an event, which properties userspace should be re-fetching -
everything or guess randomly?

Looking through the documentation, I cannot see a clear answer :-\

Thanks
Emil

Re: [Intel-gfx] [PATCH v8 1/3] gpu: drm: separate panel orientation property creating and value setting

[Emil Velikov]

Greetings everyone,

Padron for joining in so late o/

On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang  wrote:
>
> drm_dev_register() sets connector->registration_state to
> DRM_CONNECTOR_REGISTERED and dev->registered to true. If
> drm_connector_set_panel_orientation() is first called after
> drm_dev_register(), it will fail several checks and results in following
> warning.
>
> Add a function to create panel orientation property and set default value
> to UNKNOWN, so drivers can call this function to init the property earlier
> , and let the panel set the real value later.
>

The warning illustrates a genuine race condition, where userspace will
read the old/invalid property value/state. So this patch masks away
the WARNING without addressing the actual issue.
Instead can we fix the respective drivers, so that no properties are
created after drm_dev_register()?

Longer version:
As we look into drm_dev_register() it's in charge of creating the
dev/sysfs nodes (et al). Note that connectors cannot disappear at
runtime.
For panel orientation, we are creating an immutable connector
properly, meaning that as soon as drm_dev_register() is called we must
ensure that the property is available (if applicable) and set to the
correct value.

For illustration, consider the following scenario:
 - DRM modules are loaded late - are not built-in and not part of
initrd (or there's no initrd)
 - kernel boots
 - plymouth/similar user-space component kicks in before the
driver/module is loaded
 - module gets loaded, drm_dev_register() kicks in populating /dev/dri/card0
 - plymouth opens the dev node and reads DRM_MODE_PANEL_ORIENTATION_UNKNOWN
 - module updates the orientation property

Thanks
Emil

Re: [PATCH v4 5/7] i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt

[Emil Velikov]

Hi Jim,

On Sat, 31 Jul 2021 at 22:42, Jim Cromie  wrote:

> DYNDBG_BITMAP_DESC(__gvt_debug, "dyndbg bitmap desc",
> { "gvt: cmd: ",  "command processing" },
> { "gvt: core: ", "core help" },
> { "gvt: dpy: ",  "display help" },
> { "gvt: el: ",   "help" },
> { "gvt: irq: ",  "help" },
> { "gvt: mm: ",   "help" },
> { "gvt: mmio: ", "help" },
> { "gvt: render: ", "help" },
> { "gvt: sched: " "help" });
>
Previous commit removed the space after the colon. The above example
needs updating.

This concludes a casual read-through on my end. Hope it helps o/
-Emil

Re: [Intel-gfx] [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks

[Emil Velikov]

Hi Jim,

On Sat, 31 Jul 2021 at 22:42, Jim Cromie  wrote:

> +struct dyndbg_bitdesc {
> +   /* bitpos is inferred from index in containing array */
> +   char *prefix;
> +   char *help;
AFAICT these two should also be constant, right?


> +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> +{
> +   unsigned int val;
> +   unsigned long changes, result;
> +   int rc, chgct = 0, totct = 0, bitpos, bitsmax;
> +   char query[OUR_QUERY_SIZE];
> +   struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
> +
> +   // pr_info("set_dyndbg: instr: %s curr: %d\n", instr, *kp->arg);
Left-over debug code, here and below?

-Emil

Re: [PATCH v4 2/7] moduleparam: add data member to struct kernel_param

[Emil Velikov]

Hi Jim,

On Sat, 31 Jul 2021 at 22:42, Jim Cromie  wrote:

> Use of this new data member will be rare, it might be worth redoing
> this as a separate/sub-type to keep the base case.
>
> Signed-off-by: Jim Cromie 
> ---
>  include/linux/moduleparam.h | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index eed280fae433..e9495b1e794d 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -78,6 +78,7 @@ struct kernel_param {
> const struct kparam_string *str;
> const struct kparam_array *arr;
> };
> +   void *data;

Might as well make this "const void *" since it is a compile-time constant?

-Emil

Re: [PATCH] drm/amdgpu: fix leftover drm_gem_object_put_unlocked call

[Emil Velikov]

On Fri, 22 May 2020 at 20:43, Emil Velikov  wrote:
>
> On Fri, 22 May 2020 at 20:38, Simon Ser  wrote:
> >
> > drm_gem_object_put_unlocked has been renamed to drm_gem_object_put.
> >
> > Signed-off-by: Simon Ser 
> > Fixes: e07ddb0ce7cd ("drm/amd: remove _unlocked suffix in 
> > drm_gem_object_put_unlocked")
> Wrong tag it seems. At that commit, the amdgpu code uses it's own
> wrapper - amdgpu_bo_unref()
>
Alex, team, this seems like merge clash.

With the Fixes tag dropped, the patch is:
Reviewed-by: Emil Velikov 

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix leftover drm_gem_object_put_unlocked call

[Emil Velikov]

On Fri, 22 May 2020 at 20:38, Simon Ser  wrote:
>
> drm_gem_object_put_unlocked has been renamed to drm_gem_object_put.
>
> Signed-off-by: Simon Ser 
> Fixes: e07ddb0ce7cd ("drm/amd: remove _unlocked suffix in 
> drm_gem_object_put_unlocked")
Wrong tag it seems. At that commit, the amdgpu code uses it's own
wrapper - amdgpu_bo_unref()

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 07/38] drm/amdgpu: use the unlocked drm_gem_object_put

[Emil Velikov]

From: Emil Velikov 

The driver does not hold struct_mutex, thus using the locked version of
the helper is incorrect.

Cc: Alex Deucher 
Cc: Christian König 
Cc: amd-gfx@lists.freedesktop.org
Fixes: a39414716ca0 ("drm/amdgpu: add independent DMA-buf import v9"):
Signed-off-by: Emil Velikov 
Acked-by: Sam Ravnborg 
Reviewed-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 43d8ed7dbd00..652c57a3b847 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -587,7 +587,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct 
drm_device *dev,
attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
_dma_buf_attach_ops, obj);
if (IS_ERR(attach)) {
-   drm_gem_object_put(obj);
+   drm_gem_object_put_unlocked(obj);
return ERR_CAST(attach);
}
 
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Nouveau] [RFC] Remove AGP support from Radeon/Nouveau/TTM

[Emil Velikov]

On Tue, 12 May 2020 at 20:48, Alex Deucher  wrote:

> > >>
> > >> There's some AGP support code in the DRM core. Can some of that declared
> > >> as legacy?
> > >>
> > >> Specifically, what about these AGP-related ioctl calls? Can they be
> > >> declared as legacy? It would appear to me that KMS-based drivers don't
> > >> have to manage AGP by themselves. (?)
> > >
> > > The old drm core AGP code is mainly (only?) for the non-KMS drivers.
> > > E.g., mach64, r128, sis, savage, etc.
> >
> > Exactly my point. There's one drm_agp_init() call left in radeon. The
> > rest of the AGP code is apparently for legacy non-KMS drivers. Should
> > the related ioctl calls be declared as legacy (i.e., listed with
> > DRM_LEGACY_IOCTL_DEF instead of DRM_IOCTL_DEF)? If so, much of the AGP
> > core code could probably be moved behind CONFIG_DRM_LEGACY as well.
>
> Ah, I forgot about drm_agp_init().  I was just thinking the other AGP
> stuff.  Yeah, I think we could make it legacy.
>
Fwiw I've got local patches a) removing drm_agp_init() from radeon and
b) making the core drm code legacy only.
Will try to finish them over the weekend and send out.

Even if AGP GART gets dropped the b) patches will be good ;-)

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC] Remove AGP support from Radeon/Nouveau/TTM

[Emil Velikov]

On Mon, 11 May 2020 at 21:43, Dave Airlie  wrote:
>
> On Tue, 12 May 2020 at 06:28, Alex Deucher  wrote:
> >
> > On Mon, May 11, 2020 at 4:22 PM Al Dunsmuir  
> > wrote:
> > >
> > > On Monday, May 11, 2020, 1:17:19 PM, "Christian König" wrote:
> > > > Hi guys,
> > >
> > > > Well let's face it AGP is a total headache to maintain and dead for at 
> > > > least 10+ years.
> > >
> > > > We have a lot of x86 specific stuff in the architecture independent
> > > > graphics memory management to get the caching right, abusing the DMA
> > > > API on multiple occasions, need to distinct between AGP and driver 
> > > > specific page tables etc etc...
> > >
> > > > So the idea here is to just go ahead and remove the support from
> > > > Radeon and Nouveau and then drop the necessary code from TTM.
> > >
> > > > For Radeon this means that we just switch over to the driver
> > > > specific page tables and everything should more or less continue to 
> > > > work.
> > >
> > > > For Nouveau I'm not 100% sure, but from the code it of hand looks
> > > > like we can do it similar to Radeon.
> > >
> > > > Please comment what you think about this.
> > >
> > > > Regards,
> > > > Christian.
> > >
> > > Christian,
> > >
> > > I would respectfully ask that this change be rejected.
> > >
> > > I'm  currently  an  end user on both Intel (32-bit and 64-bit) and PPC
> > > (Macs, IBM Power - BE and LE).
> > >
> > > Linux is not just used for modern hardware. There is also a subset of
> > > the user base that uses it for what is often termed retro-computing.
> > > No it's not commercial usage, but no one can seriously claim that that
> > > Linux is for business only.
> > >
> > > Often the old hardware is built far batter than the modern junk, and
> > > will continue to run for years to come. This group of folks either has
> > > existing hardware they wish to continue to use, or are acquiring the
> > > same because they are tired of generic locked-down hardware.
> > >
> > > A significant percentage of the video hardware that falls in the retro
> > > category uses the AGP video bus. Removing that support for those cases
> > > where it works would severely limit performance and in some cases
> > > functionality. This can mean the difference between being able to run
> > > an application, or having it fail.
> > >
> >
> > Note there is no loss of functionality here, at least on radeon
> > hardware.  It just comes down to which MMU gets used for access to
> > system memory, the AGP MMU on the chipset or the MMU built into the
> > GPU.  On powerpc hardware, AGP has been particularly unstable, and AGP
> > has been disabled by default on radeon on powerpc for years now.  In
> > fact, this will probably make older hardware more reliable as it takes
> > AGP out of the equation.
> >
>
> From memory there is quite a loss in speed though, like pretty severe.
>
> The radeon PCI GART has a single slot TLB, if memory serves.
>
> I think this is going to be a hard sell at this stage, I'm guessing
> users will crawl out of the woodwork, I'm sure with 2 hours after I'm
> able to access the office, I can boot the 865 AGP box with an rv350 in
> it on a modern distro.
>
I have a system with nforce2 motherboard and Nvidia fx5500 GPU. I
could dust it off and some quick performance tests over the weekend.

Unless someone beats me to it, of course :-)

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 06/36] drm/amdgpu: use the unlocked drm_gem_object_put

[Emil Velikov]

On Fri, 8 May 2020 at 09:16, Christian König  wrote:
>
> Am 07.05.20 um 20:03 schrieb Sam Ravnborg:
> > Hi Emil.
> >
> > On Thu, May 07, 2020 at 04:07:52PM +0100, Emil Velikov wrote:
> >> From: Emil Velikov 
> >>
> >> The driver does not hold struct_mutex, thus using the locked version of
> >> the helper is incorrect.
> >>
> >> Cc: Alex Deucher 
> >> Cc: Christian König 
> >> Cc: amd-gfx@lists.freedesktop.org
> >> Fixes: a39414716ca0 ("drm/amdgpu: add independent DMA-buf import v9"):
> >> Signed-off-by: Emil Velikov 
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >> index 43d8ed7dbd00..652c57a3b847 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >> @@ -587,7 +587,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct 
> >> drm_device *dev,
> >>  attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
> >>  _dma_buf_attach_ops, obj);
> >>  if (IS_ERR(attach)) {
> >> -drm_gem_object_put(obj);
> >> +drm_gem_object_put_unlocked(obj);
> >>  return ERR_CAST(attach);
> >>  }
> > Likewise previous patch.
> > Drop this as the patch is correct after the rename a few pathces later.
>
> Well this is a bug fix in the error path and should probably be back
> ported, so having a separate patch is certainly a good idea.
>
Precisely the goal here. The fixes tag should allow Greg and team to
pick/port it where applicable.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 06/36] drm/amdgpu: use the unlocked drm_gem_object_put

[Emil Velikov]

From: Emil Velikov 

The driver does not hold struct_mutex, thus using the locked version of
the helper is incorrect.

Cc: Alex Deucher 
Cc: Christian König 
Cc: amd-gfx@lists.freedesktop.org
Fixes: a39414716ca0 ("drm/amdgpu: add independent DMA-buf import v9"):
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 43d8ed7dbd00..652c57a3b847 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -587,7 +587,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct 
drm_device *dev,
attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
_dma_buf_attach_ops, obj);
if (IS_ERR(attach)) {
-   drm_gem_object_put(obj);
+   drm_gem_object_put_unlocked(obj);
return ERR_CAST(attach);
}
 
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/radeon: Inline drm_get_pci_dev

[Emil Velikov]

On Sat, 22 Feb 2020 at 17:54, Daniel Vetter  wrote:
>
> It's the last user, and more importantly, it's the last non-legacy
> user of anything in drm_pci.c.
>
> The only tricky bit is the agp initialization. But a close look shows
> that radeon does not use the drm_agp midlayer (the main use of that is
> drm_bufs for legacy drivers), and instead could use the agp subsystem
> directly (like nouveau does already). Hence we can just pull this in
> too.
>
> A further step would be to entirely drop the use of drm_device->agp,
> but feels like too much churn just for this patch.
>
> Signed-off-by: Daniel Vetter 
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "David (ChunMing) Zhou" 
> Cc: amd-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/radeon/radeon_drv.c | 43 +++--
>  drivers/gpu/drm/radeon/radeon_kms.c |  6 
>  2 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
> b/drivers/gpu/drm/radeon/radeon_drv.c
> index 49ce2e7d5f9e..59f8186a2415 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -322,6 +323,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  {
> unsigned long flags = 0;
> +   struct drm_device *dev;
> int ret;
>
> if (!ent)
> @@ -362,7 +364,44 @@ static int radeon_pci_probe(struct pci_dev *pdev,
> if (ret)
> return ret;
>
> -   return drm_get_pci_dev(pdev, ent, _driver);
> +   dev = drm_dev_alloc(_driver, >dev);
> +   if (IS_ERR(dev))
> +   return PTR_ERR(dev);
> +
> +   ret = pci_enable_device(pdev);
> +   if (ret)
> +   goto err_free;
> +
> +   dev->pdev = pdev;
> +#ifdef __alpha__
> +   dev->hose = pdev->sysdata;
> +#endif
> +
> +   pci_set_drvdata(pdev, dev);
> +
> +   if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
> +   dev->agp = drm_agp_init(dev);
> +   if (dev->agp) {
> +   dev->agp->agp_mtrr = arch_phys_wc_add(
> +   dev->agp->agp_info.aper_base,
> +   dev->agp->agp_info.aper_size *
> +   1024 * 1024);
> +   }
> +
IMHO a better solution is kill off the drm_agpsupport.c dependency all together.
As-is it's still used, making the legacy vs not line really moot.

Especially, since the AGP ioctl (in the legacy code) can manipulate
the underlying state.

Off the top of my head, in radeon_agp_init():
 - at the top agp_backend_acquire() + agp_copy_info()
 - followed up by existing mode magic
 - opencode the enable - agp_enable() + acquired = true;
 - mtrr = arch_phys_wc_add() and the rest

In radeon_agp_fini()
 - if !acquired { agp_backend_release(); acquired = false }


Something like ^^ should result in a net diffstat of around zero.
All thanks to the interesting layer that drm_agp is ;-)

With this in place we can make move drm_device::agp and
DRM_IOCTL_AGP_INFO behind CONFIG_DRM_LEGACY.

-Emil
P.S. Watch out for radeon_ttm.c warnings/errors
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

On Wed, 27 Nov 2019 at 18:37, Daniel Vetter  wrote:
>
> On Wed, Nov 27, 2019 at 06:32:56PM +, Emil Velikov wrote:
> > On Wed, 27 Nov 2019 at 18:04, Daniel Vetter  wrote:
> > >
> > > On Wed, Nov 27, 2019 at 04:27:29PM +, Emil Velikov wrote:
> > > > On Wed, 27 Nov 2019 at 07:41, Boris Brezillon
> > > >  wrote:
> > > > >
> > > > > Hi Emil,
> > > > >
> > > > > On Fri,  1 Nov 2019 13:03:13 +
> > > > > Emil Velikov  wrote:
> > > > >
> > > > > > From: Emil Velikov 
> > > > > >
> > > > > > As mentioned by Christian, for drivers which support only primary 
> > > > > > nodes
> > > > > > this changes the returned error from -EACCES into 
> > > > > > -EOPNOTSUPP/-ENOSYS.
> > > > >
> > > > > Are you sure this is true for MODESET-only nodes (those that do not
> > > > > have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()?
> > > > > Shouldn't the is_authenticated() check still be done in that case?
> > > > >
> > > > Thanks for catching this. Just sent out v2, which I should address the 
> > > > concern.
> > >
> > > Why do we need this additional check in v2? What can go wrong on modeset
> > > drivers if non-authenticated legacy things can use this? modeset-only
> > > drivers have all their resources segregated by the drm core (drm_fb,
> > > mmaps, buffer lists), so there's really no access limitations that can go
> > > wrong here.
> >
> > Welcome back Daniel.
> >
> > I haven't audited the core drm code, so wasn't sure if there's any
> > issues that may arise.
> > Hence the conservative approach in v2.
> >
> > If you think this is fine as-is a formal Reviewed-by would be highly
> > appreciated.
>
> I think there's a non-zero chance I'll have to eat a few hats on this, but
> I think v1 is solid.
>
> Reviewed-by: Daniel Vetter 
>
Thanks. I've just re-read the DIM instructions and pushed this to drm-misc-next.
Fingers crossed, I did not butcher it this time around.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

On Wed, 27 Nov 2019 at 18:04, Daniel Vetter  wrote:
>
> On Wed, Nov 27, 2019 at 04:27:29PM +, Emil Velikov wrote:
> > On Wed, 27 Nov 2019 at 07:41, Boris Brezillon
> >  wrote:
> > >
> > > Hi Emil,
> > >
> > > On Fri,  1 Nov 2019 13:03:13 +0000
> > > Emil Velikov  wrote:
> > >
> > > > From: Emil Velikov 
> > > >
> > > > As mentioned by Christian, for drivers which support only primary nodes
> > > > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
> > >
> > > Are you sure this is true for MODESET-only nodes (those that do not
> > > have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()?
> > > Shouldn't the is_authenticated() check still be done in that case?
> > >
> > Thanks for catching this. Just sent out v2, which I should address the 
> > concern.
>
> Why do we need this additional check in v2? What can go wrong on modeset
> drivers if non-authenticated legacy things can use this? modeset-only
> drivers have all their resources segregated by the drm core (drm_fb,
> mmaps, buffer lists), so there's really no access limitations that can go
> wrong here.

Welcome back Daniel.

I haven't audited the core drm code, so wasn't sure if there's any
issues that may arise.
Hence the conservative approach in v2.

If you think this is fine as-is a formal Reviewed-by would be highly
appreciated.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

On Wed, 27 Nov 2019 at 07:41, Boris Brezillon
 wrote:
>
> Hi Emil,
>
> On Fri,  1 Nov 2019 13:03:13 +
> Emil Velikov  wrote:
>
> > From: Emil Velikov 
> >
> > As mentioned by Christian, for drivers which support only primary nodes
> > this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
>
> Are you sure this is true for MODESET-only nodes (those that do not
> have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()?
> Shouldn't the is_authenticated() check still be done in that case?
>
Thanks for catching this. Just sent out v2, which I should address the concern.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

From: Emil Velikov 

Current validation requires that we're authenticated, even though we can
bypass (by design) the authentication when using a render node.

Let's address the former by following the design decision.

v2: Add simpler validation in the ioctls themselves (Boris)

Cc: Alex Deucher 
Cc: amd-gfx@lists.freedesktop.org
Cc: Boris Brezillon 
Cc: Daniel Vetter 
Cc: Sean Paul 
Acked-by: Christian König 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/drm_ioctl.c |  4 ++--
 drivers/gpu/drm/drm_prime.c | 19 +++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index fcd728d7cf72..5afb39688b55 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
 
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 
0),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 0a2316e0e812..dab166c860ec 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -358,11 +358,27 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
 
+static inline bool
+allowed_ioctl(struct drm_device *dev, struct drm_file *file_priv)
+{
+   /* Unauthenticated master is allowed, for render capable devices */
+   if (drm_is_primary_client(file_priv)) {
+   if (!file_priv->authenticated &&
+   !drm_core_check_feature(dev, DRIVER_RENDER))
+   return false;
+   }
+
+   return true;
+}
+
 int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 struct drm_file *file_priv)
 {
struct drm_prime_handle *args = data;
 
+   if (!allowed_ioctl(dev, file_priv))
+   return -EACCES;
+
if (!dev->driver->prime_fd_to_handle)
return -ENOSYS;
 
@@ -511,6 +527,9 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, 
void *data,
 {
struct drm_prime_handle *args = data;
 
+   if (!allowed_ioctl(dev, file_priv))
+   return -EACCES;
+
if (!dev->driver->prime_handle_to_fd)
return -ENOSYS;
 
-- 
2.23.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

On Fri, 1 Nov 2019 at 13:05, Emil Velikov  wrote:
>
> From: Emil Velikov 
>
> As mentioned by Christian, for drivers which support only primary nodes
> this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
>
> For others, this check in particular will be a noop. So let's remove it
> as suggested by Christian.
>
> Cc: Alex Deucher 
> Cc: amd-gfx@lists.freedesktop.org
> Cc: Daniel Vetter 
> Cc: Sean Paul 
> Acked-by: Christian König 
> Signed-off-by: Emil Velikov 
> ---
>  drivers/gpu/drm/drm_ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index fcd728d7cf72..5afb39688b55 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
>
> -   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
> drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
> drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
> drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
> drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, 
> drm_mode_getplane_res, 0),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
> --
Humble poke? Sean, others?

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/5] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

From: Emil Velikov 

As mentioned by Christian, for drivers which support only primary nodes
this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.

For others, this check in particular will be a noop. So let's remove it
as suggested by Christian.

Cc: Alex Deucher 
Cc: amd-gfx@lists.freedesktop.org
Cc: Daniel Vetter 
Cc: Sean Paul 
Acked-by: Christian König 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/drm_ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index fcd728d7cf72..5afb39688b55 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
 
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 
0),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
-- 
2.23.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 2/2] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

From: Emil Velikov 

As mentioned by Christian, for drivers which support only primary nodes
this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.

For others, this check in particular will be a noop. So let's remove it
as suggested by Christian.

Cc: Alex Deucher 
Cc: amd-gfx@lists.freedesktop.org
Cc: Daniel Vetter 
Acked-by: Christian König 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/drm_ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f675a3bb2c88..867ab4e50374 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -647,8 +647,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
 
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 
0),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] drm: drop DRM_AUTH from PRIME_TO/FROM_HANDLE ioctls

[Emil Velikov]

From: Emil Velikov 

As mentioned by Christian, for drivers which support only primary nodes
this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.

For others, this check in particular will be a noop. So let's remove it
as suggested by Christian.

Cc: Alex Deucher 
Cc: Christian König 
Cc: amd-gfx@lists.freedesktop.org
Cc: Daniel Vetter 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/drm_ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 09f7f8e33fa3..a397177af369 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -647,8 +647,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
 
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
 
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 
0),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 05/24] drm/amdgpu: remove memset after kzalloc

[Emil Velikov]

On 2019/07/15, Fuqian Huang wrote:
> kzalloc has already zeroed the memory during the allocation.
> So memset is unneeded.
> 
> Signed-off-by: Fuqian Huang 
> ---
> Changes in v3:
>   - Fix subject prefix: gpu/drm -> drm/amdgpu
> 
Reviewed-by: Emil Velikov 

-Emil

Re: [PATCH 06/30] drm/amdgpu: Use kmemdup rather than duplicating its implementation

[Emil Velikov]

On Wed, 3 Jul 2019 at 14:15, Fuqian Huang  wrote:
>
> kmemdup is introduced to duplicate a region of memory in a neat way.
> Rather than kmalloc/kzalloc + memset, which the programmer needs to
> write the size twice (sometimes lead to mistakes), kmemdup improves
> readability, leads to smaller code and also reduce the chances of mistakes.
> Suggestion to use kmemdup rather than using kmalloc/kzalloc + memset.
>
> Signed-off-by: Fuqian Huang 
Fuqian please add reviewed-by and other tags when sending new revisions.

Fwiw the patch is:
Reviewed-by: Emil Velikov 

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

On Wed, 3 Jul 2019 at 15:58, Koenig, Christian  wrote:
> Am 03.07.2019 16:51 schrieb Emil Velikov :
>
> On Wed, 3 Jul 2019 at 15:33, Koenig, Christian  
> wrote:
> > Am 03.07.2019 16:00 schrieb Emil Velikov :
> >
> > On Wed, 3 Jul 2019 at 14:48, Koenig, Christian  
> > wrote:
> > >
> > > Well this is still a NAK.
> > >
> > > As stated previously please just don't remove DRM_AUTH and keep the 
> > > functionality as it is.
> > >
> > AFAICT nobody was in favour of your suggestion to remove DRM_AUTH from
> > the handle to/from fd ioclts.
> > Thus this seems like the second best option.
> >
> >
> > Well just keep it. As I said please don't change anything here.
> >
> > Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the 
> > problems at hand far as I know.
> >
> We also need the DRM_AUTH for handle to/from fd ones. Mesa drivers use
> those ioctls.
>
>
> Yeah, but only for importing/exporting things.
>
> And in those cases we either already gave render nodes or correctly 
> authenticated primary nodes.
>
> So no need to change anything here as far as I see.
>
Not quite. When working with the primary node we have the following scenarios:
 - handle to fd -> pass fd to other APIs - gbm, opencl, vdpau, etc
 - handle to fd -> fd to handle - use it internally

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

On Wed, 3 Jul 2019 at 15:33, Koenig, Christian  wrote:
> Am 03.07.2019 16:00 schrieb Emil Velikov :
>
> On Wed, 3 Jul 2019 at 14:48, Koenig, Christian  
> wrote:
> >
> > Well this is still a NAK.
> >
> > As stated previously please just don't remove DRM_AUTH and keep the 
> > functionality as it is.
> >
> AFAICT nobody was in favour of your suggestion to remove DRM_AUTH from
> the handle to/from fd ioclts.
> Thus this seems like the second best option.
>
>
> Well just keep it. As I said please don't change anything here.
>
> Dropping DRM_AUTH from the driver IOCTLs was sufficient to work around the 
> problems at hand far as I know.
>
We also need the DRM_AUTH for handle to/from fd ones. Mesa drivers use
those ioctls.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

On Wed, 3 Jul 2019 at 14:48, Koenig, Christian  wrote:
>
> Well this is still a NAK.
>
> As stated previously please just don't remove DRM_AUTH and keep the 
> functionality as it is.
>
AFAICT nobody was in favour of your suggestion to remove DRM_AUTH from
the handle to/from fd ioclts.
Thus this seems like the second best option.

Third route that I see is doing driver_name == "amdgpu" || driver_name
== "radeon" in core.
If you have alternative solution I'm all ears.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm: introduce DRIVER_FORCE_AUTH

[Emil Velikov]

From: Emil Velikov 

With earlier commits we've removed DRM_AUTH for driver ioctls annotated
with DRM_AUTH | DRM_RENDER_ALLOW, as the protection it introduces is
effectively not existent.

With next commit, we'll effectively do the same for DRM core.

Yet the AMD developers have voiced concerns that by doing so, developers
working on the closed source user-space driver might remove render node
support.

Since we do _not_ want that to happen, add workaround for those two
drivers

Cc: Alex Deucher 
Cc: Christian König 
Cc: amd-gfx@lists.freedesktop.org
Cc: Daniel Vetter 
Signed-off-by: Emil Velikov 
---
Christian, Alex this is the cleaner way to handle AMDGPU/radeon although
if you prefer alternative methods let me know.

Review, acks and others are appreciated, since I'd like to get this
through the drm-misc tree.

Thanks
Emil

Unrelated:
The USE_AGP flag in AMDGPU should be nuked. While for radeon, one can
copy in the driver the 10-20 lines worth of agp_init/release and also
drop the flag.

Bonus points of agp_init code gets a LEGACY check alongside the USE_AGP
one.
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.c |  2 +-
 include/drm/drm_drv.h   | 10 ++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 8e1b269351e8..cfc2ef11330c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1307,7 +1307,7 @@ amdgpu_get_crtc_scanout_position(struct drm_device *dev, 
unsigned int pipe,
 
 static struct drm_driver kms_driver = {
.driver_features =
-   DRIVER_USE_AGP | DRIVER_ATOMIC |
+   DRIVER_USE_AGP | DRIVER_ATOMIC | DRIVER_FORCE_AUTH |
DRIVER_GEM |
DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
.load = amdgpu_driver_load_kms,
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index 4403e76e1ae0..5a1bfad1ad5e 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -538,7 +538,7 @@ radeon_get_crtc_scanout_position(struct drm_device *dev, 
unsigned int pipe,
 
 static struct drm_driver kms_driver = {
.driver_features =
-   DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER,
+   DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER | DRIVER_FORCE_AUTH,
.load = radeon_driver_load_kms,
.open = radeon_driver_open_kms,
.postclose = radeon_driver_postclose_kms,
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b33f2cee2099..5fb2846396bc 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -92,6 +92,16 @@ enum drm_driver_feature {
 * synchronization of command submission.
 */
DRIVER_SYNCOBJ_TIMELINE = BIT(6),
+   /**
+* @DRIVER_FORCE_AUTH:
+*
+* Driver mandates that DRM_AUTH is honoured, even if the same ioctl
+* is exposed via the render node - aka any of an "authentication" is
+* a fallacy.
+*
+* Used only by amdgpu and radeon. Do not use.
+*/
+   DRIVER_FORCE_AUTH   = BIT(7),
 
/* IMPORTANT: Below are all the legacy flags, add new ones above. */
 
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: extend AMDGPU_CTX_PRIORITY_NORMAL comment

[Emil Velikov]

On Fri, 14 Jun 2019 at 19:02, Koenig, Christian
 wrote:
>
> Am 14.06.19 um 19:33 schrieb Emil Velikov:
> > From: Emil Velikov 
> >
> > Currently the AMDGPU_CTX_PRIORITY_* defines are used in both
> > drm_amdgpu_ctx_in::priority and drm_amdgpu_sched_in::priority.
> >
> > Extend the comment to mention the CAP_SYS_NICE or DRM_MASTER requirement
> > is only applicable with the former.
> >
> > Cc: Bas Nieuwenhuizen 
> > Cc: Christian König 
> > Cc: Alex Deucher 
> > Signed-off-by: Emil Velikov 
> > ---
> > Mildly curious: why didn't one extend ctx_amdgpu_ctx instead of adding
> > drm_amdgpu_sched? New flag + _u32 fd at the end (for the former) would
> > have been enough (and tweaking the ioctl permission thingy).
>
> The drm_amdgpu_sched is only allowed for DRM_MASTER.
>
Fair enough.

Is the patch wrong or did it slip through the cracks? I cannot see it
in Alex's tree.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 00/22] Associate ddc adapters with connectors

[Emil Velikov]

On Fri, 28 Jun 2019 at 17:45, Daniel Vetter  wrote:
>
> On Fri, Jun 28, 2019 at 06:01:14PM +0200, Andrzej Pietrasiewicz wrote:
> > It is difficult for a user to know which of the i2c adapters is for which
> > drm connector. This series addresses this problem.
> >
> > The idea is to have a symbolic link in connector's sysfs directory, e.g.:
> >
> > ls -l /sys/class/drm/card0-HDMI-A-1/ddc
> > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> >   -> ../../../../soc/1388.i2c/i2c-2
> >
> > The user then knows that their card0-HDMI-A-1 uses i2c-2 and can e.g. run
> > ddcutil:
> >
> > ddcutil -b 2 getvcp 0x10
> > VCP code 0x10 (Brightness): current value =90, max 
> > value =   100
> >
> > The first patch in the series adds struct i2c_adapter pointer to struct
> > drm_connector. If the field is used by a particular driver, then an
> > appropriate symbolic link is created by the generic code, which is also 
> > added
> > by this patch.
> >
> > The second patch is an example of how to convert a driver to this new 
> > scheme.
> >
> > v1..v2:
> >
> > - used fixed name "ddc" for the symbolic link in order to make it easy for
> > userspace to find the i2c adapter
> >
> > v2..v3:
> >
> > - converted as many drivers as possible.
> >
> > PATCHES 3/22-22/22 SHOULD BE CONSIDERED RFC!
>
> There's a lot more drivers than this I think (i915 is absent as an
> example, but there should be tons more). Why are those not possible?

While I fully agree there are more drivers, at the same time I wonder.
Is it a good idea to expect all of those to be fixed in one go and
block patches addressing 15+ drivers?

Personally I think it's reasonable to have this, alongside a TODO
entry for other drivers.

HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 07/27] gpu: drm: remove memset after zalloc

[Emil Velikov]

Hi Fuqian,

On Fri, 28 Jun 2019 at 09:30, Fuqian Huang  wrote:
>
> zalloc has already zeroed the memory.
> so memset is unneeded.
>
> Signed-off-by: Fuqian Huang 
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c   | 2 --
>  drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c | 2 --
>  drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c| 2 --
>  drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c   | 2 --
>  drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 2 --
>  5 files changed, 10 deletions(-)
>
Thanks for fixing these. One nit pick: the commit message should start
with "drm/amdgpu:" as you can see from git log.
With that:

Reviewed-by: Emil Velikov 

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 1/9] amdgpu: Pass file descriptor directly to amdgpu_close_kms_handle

[Emil Velikov]

On Mon, 24 Jun 2019 at 17:54, Michel Dänzer  wrote:
>
> From: Michel Dänzer 
>
> And propagate drmIoctl's return value.
>
> This allows replacing all remaining open-coded DRM_IOCTL_GEM_CLOSE
> ioctl calls with amdgpu_close_kms_handle calls.
>
> Signed-off-by: Michel Dänzer 
> ---
>  amdgpu/amdgpu_bo.c | 35 +++
>  1 file changed, 15 insertions(+), 20 deletions(-)
>
Fwiw patches 1-3 are:
Reviewed-by: Emil Velikov 

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Koenig, Christian wrote:
> Am 21.06.19 um 13:58 schrieb Emil Velikov:
> > On 2019/06/21, Koenig, Christian wrote:
> >> Am 21.06.19 um 12:53 schrieb Emil Velikov:
> >>> On 2019/06/21, Koenig, Christian wrote:
> >>>> [SNIP]
> >>>> Well partially. That RADV broke is unfortunate, but we have done so many
> >>>> customized userspace stuff both based on Mesa/DDX as well as closed
> >>>> source components that it is just highly likely that we would break
> >>>> something else as well.
> >>>>
> >>> As an engineer I like to work with tangibles. The highly likely part is
> >>> probable, but as mentioned multiple times the series allows for a _dead_
> >>> trivial way to address any such problems.
> >> Well to clarify my concern is that this won't be so trivial.
> >>
> >> You implemented on workaround for one specific case and it is perfectly
> >> possible that for other cases we would have to completely revert the
> >> removal of DRM_AUTH.
> >>
> > I would encourage you to take a closer look at my patch and point out
> > how parcicular cases cannot be handled.
> 
> Well the last time I looked it only blocked the first call to the IOCTL.
> 
Hmm interesting, you're replied to my patch without even reading it :'-(
Can you please give it a close look and reply inline?

[1] https://lists.freedesktop.org/archives/dri-devel/2019-May/219238.html

> >  From your experiense, do user-space developers care about doing (or
> > generally do) the right thing?
> 
> No, not at all. Except for Marek and Michel they just take what works 
> and even Marek is often short-cutting on this.
> 
Interesting, guess I should have asked this question from the start.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Daniel Vetter wrote:
> On Fri, Jun 21, 2019 at 1:50 PM Daniel Vetter  wrote:
> >
> > On Fri, Jun 21, 2019 at 1:37 PM Christian König
> >  wrote:
> > >
> > > Am 21.06.19 um 13:03 schrieb Daniel Vetter:
> > > > On Fri, Jun 21, 2019 at 12:31 PM Koenig, Christian
> > > >  wrote:
> > > >> Am 21.06.19 um 12:20 schrieb Emil Velikov:
> > > >>> In particular, that user-space will "remove" render nodes.
> > > >> Yes, that is my main concern here. You basically make render nodes
> > > >> superfluously. That gives userspace all legitimization to also remove
> > > >> support for them. That is not stupid or evil or whatever, userspace
> > > >> would just follow the kernel design.
> > > > This already happened. At least for kms-only display socs we had to
> > > > hide the separate render node (and there you really have to deal with
> > > > the separate render node, because it's a distinct driver) entirely
> > > > within gbm/mesa.
> > >
> > > Ok, that is something I didn't knew before and is rather interesting.
> > >
> > > > So if you want to depracate render functionality on primary nodes, you
> > > > _have_ to do that hiding in userspace. Or you'll break a lot of
> > > > compositors everywhere. Just testing -amdgpu doesn't really prove
> > > > anything here. So you won't move the larger ecosystem with this at
> > > > all, that ship sailed.
> > >
> > > So what else can we do? That sounds like you suggest we should
> > > completely drop render node functionality.
> > >
> > > I mean it's not my preferred option, but certainly something that
> > > everybody could do.
> > >
> > > My primary concern is that we somehow need to get rid of thinks like GEM
> > > flink and all that other crufty stuff we still have around on the
> > > primary node (we should probably make a list of that).
> > >
> > > Switching everything over to render nodes just sounded like the best
> > > alternative so far to archive that.
> >
> > tbh I do like that plan too, but it's a lot more work. And I think to
> > have any push whatsoever we probably need to roll it out in gbm as a
> > hack to keep things going. But probably not enough.
> >
> > I also think that at least some compositors will bother to do the
> > right thing, and actually bother with render nodes and all that
> > correctly. It's just that there's way more which dont.
> >
> > Also for server rendering it'll be render nodes all the way down I
> > hope (or we need to seriously educate cloud people about the
> > permissions they set on their default images, and distros on how this
> > cloud stuff should work.
> >
> > > > At that point this all feels like a bikeshed, and for a bikeshed I
> > > > don't care what the color is we pick, as long as they're all painted
> > > > the same.
> > > >
> > > > Once we picked a color it's a simple technical matter of how to roll
> > > > it out, using Kconfig options, or only enabling on new hw, or "merge
> > > > and fix the regressions as they come" or any of the other well proven
> > > > paths forward.
> > >
> > > Yeah, the problem is I don't see an option which currently works for
> > > everyone.
> > >
> > > I absolutely need a grace time of multiple years until we can apply this
> > > to amdgpu/radeon.
> >
> > Yeah that's what I meant with "pick a color, pick a way to roll it
> > out". "enable for new hw, roll out years and years later" is one of
> > the options for roll out.
> >
> > > And that under the prerequisite to have a plan to somehow enable that
> > > functionality now to make it at least painful for userspace to rely on
> > > hack around that.
> > >
> > > > So if you want to do this, please start with the mesa side work (as
> > > > the biggest userspace, not all of it) with patches to redirect all
> > > > primary node rendering to render nodes. The code is there already for
> > > > socs, just needs to be rolled out more. Or we go with the DRM_AUTH
> > > > horrors. Or a 3rd option, I really don't care which it is, as long as
> > > > its consistent.
> > >
> > > How about this:
> > > 1. We keep DRM_AUTH around for amdgpu/radeon for now.
> > > 2. We add a Kconfig option to ignore DRM_AUTH

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Koenig, Christian wrote:
> Am 21.06.19 um 12:53 schrieb Emil Velikov:
> > On 2019/06/21, Koenig, Christian wrote:
> >> [SNIP]
> >> Well partially. That RADV broke is unfortunate, but we have done so many
> >> customized userspace stuff both based on Mesa/DDX as well as closed
> >> source components that it is just highly likely that we would break
> >> something else as well.
> >>
> > As an engineer I like to work with tangibles. The highly likely part is
> > probable, but as mentioned multiple times the series allows for a _dead_
> > trivial way to address any such problems.
> 
> Well to clarify my concern is that this won't be so trivial.
> 
> You implemented on workaround for one specific case and it is perfectly 
> possible that for other cases we would have to completely revert the 
> removal of DRM_AUTH.
> 
I would encourage you to take a closer look at my patch and point out
how parcicular cases cannot be handled.

> >>> In particular, that user-space will "remove" render nodes.
> >> Yes, that is my main concern here. You basically make render nodes
> >> superfluously. That gives userspace all legitimization to also remove
> >> support for them. That is not stupid or evil or whatever, userspace
> >> would just follow the kernel design.
> >>
> > Do you have an example of userspace doing such an extremely silly thing?
> > It does seem like suspect you're a couple of steps beyond overcautious,
> > perhaps rightfully so. Maybe you've seen some closed-source user-space
> > going crazy? Or any other projects?
> 
> The key point is that I don't think this is silly or strange or crazy at 
> all. See the kernel defines the interface userspace can and should use.
> 
> When the kernel defines that everything will work with the primary node 
> it is perfectly valid for userspace to drop support for the render node.
> 
> I mean why should they keep this? Just because we tell them not to do this?
> 
From your experiense, do user-space developers care about doing (or
generally do) the right thing?

In either case, as pointed already the cat is out of the bag - has been
for years, and if developers did behave as you describe them they would
have "removed" render nodes already.

> > In other words, being cautious is great, but without references of
> > misuse it's very hard for others to draw the full picture.
> >
> >>> I'm really sad that amdgpu insists on standing out, hope one day it will
> >>> converge. Yet since all other maintainers are ok with the series, I'll
> >>> start merging patches in a few hours. I'm really sad that amdgpu wants
> >>> to stand out, hope it will converge sooner rather than later.
> >>>
> >>> Christian, how would you like amdgpu handled - with a separate flag in
> >>> the driver or shall we special case amdgpu within core drm?
> >> No, I ask you to please stick around DRM_AUTH for at least amdgpu and
> >> radeon. And NOT enable any render node functionality for them on the
> >> primary node.
> >>
> > My question is how do you want this handled:
> >   - DRM_PLEASE_FORCE_AUTH - added to AMDGPU/RADEON, or
> >   - driver_name == amdgpu, in core DRM.
> 
> I want to keep DRM_AUTH in amdgpu and radeon for at least the next 5-10 
> years.
> 
Believe we're all fully aware of that fact. I'm asking which _approach_
you prefer. That said, I'll send a new patch/series and we'll nitpick it
there.

Thanks
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Koenig, Christian wrote:
> No this should apply to all drivers, not just amdgpu.
> 
> > While I suggested:
> >   - keeping amdgpu consistent with the rest
> >   - exposing the KConfig option for the whole of the kernel, and
> >   - merging it alongside this work
> >
> > So I effectively waited for weeks, instead of simply going ahead and
> > writing/submitting patches. That's a bit unfortunate...
> >
> >>> Because we simply made sure that userspace is using the render node for
> >>> command submission and not the primary node.
> >>>
> >>> So nobody can go ahead and remove render node support any more :)
> >> How does this work? I thought the entire problem of DRM_AUTH removal
> >> for you was that it broke stuff, and you didn't want to deal with
> >> that. We still have that exact problem afaics ... old userspace
> >> doesn't simply vanish, even if you entirely nuke it going forward.
> >>
> >> Also, testing on the amdgpu stack isn't really testing a hole lot
> >> here, it's all the various silly compositors out there I'd expect to
> >> fall over. Will gbm/radeonsi/whatever just internally do all the
> >> necessary dma-buf import/export between the primary node and display
> >> node to keep this all working?
> > If I understood Christian, feel free to correct me, the fact that my
> > earlier patch broke RADV was not the argument. The problem was was the
> > patch does.
> 
> Well partially. That RADV broke is unfortunate, but we have done so many 
> customized userspace stuff both based on Mesa/DDX as well as closed 
> source components that it is just highly likely that we would break 
> something else as well.
> 
As an engineer I like to work with tangibles. The highly likely part is
probable, but as mentioned multiple times the series allows for a _dead_
trivial way to address any such problems.

> > In particular, that user-space will "remove" render nodes.
> 
> Yes, that is my main concern here. You basically make render nodes 
> superfluously. That gives userspace all legitimization to also remove 
> support for them. That is not stupid or evil or whatever, userspace 
> would just follow the kernel design.
> 
Do you have an example of userspace doing such an extremely silly thing?
It does seem like suspect you're a couple of steps beyond overcautious,
perhaps rightfully so. Maybe you've seen some closed-source user-space
going crazy? Or any other projects?

In other words, being cautious is great, but without references of
misuse it's very hard for others to draw the full picture.

> > I'm really sad that amdgpu insists on standing out, hope one day it will
> > converge. Yet since all other maintainers are ok with the series, I'll
> > start merging patches in a few hours. I'm really sad that amdgpu wants
> > to stand out, hope it will converge sooner rather than later.
> >
> > Christian, how would you like amdgpu handled - with a separate flag in
> > the driver or shall we special case amdgpu within core drm?
> 
> No, I ask you to please stick around DRM_AUTH for at least amdgpu and 
> radeon. And NOT enable any render node functionality for them on the 
> primary node.
> 
My question is how do you want this handled:
 - DRM_PLEASE_FORCE_AUTH - added to AMDGPU/RADEON, or
 - driver_name == amdgpu, in core DRM.


Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Daniel Vetter wrote:
> On Fri, Jun 21, 2019 at 11:25 AM Koenig, Christian
>  wrote:
> >
> > Am 21.06.19 um 11:09 schrieb Daniel Vetter:
> > > On Fri, Jun 21, 2019 at 07:12:14AM +, Koenig, Christian wrote:
> > >> Am 20.06.19 um 18:30 schrieb Emil Velikov:
> > >>> On 2019/06/14, Koenig, Christian wrote:
> > >>>> Am 14.06.19 um 17:53 schrieb Emil Velikov:
> > >>>>> On 2019/06/14, Koenig, Christian wrote:
> > >>>>>> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> > >>>>>>> On 2019/05/27, Emil Velikov wrote:
> > >>>>>>> [SNIP]
> > >>>>>>> Hi Christian,
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> In the following, I would like to summarise and emphasize the need 
> > >>>>>>> for
> > >>>>>>> DRM_AUTH removal. I would kindly ask you to spend a couple of 
> > >>>>>>> minutes
> > >>>>>>> extra reading it.
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> Today DRM drivers* do not make any distinction between primary and
> > >>>>>>> render node clients.
> > >>>>>> That is actually not 100% correct. We have a special case where a DRM
> > >>>>>> master is allowed to change the priority of render node clients.
> > >>>>>>
> > >>>>> Can you provide a link? I cannot find that code.
> > >>>> See amdgpu_sched_ioctl().
> > >>>>
> > >>>>>>> Thus for a render capable driver, any premise of
> > >>>>>>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> > >>>>>> Yeah, that's what I agree on. I just don't think that removing 
> > >>>>>> DRM_AUTH
> > >>>>>> now is the right direction to take.
> > >>>>>>
> > >>>>> Could have been clearer - I'm talking about DRM_AUTH | 
> > >>>>> DRM_RENDER_ALLOW
> > >>>>> ioctls.
> > >>>>>
> > >>>>> That aside, can you propose an alternative solution that addresses 
> > >>>>> this
> > >>>>> and the second point just below?
> > >>>> Give me a few days to work on this, it's already Friday 6pm here.
> > >>>>
> > >>> Hi Christian,
> > >>>
> > >>> Any progress? As mentioned earlier, I'm OK with writing the patches 
> > >>> although
> > >>> I would love to hear your plan.
> > >> First of all I tried to disable DRM authentication completely with a
> > >> kernel config option. Surprisingly that actually works out of the box at
> > >> least on the AMDGPU stack.
> > >>
> > >> This effectively allows us to get rid of DRI2 and the related security
> > >> problems. Only thing left for that is that I'm just not sure how to
> > >> signal this to userspace so that the DDX wouldn't advertise DRI2 at all
> > >> any more.
> > >>
> > >>
> > >> As a next step I looked into if we can disable the command submission
> > >> for DRM master. Turned out that this is relatively easy as well.
> > >>
> > >> All we have to do is to fix the bug Michel pointed out about KMS handles
> > >> for display and let the DDX use a render node instead of the DRM master
> > >> for Glamor. Still need to sync up with Michel and/or Marek whats the
> > >> best way of doing this.
> > >>
> > >>
> > >> The last step (and that's what I haven't tried yet) would be to disable
> > >> DRM authentication for Navi even when it is still compiled into the
> > >> kernel. But that is more or less just a typing exercise.
> > > So just to get this straight, we're now full on embracing a subsystem
> > > split here:
> > > - amdgpu plans to ditch dri2/rendering on primary nodes
> > > - bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
> > >removal
> > > - others are just hanging in there somehow
> > >
> > > "best of both^W worlds" ftw?
> >
> > Yes, exactly!
> >
> > Think a step further, when this is upstream we can apply the DRM_AUTH
> 

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/14, Koenig, Christian wrote:
> Am 14.06.19 um 17:53 schrieb Emil Velikov:
> > On 2019/06/14, Koenig, Christian wrote:
> >> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> >>> On 2019/05/27, Emil Velikov wrote:
> >>> [SNIP]
> >>> Hi Christian,
> >>>
> >>>
> >>> In the following, I would like to summarise and emphasize the need for
> >>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> >>> extra reading it.
> >>>
> >>>
> >>> Today DRM drivers* do not make any distinction between primary and
> >>> render node clients.
> >> That is actually not 100% correct. We have a special case where a DRM
> >> master is allowed to change the priority of render node clients.
> >>
> > Can you provide a link? I cannot find that code.
> 
> See amdgpu_sched_ioctl().
> 
> >>> Thus for a render capable driver, any premise of
> >>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> >> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
> >> now is the right direction to take.
> >>
> > Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
> > ioctls.
> >
> > That aside, can you propose an alternative solution that addresses this
> > and the second point just below?
> 
> Give me a few days to work on this, it's already Friday 6pm here.
> 
Hi Christian,

Any progress? As mentioned earlier, I'm OK with writing the patches although
I would love to hear your plan.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 06/59] drm/prime: Actually remove DRIVER_PRIME everywhere

[Emil Velikov]

On 2019/06/14, Daniel Vetter wrote:
> Split out to make the functional changes stick out more.
> 
Since this patch flew-by, as standalone one (intentionally or not) I'd
add, anything vaguely like:

"Core users of DRIVER_PRIME were removed from core with prior patches."

HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: extend AMDGPU_CTX_PRIORITY_NORMAL comment

[Emil Velikov]

From: Emil Velikov 

Currently the AMDGPU_CTX_PRIORITY_* defines are used in both
drm_amdgpu_ctx_in::priority and drm_amdgpu_sched_in::priority.

Extend the comment to mention the CAP_SYS_NICE or DRM_MASTER requirement
is only applicable with the former.

Cc: Bas Nieuwenhuizen 
Cc: Christian König 
Cc: Alex Deucher 
Signed-off-by: Emil Velikov 
---
Mildly curious: why didn't one extend ctx_amdgpu_ctx instead of adding
drm_amdgpu_sched? New flag + _u32 fd at the end (for the former) would
have been enough (and tweaking the ioctl permission thingy).

Speaking of flags, drm_amdgpu_sched_in lost its so extending it will
be "fun"
---
 include/uapi/drm/amdgpu_drm.h | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 4788730dbe78..dfb10fba2fe8 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -219,7 +219,10 @@ union drm_amdgpu_bo_list {
 #define AMDGPU_CTX_PRIORITY_VERY_LOW-1023
 #define AMDGPU_CTX_PRIORITY_LOW -512
 #define AMDGPU_CTX_PRIORITY_NORMAL  0
-/* Selecting a priority above NORMAL requires CAP_SYS_NICE or DRM_MASTER */
+/*
+ * When used in struct drm_amdgpu_ctx_in, a priority above NORMAL requires
+ * CAP_SYS_NICE or DRM_MASTER
+*/
 #define AMDGPU_CTX_PRIORITY_HIGH512
 #define AMDGPU_CTX_PRIORITY_VERY_HIGH   1023
 
@@ -229,6 +232,7 @@ struct drm_amdgpu_ctx_in {
/** For future use, no flags defined so far */
__u32   flags;
__u32   ctx_id;
+   /** AMDGPU_CTX_PRIORITY_* */
__s32   priority;
 };
 
@@ -281,6 +285,7 @@ struct drm_amdgpu_sched_in {
/* AMDGPU_SCHED_OP_* */
__u32   op;
__u32   fd;
+   /** AMDGPU_CTX_PRIORITY_* */
__s32   priority;
__u32   ctx_id;
 };
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/14, Koenig, Christian wrote:
> Am 14.06.19 um 17:53 schrieb Emil Velikov:
> > On 2019/06/14, Koenig, Christian wrote:
> >> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> >>> On 2019/05/27, Emil Velikov wrote:
> >>> [SNIP]
> >>> Hi Christian,
> >>>
> >>>
> >>> In the following, I would like to summarise and emphasize the need for
> >>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> >>> extra reading it.
> >>>
> >>>
> >>> Today DRM drivers* do not make any distinction between primary and
> >>> render node clients.
> >> That is actually not 100% correct. We have a special case where a DRM
> >> master is allowed to change the priority of render node clients.
> >>
> > Can you provide a link? I cannot find that code.
> 
> See amdgpu_sched_ioctl().
> 
> >>> Thus for a render capable driver, any premise of
> >>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> >> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
> >> now is the right direction to take.
> >>
> > Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
> > ioctls.
> >
> > That aside, can you propose an alternative solution that addresses this
> > and the second point just below?
> 
> Give me a few days to work on this, it's already Friday 6pm here.
> 
Great thanks. Fwiw I was asking for a ideas/proposal, was not expecting you
to write the patches ;-)

Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/14, Koenig, Christian wrote:
> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> > On 2019/05/27, Emil Velikov wrote:
> > [SNIP]
> > Hi Christian,
> >
> >
> > In the following, I would like to summarise and emphasize the need for
> > DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> > extra reading it.
> >
> >
> > Today DRM drivers* do not make any distinction between primary and
> > render node clients.
> 
> That is actually not 100% correct. We have a special case where a DRM 
> master is allowed to change the priority of render node clients.
> 
Can you provide a link? I cannot find that code.

> > Thus for a render capable driver, any premise of
> > separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> 
> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH 
> now is the right direction to take.
> 
Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
ioctls.

That aside, can you propose an alternative solution that addresses this
and the second point just below?

> > Considering the examples of flaky or outright missing drmAuth in prime
> > open-source projects (mesa, kmscube, libva) we can reasonably assume
> > other projects exbibit the same problem.
> >
> > For these and/or other reasons, two DRM drivers have dropped DRM_AUTH
> > since day one.
> >
> > Since we are interested in providing consistent and predictable
> > behaviour to user-space, dropping DRM_AUTH seems to be the most
> > effective way forward.
> 
> Well and what do you do with drivers which doesn't implement render nodes?
> 
AFAICT there is a single non-DC driver which does not expose - QXL.
I would expect it runs on a rather customised stack.

Would be great to fix that, but ETIME and it's the only exception to the
rule.


> > Of course, I'd be more than happy to hear for any other way to achieve
> > the goal outlined.
> >
> > Based on the series, other maintainers agree with the idea/goal here.
> > Amdgpu not following the same pattern would compromise predictability
> > across drivers and complicate userspace, so I would kindly ask you to
> > reconsider.
> >
> > Alternatively, I see two ways to special case:
> >   - New flag annotating amdgpu/radeon - akin to the one proposed earlier
> >   - Check for amdgpu/radeon in core DRM
> 
> I perfectly agree that I don't want any special handling for amdgpu/radeon.
> 
> My key point is that with DRM_AUTH we created an authorization and 
> authentication mess in DRM which is functionality which doesn't belong 
> into the DRM subsystem in the first place.
> 
Precisely and I've outlined below how to resolve this in the long run.

> > Now on your pain point - not allowing render iocts via the primary node,
> > and how this patch could make it harder to achieve that goal.
> >
> > While I love the idea, there are number of obstacles that prevent us
> > from doing so at this time:
> >   - Ensuring both old and new userspace does not regress
> 
> Yeah, agree totally. That's why I said we should probably start doing 
> this for only for upcoming hardware generations.
> 
That will side-step the regression issue, but will enforce driver
specific behaviour outlined before.

> >   - Drivers (QXL, others?) do not expose a render node
> 
> Well isn't that is a rather big problem for the removal of DRM_AUTH in 
> general?
> 
> E.g. at least QXL would then expose functionality on the primary node 
> without authentication.
> 
With this series QXL remains functionally unchanged. I would love to fix
that as well yet ETIME as mentioned above.

> >   - We want to avoid driver specific behaviour
> >
> > The only way forward that I can see is:
> >   - Address QXL/others to expose render nodes
> >   - Add a Kconfig toggle to disable !KMS ioctls via the primary node
> >   - Jump-start ^^ with distribution X
> >   - Fix user-space fallouts
> >   - X months down the line, flip the Kconfig
> >   - In case of regressions - revert the KConfig, goto Fix user-space...
> 
> Well that at least sounds like a plan :) Let's to this!
> 
We're talking about months if not years until it comes to fruition. We
need something quicker.

That said, I'm quite happy to take the first stab, yet this is not a
replacement for this series.

> > That said, the proposal will not conflict with the DRM_AUTH removal. If
> > anything it is step 0.5 of the grand master plan.
> 
> That's the point I strongly disagree on.
> 
> By lowering the DRM_AUTH restriction you are encouraging userspace to 
> use the shortcut and use the primary node for

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/27, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> render node. A seemingly deliberate design decision.
> 
> Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> yet not all userspace checks if it's authenticated, but instead uses
> uncommon assumptions.
> 
> After days of digging through git log and testing, only a single (ab)use
> was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> assuming that failure implies lack of authentication.
> 
> Affected versions are:
>  - the whole 18.2.x series, which is EOL
>  - the whole 18.3.x series, which is EOL
>  - the 19.0.x series, prior to 19.0.4
> 
> Add a special quirk for that case, thus we can drop DRM_AUTH bits as
> mentioned earlier.
> 
> Since all the affected userspace is EOL, we also add a kconfig option
> to disable this quirk.
> 
> The whole approach is inspired by DRIVER_KMS_LEGACY_CONTEXT
> 
> Cc: Alex Deucher 
> Cc: Christian König 
> Cc: amd-gfx@lists.freedesktop.org
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Signed-off-by: Emil Velikov 
> ---
>  drivers/gpu/drm/amd/amdgpu/Kconfig  | 16 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++-
>  drivers/gpu/drm/drm_ioctl.c |  5 +
>  include/drm/drm_ioctl.h | 17 +
>  4 files changed, 49 insertions(+), 1 deletion(-)
> 

Hi Christian,


In the following, I would like to summarise and emphasize the need for
DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
extra reading it.


Today DRM drivers* do not make any distinction between primary and
render node clients. Thus for a render capable driver, any premise of
separation, security or otherwise imposed via DRM_AUTH is a fallacy.

Considering the examples of flaky or outright missing drmAuth in prime
open-source projects (mesa, kmscube, libva) we can reasonably assume
other projects exbibit the same problem.

For these and/or other reasons, two DRM drivers have dropped DRM_AUTH
since day one.

Since we are interested in providing consistent and predictable
behaviour to user-space, dropping DRM_AUTH seems to be the most
effective way forward.

Of course, I'd be more than happy to hear for any other way to achieve
the goal outlined.

Based on the series, other maintainers agree with the idea/goal here.
Amdgpu not following the same pattern would compromise predictability
across drivers and complicate userspace, so I would kindly ask you to
reconsider.

Alternatively, I see two ways to special case:
 - New flag annotating amdgpu/radeon - akin to the one proposed earlier
 - Check for amdgpu/radeon in core DRM



Now on your pain point - not allowing render iocts via the primary node,
and how this patch could make it harder to achieve that goal.

While I love the idea, there are number of obstacles that prevent us
from doing so at this time:
 - Ensuring both old and new userspace does not regress
 - Drivers (QXL, others?) do not expose a render node
 - We want to avoid driver specific behaviour

The only way forward that I can see is:
 - Address QXL/others to expose render nodes
 - Add a Kconfig toggle to disable !KMS ioctls via the primary node
 - Jump-start ^^ with distribution X
 - Fix user-space fallouts
 - X months down the line, flip the Kconfig
 - In case of regressions - revert the KConfig, goto Fix user-space...


That said, the proposal will not conflict with the DRM_AUTH removal. If
anything it is step 0.5 of the grand master plan.


Tl;Dr: Removing DRM_AUTH is orthogonal to not allowing render iocts via
the primary node. Here's an idea how to achieve the latter.


Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/31, Koenig, Christian wrote:
> Am 29.05.19 um 18:29 schrieb Emil Velikov:
> > On 2019/05/29, Koenig, Christian wrote:
> >> Am 29.05.19 um 15:03 schrieb Emil Velikov:
> >>> On 2019/05/29, Dave Airlie wrote:
> >>>> On Wed, 29 May 2019 at 02:47, Emil Velikov  
> >>>> wrote:
> >>>>> On 2019/05/28, Koenig, Christian wrote:
> >>>>>> Am 28.05.19 um 18:10 schrieb Emil Velikov:
> >>>>>>> On 2019/05/28, Daniel Vetter wrote:
> >>>>>>>> On Tue, May 28, 2019 at 10:03 AM Koenig, Christian
> >>>>>>>>  wrote:
> >>>>>>>>> Am 28.05.19 um 09:38 schrieb Daniel Vetter:
> >>>>>>>>>> [SNIP]
> >>>>>>>>>>> Might be a good idea looking into reverting it partially, so that 
> >>>>>>>>>>> at
> >>>>>>>>>>> least command submission and buffer allocation is still blocked.
> >>>>>>>>>> I thought the issue is a lot more than vainfo, it's pretty much 
> >>>>>>>>>> every
> >>>>>>>>>> hacked up compositor under the sun getting this wrong one way or
> >>>>>>>>>> another. Thinking about this some more, I also have no idea how 
> >>>>>>>>>> you'd
> >>>>>>>>>> want to deprecate rendering on primary nodes in general. Apparently
> >>>>>>>>>> that breaks -modesetting already, and probably lots more 
> >>>>>>>>>> compositors.
> >>>>>>>>>> And it looks like we're finally achieve the goal kms set out to 10
> >>>>>>>>>> years ago, and new compositors are sprouting up all the time. I 
> >>>>>>>>>> guess
> >>>>>>>>>> we could just break them all (on new hardware) and tell them to all
> >>>>>>>>>> suck it up. But I don't think that's a great option. And just
> >>>>>>>>>> deprecating this on amdgpu is going to be even harder, since then
> >>>>>>>>>> everywhere else it'll keep working, and it's just amdgpu.ko that 
> >>>>>>>>>> looks
> >>>>>>>>>> broken.
> >>>>>>>>>>
> >>>>>>>>>> Aside: I'm not supporting Emil's idea here because it fixes any 
> >>>>>>>>>> issues
> >>>>>>>>>> Intel has - Intel doesn't care. I support it because reality sucks,
> >>>>>>>>>> people get this render vs. primary vs. multi-gpu prime wrong all 
> >>>>>>>>>> the
> >>>>>>>>>> time (that's also why we have hardcoded display+gpu pairs in mesa 
> >>>>>>>>>> for
> >>>>>>>>>> the various soc combinations out there), and this looks like a
> >>>>>>>>>> pragmatic solution. It'd be nice if every compositor and everything
> >>>>>>>>>> else would perfectly support multi gpu and only use render nodes 
> >>>>>>>>>> for
> >>>>>>>>>> rendering, and only primary nodes for display. But reality is that
> >>>>>>>>>> people hack on stuff until gears on screen and then move on to more
> >>>>>>>>>> interesting things (to them). So I don't think we'll ever win this 
> >>>>>>>>>> :-/
> >>>>>>>>> Yeah, but this is a classic case of working around user space 
> >>>>>>>>> issues by
> >>>>>>>>> making kernel changes instead of fixing user space.
> >>>>>>>>>
> >>>>>>>>> Having privileged (output control) and unprivileged (rendering 
> >>>>>>>>> control)
> >>>>>>>>> functionality behind the same node is a mistake we have made a long 
> >>>>>>>>> time
> >>>>>>>>> ago and render nodes finally seemed to be a way to fix that.
> >>>>>>>>>
> >>>>>>>>> I mean why are compositors using the primary node in the first 
> >>>>>>>>> place?
> >&

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/29, Koenig, Christian wrote:
> Am 29.05.19 um 15:03 schrieb Emil Velikov:
> > On 2019/05/29, Dave Airlie wrote:
> >> On Wed, 29 May 2019 at 02:47, Emil Velikov  
> >> wrote:
> >>> On 2019/05/28, Koenig, Christian wrote:
> >>>> Am 28.05.19 um 18:10 schrieb Emil Velikov:
> >>>>> On 2019/05/28, Daniel Vetter wrote:
> >>>>>> On Tue, May 28, 2019 at 10:03 AM Koenig, Christian
> >>>>>>  wrote:
> >>>>>>> Am 28.05.19 um 09:38 schrieb Daniel Vetter:
> >>>>>>>> [SNIP]
> >>>>>>>>> Might be a good idea looking into reverting it partially, so that at
> >>>>>>>>> least command submission and buffer allocation is still blocked.
> >>>>>>>> I thought the issue is a lot more than vainfo, it's pretty much every
> >>>>>>>> hacked up compositor under the sun getting this wrong one way or
> >>>>>>>> another. Thinking about this some more, I also have no idea how you'd
> >>>>>>>> want to deprecate rendering on primary nodes in general. Apparently
> >>>>>>>> that breaks -modesetting already, and probably lots more compositors.
> >>>>>>>> And it looks like we're finally achieve the goal kms set out to 10
> >>>>>>>> years ago, and new compositors are sprouting up all the time. I guess
> >>>>>>>> we could just break them all (on new hardware) and tell them to all
> >>>>>>>> suck it up. But I don't think that's a great option. And just
> >>>>>>>> deprecating this on amdgpu is going to be even harder, since then
> >>>>>>>> everywhere else it'll keep working, and it's just amdgpu.ko that 
> >>>>>>>> looks
> >>>>>>>> broken.
> >>>>>>>>
> >>>>>>>> Aside: I'm not supporting Emil's idea here because it fixes any 
> >>>>>>>> issues
> >>>>>>>> Intel has - Intel doesn't care. I support it because reality sucks,
> >>>>>>>> people get this render vs. primary vs. multi-gpu prime wrong all the
> >>>>>>>> time (that's also why we have hardcoded display+gpu pairs in mesa for
> >>>>>>>> the various soc combinations out there), and this looks like a
> >>>>>>>> pragmatic solution. It'd be nice if every compositor and everything
> >>>>>>>> else would perfectly support multi gpu and only use render nodes for
> >>>>>>>> rendering, and only primary nodes for display. But reality is that
> >>>>>>>> people hack on stuff until gears on screen and then move on to more
> >>>>>>>> interesting things (to them). So I don't think we'll ever win this 
> >>>>>>>> :-/
> >>>>>>> Yeah, but this is a classic case of working around user space issues 
> >>>>>>> by
> >>>>>>> making kernel changes instead of fixing user space.
> >>>>>>>
> >>>>>>> Having privileged (output control) and unprivileged (rendering 
> >>>>>>> control)
> >>>>>>> functionality behind the same node is a mistake we have made a long 
> >>>>>>> time
> >>>>>>> ago and render nodes finally seemed to be a way to fix that.
> >>>>>>>
> >>>>>>> I mean why are compositors using the primary node in the first place?
> >>>>>>> Because they want to have access to privileged resources I think and 
> >>>>>>> in
> >>>>>>> this case it is perfectly ok to do so.
> >>>>>>>
> >>>>>>> Now extending unprivileged access to the primary node actually sounds
> >>>>>>> like a step into the wrong direction to me.
> >>>>>>>
> >>>>>>> I rather think that we should go down the route of completely dropping
> >>>>>>> command submission and buffer allocation through the primary node for
> >>>>>>> non master clients. And then as next step at some point drop support 
> >>>>>>> for
> >>>>>>> authentication/flink.
> >>>>>>>
> >>>>>&

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/29, Dave Airlie wrote:
> On Wed, 29 May 2019 at 02:47, Emil Velikov  wrote:
> >
> > On 2019/05/28, Koenig, Christian wrote:
> > > Am 28.05.19 um 18:10 schrieb Emil Velikov:
> > > > On 2019/05/28, Daniel Vetter wrote:
> > > >> On Tue, May 28, 2019 at 10:03 AM Koenig, Christian
> > > >>  wrote:
> > > >>> Am 28.05.19 um 09:38 schrieb Daniel Vetter:
> > > >>>> [SNIP]
> > > >>>>> Might be a good idea looking into reverting it partially, so that at
> > > >>>>> least command submission and buffer allocation is still blocked.
> > > >>>> I thought the issue is a lot more than vainfo, it's pretty much every
> > > >>>> hacked up compositor under the sun getting this wrong one way or
> > > >>>> another. Thinking about this some more, I also have no idea how you'd
> > > >>>> want to deprecate rendering on primary nodes in general. Apparently
> > > >>>> that breaks -modesetting already, and probably lots more compositors.
> > > >>>> And it looks like we're finally achieve the goal kms set out to 10
> > > >>>> years ago, and new compositors are sprouting up all the time. I guess
> > > >>>> we could just break them all (on new hardware) and tell them to all
> > > >>>> suck it up. But I don't think that's a great option. And just
> > > >>>> deprecating this on amdgpu is going to be even harder, since then
> > > >>>> everywhere else it'll keep working, and it's just amdgpu.ko that 
> > > >>>> looks
> > > >>>> broken.
> > > >>>>
> > > >>>> Aside: I'm not supporting Emil's idea here because it fixes any 
> > > >>>> issues
> > > >>>> Intel has - Intel doesn't care. I support it because reality sucks,
> > > >>>> people get this render vs. primary vs. multi-gpu prime wrong all the
> > > >>>> time (that's also why we have hardcoded display+gpu pairs in mesa for
> > > >>>> the various soc combinations out there), and this looks like a
> > > >>>> pragmatic solution. It'd be nice if every compositor and everything
> > > >>>> else would perfectly support multi gpu and only use render nodes for
> > > >>>> rendering, and only primary nodes for display. But reality is that
> > > >>>> people hack on stuff until gears on screen and then move on to more
> > > >>>> interesting things (to them). So I don't think we'll ever win this 
> > > >>>> :-/
> > > >>> Yeah, but this is a classic case of working around user space issues 
> > > >>> by
> > > >>> making kernel changes instead of fixing user space.
> > > >>>
> > > >>> Having privileged (output control) and unprivileged (rendering 
> > > >>> control)
> > > >>> functionality behind the same node is a mistake we have made a long 
> > > >>> time
> > > >>> ago and render nodes finally seemed to be a way to fix that.
> > > >>>
> > > >>> I mean why are compositors using the primary node in the first place?
> > > >>> Because they want to have access to privileged resources I think and 
> > > >>> in
> > > >>> this case it is perfectly ok to do so.
> > > >>>
> > > >>> Now extending unprivileged access to the primary node actually sounds
> > > >>> like a step into the wrong direction to me.
> > > >>>
> > > >>> I rather think that we should go down the route of completely dropping
> > > >>> command submission and buffer allocation through the primary node for
> > > >>> non master clients. And then as next step at some point drop support 
> > > >>> for
> > > >>> authentication/flink.
> > > >>>
> > > >>> I mean we have done this with UMS as well and I don't see much other 
> > > >>> way
> > > >>> to move forward and get rid of those ancient interface in the long 
> > > >>> term.
> > > >> Well kms had some really good benefits that drove quick adoption, like
> > > >> "suspend/resume actually has a chance of working" or "comes with
> > > >> buffer managem

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/28, Koenig, Christian wrote:
> Am 28.05.19 um 18:10 schrieb Emil Velikov:
> > On 2019/05/28, Daniel Vetter wrote:
> >> On Tue, May 28, 2019 at 10:03 AM Koenig, Christian
> >>  wrote:
> >>> Am 28.05.19 um 09:38 schrieb Daniel Vetter:
> >>>> [SNIP]
> >>>>> Might be a good idea looking into reverting it partially, so that at
> >>>>> least command submission and buffer allocation is still blocked.
> >>>> I thought the issue is a lot more than vainfo, it's pretty much every
> >>>> hacked up compositor under the sun getting this wrong one way or
> >>>> another. Thinking about this some more, I also have no idea how you'd
> >>>> want to deprecate rendering on primary nodes in general. Apparently
> >>>> that breaks -modesetting already, and probably lots more compositors.
> >>>> And it looks like we're finally achieve the goal kms set out to 10
> >>>> years ago, and new compositors are sprouting up all the time. I guess
> >>>> we could just break them all (on new hardware) and tell them to all
> >>>> suck it up. But I don't think that's a great option. And just
> >>>> deprecating this on amdgpu is going to be even harder, since then
> >>>> everywhere else it'll keep working, and it's just amdgpu.ko that looks
> >>>> broken.
> >>>>
> >>>> Aside: I'm not supporting Emil's idea here because it fixes any issues
> >>>> Intel has - Intel doesn't care. I support it because reality sucks,
> >>>> people get this render vs. primary vs. multi-gpu prime wrong all the
> >>>> time (that's also why we have hardcoded display+gpu pairs in mesa for
> >>>> the various soc combinations out there), and this looks like a
> >>>> pragmatic solution. It'd be nice if every compositor and everything
> >>>> else would perfectly support multi gpu and only use render nodes for
> >>>> rendering, and only primary nodes for display. But reality is that
> >>>> people hack on stuff until gears on screen and then move on to more
> >>>> interesting things (to them). So I don't think we'll ever win this :-/
> >>> Yeah, but this is a classic case of working around user space issues by
> >>> making kernel changes instead of fixing user space.
> >>>
> >>> Having privileged (output control) and unprivileged (rendering control)
> >>> functionality behind the same node is a mistake we have made a long time
> >>> ago and render nodes finally seemed to be a way to fix that.
> >>>
> >>> I mean why are compositors using the primary node in the first place?
> >>> Because they want to have access to privileged resources I think and in
> >>> this case it is perfectly ok to do so.
> >>>
> >>> Now extending unprivileged access to the primary node actually sounds
> >>> like a step into the wrong direction to me.
> >>>
> >>> I rather think that we should go down the route of completely dropping
> >>> command submission and buffer allocation through the primary node for
> >>> non master clients. And then as next step at some point drop support for
> >>> authentication/flink.
> >>>
> >>> I mean we have done this with UMS as well and I don't see much other way
> >>> to move forward and get rid of those ancient interface in the long term.
> >> Well kms had some really good benefits that drove quick adoption, like
> >> "suspend/resume actually has a chance of working" or "comes with
> >> buffer management so you can run multiple gears".
> >>
> >> The render node thing is a lot more niche use case (prime, better priv
> >> separation), plus "it's cleaner design". And the "cleaner design" part
> >> is something that empirically doesn't seem to matter :-/ Just two
> >> examples:
> >> - KHR_display/leases just iterated display resources on the fd needed
> >> for rendering (and iirc there was even a patch to expose that for
> >> render nodes too so it works with DRI3), because implementing
> >> protocols is too hard. Barely managed to stop that one before it
> >> happened.
> >> - Various video players use the vblank ioctl on directly to schedule
> >> frames, without telling the compositor. I discovered that when I
> >> wanted to limite the vblank ioctl to master clients only. Again,
> >> apparently too h

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/28, Daniel Vetter wrote:
> On Tue, May 28, 2019 at 10:03 AM Koenig, Christian
>  wrote:
> >
> > Am 28.05.19 um 09:38 schrieb Daniel Vetter:
> > > [SNIP]
> > >> Might be a good idea looking into reverting it partially, so that at
> > >> least command submission and buffer allocation is still blocked.
> > > I thought the issue is a lot more than vainfo, it's pretty much every
> > > hacked up compositor under the sun getting this wrong one way or
> > > another. Thinking about this some more, I also have no idea how you'd
> > > want to deprecate rendering on primary nodes in general. Apparently
> > > that breaks -modesetting already, and probably lots more compositors.
> > > And it looks like we're finally achieve the goal kms set out to 10
> > > years ago, and new compositors are sprouting up all the time. I guess
> > > we could just break them all (on new hardware) and tell them to all
> > > suck it up. But I don't think that's a great option. And just
> > > deprecating this on amdgpu is going to be even harder, since then
> > > everywhere else it'll keep working, and it's just amdgpu.ko that looks
> > > broken.
> > >
> > > Aside: I'm not supporting Emil's idea here because it fixes any issues
> > > Intel has - Intel doesn't care. I support it because reality sucks,
> > > people get this render vs. primary vs. multi-gpu prime wrong all the
> > > time (that's also why we have hardcoded display+gpu pairs in mesa for
> > > the various soc combinations out there), and this looks like a
> > > pragmatic solution. It'd be nice if every compositor and everything
> > > else would perfectly support multi gpu and only use render nodes for
> > > rendering, and only primary nodes for display. But reality is that
> > > people hack on stuff until gears on screen and then move on to more
> > > interesting things (to them). So I don't think we'll ever win this :-/
> >
> > Yeah, but this is a classic case of working around user space issues by
> > making kernel changes instead of fixing user space.
> >
> > Having privileged (output control) and unprivileged (rendering control)
> > functionality behind the same node is a mistake we have made a long time
> > ago and render nodes finally seemed to be a way to fix that.
> >
> > I mean why are compositors using the primary node in the first place?
> > Because they want to have access to privileged resources I think and in
> > this case it is perfectly ok to do so.
> >
> > Now extending unprivileged access to the primary node actually sounds
> > like a step into the wrong direction to me.
> >
> > I rather think that we should go down the route of completely dropping
> > command submission and buffer allocation through the primary node for
> > non master clients. And then as next step at some point drop support for
> > authentication/flink.
> >
> > I mean we have done this with UMS as well and I don't see much other way
> > to move forward and get rid of those ancient interface in the long term.
> 
> Well kms had some really good benefits that drove quick adoption, like
> "suspend/resume actually has a chance of working" or "comes with
> buffer management so you can run multiple gears".
> 
> The render node thing is a lot more niche use case (prime, better priv
> separation), plus "it's cleaner design". And the "cleaner design" part
> is something that empirically doesn't seem to matter :-/ Just two
> examples:
> - KHR_display/leases just iterated display resources on the fd needed
> for rendering (and iirc there was even a patch to expose that for
> render nodes too so it works with DRI3), because implementing
> protocols is too hard. Barely managed to stop that one before it
> happened.
> - Various video players use the vblank ioctl on directly to schedule
> frames, without telling the compositor. I discovered that when I
> wanted to limite the vblank ioctl to master clients only. Again,
> apparently too hard to use the existing extensions, or fix the bugs in
> there, or whatever. One userspace got fixed last year, but it'll
> probably get copypasted around forever :-/
> 
> So I don't think we'll ever manage to roll a clean split out, and best
> we can do is give in and just hand userspace what it wants. As much as
> that's misguided and unclean and all that. Maybe it'll result in a
> least fewer stuff getting run as root to hack around this, because
> fixing properly seems not to be on the table.
> 
> The beauty of kms is that we've achieved the mission, everyone's
> writing their own thing. Which is also terrible, and I don't think
> it'll get better.


With the risk of coming rude I will repeat my earlier comment:

The problem is _neither_ Intel nor libva specific.



That said, let's step back for a moment and consider:

 - the "block everything but KMS via the primary node" idea is great but
orthogonal

 - the series does address issues that are vendor-agnostic

 - by default this series does _not_ cause any regression be that for
new or old userspace

 - there 

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/27, Koenig, Christian wrote:
> Am 27.05.19 um 15:26 schrieb Emil Velikov:
> > On 2019/05/27, Daniel Vetter wrote:
> >> On Mon, May 27, 2019 at 10:47:39AM +, Koenig, Christian wrote:
> >>> Am 27.05.19 um 10:17 schrieb Emil Velikov:
> >>>> From: Emil Velikov 
> >>>>
> >>>> Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> >>>> render node. A seemingly deliberate design decision.
> >>>>
> >>>> Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> >>>> yet not all userspace checks if it's authenticated, but instead uses
> >>>> uncommon assumptions.
> >>>>
> >>>> After days of digging through git log and testing, only a single (ab)use
> >>>> was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> >>>> assuming that failure implies lack of authentication.
> >>>>
> >>>> Affected versions are:
> >>>>- the whole 18.2.x series, which is EOL
> >>>>- the whole 18.3.x series, which is EOL
> >>>>- the 19.0.x series, prior to 19.0.4
> >>> Well you could have saved your time, cause this is still a NAK.
> >>>
> >>> For the record: I strongly think that we don't want to expose any render
> >>> functionality on the primary node.
> >>>
> >>> To even go a step further I would say that at least on AMD hardware we
> >>> should completely disable DRI2 for one of the next generations.
> >>>
> >>> As a follow up I would then completely disallow the DRM authentication
> >>> for amdgpu, so that the command submission interface on the primary node
> >>> can only be used by the display server.
> >> So amdgpu is running in one direction, while everyone else is running in
> >> the other direction? Please note that your patch to change i915 landed
> >> already, so that ship is sailing (but we could ofc revert that back
> >> again).
> >>
> >> Imo really not a great idea if we do a amdgpu vs. everyone else split
> >> here. If we want to deprecate dri2/flink/rendering on primary nodes across
> >> the stack, then that should be a stack wide decision, not an amdgpu one.
> >>
> > Cannot agree more - I would love to see drivers stay consistent.
> 
> Yeah, completely agree to that. That's why I think we should not do this 
> at all and just let Intel fix it's userspace bugs :P
> 
Pretty sure I mentioned it before - might have been too subtle:

The problem is _neither_ Intel nor libva specific.


> Anyway my concern is really that we should stop extending functionality 
> on the primary node.
> 
> E.g. the render node is for use by the clients and the primary node for 
> mode setting and use by the display server only.
> 
Fully agreed. I'm not extending anything really. If anything - code is
removed from drivers and core :-)

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/27, Daniel Vetter wrote:
> On Mon, May 27, 2019 at 09:17:29AM +0100, Emil Velikov wrote:
> > From: Emil Velikov 
> > 
> > Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> > render node. A seemingly deliberate design decision.
> > 
> > Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> > yet not all userspace checks if it's authenticated, but instead uses
> > uncommon assumptions.
> > 
> > After days of digging through git log and testing, only a single (ab)use
> > was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> > assuming that failure implies lack of authentication.
> 
> Maybe correction here: Id does not care about authentication at all. It
> wants to figure out whether it has access to modeset resources, which is
> something entirely different, but happened to match in amdgpu's case.
> 
It feels like we have conflated the two (perhaps due to historical
reasons) and I'm not 100% sure which one the AMDGPU code inspects.

How about:

Hence we can drop the DRM_AUTH all together (details in follow-up patch)
yet that cause a regression in some userspace, when it conflates the
authentication status with access to modeset resources.

After days of digging through git log and testing, only a single (ab)use
was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl.

> > Affected versions are:
> >  - the whole 18.2.x series, which is EOL
> >  - the whole 18.3.x series, which is EOL
> >  - the 19.0.x series, prior to 19.0.4
> 
> Hm I think I'm blind, I'm not seeing the fix merged to mesa master. Still
> there on gitlab:
> 
> https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/amd/vulkan/radv_device.c#L291
> 
> What am I missing?
> 
Opted for a simple, more generic solution see commit
c962a78f18284e2971301bf68c9c60500ca398e4

This way, the same check is:
 - enforced where it's used
 - present for all Mesa Vulkan drivers


Will include the sha + commit title for v2.

> > Add a special quirk for that case, thus we can drop DRM_AUTH bits as
> > mentioned earlier.
> > 
> > Since all the affected userspace is EOL, we also add a kconfig option
> > to disable this quirk.
> > 
> > The whole approach is inspired by DRIVER_KMS_LEGACY_CONTEXT
> > 
> > Cc: Alex Deucher 
> > Cc: Christian König 
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: David Airlie 
> > Cc: Daniel Vetter 
> > Signed-off-by: Emil Velikov 
> 
> Aside from the nits I think reasonable approach.

Great, glad to hear. Now all we need is to bribe^Wconvince Christian.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/27, Daniel Vetter wrote:
> On Mon, May 27, 2019 at 10:47:39AM +, Koenig, Christian wrote:
> > Am 27.05.19 um 10:17 schrieb Emil Velikov:
> > > From: Emil Velikov 
> > >
> > > Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> > > render node. A seemingly deliberate design decision.
> > >
> > > Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> > > yet not all userspace checks if it's authenticated, but instead uses
> > > uncommon assumptions.
> > >
> > > After days of digging through git log and testing, only a single (ab)use
> > > was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> > > assuming that failure implies lack of authentication.
> > >
> > > Affected versions are:
> > >   - the whole 18.2.x series, which is EOL
> > >   - the whole 18.3.x series, which is EOL
> > >   - the 19.0.x series, prior to 19.0.4
> > 
> > Well you could have saved your time, cause this is still a NAK.
> > 
> > For the record: I strongly think that we don't want to expose any render 
> > functionality on the primary node.
> > 
> > To even go a step further I would say that at least on AMD hardware we 
> > should completely disable DRI2 for one of the next generations.
> > 
> > As a follow up I would then completely disallow the DRM authentication 
> > for amdgpu, so that the command submission interface on the primary node 
> > can only be used by the display server.
> 
> So amdgpu is running in one direction, while everyone else is running in
> the other direction? Please note that your patch to change i915 landed
> already, so that ship is sailing (but we could ofc revert that back
> again).
> 
> Imo really not a great idea if we do a amdgpu vs. everyone else split
> here. If we want to deprecate dri2/flink/rendering on primary nodes across
> the stack, then that should be a stack wide decision, not an amdgpu one.
> 
Cannot agree more - I would love to see drivers stay consistent.

Fwiw, this series consistently paves the way toward nuking DRM_AUTH ;-)

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/27, Koenig, Christian wrote:
> Am 27.05.19 um 14:05 schrieb Emil Velikov:
> > On 2019/05/27, Koenig, Christian wrote:
> >> Am 27.05.19 um 10:17 schrieb Emil Velikov:
> >>> From: Emil Velikov 
> >>>
> >>> Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> >>> render node. A seemingly deliberate design decision.
> >>>
> >>> Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> >>> yet not all userspace checks if it's authenticated, but instead uses
> >>> uncommon assumptions.
> >>>
> >>> After days of digging through git log and testing, only a eingle (ab)use
> >>> was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> >>> assuming that failure implies lack of authentication.
> >>>
> >>> Affected versions are:
> >>>- the whole 18.2.x series, which is EOL
> >>>- the whole 18.3.x series, which is EOL
> >>>- the 19.0.x series, prior to 19.0.4
> >> Well you could have saved your time, cause this is still a NAK.
> >>
> > Did I mention that I've spent quite a bit of time in AMDVLK? Even fixed
> > a bug while I was there :-)
> 
> Yeah, thanks for doing this.
> 
> But we have done so much stuff with customers which can't be audited 
> this way, that I still have a really bad feeling about this :/
> 
Fair point, I've already thought about those.

Example A:  customer X, using closed source/private software Y.
Ioctls A, B and C require the old behaviour - simply add FORCE_AUTH to
the A B C and carry on happily.

Example B: the team, as a whole thinks that this will be problematic for
customer X - add FORCE_AUTH to all ioctls and carry on.

I do see and understand why anyone can be hesitant about the series.

IMHO the above examples, illustrate quite reasonable compromise between
open-source and closed-source/private solutions.

Don't you agree?

> >> For the record: I strongly think that we don't want to expose any render
> >> functionality on the primary node.
> >>
> >> To even go a step further I would say that at least on AMD hardware we
> >> should completely disable DRI2 for one of the next generations.
> >>
> > It's doable and overall pretty neat idea.
> >
> > There is a consern that this approach may cause far more regressions
> > that this series. We are talking about years worth of Mesa drivers (et
> > al) that depend on render functionality exposed via the primary node.
> 
> Yeah, that's I'm perfectly aware of. It's the reason why I said we 
> should only do it for new hardware generations.
> 
> But at some point I think we should do this and get rid of 
> authentication/flink/DRI2/
> 
Fwiw I share a similar sentiment. If you've looked through the series,
this is effectively step 1, towards nuking DRM_AUTH :-)

> > I'm OK with writing the patches, but it'll be up-to the AMDGPU team to
> > follow-up with any regressions. Are you ok with that?
> 
> As long as we have a check like adev->family_id > 
> WHATEVER_IS_THE_CURRENT_LATEST_UPSTREAM_HW, I think we are fine with that.
> 
> If I understood Michel correctly xf86-video-amdgpu should work, but 
> modeset might break and needs a patch.
> 
Unless I have concrete WHATEVER_IS_THE... I cannot do much here :-(


Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/27, Koenig, Christian wrote:
> Am 27.05.19 um 10:17 schrieb Emil Velikov:
> > From: Emil Velikov 
> >
> > Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> > render node. A seemingly deliberate design decision.
> >
> > Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> > yet not all userspace checks if it's authenticated, but instead uses
> > uncommon assumptions.
> >
> > After days of digging through git log and testing, only a single (ab)use
> > was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> > assuming that failure implies lack of authentication.
> >
> > Affected versions are:
> >   - the whole 18.2.x series, which is EOL
> >   - the whole 18.3.x series, which is EOL
> >   - the 19.0.x series, prior to 19.0.4
> 
> Well you could have saved your time, cause this is still a NAK.
> 
Did I mention that I've spent quite a bit of time in AMDVLK? Even fixed 
a bug while I was there :-)

> For the record: I strongly think that we don't want to expose any render 
> functionality on the primary node.
> 
> To even go a step further I would say that at least on AMD hardware we 
> should completely disable DRI2 for one of the next generations.
>
It's doable and overall pretty neat idea.

There is a consern that this approach may cause far more regressions
that this series. We are talking about years worth of Mesa drivers (et
al) that depend on render functionality exposed via the primary node.

I'm OK with writing the patches, but it'll be up-to the AMDGPU team to
follow-up with any regressions. Are you ok with that?

Fwiw I could also move the FORCE_AUTH hack to core drm, if you prefer.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 02/13] drm/amdgpu: drop DRM_AUTH usage from the driver

[Emil Velikov]

From: Emil Velikov 

The authentication can be circumvented, by design, by using the render
node.

From the driver POV htere is no distinction between primary and render
nodes, thus we can drop the token.

Note: authentication is required on a single ioctl, due to a bug in
userspace. The issue has been fixed recently, but as an interim solution
we're using DRM_FORCE_AUTH for the ioctl.

Cc: Alex Deucher 
Cc: Christian König 
Cc: amd-gfx@lists.freedesktop.org
Cc: David Airlie 
Cc: Daniel Vetter 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 31 +
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index b8076929440b..6c2ba38a33ff 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1204,16 +1204,16 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, 
unsigned int pipe)
 }
 
 const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {
-   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_CREATE, amdgpu_gem_create_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_CTX, amdgpu_ctx_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_VM, amdgpu_vm_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_CREATE, amdgpu_gem_create_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_CTX, amdgpu_ctx_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_VM, amdgpu_vm_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(AMDGPU_SCHED, amdgpu_sched_ioctl, DRM_MASTER),
-   DRM_IOCTL_DEF_DRV(AMDGPU_BO_LIST, amdgpu_bo_list_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_FENCE_TO_HANDLE, 
amdgpu_cs_fence_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_BO_LIST, amdgpu_bo_list_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_FENCE_TO_HANDLE, 
amdgpu_cs_fence_to_handle_ioctl, DRM_RENDER_ALLOW),
/* KMS */
-   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_MMAP, amdgpu_gem_mmap_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_WAIT_IDLE, amdgpu_gem_wait_idle_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_CS, amdgpu_cs_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_MMAP, amdgpu_gem_mmap_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_WAIT_IDLE, amdgpu_gem_wait_idle_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_CS, amdgpu_cs_ioctl, DRM_RENDER_ALLOW),
/* The DRM_FORCE_AUTH is effectively a workaround for the RADV Mesa 
driver.
 * This is required for Mesa:
 *  - the whole 18.2.x series, which is EOL
@@ -1224,13 +1224,14 @@ const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {
 #if defined(DRM_AMDGPU_FORCE_AUTH)
DRM_FORCE_AUTH|
 #endif
-   DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_CS, amdgpu_cs_wait_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_FENCES, amdgpu_cs_wait_fences_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_METADATA, amdgpu_gem_metadata_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW)
+   DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_INFO, amdgpu_info_ioctl, 
DRM_FORCE_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_CS, amdgpu_cs_wait_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_FENCES, amdgpu_cs_wait_fences_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_METADATA, amdgpu_gem_metadata_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, 
DRM_RENDER_ALLOW)
 };
 const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
 
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

From: Emil Velikov 

Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
render node. A seemingly deliberate design decision.

Hence we can drop the DRM_AUTH all together (details in follow-up patch)
yet not all userspace checks if it's authenticated, but instead uses
uncommon assumptions.

After days of digging through git log and testing, only a single (ab)use
was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
assuming that failure implies lack of authentication.

Affected versions are:
 - the whole 18.2.x series, which is EOL
 - the whole 18.3.x series, which is EOL
 - the 19.0.x series, prior to 19.0.4

Add a special quirk for that case, thus we can drop DRM_AUTH bits as
mentioned earlier.

Since all the affected userspace is EOL, we also add a kconfig option
to disable this quirk.

The whole approach is inspired by DRIVER_KMS_LEGACY_CONTEXT

Cc: Alex Deucher 
Cc: Christian König 
Cc: amd-gfx@lists.freedesktop.org
Cc: David Airlie 
Cc: Daniel Vetter 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/amd/amdgpu/Kconfig  | 16 
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++-
 drivers/gpu/drm/drm_ioctl.c |  5 +
 include/drm/drm_ioctl.h | 17 +
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 9221e5489069..da415f445187 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -40,6 +40,22 @@ config DRM_AMDGPU_GART_DEBUGFS
  Selecting this option creates a debugfs file to inspect the mapped
  pages. Uses more memory for housekeeping, enable only for debugging.
 
+config DRM_AMDGPU_FORCE_AUTH
+   bool "Force authentication check on AMDGPU_INFO ioctl"
+   default y
+   help
+ There were some version of the Mesa RADV drivers, which relied on
+ the ioctl failing, if the client is not authenticated.
+
+ Namely, the following versions are affected:
+   - the whole 18.2.x series, which is EOL
+   - the whole 18.3.x series, which is EOL
+   - the 19.0.x series, prior to 19.0.4
+
+ Modern distributions, should disable this. That will allow various
+ other clients to work, that would otherwise require root privileges.
+
+
 source "drivers/gpu/drm/amd/acp/Kconfig"
 source "drivers/gpu/drm/amd/display/Kconfig"
 source "drivers/gpu/drm/amd/amdkfd/Kconfig"
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index b17d0545728e..b8076929440b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1214,7 +1214,17 @@ const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {
DRM_IOCTL_DEF_DRV(AMDGPU_GEM_MMAP, amdgpu_gem_mmap_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(AMDGPU_GEM_WAIT_IDLE, amdgpu_gem_wait_idle_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(AMDGPU_CS, amdgpu_cs_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(AMDGPU_INFO, amdgpu_info_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
+   /* The DRM_FORCE_AUTH is effectively a workaround for the RADV Mesa 
driver.
+* This is required for Mesa:
+*  - the whole 18.2.x series, which is EOL
+*  - the whole 18.3.x series, which is EOL
+*  - the 19.0.x series, prior to 19.0.4
+*/
+   DRM_IOCTL_DEF_DRV(AMDGPU_INFO, amdgpu_info_ioctl,
+#if defined(DRM_AMDGPU_FORCE_AUTH)
+   DRM_FORCE_AUTH|
+#endif
+   DRM_AUTH|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_CS, amdgpu_cs_wait_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(AMDGPU_WAIT_FENCES, amdgpu_cs_wait_fences_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(AMDGPU_GEM_METADATA, amdgpu_gem_metadata_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 2263e3ddd822..9841c0076f02 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -544,6 +544,11 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 drm_is_render_client(file_priv)))
return -EACCES;
 
+   /* FORCE_AUTH is only for authenticated or render client */
+   if (unlikely((flags & DRM_FORCE_AUTH) && 
!drm_is_render_client(file_priv) &&
+!file_priv->authenticated))
+   return -EACCES;
+
return 0;
 }
 EXPORT_SYMBOL(drm_ioctl_permit);
diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h
index fafb6f592c4b..6084ee32043d 100644
--- a/include/drm/drm_ioctl.h
+++ b/include/drm/drm_ioctl.h
@@ -126,6 +126,23 @@ enum drm_ioctl_flags {
 * not set DRM_AUTH because they do not require authentication.
 */
DRM_RENDE

[PATCH 10/13] drm/radeon: drop DRM_AUTH from DRM_RENDER_ALLOW ioctls

[Emil Velikov]

From: Emil Velikov 

The authentication can be circumvented, by design, by using the render
node.

From the driver POV there is no distinction between primary and render
nodes, thus we can drop the token.

Note: the outstanding DRM_AUTH instances are:
 - legacy DRI1 ioctls, which are already neutered
 - modern but deprecated ioctls

Cc: Alex Deucher 
Cc: Christian König 
Cc: amd-gfx@lists.freedesktop.org
Cc: David Airlie 
Cc: Daniel Vetter 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/radeon/radeon_kms.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c 
b/drivers/gpu/drm/radeon/radeon_kms.c
index 6a8fb6fd183c..c010b8a88b8a 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -905,20 +905,20 @@ const struct drm_ioctl_desc radeon_ioctls_kms[] = {
DRM_IOCTL_DEF_DRV(RADEON_SURF_ALLOC, drm_invalid_op, DRM_AUTH),
DRM_IOCTL_DEF_DRV(RADEON_SURF_FREE, drm_invalid_op, DRM_AUTH),
/* KMS */
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_INFO, radeon_gem_info_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_CREATE, radeon_gem_create_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_MMAP, radeon_gem_mmap_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_SET_DOMAIN, radeon_gem_set_domain_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_INFO, radeon_gem_info_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_CREATE, radeon_gem_create_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_MMAP, radeon_gem_mmap_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_SET_DOMAIN, radeon_gem_set_domain_ioctl, 
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(RADEON_GEM_PREAD, radeon_gem_pread_ioctl, DRM_AUTH),
DRM_IOCTL_DEF_DRV(RADEON_GEM_PWRITE, radeon_gem_pwrite_ioctl, DRM_AUTH),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_WAIT_IDLE, radeon_gem_wait_idle_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_CS, radeon_cs_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_INFO, radeon_info_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_SET_TILING, radeon_gem_set_tiling_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_GET_TILING, radeon_gem_get_tiling_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_BUSY, radeon_gem_busy_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_VA, radeon_gem_va_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_OP, radeon_gem_op_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(RADEON_GEM_USERPTR, radeon_gem_userptr_ioctl, 
DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_WAIT_IDLE, radeon_gem_wait_idle_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_CS, radeon_cs_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_INFO, radeon_info_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_SET_TILING, radeon_gem_set_tiling_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_GET_TILING, radeon_gem_get_tiling_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_BUSY, radeon_gem_busy_ioctl, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_VA, radeon_gem_va_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_OP, radeon_gem_op_ioctl, DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(RADEON_GEM_USERPTR, radeon_gem_userptr_ioctl, 
DRM_RENDER_ALLOW),
 };
 int radeon_max_kms_ioctl = ARRAY_SIZE(radeon_ioctls_kms);
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Fix issue about differrent domainID but same BDF

[Emil Velikov]

On Mon, 25 Feb 2019 at 19:53, Deucher, Alexander
 wrote:
>
> > -Original Message-
> > From: amd-gfx  On Behalf Of Emil
> > Velikov
> > Sent: Monday, February 25, 2019 8:09 AM
> > To: Alex Deucher 
> > Cc: Deng, Emily ; Maling list - DRI developers  > de...@lists.freedesktop.org>; amd-gfx list 
> > Subject: Re: [PATCH libdrm] libdrm: Fix issue about differrent domainID but
> > same BDF
> >
> > Hi all,
> >
> > This patch causes unnecessary round trip by openning the nodes. As
> > mentioned previously this could be trivially fixed [1].
> >
> > Even Emily acknowledged that [1], yet the sub-par fix was merged. Can we
> > revert+fixup this properly?
> >
>
> Sorry, I totally missed your reply.  I'm having Internet issues at the moment 
> so if you want to revert for now, I'll work with Emily to address your 
> suggestions later in the week or next.  Emily, can you take a look at 
> addressing Emil's concerns with an updated patch?
>

Doesn't seem like there's any follow-up work on this, so I've reverted
this for now.
I'm a sad panda for doing it - hopefully v2 will come shortly.


-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Fix issue about differrent domainID but same BDF

[Emil Velikov]

Hi all,

This patch causes unnecessary round trip by openning the nodes. As
mentioned previously this could be trivially fixed [1].

Even Emily acknowledged that [1], yet the sub-par fix was merged. Can
we revert+fixup this properly?

Thanks
Emil

[1] https://lists.freedesktop.org/archives/amd-gfx/2019-February/031573.html

On Fri, 22 Feb 2019 at 21:05, Alex Deucher  wrote:
>
> Pushed.  Thanks!
>
> Alex
>
> On Thu, Feb 21, 2019 at 9:36 PM Deng, Emily  wrote:
> >
> > Hi Alex,
> > Please help, thanks.
> >
> > Best wishes
> > Emily Deng
> >
> >
> >
> > >-Original Message-
> > >From: Alex Deucher 
> > >Sent: Friday, February 22, 2019 12:13 AM
> > >To: Deng, Emily ; Maling list - DRI developers  > >de...@lists.freedesktop.org>
> > >Cc: amd-gfx list 
> > >Subject: Re: [PATCH libdrm] libdrm: Fix issue about differrent domainID 
> > >but same
> > >BDF
> > >
> > >On Thu, Feb 14, 2019 at 2:53 AM Emily Deng  wrote:
> > >>
> > >> For multiple GPUs which has the same BDF, but has different domain ID,
> > >> the drmOpenByBusid will return the wrong fd when startx.
> > >>
> > >> The reproduce sequence as below:
> > >> 1. Call drmOpenByBusid to open Card0, then will return the right fd0,
> > >> and the
> > >> fd0 is master privilege;
> > >> 2. Call drmOpenByBusid to open Card1. In function drmOpenByBusid, it
> > >> will open Card0 first, this time, the fd1 for opening Card0 is not
> > >> master privilege, and will call drmSetInterfaceVersion to identify the
> > >> domain ID feature, as the fd1 is not master privilege, then
> > >> drmSetInterfaceVersion will fail, and then won't compare domain ID, then
> > >return the wrong fd for Card1.
> > >>
> > >> Solution:
> > >> First loop search the best match fd about drm 1.4.
> > >>
> > >> Signed-off-by: Emily Deng 
> > >
> > >Reviewed-by: Alex Deucher 
> > >
> > >Do you need someone to commit this for you?
> > >
> > >Alex
> > >
> > >> ---
> > >>  xf86drm.c | 23 +++
> > >>  1 file changed, 23 insertions(+)
> > >>
> > >> diff --git a/xf86drm.c b/xf86drm.c
> > >> index 336d64d..b60e029 100644
> > >> --- a/xf86drm.c
> > >> +++ b/xf86drm.c
> > >> @@ -584,11 +584,34 @@ static int drmOpenByBusid(const char *busid, int
> > >type)
> > >>  if (base < 0)
> > >>  return -1;
> > >>
> > >> +/* We need to try for 1.4 first for proper PCI domain support */
> > >>  drmMsg("drmOpenByBusid: Searching for BusID %s\n", busid);
> > >>  for (i = base; i < base + DRM_MAX_MINOR; i++) {
> > >>  fd = drmOpenMinor(i, 1, type);
> > >>  drmMsg("drmOpenByBusid: drmOpenMinor returns %d\n", fd);
> > >>  if (fd >= 0) {
> > >> +sv.drm_di_major = 1;
> > >> +sv.drm_di_minor = 4;
> > >> +sv.drm_dd_major = -1;/* Don't care */
> > >> +sv.drm_dd_minor = -1;/* Don't care */
> > >> +if (!drmSetInterfaceVersion(fd, )) {
> > >> +buf = drmGetBusid(fd);
> > >> +drmMsg("drmOpenByBusid: drmGetBusid reports %s\n", buf);
> > >> +if (buf && drmMatchBusID(buf, busid, 1)) {
> > >> +drmFreeBusid(buf);
> > >> +return fd;
> > >> +}
> > >> +if (buf)
> > >> +drmFreeBusid(buf);
> > >> +}
> > >> +close(fd);
> > >> +}
> > >> +}
> > >> +
> > >> +   for (i = base; i < base + DRM_MAX_MINOR; i++) {
> > >> +fd = drmOpenMinor(i, 1, type);
> > >> +drmMsg("drmOpenByBusid: drmOpenMinor returns %d\n", fd);
> > >> +if (fd >= 0) {
> > >>  /* We need to try for 1.4 first for proper PCI domain 
> > >> support
> > >>   * and if that fails, we know the kernel is busted
> > >>   */
> > >> --
> > >> 2.7.4
> > >>
> > >> ___
> > >> amd-gfx mailing list
> > >> amd-gfx@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Fix issue about differrent domainID but same BDF

[Emil Velikov via amd-gfx]

Hi Emily,

Please note that code outside of amdgpu/ is used by all open source drivers.
Thus patches should have dri-deve@ in to/cc as mentioned in CONTRIBUTING

On Thu, 14 Feb 2019 at 07:53, Emily Deng  wrote:
>
> For multiple GPUs which has the same BDF, but has different domain ID,
> the drmOpenByBusid will return the wrong fd when startx.
>
> The reproduce sequence as below:
> 1. Call drmOpenByBusid to open Card0, then will return the right fd0, and the
> fd0 is master privilege;
> 2. Call drmOpenByBusid to open Card1. In function drmOpenByBusid, it will
> open Card0 first, this time, the fd1 for opening Card0 is not master
> privilege, and will call drmSetInterfaceVersion to identify the
> domain ID feature, as the fd1 is not master privilege, then 
> drmSetInterfaceVersion
> will fail, and then won't compare domain ID, then return the wrong fd for 
> Card1.
>
> Solution:
> First loop search the best match fd about drm 1.4.
>
First and foremost, I wish we can stop using using these legacy APIs.
They're fairly fragile and as you can see the are strange things
happening.
We could instead use drmGetDevices2() to gather a list of devices and
pick the one we're interested.

That aside, I think we can do a slightly better fix. Have you tried:
 - resetting the pci_domain_ok=1 on each iteration, and
 - continuing to the next device when the second
drmSetInterfaceVersion() call fails

AFAICT it should produce the same result, while being shorter and faster.

Thanks
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/26] drm/irq: Don't check for DRIVER_HAVE_IRQ in drm_irq_(un)install

[Emil Velikov]

On Thu, 24 Jan 2019 at 16:58, Daniel Vetter  wrote:
>
> If a non-legacy driver calls these it's valid to assume there is
> interrupt support. The flag is really only needed for legacy drivers.

... legacy drivers which issue the IRQ via the DRM_IOCTL_CONTROL legacy IOCTL.

At a later stage, we might as well add LEGACY to the name:
DRIVER_LEGACY_HAVE_IRQ?


>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 2 +-
>  drivers/gpu/drm/arm/hdlcd_drv.c  | 2 +-
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
>  drivers/gpu/drm/drm_irq.c| 6 --
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 2 +-
>  drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>  drivers/gpu/drm/i915/i915_drv.c  | 2 +-
>  drivers/gpu/drm/meson/meson_drv.c| 2 +-
>  drivers/gpu/drm/msm/msm_drv.c| 3 +--
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c| 3 +--
>  drivers/gpu/drm/qxl/qxl_drv.c| 2 +-
>  drivers/gpu/drm/radeon/radeon_drv.c  | 2 +-
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c | 2 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 2 +-
>  drivers/gpu/drm/vc4/vc4_drv.c| 1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c  | 2 +-
>  drivers/staging/vboxvideo/vbox_drv.c | 2 +-

Local grep shows one more non-legacy entry in
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c

With that file addressed and trivial comment additions, patches: 1, 2,
3 and 26 are
Reviewed-by: Emil Velikov 

HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 1/5] [libdrm] new syncobj extension

[Emil Velikov]

Hi Chunming Zhou,

On Fri, 2 Nov 2018 at 08:27, Chunming Zhou  wrote:
>
> Signed-off-by: Chunming Zhou 
> ---
>  include/drm/drm.h | 38 ++

Please read through include/drm/README about how include/drm/ should be updated.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Remove dead static variable

[Emil Velikov]

On Mon, 19 Nov 2018 at 11:19, Christian König
 wrote:
>
> Am 19.11.18 um 12:07 schrieb Rex Zhu:
> > The static struct drm_driver *driver was
> > not used because drm_pci_init was deprecated
> >
> > Signed-off-by: Rex Zhu 
>
> Reviewed-by: Christian König 
>
> Can you of hand see what "pdriver" is used for? That looks suspicious
> like something deprecated as well.
>
Seeming copy/paste from the radeon driver. The latter used to support
UMS and KMS at some point in the past.

HTH
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Emil Velikov]

On Sun, 30 Sep 2018 at 18:31, Thomas Hellstrom  wrote:
>
> Hi, Emil,
>
> On 09/05/2018 03:53 PM, Emil Velikov wrote:
> > On 5 September 2018 at 14:20, Thomas Hellstrom  
> > wrote:
> >
> >>> In that case, please give me 24h to do a libdrm release before pushing.
> >>> I had to push some workarounds for the sandboxing mentioned earlier :-\
> >>>
> >>> Thanks
> >>> Emil
> >>
> >> Ouch, I just pushed the patch, but feel free to cut the release just before
> >> that commit.
> >>
> > That doesn't quite work. Barring any objections I'll: revert, release, 
> > reapply.
> >
> > -Emil
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> What happened here? I can't really see my commit nor a revert nor a
> release in libdrm.
>
Coming back from holidays+XDC. I' m doing a release in a moment and
will pick your patch just after that.

Hmm you said you pushed the patch, yet it's not in master ... Not sure
what happened there.
Either way - it'll be there shortly.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 2/5] [libdrm] addr cs chunk for syncobj timeline

[Emil Velikov]

On Wed, 19 Sep 2018 at 10:31, Chunming Zhou  wrote:
>
> Signed-off-by: Chunming Zhou 
> ---
>  include/drm/amdgpu_drm.h | 10 ++
>  1 file changed, 10 insertions(+)
>
For this and 1/5 - see include/drm/README and git log for examples how
files in include/drm should be updated.
I'll pull merge some patches in a few moments so make sure you've got
latest master first.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Emil Velikov]

On 5 September 2018 at 14:20, Thomas Hellstrom  wrote:

>> In that case, please give me 24h to do a libdrm release before pushing.
>> I had to push some workarounds for the sandboxing mentioned earlier :-\
>>
>> Thanks
>> Emil
>
>
> Ouch, I just pushed the patch, but feel free to cut the release just before
> that commit.
>
That doesn't quite work. Barring any objections I'll: revert, release, reapply.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Emil Velikov]

On 5 September 2018 at 11:10, Thomas Hellstrom  wrote:
> Hi, Emil,
>
>
> On 09/05/2018 11:33 AM, Emil Velikov wrote:
>>
>> On 4 September 2018 at 23:33, Dave Airlie  wrote:
>>>
>>> On Tue, 4 Sep 2018 at 03:00, Thomas Hellstrom 
>>> wrote:
>>>>
>>>> On 09/03/2018 06:33 PM, Daniel Vetter wrote:
>>>>>
>>>>> On Mon, Sep 03, 2018 at 11:16:29AM +0200, Thomas Hellstrom wrote:
>>>>>>
>>>>>> On 08/31/2018 05:30 PM, Thomas Hellstrom wrote:
>>>>>>>
>>>>>>> On 08/31/2018 05:27 PM, Emil Velikov wrote:
>>>>>>>>
>>>>>>>> On 31 August 2018 at 15:38, Michel Dänzer 
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> [ Adding the amd-gfx list ]
>>>>>>>>>
>>>>>>>>> On 2018-08-31 3:05 p.m., Thomas Hellstrom wrote:
>>>>>>>>>>
>>>>>>>>>> On 08/31/2018 02:30 PM, Emil Velikov wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 31 August 2018 at 12:54, Thomas Hellstrom
>>>>>>>>>>> 
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> To determine whether a device node is a drm device
>>>>>>>>>>>> node or not, the code
>>>>>>>>>>>> currently compares the node's major number to the static drm
>>>>>>>>>>>> major
>>>>>>>>>>>> device
>>>>>>>>>>>> number.
>>>>>>>>>>>>
>>>>>>>>>>>> This breaks the standalone vmwgfx driver on XWayland dri
>>>>>>>>>>>> clients,
>>>>>>>>>>>>
>>>>>>>>>>> Any particular reason why the code doesn't use a fixed node
>>>>>>>>>>> there?
>>>>>>>>>>> It will make the diff vs the in-kernel driver a bit smaller.
>>>>>>>>>>
>>>>>>>>>> Because then it won't be able to interoperate with other in-tree
>>>>>>>>>> drivers, like virtual drm drivers or passthrough usb drm drivers.
>>>>>>>>>> There is no clean way to share the minor number allocation
>>>>>>>>>> with in-tree
>>>>>>>>>> drm, so standalone vmwgfx is using dynamic major allocation.
>>>>>>>>>
>>>>>>>>> I wonder why I haven't heard of any of these issues with the
>>>>>>>>> standalone
>>>>>>>>> version of amdgpu shipped in packaged AMD releases. Does that
>>>>>>>>> also use a
>>>>>>>>> different major number? If yes, maybe it's just that nobody has
>>>>>>>>> tried
>>>>>>>>> Xwayland clients with that driver. If no, how does it avoid the
>>>>>>>>> other
>>>>>>>>> issues described above?
>>>>>>>>>
>>>>>>>> AFAICT, the difference is that the standalone vmwgfx uses an
>>>>>>>> internal
>>>>>>>> copy of drm core.
>>>>>>>> It doesn't reuse the in-kernel drm, hence it cannot know which minor
>>>>>>>> it can use.
>>>>>>>>
>>>>>>>> -Emil
>>>>>>>
>>>>>>> Actually, standalone vmwgfx could perhaps also try to allocate minors
>>>>>>> from 63 and downwards. That might work, but needs some verification.
>>>>>>>
>>>>>> So unfortuntately this doesn't work since the in-tree drm's file
>>>>>> operations
>>>>>> are registered with the DRM_MAJOR.
>>>>>> So I still think the patch is the way to go. If people are concerned
>>>>>> that
>>>>>> also fbdev file descriptors are allowed, perhaps there are other sysfs
>>>>>> traits we can look at?
>>>>>
>>>>> Somewhat out of curiosity, but why do you have to overwrite all of drm?
>>>>> amdgpu seems to be able to pull their stunt off without ...
>>>>> -Daniel
>>>>
>>>> At the time we launched the standalone vmwgfx, the DRM <-> driver
>>>> interface was moving considerably more rapidly than the DRM <-> kernel
>>>> interface. I think that's still the case. Hence less work for us. Also
>>>> meant we can install the full driver stack with latest features on
>>>> fairly old VMs without backported DRM functionality.
>>>>
>>> I think this should be fine for 99% of drm usage, there may be corner
>>> cases in wierd places, but I can't point to any that really matter
>>> (maybe strace?)
>>>
>> Having a closer look, I think this will break the Firefox/Chrome
>> sandboxing :-\
>> I cannot see the path /sys/dev/char/%d:%d/device/drm in the allowed
>> list [1] [2].
>
> Thanks for pointing this out.
>
> The function drmGetMinorNameForFD() already opens this path, so any
> user-space using that function would not work before either.
>
> Also mozilla/firefox adds /sys/dev/char/226:* Which means that while it
> still won't work on standalone vmwgfx, there should at least be no
> regression.
>
> For Chromium it seems they allow /sys/dev/char/ for AmdGpu, but only under
> ChromOS, so I'll ping those to be safe.
>
> I also won't be doing an immediate release after pushing.
>
In that case, please give me 24h to do a libdrm release before pushing.
I had to push some workarounds for the sandboxing mentioned earlier :-\

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Emil Velikov]

On 4 September 2018 at 23:33, Dave Airlie  wrote:
> On Tue, 4 Sep 2018 at 03:00, Thomas Hellstrom  wrote:
>>
>> On 09/03/2018 06:33 PM, Daniel Vetter wrote:
>> > On Mon, Sep 03, 2018 at 11:16:29AM +0200, Thomas Hellstrom wrote:
>> >> On 08/31/2018 05:30 PM, Thomas Hellstrom wrote:
>> >>> On 08/31/2018 05:27 PM, Emil Velikov wrote:
>> >>>> On 31 August 2018 at 15:38, Michel Dänzer  wrote:
>> >>>>> [ Adding the amd-gfx list ]
>> >>>>>
>> >>>>> On 2018-08-31 3:05 p.m., Thomas Hellstrom wrote:
>> >>>>>> On 08/31/2018 02:30 PM, Emil Velikov wrote:
>> >>>>>>> On 31 August 2018 at 12:54, Thomas Hellstrom 
>> >>>>>>> wrote:
>> >>>>>>>> To determine whether a device node is a drm device
>> >>>>>>>> node or not, the code
>> >>>>>>>> currently compares the node's major number to the static drm major
>> >>>>>>>> device
>> >>>>>>>> number.
>> >>>>>>>>
>> >>>>>>>> This breaks the standalone vmwgfx driver on XWayland dri clients,
>> >>>>>>>>
>> >>>>>>> Any particular reason why the code doesn't use a fixed node there?
>> >>>>>>> It will make the diff vs the in-kernel driver a bit smaller.
>> >>>>>> Because then it won't be able to interoperate with other in-tree
>> >>>>>> drivers, like virtual drm drivers or passthrough usb drm drivers.
>> >>>>>> There is no clean way to share the minor number allocation
>> >>>>>> with in-tree
>> >>>>>> drm, so standalone vmwgfx is using dynamic major allocation.
>> >>>>> I wonder why I haven't heard of any of these issues with the standalone
>> >>>>> version of amdgpu shipped in packaged AMD releases. Does that
>> >>>>> also use a
>> >>>>> different major number? If yes, maybe it's just that nobody has tried
>> >>>>> Xwayland clients with that driver. If no, how does it avoid the other
>> >>>>> issues described above?
>> >>>>>
>> >>>> AFAICT, the difference is that the standalone vmwgfx uses an internal
>> >>>> copy of drm core.
>> >>>> It doesn't reuse the in-kernel drm, hence it cannot know which minor
>> >>>> it can use.
>> >>>>
>> >>>> -Emil
>> >>> Actually, standalone vmwgfx could perhaps also try to allocate minors
>> >>> from 63 and downwards. That might work, but needs some verification.
>> >>>
>> >> So unfortuntately this doesn't work since the in-tree drm's file 
>> >> operations
>> >> are registered with the DRM_MAJOR.
>> >> So I still think the patch is the way to go. If people are concerned that
>> >> also fbdev file descriptors are allowed, perhaps there are other sysfs
>> >> traits we can look at?
>> > Somewhat out of curiosity, but why do you have to overwrite all of drm?
>> > amdgpu seems to be able to pull their stunt off without ...
>> > -Daniel
>>
>> At the time we launched the standalone vmwgfx, the DRM <-> driver
>> interface was moving considerably more rapidly than the DRM <-> kernel
>> interface. I think that's still the case. Hence less work for us. Also
>> meant we can install the full driver stack with latest features on
>> fairly old VMs without backported DRM functionality.
>>
>
> I think this should be fine for 99% of drm usage, there may be corner
> cases in wierd places, but I can't point to any that really matter
> (maybe strace?)
>
Having a closer look, I think this will break the Firefox/Chrome sandboxing :-\
I cannot see the path /sys/dev/char/%d:%d/device/drm in the allowed
list [1] [2].

Thomas, can you please send a patch to the respective teams or give
them a heads up?

Thanks
Emil

[1] 
https://hg.mozilla.org/releases/mozilla-beta/file/264fcd3206a6/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
[2] 
https://chromium.googlesource.com/chromium/src/+/8655d49f657d3878c937f1387b3d31fa66c8e76a/content/gpu/gpu_sandbox_hook_linux.cc
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Emil Velikov]

On 31 August 2018 at 15:38, Michel Dänzer  wrote:
>
> [ Adding the amd-gfx list ]
>
> On 2018-08-31 3:05 p.m., Thomas Hellstrom wrote:
>> On 08/31/2018 02:30 PM, Emil Velikov wrote:
>>> On 31 August 2018 at 12:54, Thomas Hellstrom 
>>> wrote:
>>>> To determine whether a device node is a drm device node or not, the code
>>>> currently compares the node's major number to the static drm major
>>>> device
>>>> number.
>>>>
>>>> This breaks the standalone vmwgfx driver on XWayland dri clients,
>>>>
>>> Any particular reason why the code doesn't use a fixed node there?
>>> It will make the diff vs the in-kernel driver a bit smaller.
>> Because then it won't be able to interoperate with other in-tree
>> drivers, like virtual drm drivers or passthrough usb drm drivers.
>> There is no clean way to share the minor number allocation with in-tree
>> drm, so standalone vmwgfx is using dynamic major allocation.
>
> I wonder why I haven't heard of any of these issues with the standalone
> version of amdgpu shipped in packaged AMD releases. Does that also use a
> different major number? If yes, maybe it's just that nobody has tried
> Xwayland clients with that driver. If no, how does it avoid the other
> issues described above?
>
AFAICT, the difference is that the standalone vmwgfx uses an internal
copy of drm core.
It doesn't reuse the in-kernel drm, hence it cannot know which minor it can use.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: RFC: Migration to Gitlab

[Emil Velikov]

Hi Dan,

On 22 August 2018 at 12:44, Daniel Vetter  wrote:
> Hi all,
>
> I think it's time to brainstorm a bit about the gitlab migration. Basic 
> reasons:
>
> - fd.o admins want to deprecate shell accounts and hand-rolled
> infrastructure, because it's a pain to keep secure
>
> - gitlab will allow us to add committers on our own, greatly
> simplifying that process (and offloading that task from fd.o admins).
>
Random thought - I really wish the admins spoke early and louder about issues.
From infra to manpower and adhoc tool maintenance.

> There's also some more benefits we might want to reap, like better CI
> integration for basic build testing - no more "oops didn't build
> drm-misc defconfigs" or "sry, forgot make check in maintainer-tools".
> But that's all fully optional.
>
> For the full in-depth writeup of everything, see
>
> https://www.fooishbar.org/blog/gitlab-fdo-introduction/
>
> I think now is also a good time, with mesa, xorg, wayland/weston and
> others moved, to start thinking about how we'll move drm. There's a
> few things to figure out though:
>
> - We probably want to split out maintainer-tools. That would address
> the concern that there's 50+ committers to an auto-updating shell
> script ...
>
> - We need to figure out how to handle the ACL trickery around drm-tip in 
> gitlab.
>
> - Probably good to stage the migration, with maintainer-tools, igt
> leading. That will also make fd.o admins happy, who want to rework
> their cloud infrastructure a bit before migrating the big kernel repos
> over.
>
> - Figuring out the actual migration - we've been adding a pile of
> committers since fd.o LDAP was converted to gitlab once back in
> spring. We need to at least figure out how to move the new
> accounts/committers.
>
As a observer, allow me to put some ideas. You've mostly covered them
all, my emphasis is to seriously stick with _one_ thing at a time.
Attempting to do multiple things in parallel will end up with
sub-optimal results.

 - (at random point) cleanup the committers list - people who have not
contributed in the last 1 year?
 - setup drm group, copy/migrate accounts - one could even reuse the
existing credentials
 - move git repos to gitlab, the push URL change, cgit mirror
preserves the normal fetch ones as well as PW hooks
 - work out how new accounts are handled - still in bugzilla, otherwise

At this stage only workflow change is a) once-off account setup and b)
pushURL update
As a follow-up one can setup anything fancy.
 - migrate PW/other hooks
 - migrate bugs - if applicable
 - add new hooks - DRM docs, other
 - etc


> - Similar, maintainer-tools needs to move. We probably want to move
> all the dim maintained kernel repos in one go, to avoid headaches with
> double-accounts needed for committers.
>
One should be able to create a separate repo for these. And then either:
 - one by one add the required features into the gitlab MR machinery,
 - or, wire the execution in pre/post merge stage.

IIRC there are some upstream requests about the former.

> - CI, linux-next and everyone else should be fine, since the
> cgit/non-ssh paths will keep working (they'll be read-only mirrors).
> Need to double-check that with everyone.
>
> - Some organization structure would be good.
>
> https://cgit.freedesktop.org/drm
>
> libdrm won't be part of the gitlab drm group because that's already
> moved under mesa (and you can't symlink/mulit-home anymore on gitlab):
>
> https://gitlab.freedesktop.org/mesa/drm
>
> But there's also drm_hwcomposer, which we might want to migrate into
> drm too - gitlab requires a containing group, and
> drm_hwcomposer/drm_hwcomposer is a bit silly.
>
It did strike me a lot when drm_hwcomposer/drm_hwcomposer was
introduced. Fortunately moving repos in gitlab is reasonably pain-free


HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 03/19] Use ODEV_ATTRIB_PATH where possible for the device node.

[Emil Velikov]

On 10 April 2018 at 09:27, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>> ---
>>  src/amdgpu_probe.c | 16 ++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index e65c83b..78cc005 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -33,6 +33,8 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>>
>>  /*
>>   * Authors:
>> @@ -117,18 +119,28 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>>struct xf86_platform_device *platform_dev)
>>  {
>>   struct pci_device *dev;
>> + const char *path;
>>   char *busid;
>>   int fd;
>>
>> -#ifdef ODEV_ATTRIB_FD
>>   if (platform_dev) {
>> +#ifdef ODEV_ATTRIB_FD
>>   fd = xf86_get_platform_device_int_attrib(platform_dev,
>>ODEV_ATTRIB_FD, -1);
>>   if (fd != -1)
>>   return fd;
>> - }
>>  #endif
>>
>> +#ifdef ODEV_ATTRIB_PATH
>
> This guard is superfluous: ODEV_ATTRIB_PATH was added in xserver 1.13,
> and we require >= 1.13.
>
Was respinning the patches and noticed that the guard is needed. Namely:

The ODEV_ATTRIB_FD macro is set in xf86platformBus.h which is included
only as XSERVER_PLATFORM_BUS is set.
We can use either macro as a guard, yet the former seems more natural/obvious.

What do you think?

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Emil Velikov]

On 10 April 2018 at 10:27, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-10 11:20 AM, Emil Velikov wrote:
>> On 10 April 2018 at 09:26, Michel Dänzer <mic...@daenzer.net> wrote:
>>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>>> From: Emil Velikov <emil.veli...@collabora.com>
>>>>
>>>> The former of these is a UMS artefact which gives incorrect and
>>>> misleading promise whether "KMS" is supported. Not to mention that
>>>> AMDGPU is a only KMS driver.
>>>>
>>>> In a similar fashion xf86LoadKernelModule() is a relic of the times,
>>>> where platforms had no scheme of detecting and loading the appropriate
>>>> kernel module.
>>>>
>>>> Cc: Robert Millan <r...@freebsd.org>
>>>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>>>> ---
>>>> Robert, off the top of my head this should work with FreeBSD. Admittedly
>>>> I'm not an expert on the platform. Please give it a test.
>>>
>>> I want to get confirmation from Robert that this will work on FreeBSD
>>> now, since he explicitly restored the kernel module loading code in
>>> https://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=bfbff3b246db509c820df17b8fcf5899882ffcfa
>>> .
>>>
>> Fully agreed!. That's why I added him to the CC list.
>>
>> Throwing some ideas:
>>  - If it's still needed can we keep it !Linux only?
>
> The first drmCheckModesettingSupported call? Fine with me.
>
Since ifndef out the drmCheckModesettingSupported call makes
amdgpu_kernel_mode_enabled function a no-op, I was thinking about
ifndef the whole thing.
Yet again, it would be so much better to actually nuke it (if possible).

Robert, does one still need xf86LoadKernelModule and/or
drmCheckModesettingSupported for FreeBSD?

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

On 10 April 2018 at 10:58, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-10 11:47 AM, Emil Velikov wrote:
>> On 10 April 2018 at 09:28, Michel Dänzer <mic...@daenzer.net> wrote:
>>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>>> From: Emil Velikov <emil.veli...@collabora.com>
>>>>
>>>> Seems like we've been leaking this for years. It became more obvious
>>>> with the recent refactoring.
>>>>
>>>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>>>> ---
>>>>  src/amdgpu_probe.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>>> index 537d44c..588891c 100644
>>>> --- a/src/amdgpu_probe.c
>>>> +++ b/src/amdgpu_probe.c
>>>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>>>   return TRUE;
>>>>
>>>>  error:
>>>> + free(pPriv->ptr);
>>>> + pPriv->ptr = NULL;
>>>>   return FALSE;
>>>>  }
>>>>
>>>>
>>>
>>> valgrind doesn't report a leak if I force this error path; presumably
>>> Xorg frees the private after returning FALSE here.
>>>
>> Just double-checked and Xorg does not know anything about ptr. The
>> only one who clears it up is AMDGPUFreeScreen_KMS.
>>
>> The magic (for this and the other 'leak') seems to be happening in
>> xf86platformAddDevice. Namely:
>>  - ::platformProbe is called via doPlatformProbe
>>  - the driver explicitly calls xf86AllocateScreen, yet fails later on
>>  - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
>>  - ::PreInit fails, ::configured is false
>>  - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
>> AMDGPUFreeScreen_KMS)
>>
>> Eventually, I could unwrap all that although it makes sense to keep
>> things simpler. As effectively done by the patch.
>>
>> I believe you'll agree?
>
> I'm afraid not. There's no leak because it's getting cleaned up as
> designed, so there's no need for this change.
>
Fair enough. I'll swap the commit with a comment one for v2.
This way, the next person will be less tempted to send the same patch.

Something like:

pPriv->ptr is freed in our ::FreeScreen callback. Latter of which gets
called by xf86DeleteScreen() as the driver ::*Probe call fails.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 19/19] TODO

[Emil Velikov]

On 10 April 2018 at 09:30, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
>> ---
>>  todo | 9 +
>>  1 file changed, 9 insertions(+)
>>  create mode 100644 todo
>>
>> diff --git a/todo b/todo
>> new file mode 100644
>> index 000..10c1ad5
>> --- /dev/null
>> +++ b/todo
>> @@ -0,0 +1,9 @@
>> + - on amdgpu_probe failure, the pScrn entry is leaked - missing 
>> api/examples?
>
> Might be similar to patch 11; does valgrind actually report a leak if
> you force this?
>
>
>> + - introduce xf86ConfigEntity and use it
>> + - remove embedded AMDGPUInfoRec::pEnt
>> + - consistently use gAMDGPUEntityIndex or getAMDGPUEntityIndex
>> + - consistently use of pEnt/entity_num -> pScrn->list[], AMDPRIV
>> + - kill off DRI_1_ DRICreatePCIBusID - demote again to DRI1 only in X 
>> codebase
>> + - compose bus string early & strcmp instead of device_match?
>> + - remove embedded AMDGPUInfoRec::PciInfo - reuse EntityInfoRec::chipset, 
>> GDevRec::chiIDi, amdgpu_gpu_info::asic_id or ...
>> + - use odev to fetch render_node?
>
> I'm afraid I don't really see these as important enough to be tracked
> like this.
>
Agreed - no reason to keep these in-tree.

Idea was to gather feedback on the topics. One example:
Do we need the getAMDGPUEntityIndex helper, considering ~half of the
existing codebase uses it. Yet other half references
gAMDGPUEntityIndex directly.

Most of the above, seem to be a copy/paste from the radeon driver,
which in turn is a copy from (?) and the original commit lacks any
information :-\

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 17/19] Store device_name as AMDGPUEntRec::master_node

[Emil Velikov]

On 10 April 2018 at 09:29, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> Rename the variable to reflect what it is. Plus move it out of the dri2
>> section - it's used in dri2 and dri3.
>>
>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>
> [...]
>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 4959bd6..e9afe42 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -178,6 +178,10 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
>>   if (pAMDGPUEnt->fd < 0)
>>   return FALSE;
>>
>> + pAMDGPUEnt->master_node = drmGetDeviceNameFromFd2(pAMDGPUEnt->fd);
>> + if (pAMDGPUEnt->master_node)
>> +goto error_amdgpu;
>
> This should be
>
> if (!pAMDGPUEnt->master_node)
>
> shouldn't it?
>
>
> ... Which raises the question: How did you test these patches? :)
>
I mentioned it in the cover letter, but seems to have dropped it -
they are untested.
There's a r600 card close-by I could test with, but no amdgpu one :-\

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

On 10 April 2018 at 09:28, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> Seems like we've been leaking this for years. It became more obvious
>> with the recent refactoring.
>>
>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>> ---
>>  src/amdgpu_probe.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 537d44c..588891c 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>   return TRUE;
>>
>>  error:
>> + free(pPriv->ptr);
>> + pPriv->ptr = NULL;
>>   return FALSE;
>>  }
>>
>>
>
> valgrind doesn't report a leak if I force this error path; presumably
> Xorg frees the private after returning FALSE here.
>
Just double-checked and Xorg does not know anything about ptr. The
only one who clears it up is AMDGPUFreeScreen_KMS.

The magic (for this and the other 'leak') seems to be happening in
xf86platformAddDevice. Namely:
 - ::platformProbe is called via doPlatformProbe
 - the driver explicitly calls xf86AllocateScreen, yet fails later on
 - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
 - ::PreInit fails, ::configured is false
 - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
AMDGPUFreeScreen_KMS)

Eventually, I could unwrap all that although it makes sense to keep
things simpler. As effectively done by the patch.

I believe you'll agree?
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 02/19] Guard ODEV_ATTRIB_FD usage with the correct ifdef

[Emil Velikov]

On 10 April 2018 at 09:27, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>> ---
>>  src/amdgpu_probe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 075e5c1..e65c83b 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -120,7 +120,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
>>   char *busid;
>>   int fd;
>>
>> -#ifdef XF86_PDEV_SERVER_FD
>> +#ifdef ODEV_ATTRIB_FD
>>   if (platform_dev) {
>>   fd = xf86_get_platform_device_int_attrib(platform_dev,
>>ODEV_ATTRIB_FD, -1);
>>
>
> ODEV_ATTRIB_FD doesn't seem obviously more "correct" than
> XF86_PDEV_SERVER_FD, since both were added in the same xserver commit,
> and the latter might be helpful for understanding this is related to the
> other code guarded by XF86_PDEV_SERVER_FD.
>
All the XF86_PDEV_SERVER_FD code is dropped with a later commit ;-)
I could move this patch just after said commit, or you prefer to keep
the original guard?

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Emil Velikov]

On 10 April 2018 at 09:26, Michel Dänzer <mic...@daenzer.net> wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> The former of these is a UMS artefact which gives incorrect and
>> misleading promise whether "KMS" is supported. Not to mention that
>> AMDGPU is a only KMS driver.
>>
>> In a similar fashion xf86LoadKernelModule() is a relic of the times,
>> where platforms had no scheme of detecting and loading the appropriate
>> kernel module.
>>
>> Cc: Robert Millan <r...@freebsd.org>
>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>> ---
>> Robert, off the top of my head this should work with FreeBSD. Admittedly
>> I'm not an expert on the platform. Please give it a test.
>
> I want to get confirmation from Robert that this will work on FreeBSD
> now, since he explicitly restored the kernel module loading code in
> https://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=bfbff3b246db509c820df17b8fcf5899882ffcfa
> .
>
Fully agreed!. That's why I added him to the CC list.

Throwing some ideas:
 - If it's still needed can we keep it !Linux only?
 - Wayland does not have a kernel module loading mechanism.
 - To prevent fan noise and/or card overheating, one really wants to
load the kernel module early. Not when X starts ;-)

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 09/19] Remove error handling on xnfcalloc()

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The function "cannot fail", thus we'll never hit the error path.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 8a16060..8842868 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -232,8 +232,6 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 
if (!pPriv->ptr) {
pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
-   if (!pPriv->ptr)
-   goto error;
 
if (!amdgpu_device_setup(pScrn, pci_dev, dev, pPriv->ptr))
goto error;
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 12/19] Remove ancient "pointer" macro

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Use "void *" over the macro.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Worth splitting this into separate patches and/or keeping compat-api.h
as-is?
---
 src/amdgpu_dri2.c| 4 ++--
 src/amdgpu_glamor.c  | 2 +-
 src/amdgpu_glamor_wrappers.c | 4 ++--
 src/amdgpu_kms.c | 6 +++---
 src/amdgpu_misc.c| 2 +-
 src/compat-api.h | 4 ++--
 src/drmmode_display.c| 2 +-
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/amdgpu_dri2.c b/src/amdgpu_dri2.c
index 4a0f8bf..62964ab 100644
--- a/src/amdgpu_dri2.c
+++ b/src/amdgpu_dri2.c
@@ -341,7 +341,7 @@ static void amdgpu_dri2_unref_buffer(BufferPtr buffer)
 
 static void
 amdgpu_dri2_client_state_changed(CallbackListPtr * ClientStateCallback,
-pointer data, pointer calldata)
+void *data, void *calldata)
 {
NewClientInfoRec *clientinfo = calldata;
ClientPtr pClient = clientinfo->client;
@@ -846,7 +846,7 @@ static int amdgpu_dri2_get_msc(DrawablePtr draw, CARD64 * 
ust, CARD64 * msc)
 }
 
 static
-CARD32 amdgpu_dri2_deferred_event(OsTimerPtr timer, CARD32 now, pointer data)
+CARD32 amdgpu_dri2_deferred_event(OsTimerPtr timer, CARD32 now, void *data)
 {
DRI2FrameEventPtr event_info = (DRI2FrameEventPtr) data;
xf86CrtcPtr crtc = event_info->crtc;
diff --git a/src/amdgpu_glamor.c b/src/amdgpu_glamor.c
index 6efc372..9b3a4e6 100644
--- a/src/amdgpu_glamor.c
+++ b/src/amdgpu_glamor.c
@@ -78,7 +78,7 @@ Bool amdgpu_glamor_create_screen_resources(ScreenPtr screen)
 Bool amdgpu_glamor_pre_init(ScrnInfoPtr scrn)
 {
AMDGPUInfoPtr info = AMDGPUPTR(scrn);
-   pointer glamor_module;
+   void *glamor_module;
CARD32 version;
 
if (scrn->depth < 24) {
diff --git a/src/amdgpu_glamor_wrappers.c b/src/amdgpu_glamor_wrappers.c
index fab8e5a..cf3b6c0 100644
--- a/src/amdgpu_glamor_wrappers.c
+++ b/src/amdgpu_glamor_wrappers.c
@@ -434,7 +434,7 @@ amdgpu_glamor_poly_fill_rect(DrawablePtr pDrawable, GCPtr 
pGC,
 static void
 amdgpu_glamor_image_glyph_blt(DrawablePtr pDrawable, GCPtr pGC,
  int x, int y, unsigned int nglyph,
- CharInfoPtr *ppci, pointer pglyphBase)
+ CharInfoPtr *ppci, void *pglyphBase)
 {
ScrnInfoPtr scrn = xf86ScreenToScrn(pDrawable->pScreen);
PixmapPtr pixmap = get_drawable_pixmap(pDrawable);
@@ -453,7 +453,7 @@ amdgpu_glamor_image_glyph_blt(DrawablePtr pDrawable, GCPtr 
pGC,
 static void
 amdgpu_glamor_poly_glyph_blt(DrawablePtr pDrawable, GCPtr pGC,
 int x, int y, unsigned int nglyph,
-CharInfoPtr *ppci, pointer pglyphBase)
+CharInfoPtr *ppci, void *pglyphBase)
 {
ScrnInfoPtr scrn = xf86ScreenToScrn(pDrawable->pScreen);
PixmapPtr pixmap = get_drawable_pixmap(pDrawable);
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 7ec610f..2ee7007 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -183,7 +183,7 @@ callback_needs_flush(AMDGPUInfoPtr info, struct 
amdgpu_client_priv *client_priv)
 
 static void
 amdgpu_event_callback(CallbackListPtr *list,
- pointer user_data, pointer call_data)
+ void *user_data, void *call_data)
 {
EventInfoRec *eventinfo = call_data;
ScrnInfoPtr pScrn = user_data;
@@ -218,7 +218,7 @@ amdgpu_event_callback(CallbackListPtr *list,
 
 static void
 amdgpu_flush_callback(CallbackListPtr *list,
- pointer user_data, pointer call_data)
+ void *user_data, void *call_data)
 {
ScrnInfoPtr pScrn = user_data;
ScreenPtr pScreen = pScrn->pScreen;
@@ -1644,7 +1644,7 @@ static void amdgpu_drop_drm_master(ScrnInfoPtr pScrn)
 
 
 static
-CARD32 cleanup_black_fb(OsTimerPtr timer, CARD32 now, pointer data)
+CARD32 cleanup_black_fb(OsTimerPtr timer, CARD32 now, void *data)
 {
ScreenPtr screen = data;
ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
diff --git a/src/amdgpu_misc.c b/src/amdgpu_misc.c
index 560a877..1e5b7cb 100644
--- a/src/amdgpu_misc.c
+++ b/src/amdgpu_misc.c
@@ -50,7 +50,7 @@ static XF86ModuleVersionInfo AMDGPUVersionRec = {
  * This function is called every time the module is loaded.
  */
 static pointer
-AMDGPUSetup(pointer Module, pointer Options, int *ErrorMajor, int *ErrorMinor)
+AMDGPUSetup(void *Module, void *Options, int *ErrorMajor, int *ErrorMinor)
 {
static Bool Inited = FALSE;
 
diff --git a/src/compat-api.h b/src/compat-api.h
index a703e5c..cc48b21 100644
--- a/src/compat-api.h
+++ b/src/compat-api.h
@@ -31,10 +31,10 @@
 #endif
 
 #if ABI_VIDEODRV_VERSION >= SET_ABI_VERSION(23, 0)
-#define BLOCKHANDLER_ARGS_DECL ScreenPtr pScreen, pointer pTimeout
+#define BLOCKHANDLER_ARGS_DECL ScreenPtr pScr

[PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Seems like we've been leaking this for years. It became more obvious
with the recent refactoring.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 537d44c..588891c 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
return TRUE;
 
 error:
+   free(pPriv->ptr);
+   pPriv->ptr = NULL;
return FALSE;
 }
 
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 13/19] configure: check for XORG_DRIVER_CHECK_EXT prior to using it

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Noticed while skimming through the intel driver - haven't came across
this while building the amdgpu driver.
---
 configure.ac | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/configure.ac b/configure.ac
index 7b7a4b1..91fbb7b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -43,6 +43,12 @@ m4_ifndef([XORG_MACROS_VERSION],
 XORG_MACROS_VERSION(1.8)
 XORG_DEFAULT_OPTIONS
 
+# Require X.Org server macros (i.e. XORG_DRIVER_CHECK_EXT) to check for 
required modules
+m4_ifndef([XORG_DRIVER_CHECK_EXT],
+  [m4_fatal([must install xorg-server macros before running 
autoconf/autogen.
+  Hint: either install from source, git://anongit.freedesktop.org/xorg/xserver 
or,
+  depending on your distribution, try package 'xserver-xorg-dev' or 
'xorg-x11-server-devel'])])
+
 # Initialize libtool
 AC_DISABLE_STATIC
 AC_PROG_LIBTOOL
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 19/19] TODO

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
---
 todo | 9 +
 1 file changed, 9 insertions(+)
 create mode 100644 todo

diff --git a/todo b/todo
new file mode 100644
index 000..10c1ad5
--- /dev/null
+++ b/todo
@@ -0,0 +1,9 @@
+ - on amdgpu_probe failure, the pScrn entry is leaked - missing api/examples?
+ - introduce xf86ConfigEntity and use it
+ - remove embedded AMDGPUInfoRec::pEnt
+ - consistently use gAMDGPUEntityIndex or getAMDGPUEntityIndex
+ - consistently use of pEnt/entity_num -> pScrn->list[], AMDPRIV
+ - kill off DRI_1_ DRICreatePCIBusID - demote again to DRI1 only in X codebase
+ - compose bus string early & strcmp instead of device_match?
+ - remove embedded AMDGPUInfoRec::PciInfo - reuse EntityInfoRec::chipset, 
GDevRec::chiIDi, amdgpu_gpu_info::asic_id or ...
+ - use odev to fetch render_node?
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 14/19] Do not export gAMDGPUEntityIndex

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The modules should not export anything but a single *ModuleData symbol.
Seemingly a copy/paste mistake from the radeon driver.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
I'm aroung 70% sure on this one.

Yet again, perhaps multi GPU setups do - somehow pulling the _X_EXPORT
variable over the local one? I seriously doubt it though.
---
 src/amdgpu_kms.c   | 2 +-
 src/amdgpu_probe.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 2ee7007..5d85bd7 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -87,7 +87,7 @@ const OptionInfoRec *AMDGPUOptionsWeak(void)
return AMDGPUOptions_KMS;
 }
 
-extern _X_EXPORT int gAMDGPUEntityIndex;
+extern int gAMDGPUEntityIndex;
 
 static int getAMDGPUEntityIndex(void)
 {
diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 588891c..0a46cf1 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -56,7 +56,7 @@
 #include 
 #endif
 
-_X_EXPORT int gAMDGPUEntityIndex = -1;
+int gAMDGPUEntityIndex = -1;
 
 /* Return the options for supported chipset 'n'; NULL otherwise */
 static const OptionInfoRec *AMDGPUAvailableOptions(int chipid, int busid)
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 10/19] Remove unneeded xf86GetEntityInfo() call

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

We only use ent->index, which is the same as the argument fed into the
function.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Note: I'm not 100% sure if the X API guarantees that, yet it seems to be
the case for over 3 years.
---
 src/amdgpu_probe.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 8842868..537d44c 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -198,7 +198,6 @@ static Bool
 amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 struct pci_device *pci_dev, struct xf86_platform_device *dev)
 {
-   EntityInfoPtr pEnt = NULL;
DevUnion *pPriv;
AMDGPUEntPtr pAMDGPUEnt;
 
@@ -218,8 +217,6 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
pScrn->FreeScreen = AMDGPUFreeScreen_KMS;
pScrn->ValidMode = AMDGPUValidMode;
 
-   pEnt = xf86GetEntityInfo(entity_num);
-
/* Create a AMDGPUEntity for all chips, even with old single head
 * Radeon, need to use pAMDGPUEnt for new monitor detection routines.
 */
@@ -228,7 +225,7 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
if (gAMDGPUEntityIndex == -1)
gAMDGPUEntityIndex = xf86AllocateEntityPrivateIndex();
 
-   pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
+   pPriv = xf86GetEntityPrivate(entity_num, gAMDGPUEntityIndex);
 
if (!pPriv->ptr) {
pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
@@ -239,16 +236,13 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
pAMDGPUEnt = pPriv->ptr;
pAMDGPUEnt->fd_ref++;
 
-   xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
-  xf86GetNumEntityInstances(pEnt->
-index)
+   xf86SetEntityInstanceForScreen(pScrn, entity_num,
+  xf86GetNumEntityInstances(entity_num)
   - 1);
-   free(pEnt);
 
return TRUE;
 
 error:
-   free(pEnt);
return FALSE;
 }
 
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 18/19] Keep track of who owns the fd in AMDGPUEnt

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Currently we're having xf86_platform_device pointer embedded, alongside
a bunch of ifdef compiler guards.

Swap that with a simple is_server_fd bool ;-)

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_kms.c   | 10 ++
 src/amdgpu_probe.c | 15 +++
 src/amdgpu_probe.h |  2 +-
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index d27f71d..d625f56 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -1611,11 +1611,8 @@ static Bool amdgpu_set_drm_master(ScrnInfoPtr pScrn)
AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
int err;
 
-#ifdef XF86_PDEV_SERVER_FD
-   if (pAMDGPUEnt->platform_dev &&
-   (pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
+   if (pAMDGPUEnt->is_server_fd)
return TRUE;
-#endif
 
err = drmSetMaster(pAMDGPUEnt->fd);
if (err)
@@ -1628,11 +1625,8 @@ static void amdgpu_drop_drm_master(ScrnInfoPtr pScrn)
 {
AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
 
-#ifdef XF86_PDEV_SERVER_FD
-   if (pAMDGPUEnt->platform_dev &&
-   (pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
+   if (pAMDGPUEnt->is_server_fd)
return;
-#endif
 
drmDropMaster(pAMDGPUEnt->fd);
 }
diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index e9afe42..5d4890b 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -87,7 +87,8 @@ static Bool amdgpu_device_matches(const drmDevicePtr device,
 
 static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 struct pci_device *pci_dev,
-struct xf86_platform_device *platform_dev)
+struct xf86_platform_device *platform_dev,
+AMDGPUEntPtr pAMDGPUEnt)
 {
drmDevicePtr *devices;
struct pci_device *dev;
@@ -98,8 +99,10 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 #ifdef ODEV_ATTRIB_FD
fd = xf86_get_platform_device_int_attrib(platform_dev,
 ODEV_ATTRIB_FD, -1);
-   if (fd != -1)
+   if (fd != -1) {
+   pAMDGPUEnt->is_server_fd = TRUE;
return fd;
+   }
 #endif
 
 #ifdef ODEV_ATTRIB_PATH
@@ -157,10 +160,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 
 void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
 {
-#ifdef XF86_PDEV_SERVER_FD
-   if (!(pAMDGPUEnt->platform_dev &&
- pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
-#endif
+   if (!pAMDGPUEnt->is_server_fd)
close(pAMDGPUEnt->fd);
pAMDGPUEnt->fd = -1;
 }
@@ -173,8 +173,7 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
uint32_t major_version;
uint32_t minor_version;
 
-   pAMDGPUEnt->platform_dev = platform_dev;
-   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, platform_dev);
+   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, platform_dev, 
pAMDGPUEnt);
if (pAMDGPUEnt->fd < 0)
return FALSE;
 
diff --git a/src/amdgpu_probe.h b/src/amdgpu_probe.h
index 94f204f..81fe4a1 100644
--- a/src/amdgpu_probe.h
+++ b/src/amdgpu_probe.h
@@ -65,10 +65,10 @@ typedef struct {
char *master_node;
unsigned long fd_wakeup_registered; /* server generation for which 
fd has been registered for wakeup handling */
int fd_wakeup_ref;
+   Bool is_server_fd;
unsigned int assigned_crtcs;
ScrnInfoPtr primary_scrn;
ScrnInfoPtr secondary_scrn;
-   struct xf86_platform_device *platform_dev;
char *render_node;
 } AMDGPUEntRec, *AMDGPUEntPtr;
 
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 15/19] Do not export the DriverRec AMDGPU

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Analogous to previous commit - unused externally and should not be
exported.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 0a46cf1..4959bd6 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -305,7 +305,7 @@ static const struct pci_id_match amdgpu_device_match[] = {
 {0, 0, 0},
 };
 
-_X_EXPORT DriverRec AMDGPU = {
+DriverRec AMDGPU = {
AMDGPU_VERSION_CURRENT,
AMDGPU_DRIVER_NAME,
AMDGPUIdentify,
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 04/19] Remove drmCheckModesettingSupported and kernel module loading

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The former of these is a UMS artefact which gives incorrect and
misleading promise whether "KMS" is supported. Not to mention that
AMDGPU is a only KMS driver.

In a similar fashion xf86LoadKernelModule() is a relic of the times,
where platforms had no scheme of detecting and loading the appropriate
kernel module.

Cc: Robert Millan <r...@freebsd.org>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Robert, off the top of my head this should work with FreeBSD. Admittedly
I'm not an expert on the platform. Please give it a test.
---
 src/amdgpu_probe.c | 30 --
 1 file changed, 30 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 78cc005..d8d8383 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -52,10 +52,6 @@
 #include "xf86drmMode.h"
 #include "dri.h"
 
-#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-#include 
-#endif
-
 #ifdef XSERVER_PLATFORM_BUS
 #include 
 #endif
@@ -93,27 +89,6 @@ static char *amdgpu_bus_id(ScrnInfoPtr pScrn, struct 
pci_device *dev)
return busid;
 }
 
-static Bool amdgpu_kernel_mode_enabled(ScrnInfoPtr pScrn, char *busIdString)
-{
-   int ret = drmCheckModesettingSupported(busIdString);
-
-#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-   if (ret) {
-   if (xf86LoadKernelModule("amdgpukms"))
-   ret = drmCheckModesettingSupported(busIdString);
-   }
-#endif
-   if (ret) {
-   xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, 0,
-  "[KMS] drm report modesetting isn't 
supported.\n");
-   return FALSE;
-   }
-
-   xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, 0,
-  "[KMS] Kernel modesetting enabled.\n");
-   return TRUE;
-}
-
 static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 struct pci_device *pci_dev,
 struct xf86_platform_device *platform_dev)
@@ -150,11 +125,6 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
if (!busid)
return -1;
 
-   if (!amdgpu_kernel_mode_enabled(pScrn, busid)) {
-   free(busid);
-   return -1;
-   }
-
fd = drmOpen(NULL, busid);
if (fd == -1)
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 02/19] Guard ODEV_ATTRIB_FD usage with the correct ifdef

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 075e5c1..e65c83b 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -120,7 +120,7 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
char *busid;
int fd;
 
-#ifdef XF86_PDEV_SERVER_FD
+#ifdef ODEV_ATTRIB_FD
if (platform_dev) {
fd = xf86_get_platform_device_int_attrib(platform_dev,
 ODEV_ATTRIB_FD, -1);
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 16/19] Remove set but unused amdgpu_dri2::pKernelDRMVersion

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_dri2.h | 1 -
 src/amdgpu_kms.c  | 6 --
 2 files changed, 7 deletions(-)

diff --git a/src/amdgpu_dri2.h b/src/amdgpu_dri2.h
index c6a2ab6..a345e6b 100644
--- a/src/amdgpu_dri2.h
+++ b/src/amdgpu_dri2.h
@@ -30,7 +30,6 @@
 #include 
 
 struct amdgpu_dri2 {
-   drmVersionPtr pKernelDRMVersion;
Bool available;
Bool enabled;
char *device_name;
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 5d85bd7..c70cfe5 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -1397,12 +1397,6 @@ Bool AMDGPUPreInit_KMS(ScrnInfoPtr pScrn, int flags)
 
info->dri2.available = FALSE;
info->dri2.enabled = FALSE;
-   info->dri2.pKernelDRMVersion = drmGetVersion(pAMDGPUEnt->fd);
-   if (info->dri2.pKernelDRMVersion == NULL) {
-   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-  "AMDGPUDRIGetVersion failed to get the DRM 
version\n");
-   return FALSE;
-   }
 
/* Get ScreenInit function */
if (!xf86LoadSubModule(pScrn, "fb"))
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 17/19] Store device_name as AMDGPUEntRec::master_node

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Rename the variable to reflect what it is. Plus move it out of the dri2
section - it's used in dri2 and dri3.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_dri2.c  | 8 +---
 src/amdgpu_dri2.h  | 1 -
 src/amdgpu_dri3.c  | 3 +--
 src/amdgpu_kms.c   | 1 +
 src/amdgpu_probe.c | 5 +
 src/amdgpu_probe.h | 1 +
 6 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/amdgpu_dri2.c b/src/amdgpu_dri2.c
index 62964ab..082520a 100644
--- a/src/amdgpu_dri2.c
+++ b/src/amdgpu_dri2.c
@@ -1282,11 +1282,9 @@ Bool amdgpu_dri2_screen_init(ScreenPtr pScreen)
if (!info->dri2.available)
return FALSE;
 
-   info->dri2.device_name = drmGetDeviceNameFromFd(pAMDGPUEnt->fd);
-
dri2_info.driverName = SI_DRIVER_NAME;
dri2_info.fd = pAMDGPUEnt->fd;
-   dri2_info.deviceName = info->dri2.device_name;
+   dri2_info.deviceName = pAMDGPUEnt->master_node;
 
if (info->drmmode.count_crtcs > 2) {
uint64_t cap_value;
@@ -1340,15 +1338,11 @@ Bool amdgpu_dri2_screen_init(ScreenPtr pScreen)
 
 void amdgpu_dri2_close_screen(ScreenPtr pScreen)
 {
-   ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen);
-   AMDGPUInfoPtr info = AMDGPUPTR(pScrn);
-
if (--DRI2InfoCnt == 0)
DeleteCallback(,
   amdgpu_dri2_client_state_changed, 0);
 
DRI2CloseScreen(pScreen);
-   drmFree(info->dri2.device_name);
 }
 
 #endif /* DRI2 */
diff --git a/src/amdgpu_dri2.h b/src/amdgpu_dri2.h
index a345e6b..a9411b2 100644
--- a/src/amdgpu_dri2.h
+++ b/src/amdgpu_dri2.h
@@ -32,7 +32,6 @@
 struct amdgpu_dri2 {
Bool available;
Bool enabled;
-   char *device_name;
 };
 
 #ifdef DRI2
diff --git a/src/amdgpu_dri3.c b/src/amdgpu_dri3.c
index 87e3d85..2c06b74 100644
--- a/src/amdgpu_dri3.c
+++ b/src/amdgpu_dri3.c
@@ -44,11 +44,10 @@ static int open_master_node(ScreenPtr screen, int *out)
 {
ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
-   AMDGPUInfoPtr info = AMDGPUPTR(scrn);
drm_magic_t magic;
int fd;
 
-   fd = open(info->dri2.device_name, O_RDWR | O_CLOEXEC);
+   fd = open(pAMDGPUEnt->master_node, O_RDWR | O_CLOEXEC);
if (fd < 0)
return BadAlloc;
 
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index c70cfe5..d27f71d 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -147,6 +147,7 @@ static void AMDGPUFreeRec(ScrnInfoPtr pScrn)
pAMDGPUEnt->fd_ref--;
if (!pAMDGPUEnt->fd_ref) {
amdgpu_device_deinitialize(pAMDGPUEnt->pDev);
+   free(pAMDGPUEnt->master_node);
amdgpu_kernel_close_fd(pAMDGPUEnt);
free(pPriv->ptr);
pPriv->ptr = NULL;
diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 4959bd6..e9afe42 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -178,6 +178,10 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
if (pAMDGPUEnt->fd < 0)
return FALSE;
 
+   pAMDGPUEnt->master_node = drmGetDeviceNameFromFd2(pAMDGPUEnt->fd);
+   if (pAMDGPUEnt->master_node)
+  goto error_amdgpu;
+
if (amdgpu_device_initialize(pAMDGPUEnt->fd,
 _version,
 _version,
@@ -190,6 +194,7 @@ static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
return TRUE;
 
 error_amdgpu:
+   free(pAMDGPUEnt->master_node);
amdgpu_kernel_close_fd(pAMDGPUEnt);
return FALSE;
 }
diff --git a/src/amdgpu_probe.h b/src/amdgpu_probe.h
index 5f61aab..94f204f 100644
--- a/src/amdgpu_probe.h
+++ b/src/amdgpu_probe.h
@@ -62,6 +62,7 @@ typedef struct {
 
int fd; /* for sharing across zaphod heads   */
int fd_ref;
+   char *master_node;
unsigned long fd_wakeup_registered; /* server generation for which 
fd has been registered for wakeup handling */
int fd_wakeup_ref;
unsigned int assigned_crtcs;
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 06/19] Introduce amdgpu_device_setup helper

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

It folds the device specifics (open fd, device init) into a single
place.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 66 --
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index ec132e0..afc8d4c 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -165,6 +165,35 @@ void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
pAMDGPUEnt->fd = -1;
 }
 
+static Bool amdgpu_device_setup(ScrnInfoPtr pScrn,
+   struct pci_device *pci_dev,
+   struct xf86_platform_device *platform_dev,
+   AMDGPUEntPtr pAMDGPUEnt)
+{
+   uint32_t major_version;
+   uint32_t minor_version;
+
+   pAMDGPUEnt->platform_dev = platform_dev;
+   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, platform_dev);
+   if (pAMDGPUEnt->fd < 0)
+   return FALSE;
+
+   if (amdgpu_device_initialize(pAMDGPUEnt->fd,
+_version,
+_version,
+>pDev)) {
+   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
+  "amdgpu_device_initialize failed\n");
+   goto error_amdgpu;
+   }
+
+   return TRUE;
+
+error_amdgpu:
+   amdgpu_kernel_close_fd(pAMDGPUEnt);
+   return FALSE;
+}
+
 static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 {
ScrnInfoPtr pScrn = NULL;
@@ -205,28 +234,16 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
 
if (!pPriv->ptr) {
-   uint32_t major_version;
-   uint32_t minor_version;
-
pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
if (!pPriv->ptr)
goto error;
 
-   pAMDGPUEnt = pPriv->ptr;
-   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
-   if (pAMDGPUEnt->fd < 0)
+   if (!amdgpu_device_setup(pScrn, pci_dev, NULL, pPriv->ptr))
goto error;
 
+   pAMDGPUEnt = pPriv->ptr;
pAMDGPUEnt->fd_ref = 1;
 
-   if (amdgpu_device_initialize(pAMDGPUEnt->fd,
-_version,
-_version,
->pDev)) {
-   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-  "amdgpu_device_initialize failed\n");
-   goto error_amdgpu;
-   }
} else {
pAMDGPUEnt = pPriv->ptr;
pAMDGPUEnt->fd_ref++;
@@ -240,8 +257,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
 
return TRUE;
 
-error_amdgpu:
-   amdgpu_kernel_close_fd(pAMDGPUEnt);
 error:
free(pEnt);
return FALSE;
@@ -321,26 +336,15 @@ amdgpu_platform_probe(DriverPtr pDriver,
pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
 
if (!pPriv->ptr) {
-   uint32_t major_version;
-   uint32_t minor_version;
 
pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
-   pAMDGPUEnt = pPriv->ptr;
-   pAMDGPUEnt->platform_dev = dev;
-   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, NULL, dev);
-   if (pAMDGPUEnt->fd < 0)
+
+   if (!amdgpu_device_setup(pScrn, NULL, dev, pPriv->ptr))
goto error;
 
+   pAMDGPUEnt = pPriv->ptr;
pAMDGPUEnt->fd_ref = 1;
 
-   if (amdgpu_device_initialize(pAMDGPUEnt->fd,
-_version,
-_version,
->pDev)) {
-   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-  "amdgpu_device_initialize failed\n");
-   goto error_amdgpu;
-   }
} else {
pAMDGPUEnt = pPriv->ptr;
pAMDGPUEnt->fd_ref++;
@@ -354,8 +358,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 
return TRUE;
 
-error_amdgpu:
-   amdgpu_kernel_close_fd(pAMDGPUEnt);
 error:
free(pEnt);
return FALSE;
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 01/19] Move amdgpu_bus_id/amgpu_kernel_mode within amdgpu_kernel_open_fd

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Small step towards unifying the code paths and removing a handful of
duplication.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 45 +++--
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 0217060..075e5c1 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -112,9 +112,12 @@ static Bool amdgpu_kernel_mode_enabled(ScrnInfoPtr pScrn, 
char *busIdString)
return TRUE;
 }
 
-static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char *busid,
+static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
+struct pci_device *pci_dev,
 struct xf86_platform_device *platform_dev)
 {
+   struct pci_device *dev;
+   char *busid;
int fd;
 
 #ifdef XF86_PDEV_SERVER_FD
@@ -126,11 +129,26 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char 
*busid,
}
 #endif
 
+   if (platform_dev)
+   dev = platform_dev->pdev;
+   else
+   dev = pci_dev;
+
+   busid = amdgpu_bus_id(pScrn, dev);
+   if (!busid)
+   return -1;
+
+   if (!amdgpu_kernel_mode_enabled(pScrn, busid)) {
+   free(busid);
+   return -1;
+   }
+
fd = drmOpen(NULL, busid);
if (fd == -1)
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
   "[drm] Failed to open DRM device for %s: %s\n",
   busid, strerror(errno));
+   free(busid);
return fd;
 }
 
@@ -145,12 +163,12 @@ void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
 }
 
 static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
-  char *busid)
+  struct pci_device *pci_dev)
 {
drmSetVersion sv;
int err;
 
-   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, NULL);
+   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
if (pAMDGPUEnt->fd == -1)
return FALSE;
 
@@ -176,7 +194,6 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, 
AMDGPUEntPtr pAMDGPUEnt,
 static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
 {
ScrnInfoPtr pScrn = NULL;
-   char *busid;
EntityInfoPtr pEnt = NULL;
DevUnion *pPriv;
AMDGPUEntPtr pAMDGPUEnt;
@@ -187,10 +204,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
if (!pScrn)
return FALSE;
 
-   busid = amdgpu_bus_id(pScrn, pci_dev);
-   if (!amdgpu_kernel_mode_enabled(pScrn, busid))
-   goto error;
-
pScrn->driverVersion = AMDGPU_VERSION_CURRENT;
pScrn->driverName = AMDGPU_DRIVER_NAME;
pScrn->name = AMDGPU_NAME;
@@ -226,7 +239,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
goto error;
 
pAMDGPUEnt = pPriv->ptr;
-   if (!amdgpu_open_drm_master(pScrn, pAMDGPUEnt, busid))
+   if (!amdgpu_open_drm_master(pScrn, pAMDGPUEnt, pci_dev))
goto error;
 
pAMDGPUEnt->fd_ref = 1;
@@ -249,7 +262,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
 index)
   - 1);
free(pEnt);
-   free(busid);
 
return TRUE;
 
@@ -257,7 +269,6 @@ error_amdgpu:
amdgpu_kernel_close_fd(pAMDGPUEnt);
 error:
free(pEnt);
-   free(busid);
return FALSE;
 }
 
@@ -294,7 +305,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 {
ScrnInfoPtr pScrn;
int scr_flags = 0;
-   char *busid;
EntityInfoPtr pEnt = NULL;
DevUnion *pPriv;
AMDGPUEntPtr pAMDGPUEnt;
@@ -310,13 +320,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
xf86SetEntityShared(entity_num);
xf86AddEntityToScreen(pScrn, entity_num);
 
-   busid = amdgpu_bus_id(pScrn, dev->pdev);
-   if (!busid)
-   return FALSE;
-
-   if (!amdgpu_kernel_mode_enabled(pScrn, busid))
-   goto error;
-
pScrn->driverVersion = AMDGPU_VERSION_CURRENT;
pScrn->driverName = AMDGPU_DRIVER_NAME;
pScrn->name = AMDGPU_NAME;
@@ -349,7 +352,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
pAMDGPUEnt = pPriv->ptr;
pAMDGPUEnt->platform_dev = dev;
-   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
+   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, NULL, dev);
if (pAMDGPUEnt->fd < 0)

[PATCH xf86-video-amdgpu 03/19] Use ODEV_ATTRIB_PATH where possible for the device node.

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index e65c83b..78cc005 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -33,6 +33,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /*
  * Authors:
@@ -117,18 +119,28 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 struct xf86_platform_device *platform_dev)
 {
struct pci_device *dev;
+   const char *path;
char *busid;
int fd;
 
-#ifdef ODEV_ATTRIB_FD
if (platform_dev) {
+#ifdef ODEV_ATTRIB_FD
fd = xf86_get_platform_device_int_attrib(platform_dev,
 ODEV_ATTRIB_FD, -1);
if (fd != -1)
return fd;
-   }
 #endif
 
+#ifdef ODEV_ATTRIB_PATH
+   path = xf86_get_platform_device_attrib(platform_dev,
+  ODEV_ATTRIB_PATH);
+
+   fd = open(path, O_RDWR | O_CLOEXEC);
+   if (fd != -1)
+   return fd;
+#endif
+   }
+
if (platform_dev)
dev = platform_dev->pdev;
else
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 08/19] Simplify fd_ref handling in amdgpu_probe()

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index d1ad13f..8a16060 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -237,14 +237,9 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
 
if (!amdgpu_device_setup(pScrn, pci_dev, dev, pPriv->ptr))
goto error;
-
-   pAMDGPUEnt = pPriv->ptr;
-   pAMDGPUEnt->fd_ref = 1;
-
-   } else {
-   pAMDGPUEnt = pPriv->ptr;
-   pAMDGPUEnt->fd_ref++;
}
+   pAMDGPUEnt = pPriv->ptr;
+   pAMDGPUEnt->fd_ref++;
 
xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
   xf86GetNumEntityInstances(pEnt->
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 07/19] Factor out common code to amdgpu_probe()

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Keep the distinct pci/platform screen management in the separate probe
entry point and fold the rest into a single function.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 71 +++---
 1 file changed, 9 insertions(+), 62 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index afc8d4c..d1ad13f 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -194,16 +194,14 @@ error_amdgpu:
return FALSE;
 }
 
-static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
+static Bool
+amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
+struct pci_device *pci_dev, struct xf86_platform_device *dev)
 {
-   ScrnInfoPtr pScrn = NULL;
EntityInfoPtr pEnt = NULL;
DevUnion *pPriv;
AMDGPUEntPtr pAMDGPUEnt;
 
-   pScrn = xf86ConfigPciEntity(pScrn, 0, entity_num, NULL,
-   NULL, NULL, NULL, NULL, NULL);
-
if (!pScrn)
return FALSE;
 
@@ -211,7 +209,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
pScrn->driverName = AMDGPU_DRIVER_NAME;
pScrn->name = AMDGPU_NAME;
pScrn->Probe = NULL;
-
pScrn->PreInit = AMDGPUPreInit_KMS;
pScrn->ScreenInit = AMDGPUScreenInit_KMS;
pScrn->SwitchMode = AMDGPUSwitchMode_KMS;
@@ -238,7 +235,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
if (!pPriv->ptr)
goto error;
 
-   if (!amdgpu_device_setup(pScrn, pci_dev, NULL, pPriv->ptr))
+   if (!amdgpu_device_setup(pScrn, pci_dev, dev, pPriv->ptr))
goto error;
 
pAMDGPUEnt = pPriv->ptr;
@@ -266,7 +263,10 @@ static Bool
 amdgpu_pci_probe(DriverPtr pDriver,
 int entity_num, struct pci_device *device, intptr_t match_data)
 {
-   return amdgpu_get_scrninfo(entity_num, device);
+   ScrnInfoPtr pScrn = xf86ConfigPciEntity(NULL, 0, entity_num, NULL,
+   NULL, NULL, NULL, NULL, NULL);
+
+   return amdgpu_probe(pScrn, entity_num, device, NULL);
 }
 
 static Bool AMDGPUDriverFunc(ScrnInfoPtr scrn, xorgDriverFuncOp op, void *data)
@@ -295,9 +295,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
 {
ScrnInfoPtr pScrn;
int scr_flags = 0;
-   EntityInfoPtr pEnt = NULL;
-   DevUnion *pPriv;
-   AMDGPUEntPtr pAMDGPUEnt;
 
if (!dev->pdev)
return FALSE;
@@ -310,57 +307,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
xf86SetEntityShared(entity_num);
xf86AddEntityToScreen(pScrn, entity_num);
 
-   pScrn->driverVersion = AMDGPU_VERSION_CURRENT;
-   pScrn->driverName = AMDGPU_DRIVER_NAME;
-   pScrn->name = AMDGPU_NAME;
-   pScrn->Probe = NULL;
-   pScrn->PreInit = AMDGPUPreInit_KMS;
-   pScrn->ScreenInit = AMDGPUScreenInit_KMS;
-   pScrn->SwitchMode = AMDGPUSwitchMode_KMS;
-   pScrn->AdjustFrame = AMDGPUAdjustFrame_KMS;
-   pScrn->EnterVT = AMDGPUEnterVT_KMS;
-   pScrn->LeaveVT = AMDGPULeaveVT_KMS;
-   pScrn->FreeScreen = AMDGPUFreeScreen_KMS;
-   pScrn->ValidMode = AMDGPUValidMode;
-
-   pEnt = xf86GetEntityInfo(entity_num);
-
-   /* Create a AMDGPUEntity for all chips, even with old single head
-* Radeon, need to use pAMDGPUEnt for new monitor detection routines.
-*/
-   xf86SetEntitySharable(entity_num);
-
-   if (gAMDGPUEntityIndex == -1)
-   gAMDGPUEntityIndex = xf86AllocateEntityPrivateIndex();
-
-   pPriv = xf86GetEntityPrivate(pEnt->index, gAMDGPUEntityIndex);
-
-   if (!pPriv->ptr) {
-
-   pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
-
-   if (!amdgpu_device_setup(pScrn, NULL, dev, pPriv->ptr))
-   goto error;
-
-   pAMDGPUEnt = pPriv->ptr;
-   pAMDGPUEnt->fd_ref = 1;
-
-   } else {
-   pAMDGPUEnt = pPriv->ptr;
-   pAMDGPUEnt->fd_ref++;
-   }
-
-   xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
-  xf86GetNumEntityInstances(pEnt->
-index)
-  - 1);
-   free(pEnt);
-
-   return TRUE;
-
-error:
-   free(pEnt);
-   return FALSE;
+   return amdgpu_probe(pScrn, entity_num, NULL, dev);
 }
 #endif
 
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 00/19] Removing UMS remnants and assorted patches

[Emil Velikov]

Hi all,

Here is some work that I had for ages - was planning to do a similar
series for the radeon driver. Since ETIME for the latter I'm getting
this out.

This series has two goals, although since the split it a bit hard I've
left it one big bunch:
 - remove most of the UMS remnants
 - simplify the {pci,platform}_probe duplication

To top it up there's a TODO as 'Patch' 19. It mostly seeks to gather
feedback on the items listed.

As always, input is greatly appreciated.

Thanks
Emil

Emil Velikov (19):
  Move amdgpu_bus_id/amgpu_kernel_mode within amdgpu_kernel_open_fd
  Guard ODEV_ATTRIB_FD usage with the correct ifdef
  Use ODEV_ATTRIB_PATH where possible for the device node.
  Remove drmCheckModesettingSupported and kernel module loading
  Kill off drmOpen/Close/drmSetInterfaceVersion in favour of drmDevices
  Introduce amdgpu_device_setup helper
  Factor out common code to amdgpu_probe()
  Simplify fd_ref handling in amdgpu_probe()
  Remove error handling on xnfcalloc()
  Remove unneeded xf86GetEntityInfo() call
  Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails
  Remove ancient "pointer" macro
  configure: check for XORG_DRIVER_CHECK_EXT prior to using it
  Do not export gAMDGPUEntityIndex
  Do not export the DriverRec AMDGPU
  Remove set but unused amdgpu_dri2::pKernelDRMVersion
  Store device_name as AMDGPUEntRec::master_node
  Keep track of who owns the fd in AMDGPUEnt
  TODO

 configure.ac |   6 +
 src/amdgpu_dri2.c|  12 +-
 src/amdgpu_dri2.h|   2 -
 src/amdgpu_dri3.c|   3 +-
 src/amdgpu_glamor.c  |   2 +-
 src/amdgpu_glamor_wrappers.c |   4 +-
 src/amdgpu_kms.c |  25 +---
 src/amdgpu_misc.c|   2 +-
 src/amdgpu_probe.c   | 304 ---
 src/amdgpu_probe.h   |   3 +-
 src/compat-api.h |   4 +-
 src/drmmode_display.c|   2 +-
 todo |   9 ++
 13 files changed, 146 insertions(+), 232 deletions(-)
 create mode 100644 todo

-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 05/19] Kill off drmOpen/Close/drmSetInterfaceVersion in favour of drmDevices

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The former has very subtle semantics (see the implementation in libdrm
for details) which were required in the UMS days.

With drmDevices around, we have enough information to build our
heuristics and avoid drmOpen all together.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 src/amdgpu_probe.c | 93 +-
 1 file changed, 43 insertions(+), 50 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index d8d8383..ec132e0 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -75,28 +75,24 @@ static void AMDGPUIdentify(int flags)
xf86PrintChipsets(AMDGPU_NAME, "Driver for AMD Radeon", AMDGPUAny);
 }
 
-static char *amdgpu_bus_id(ScrnInfoPtr pScrn, struct pci_device *dev)
+static Bool amdgpu_device_matches(const drmDevicePtr device,
+ const struct pci_device *dev)
 {
-   char *busid;
-
-   XNFasprintf(, "pci:%04x:%02x:%02x.%d",
-   dev->domain, dev->bus, dev->dev, dev->func);
-
-   if (!busid)
-   xf86DrvMsgVerb(pScrn->scrnIndex, X_ERROR, 0,
-  "AMDGPU: Failed to generate bus ID string\n");
-
-   return busid;
+   return (device->bustype == DRM_BUS_PCI &&
+   device->businfo.pci->domain == dev->domain &&
+   device->businfo.pci->bus == dev->bus &&
+   device->businfo.pci->dev == dev->dev &&
+   device->businfo.pci->func == dev->func);
 }
 
 static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
 struct pci_device *pci_dev,
 struct xf86_platform_device *platform_dev)
 {
+   drmDevicePtr *devices;
struct pci_device *dev;
const char *path;
-   char *busid;
-   int fd;
+   int fd, i, ret;
 
if (platform_dev) {
 #ifdef ODEV_ATTRIB_FD
@@ -121,16 +117,41 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn,
else
dev = pci_dev;
 
-   busid = amdgpu_bus_id(pScrn, dev);
-   if (!busid)
+   ret = drmGetDevices2(0, NULL, 0);
+   if (ret <= 0) {
+   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
+  "[drm] Failed to retrieve number of DRM devices: 
%d\n",
+  ret);
return -1;
+   }
+
+   devices = xnfcalloc(sizeof(drmDevicePtr), ret);
+
+   ret = drmGetDevices2(0, devices, ret);
+   if (fd == -1) {
+   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
+  "[drm] Failed to retrieve DRM devices information: 
%d\n",
+  ret);
+   free(devices);
+   return -1;
+   }
+
+   for (i = 0; i < ret; i++) {
+   if (amdgpu_device_matches(devices[i], dev) &&
+   devices[i]->available_nodes & (1 << DRM_NODE_PRIMARY)) {
+   path = devices[i]->nodes[DRM_NODE_PRIMARY];
+   fd = open(path, O_RDWR | O_CLOEXEC);
+   break;
+   }
+   }
+   drmFreeDevices(devices, ret);
+   free(devices);
 
-   fd = drmOpen(NULL, busid);
if (fd == -1)
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
-  "[drm] Failed to open DRM device for %s: %s\n",
-  busid, strerror(errno));
-   free(busid);
+  "[drm] Failed to open DRM device for "
+  "pci:%04x:%02x:%02x.%1u\n",
+  dev->domain, dev->bus, dev->dev, dev->func);
return fd;
 }
 
@@ -140,39 +161,10 @@ void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
if (!(pAMDGPUEnt->platform_dev &&
  pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
 #endif
-   drmClose(pAMDGPUEnt->fd);
+   close(pAMDGPUEnt->fd);
pAMDGPUEnt->fd = -1;
 }
 
-static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
-  struct pci_device *pci_dev)
-{
-   drmSetVersion sv;
-   int err;
-
-   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
-   if (pAMDGPUEnt->fd == -1)
-   return FALSE;
-
-   /* Check that what we opened was a master or a master-capable FD,
-* by setting the version of the interface we'll use to talk to it.
-* (see DRIOpenDRMMaster() in DRI1)
-*/
-   sv.drm_di_major = 1;
-   sv.drm_di_minor = 1;
-   sv.drm_dd_major = -1;
-   sv.drm_dd_minor = -1;
-   err = drmSetInterfaceVersion(pAMDGPUEnt->fd, );
-   if (err !=

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

On 22 March 2018 at 18:03, Harry Wentland <harry.wentl...@amd.com> wrote:
> On 2018-03-22 01:54 PM, Emil Velikov wrote:
>> Hi Ville,
>>
>> On 22 March 2018 at 15:22, Ville Syrjala <ville.syrj...@linux.intel.com> 
>> wrote:
>>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>>
>>> I really just wanted to fix i915 to re-enable its planes afer load
>>> detection (a two line patch). This is what I actually ended up with
>>> after I ran into a framebuffer refcount leak with said two line patch.
>>>
>>> I've tested this on a few i915 boxes and so far it's looking
>>> good. Everything else is just compile tested.
>>>
>> Mostly thinking out loud:
>>
>> Wondering if one cannot somehow (re)move plane->fb/crtc altogether.
>> Otherwise drivers will reintroduce similar code, despite the WARNs and
>> beefy documentation :-\
>
> Wouldn't that require an atomic conversion of all remaining drivers?
>
That or maybe move into plane->legacy->{fb,crtc}. Feel free to swap
'legacy' with flashier name.

Hmm back in 2015 we had a GSoC that updated BOCHS and CIRRUS drivers,
but they never got merged.
Don't recall the details - from memory the conversion seemed fine, but
there was either shortage on review/other.

Might be worth reviving that... regardless it's getting a bit off-topic.
-Emil

[1] 
https://www.google-melange.com/archive/gsoc/2015/orgs/xorg/projects/johnhunter.html
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

Hi Ville,

On 22 March 2018 at 15:22, Ville Syrjala  wrote:
> From: Ville Syrjälä 
>
> I really just wanted to fix i915 to re-enable its planes afer load
> detection (a two line patch). This is what I actually ended up with
> after I ran into a framebuffer refcount leak with said two line patch.
>
> I've tested this on a few i915 boxes and so far it's looking
> good. Everything else is just compile tested.
>
Mostly thinking out loud:

Wondering if one cannot somehow (re)move plane->fb/crtc altogether.
Otherwise drivers will reintroduce similar code, despite the WARNs and
beefy documentation :-\

HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

[Emil Velikov]

On 20 February 2018 at 13:12, Peter Zijlstra  wrote:
> On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>> way of doing this is to check if the buffer object is locked with the ticket
>> of the current submission.
>>
>> Clean up the access to the ww_mutex internals by providing a function
>> for this and extend the check to the thread owning the underlying mutex.
>
>> Signed-off-by: Christian König 
>
> Much thanks for Cc'ing the relevant maintainers :/
>
Doubt it's intentional. The get-maintainer script seems confused and
lists no maintainers?

$ ./scripts/get_maintainer.pl include/linux/ww_mutex.h
linux-ker...@vger.kernel.org (open list)

While the normal mutex header works fine.

$ ./scripts/get_maintainer.pl include/linux/mutex.h
Peter Zijlstra  (maintainer:LOCKING PRIMITIVES)
Ingo Molnar  (maintainer:LOCKING PRIMITIVES)
linux-ker...@vger.kernel.org (open list:LOCKING PRIMITIVES)

HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx