Quick question for the mobile Raven Ridge auto-rotate function
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Hello team, Thank for you making mobile Raven Ridge nearly fully functional with the open source driver for multiple devices like HP Envy x360 Ryzen 2500u. However, missing is the ability to auto-rotate the screen when switching from landscape to portrait in tablet mode. Which channel will be better to request enabling that function in open source driver? See the related issue below: Red Hat bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1651886 Linux kernel report: https://bugzilla.kernel.org/show_bug.cgi?id=199715 I will be willing to test such driver to report the functionality. Thanks in advance, Luya Tshimbalanga Fedora Design Team -BEGIN PGP SIGNATURE- iQEzBAEBCAAdFiEEWyB+BQtYiFz4GUNDXlKBdNiiYJoFAlzmMM8ACgkQXlKBdNii YJq/wAgAn8jWhzXLLbBYjtai4f0C4rw1cKt1MDi48qOxlSiPaOplfG8RLrCGVFL3 jMgReHhN/U3zfy3SRBXa2zsTspdVKRnxewMxJHJmS653prOAEVsfd41b/XDMInmp kFCcTKXwR9GlUYPbSuj3pMLSwq3OHmBlPjnpL4NMXlmrcQ6psN9I992Itg8HEoh2 3vGF5qRdKuidLnu9xRNLceLjvpvTyJ5fhH/Ry5sylX5oJhdW7WlR5HE+Smsgu7hW rVGgGl6yFdUEGaiFRqhPxuEpC07i7Vi5REqB+s/YUgzM+Wn86taA4Ld1R9dMeZIm g7EJeO8c6RKII2MOKWrKtTpvuUg+RQ== =Ftfg -END PGP SIGNATURE- ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco reset
Sorry, I have pushed the change as Evan gave the reviewed-by, I will send
another patch to reference your review comments, do you think it is Ok?
>-Original Message-
>From: Alex Deucher
>Sent: Thursday, May 23, 2019 11:54 AM
>To: Deng, Emily
>Cc: amd-gfx list
>Subject: Re: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco
>reset
>
>[CAUTION: External Email]
>
>On Wed, May 22, 2019 at 11:48 PM Alex Deucher
>wrote:
>>
>> On Wed, May 22, 2019 at 11:27 PM Emily Deng
>wrote:
>> >
>>
>> Please include a patch description.
The comment is lost when using "git send-email", I will check why.
>>
>> > Signed-off-by: Emily Deng
>> > ---
>> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +-
>> > drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
>> > drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
>
>> > drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
>> > .../amd/powerplay/hwmgr/vega10_processpptables.c | 22
>++
>> > .../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
>> > drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
>> > 7 files changed, 51 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> > index d6286ed..5288763 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> > @@ -2605,7 +2605,15 @@ int amdgpu_device_init(struct amdgpu_device
>*adev,
>> > /* check if we need to reset the asic
>> > * E.g., driver was not cleanly unloaded previously, etc.
>> > */
>> > - if (!amdgpu_sriov_vf(adev) &&
>amdgpu_asic_need_reset_on_init(adev)) {
>> > + if (amdgpu_passthrough(adev) &&
>> > + amdgpu_asic_need_reset_on_init(adev)) {
>>
>> This will change the current behavior on baremetal.
Ok, I will put the passthrough check to "if(adev->powerplay.pp_funcs &&
adev->powerplay.pp_funcs->set_asic_baco_cap)"
>>
>> > + if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
>>set_asic_baco_cap) {
>> > + r =
>> > adev->powerplay.pp_funcs->set_asic_baco_cap(adev-
>>powerplay.pp_handle);
>> > + if (r) {
>> > + dev_err(adev->dev, "set baco capability
>> > failed\n");
>> > + goto failed;
>> > + }
>> > + }
>> > +
>>
>> This will also change the current behavior on bare metal.
>>
>> I think you may want to rework this a bit otherwise this change won't
>> really make any difference due to this patch:
>> https://cgit.freedesktop.org/~agd5f/linux/commit/?h=amd-staging-drm-ne
>> xt=60ae2cd5aec94dc6459bdee5c610bb5c76a1d0ae
>> We need to avoid breaking module reload (modeprobe amdgpu; modprobe
>-r
>> amdgpu; modeprobe amdgpu).
>> I think it would be cleaner to call set_asic_baco_cap() in
>> hwmgr_early_init() and then return true in soc15_need_reset_on_init()
>> is it's passthrough mode. then everything should just work as is.
Ok, will call set_asic_baco_cap in vega10_hwmgr_init
>Assuming you set the set_asic_baco_cap callbacks for vega20 and vega12 as
>well. Otherwise, you'll have to limit it to vega10 which starts to get messy
>IMHO.
I don't add the callbacks for vega20 and vega12, as no platform to test these.
And if vega20 or vega12 need this, then they could add this and test this.
Currently only for vega10.
>Alex
>
>>
>> > r = amdgpu_asic_reset(adev);
>> > if (r) {
>> > dev_err(adev->dev, "asic reset on init
>> > failed\n"); diff --git
>> > a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> > index 2b579ba..0dcc18d 100644
>> > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> > @@ -285,6 +285,7 @@ struct amd_pm_funcs {
>> > int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
>> > int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
>> > int (*get_asic_baco_capability)(void *handle, bool *cap);
>> > + int (*set_asic_baco_cap)(void *handle);
>> > int (*get_asic_baco_state)(void *handle, int *state);
>> > int (*set_asic_baco_state)(void *handle, int state);
>> > int (*get_ppfeature_status)(void *handle, char *buf); diff
>> > --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> > b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> > index bea1587..9856760 100644
>> > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> > @@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void
>*handle, uint32_t count)
>> > return ret;
>> > }
>> >
>> > +static int pp_set_asic_baco_cap(void *handle) {
>> > + struct pp_hwmgr *hwmgr = handle;
>> > +
RE: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco reset
Hi Evan,
If don’t call set_asic_baco_cap， then couldn't enable baco, so need to
modify amdgpu_sriov_vf to limit to passthrough. And the comment is in the
patch, but don't know why it lost when using " git send-email".
And as you gave the reviewed-by, I already pushed the patch to the
drm-next.
Best wishes
Emily Deng
>-Original Message-
>From: Quan, Evan
>Sent: Thursday, May 23, 2019 11:46 AM
>To: Deng, Emily ; amd-gfx@lists.freedesktop.org
>Cc: Deng, Emily
>Subject: RE: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco
>reset
>
>I would actually expect the followings
>
> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev))
>{ --> no touch for this
>+if (amdgpu_passthrough(adev) && adev->powerplay.pp_funcs &&
>adev->powerplay.pp_funcs->set_asic_baco_cap) {
>+ r = adev->powerplay.pp_funcs->set_asic_baco_cap(adev-
>>powerplay.pp_handle);
>+ if (r) {
>+ dev_err(adev->dev, "set baco capability
>failed\n");
>+ goto failed;
>+ }
>+ }
>+
>
>And btw the commit description was lost compared with v1.
>
>Regards,
>Evan
>> -Original Message-
>> From: amd-gfx On Behalf Of
>> Emily Deng
>> Sent: Thursday, May 23, 2019 11:27 AM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Deng, Emily
>> Subject: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco
>> reset
>>
>> [CAUTION: External Email]
>>
>> Signed-off-by: Emily Deng
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +-
>> drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
>> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
>>
>> drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
>> .../amd/powerplay/hwmgr/vega10_processpptables.c | 22
>> ++
>> .../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
>> drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
>> 7 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index d6286ed..5288763 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -2605,7 +2605,15 @@ int amdgpu_device_init(struct amdgpu_device
>> *adev,
>> /* check if we need to reset the asic
>> * E.g., driver was not cleanly unloaded previously, etc.
>> */
>> - if (!amdgpu_sriov_vf(adev) &&
>> amdgpu_asic_need_reset_on_init(adev)) {
>> + if (amdgpu_passthrough(adev) &&
>> amdgpu_asic_need_reset_on_init(adev)) {
>> + if (adev->powerplay.pp_funcs &&
>> + adev->powerplay.pp_funcs-
>> >set_asic_baco_cap) {
>> + r =
>> + adev->powerplay.pp_funcs->set_asic_baco_cap(adev-
>> >powerplay.pp_handle);
>> + if (r) {
>> + dev_err(adev->dev, "set baco capability
>> failed\n");
>> + goto failed;
>> + }
>> + }
>> +
>> r = amdgpu_asic_reset(adev);
>> if (r) {
>> dev_err(adev->dev, "asic reset on init
>> failed\n"); diff --git
>> a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> index 2b579ba..0dcc18d 100644
>> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> @@ -285,6 +285,7 @@ struct amd_pm_funcs {
>> int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
>> int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
>> int (*get_asic_baco_capability)(void *handle, bool *cap);
>> + int (*set_asic_baco_cap)(void *handle);
>> int (*get_asic_baco_state)(void *handle, int *state);
>> int (*set_asic_baco_state)(void *handle, int state);
>> int (*get_ppfeature_status)(void *handle, char *buf); diff
>> --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> index bea1587..9856760 100644
>> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> @@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void
>> *handle, uint32_t count)
>> return ret;
>> }
>>
>> +static int pp_set_asic_baco_cap(void *handle) {
>> + struct pp_hwmgr *hwmgr = handle;
>> +
>> + if (!hwmgr)
>> + return -EINVAL;
>> +
>> + if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
>> + return 0;
>> +
>> + hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
>> +
>> + return 0;
>> +}
>> +
>> static int pp_get_asic_baco_capability(void *handle, bool *cap) {
>> struct pp_hwmgr *hwmgr = handle; @@ -1546,6 +1561,7 @@
Re: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco reset
On Wed, May 22, 2019 at 11:48 PM Alex Deucher wrote:
>
> On Wed, May 22, 2019 at 11:27 PM Emily Deng wrote:
> >
>
> Please include a patch description.
>
> > Signed-off-by: Emily Deng
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +-
> > drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
> > drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
> > drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
> > .../amd/powerplay/hwmgr/vega10_processpptables.c | 22
> > ++
> > .../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
> > drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
> > 7 files changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index d6286ed..5288763 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2605,7 +2605,15 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> > /* check if we need to reset the asic
> > * E.g., driver was not cleanly unloaded previously, etc.
> > */
> > - if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev))
> > {
> > + if (amdgpu_passthrough(adev) &&
> > amdgpu_asic_need_reset_on_init(adev)) {
>
> This will change the current behavior on baremetal.
>
> > + if (adev->powerplay.pp_funcs &&
> > adev->powerplay.pp_funcs->set_asic_baco_cap) {
> > + r =
> > adev->powerplay.pp_funcs->set_asic_baco_cap(adev->powerplay.pp_handle);
> > + if (r) {
> > + dev_err(adev->dev, "set baco capability
> > failed\n");
> > + goto failed;
> > + }
> > + }
> > +
>
> This will also change the current behavior on bare metal.
>
> I think you may want to rework this a bit otherwise this change won't
> really make any difference due to this patch:
> https://cgit.freedesktop.org/~agd5f/linux/commit/?h=amd-staging-drm-next=60ae2cd5aec94dc6459bdee5c610bb5c76a1d0ae
> We need to avoid breaking module reload (modeprobe amdgpu; modprobe -r
> amdgpu; modeprobe amdgpu).
> I think it would be cleaner to call set_asic_baco_cap() in
> hwmgr_early_init() and then return true in soc15_need_reset_on_init()
> is it's passthrough mode. then everything should just work as is.
Assuming you set the set_asic_baco_cap callbacks for vega20 and vega12
as well. Otherwise, you'll have to limit it to vega10 which starts to
get messy IMHO.
Alex
>
> > r = amdgpu_asic_reset(adev);
> > if (r) {
> > dev_err(adev->dev, "asic reset on init failed\n");
> > diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > index 2b579ba..0dcc18d 100644
> > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > @@ -285,6 +285,7 @@ struct amd_pm_funcs {
> > int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
> > int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
> > int (*get_asic_baco_capability)(void *handle, bool *cap);
> > + int (*set_asic_baco_cap)(void *handle);
> > int (*get_asic_baco_state)(void *handle, int *state);
> > int (*set_asic_baco_state)(void *handle, int state);
> > int (*get_ppfeature_status)(void *handle, char *buf);
> > diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > index bea1587..9856760 100644
> > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > @@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void *handle,
> > uint32_t count)
> > return ret;
> > }
> >
> > +static int pp_set_asic_baco_cap(void *handle)
> > +{
> > + struct pp_hwmgr *hwmgr = handle;
> > +
> > + if (!hwmgr)
> > + return -EINVAL;
> > +
> > + if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
> > + return 0;
> > +
> > + hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
> > +
> > + return 0;
> > +}
> > +
> > static int pp_get_asic_baco_capability(void *handle, bool *cap)
> > {
> > struct pp_hwmgr *hwmgr = handle;
> > @@ -1546,6 +1561,7 @@ static const struct amd_pm_funcs pp_dpm_funcs = {
> > .set_hard_min_dcefclk_by_freq = pp_set_hard_min_dcefclk_by_freq,
> > .set_hard_min_fclk_by_freq = pp_set_hard_min_fclk_by_freq,
> > .get_asic_baco_capability = pp_get_asic_baco_capability,
> > + .set_asic_baco_cap = pp_set_asic_baco_cap,
> > .get_asic_baco_state = pp_get_asic_baco_state,
> > .set_asic_baco_state = pp_set_asic_baco_state,
> >
Re: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco reset
On Wed, May 22, 2019 at 11:27 PM Emily Deng wrote:
>
Please include a patch description.
> Signed-off-by: Emily Deng
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +-
> drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
> drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
> .../amd/powerplay/hwmgr/vega10_processpptables.c | 22
> ++
> .../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
> drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
> 7 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d6286ed..5288763 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2605,7 +2605,15 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> /* check if we need to reset the asic
> * E.g., driver was not cleanly unloaded previously, etc.
> */
> - if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
> + if (amdgpu_passthrough(adev) && amdgpu_asic_need_reset_on_init(adev))
> {
This will change the current behavior on baremetal.
> + if (adev->powerplay.pp_funcs &&
> adev->powerplay.pp_funcs->set_asic_baco_cap) {
> + r =
> adev->powerplay.pp_funcs->set_asic_baco_cap(adev->powerplay.pp_handle);
> + if (r) {
> + dev_err(adev->dev, "set baco capability
> failed\n");
> + goto failed;
> + }
> + }
> +
This will also change the current behavior on bare metal.
I think you may want to rework this a bit otherwise this change won't
really make any difference due to this patch:
https://cgit.freedesktop.org/~agd5f/linux/commit/?h=amd-staging-drm-next=60ae2cd5aec94dc6459bdee5c610bb5c76a1d0ae
We need to avoid breaking module reload (modeprobe amdgpu; modprobe -r
amdgpu; modeprobe amdgpu).
I think it would be cleaner to call set_asic_baco_cap() in
hwmgr_early_init() and then return true in soc15_need_reset_on_init()
is it's passthrough mode. then everything should just work as is.
> r = amdgpu_asic_reset(adev);
> if (r) {
> dev_err(adev->dev, "asic reset on init failed\n");
> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> index 2b579ba..0dcc18d 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -285,6 +285,7 @@ struct amd_pm_funcs {
> int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
> int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
> int (*get_asic_baco_capability)(void *handle, bool *cap);
> + int (*set_asic_baco_cap)(void *handle);
> int (*get_asic_baco_state)(void *handle, int *state);
> int (*set_asic_baco_state)(void *handle, int state);
> int (*get_ppfeature_status)(void *handle, char *buf);
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index bea1587..9856760 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void *handle,
> uint32_t count)
> return ret;
> }
>
> +static int pp_set_asic_baco_cap(void *handle)
> +{
> + struct pp_hwmgr *hwmgr = handle;
> +
> + if (!hwmgr)
> + return -EINVAL;
> +
> + if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
> + return 0;
> +
> + hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
> +
> + return 0;
> +}
> +
> static int pp_get_asic_baco_capability(void *handle, bool *cap)
> {
> struct pp_hwmgr *hwmgr = handle;
> @@ -1546,6 +1561,7 @@ static const struct amd_pm_funcs pp_dpm_funcs = {
> .set_hard_min_dcefclk_by_freq = pp_set_hard_min_dcefclk_by_freq,
> .set_hard_min_fclk_by_freq = pp_set_hard_min_fclk_by_freq,
> .get_asic_baco_capability = pp_get_asic_baco_capability,
> + .set_asic_baco_cap = pp_set_asic_baco_cap,
> .get_asic_baco_state = pp_get_asic_baco_state,
> .set_asic_baco_state = pp_set_asic_baco_state,
> .get_ppfeature_status = pp_get_ppfeature_status,
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index ed6c638..8dc23eb 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -5171,6 +5171,7 @@ static const struct pp_hwmgr_func vega10_hwmgr_funcs = {
> .odn_edit_dpm_table =
RE: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco reset
I would actually expect the followings
if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
--> no touch for this
+if (amdgpu_passthrough(adev) && adev->powerplay.pp_funcs &&
adev->powerplay.pp_funcs->set_asic_baco_cap) {
+ r =
adev->powerplay.pp_funcs->set_asic_baco_cap(adev->powerplay.pp_handle);
+ if (r) {
+ dev_err(adev->dev, "set baco capability
failed\n");
+ goto failed;
+ }
+ }
+
And btw the commit description was lost compared with v1.
Regards,
Evan
> -Original Message-
> From: amd-gfx On Behalf Of
> Emily Deng
> Sent: Thursday, May 23, 2019 11:27 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deng, Emily
> Subject: [PATCH v2] drm/amdgpu: Need to set the baco cap before baco
> reset
>
> [CAUTION: External Email]
>
> Signed-off-by: Emily Deng
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +-
> drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
>
> drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
> .../amd/powerplay/hwmgr/vega10_processpptables.c | 22
> ++
> .../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
> drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
> 7 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d6286ed..5288763 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2605,7 +2605,15 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
> /* check if we need to reset the asic
> * E.g., driver was not cleanly unloaded previously, etc.
> */
> - if (!amdgpu_sriov_vf(adev) &&
> amdgpu_asic_need_reset_on_init(adev)) {
> + if (amdgpu_passthrough(adev) &&
> amdgpu_asic_need_reset_on_init(adev)) {
> + if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
> >set_asic_baco_cap) {
> + r = adev->powerplay.pp_funcs->set_asic_baco_cap(adev-
> >powerplay.pp_handle);
> + if (r) {
> + dev_err(adev->dev, "set baco capability
> failed\n");
> + goto failed;
> + }
> + }
> +
> r = amdgpu_asic_reset(adev);
> if (r) {
> dev_err(adev->dev, "asic reset on init failed\n");
> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> index 2b579ba..0dcc18d 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -285,6 +285,7 @@ struct amd_pm_funcs {
> int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
> int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
> int (*get_asic_baco_capability)(void *handle, bool *cap);
> + int (*set_asic_baco_cap)(void *handle);
> int (*get_asic_baco_state)(void *handle, int *state);
> int (*set_asic_baco_state)(void *handle, int state);
> int (*get_ppfeature_status)(void *handle, char *buf);
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index bea1587..9856760 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void
> *handle, uint32_t count)
> return ret;
> }
>
> +static int pp_set_asic_baco_cap(void *handle)
> +{
> + struct pp_hwmgr *hwmgr = handle;
> +
> + if (!hwmgr)
> + return -EINVAL;
> +
> + if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
> + return 0;
> +
> + hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
> +
> + return 0;
> +}
> +
> static int pp_get_asic_baco_capability(void *handle, bool *cap)
> {
> struct pp_hwmgr *hwmgr = handle;
> @@ -1546,6 +1561,7 @@ static const struct amd_pm_funcs pp_dpm_funcs =
> {
> .set_hard_min_dcefclk_by_freq = pp_set_hard_min_dcefclk_by_freq,
> .set_hard_min_fclk_by_freq = pp_set_hard_min_fclk_by_freq,
> .get_asic_baco_capability = pp_get_asic_baco_capability,
> + .set_asic_baco_cap = pp_set_asic_baco_cap,
> .get_asic_baco_state = pp_get_asic_baco_state,
> .set_asic_baco_state = pp_set_asic_baco_state,
> .get_ppfeature_status = pp_get_ppfeature_status,
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index ed6c638..8dc23eb 100644
> ---
[PATCH v2] drm/amdgpu: Need to set the baco cap before baco reset
Signed-off-by: Emily Deng
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +-
drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
.../amd/powerplay/hwmgr/vega10_processpptables.c | 22 ++
.../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
7 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index d6286ed..5288763 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2605,7 +2605,15 @@ int amdgpu_device_init(struct amdgpu_device *adev,
/* check if we need to reset the asic
* E.g., driver was not cleanly unloaded previously, etc.
*/
- if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
+ if (amdgpu_passthrough(adev) && amdgpu_asic_need_reset_on_init(adev)) {
+ if (adev->powerplay.pp_funcs &&
adev->powerplay.pp_funcs->set_asic_baco_cap) {
+ r =
adev->powerplay.pp_funcs->set_asic_baco_cap(adev->powerplay.pp_handle);
+ if (r) {
+ dev_err(adev->dev, "set baco capability
failed\n");
+ goto failed;
+ }
+ }
+
r = amdgpu_asic_reset(adev);
if (r) {
dev_err(adev->dev, "asic reset on init failed\n");
diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
index 2b579ba..0dcc18d 100644
--- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
@@ -285,6 +285,7 @@ struct amd_pm_funcs {
int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
int (*get_asic_baco_capability)(void *handle, bool *cap);
+ int (*set_asic_baco_cap)(void *handle);
int (*get_asic_baco_state)(void *handle, int *state);
int (*set_asic_baco_state)(void *handle, int state);
int (*get_ppfeature_status)(void *handle, char *buf);
diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
index bea1587..9856760 100644
--- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
+++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
@@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void *handle,
uint32_t count)
return ret;
}
+static int pp_set_asic_baco_cap(void *handle)
+{
+ struct pp_hwmgr *hwmgr = handle;
+
+ if (!hwmgr)
+ return -EINVAL;
+
+ if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
+ return 0;
+
+ hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
+
+ return 0;
+}
+
static int pp_get_asic_baco_capability(void *handle, bool *cap)
{
struct pp_hwmgr *hwmgr = handle;
@@ -1546,6 +1561,7 @@ static const struct amd_pm_funcs pp_dpm_funcs = {
.set_hard_min_dcefclk_by_freq = pp_set_hard_min_dcefclk_by_freq,
.set_hard_min_fclk_by_freq = pp_set_hard_min_fclk_by_freq,
.get_asic_baco_capability = pp_get_asic_baco_capability,
+ .set_asic_baco_cap = pp_set_asic_baco_cap,
.get_asic_baco_state = pp_get_asic_baco_state,
.set_asic_baco_state = pp_set_asic_baco_state,
.get_ppfeature_status = pp_get_ppfeature_status,
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
index ed6c638..8dc23eb 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
@@ -5171,6 +5171,7 @@ static const struct pp_hwmgr_func vega10_hwmgr_funcs = {
.odn_edit_dpm_table = vega10_odn_edit_dpm_table,
.get_performance_level = vega10_get_performance_level,
.get_asic_baco_capability = smu9_baco_get_capability,
+ .set_asic_baco_cap = vega10_baco_set_cap,
.get_asic_baco_state = smu9_baco_get_state,
.set_asic_baco_state = vega10_baco_set_state,
.enable_mgpu_fan_boost = vega10_enable_mgpu_fan_boost,
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
index b6767d7..8fdeb23 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
@@ -1371,3 +1371,25 @@ int vega10_get_powerplay_table_entry(struct pp_hwmgr
*hwmgr,
return result;
}
+
+int vega10_baco_set_cap(struct pp_hwmgr *hwmgr)
+{
+ int result = 0;
+ const ATOM_Vega10_POWERPLAYTABLE *powerplay_table;
+
+
RE: [PATCH] drm/amdgpu: Need to set the baco cap before baco reset
With this limited to passthough case only, this patch is reviewed-by: Evan Quan
Regards,
Evan
> -Original Message-
> From: amd-gfx On Behalf Of
> Emily Deng
> Sent: Wednesday, May 22, 2019 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deng, Emily
> Subject: [PATCH] drm/amdgpu: Need to set the baco cap before baco reset
>
> [CAUTION: External Email]
>
> For passthrough, after rebooted the VM, driver will do a baco reset before
> doing other driver initialization during loading driver. For doing the baco
> reset, it will first check the baco reset capability. So first need to set
> the cap
> from the vbios information or baco reset won't be enabled.
>
> Signed-off-by: Emily Deng
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8
> drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
>
> drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
> .../amd/powerplay/hwmgr/vega10_processpptables.c | 22
> ++
> .../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
> drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
> 7 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d6286ed..14415b3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2606,6 +2606,14 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
> * E.g., driver was not cleanly unloaded previously, etc.
> */
> if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev))
> {
> + if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
> >set_asic_baco_cap) {
> + r = adev->powerplay.pp_funcs->set_asic_baco_cap(adev-
> >powerplay.pp_handle);
> + if (r) {
> + dev_err(adev->dev, "set baco capability
> failed\n");
> + goto failed;
> + }
> + }
> +
> r = amdgpu_asic_reset(adev);
> if (r) {
> dev_err(adev->dev, "asic reset on init failed\n");
> diff --git
> a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> index 2b579ba..0dcc18d 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -285,6 +285,7 @@ struct amd_pm_funcs {
> int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
> int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
> int (*get_asic_baco_capability)(void *handle, bool *cap);
> + int (*set_asic_baco_cap)(void *handle);
> int (*get_asic_baco_state)(void *handle, int *state);
> int (*set_asic_baco_state)(void *handle, int state);
> int (*get_ppfeature_status)(void *handle, char *buf); diff --git
> a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index bea1587..9856760 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void
> *handle, uint32_t count)
> return ret;
> }
>
> +static int pp_set_asic_baco_cap(void *handle) {
> + struct pp_hwmgr *hwmgr = handle;
> +
> + if (!hwmgr)
> + return -EINVAL;
> +
> + if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
> + return 0;
> +
> + hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
> +
> + return 0;
> +}
> +
> static int pp_get_asic_baco_capability(void *handle, bool *cap) {
> struct pp_hwmgr *hwmgr = handle; @@ -1546,6 +1561,7 @@ static const
> struct amd_pm_funcs pp_dpm_funcs = {
> .set_hard_min_dcefclk_by_freq = pp_set_hard_min_dcefclk_by_freq,
> .set_hard_min_fclk_by_freq = pp_set_hard_min_fclk_by_freq,
> .get_asic_baco_capability = pp_get_asic_baco_capability,
> + .set_asic_baco_cap = pp_set_asic_baco_cap,
> .get_asic_baco_state = pp_get_asic_baco_state,
> .set_asic_baco_state = pp_set_asic_baco_state,
> .get_ppfeature_status = pp_get_ppfeature_status, diff --git
> a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index ed6c638..8dc23eb 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -5171,6 +5171,7 @@ static const struct pp_hwmgr_func
> vega10_hwmgr_funcs = {
> .odn_edit_dpm_table = vega10_odn_edit_dpm_table,
> .get_performance_level = vega10_get_performance_level,
> .get_asic_baco_capability = smu9_baco_get_capability,
> + .set_asic_baco_cap = vega10_baco_set_cap,
>
Re: GPU passthrough support for Stoney [Radeon R2/R3/R4/R5 Graphics]?
On Wed, May 22, 2019 at 7:00 PM Micah Morton wrote: > > On Wed, May 22, 2019 at 1:39 PM Alex Deucher wrote: > > > > On Tue, May 21, 2019 at 1:46 PM Micah Morton wrote: > > > > > > On Fri, May 17, 2019 at 9:59 AM Alex Deucher > > > wrote: > > > > > > > > On Fri, May 17, 2019 at 11:36 AM Micah Morton > > > > wrote: > > > > > > > > > > On Thu, May 16, 2019 at 1:39 PM Alex Deucher > > > > > wrote: > > > > > > > > > > > > On Thu, May 16, 2019 at 4:07 PM Micah Morton > > > > > > wrote: > > > > > > > > > > > > > > On Wed, May 15, 2019 at 7:19 PM Alex Deucher > > > > > > > wrote: > > > > > > > > > > > > > > > > On Wed, May 15, 2019 at 2:26 PM Micah Morton > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > > > > > > > I'm interested in running a VM on a system with an integrated > > > > > > > > > Stoney > > > > > > > > > [Radeon R2/R3/R4/R5 Graphics] card and passing through the > > > > > > > > > graphics > > > > > > > > > card to the VM using the IOMMU. I'm wondering whether this is > > > > > > > > > feasible > > > > > > > > > and supposed to be doable with the right setup (as opposed to > > > > > > > > > passing > > > > > > > > > a discrete GPU to the VM, which I think is definitely > > > > > > > > > doable?). > > > > > > > > > > > > > > > > > > So far, I can do all the qemu/kvm/vfio/iommu stuff to run the > > > > > > > > > VM and > > > > > > > > > pass the integrated GPU to it, but the drm driver in the VM > > > > > > > > > fails > > > > > > > > > during amdgpu_device_init(). Specifically, the logs show the > > > > > > > > > SMU being > > > > > > > > > unresponsive, which leads to a 'SMU firmware load failed' > > > > > > > > > error > > > > > > > > > message and kernel panic. I can share VM logs and the > > > > > > > > > invocation of > > > > > > > > > qemu and such if helpful, but first wanted to know at a high > > > > > > > > > level if > > > > > > > > > this should be feasible? > > > > > > > > > > > > > > > > > > P.S.: I'm not initializing the GPU in the host bios or host > > > > > > > > > kernel at > > > > > > > > > all, so I should be passing a fresh GPU to the VM. Also, I'm > > > > > > > > > pretty > > > > > > > > > sure I'm running the correct VGA bios for this GPU in the > > > > > > > > > guest VM > > > > > > > > > bios before guest boot. > > > > > > > > > > > > > > > > > > Any comments/suggestions would be appreciated! > > > > > > > > > > > > > > > > It should work in at least once as long as your vm is properly > > > > > > > > set up. > > > > > > > > > > > > > > Is there any reason running coreboot vs UEFI at host boot would > > > > > > > make a > > > > > > > difference? I was running a modified version of coreboot that > > > > > > > avoids > > > > > > > doing any GPU initialization in firmware -- so the first POST > > > > > > > happens > > > > > > > inside the guest. > > > > > > > > > > > > The GPU on APUs shares a bunch of resources with the CPU. There > > > > > > are a > > > > > > bunch of blocks which are shared and need to be initialized on both > > > > > > for everything to work properly. > > > > > > > > > > Interesting. So skipping running the vbios in the host and waiting > > > > > until running it for the first time in the guest SeaBIOS is a bad > > > > > idea? Would it be better to let APU+CPU initialize normally in the > > > > > host and then skip trying to run the vbios in guest SeaBIOS and just > > > > > do some kind of reset before the drm driver starts accessing it from > > > > > the guest? > > > > > > > > If you let the sbios initialize things, it should work. The driver > > > > will do the right thing to init the card when it loads whether its > > > > running on bare metal or in a VM. We've never tested any scenarios > > > > where the GPU on APUs is not handled by the sbios. Note that the GPU > > > > does not have to be posted per se, it just needs to have been properly > > > > taken into account when the sbios comes up so that shared components > > > > are initialized correctly. I don't know what your patched system does > > > > or doesn't do with respect to the platform initialization. > > > > > > So it sounds like you are suggesting the following: > > > > > > a) Run the vbios as part of the host sbios to initialize stuff and > > > patch up the vbios > > > > > > b) Don't run the drm/amdgpu driver in the host, wait until the guest for > > > that > > > > > > c) Avoid running the vbios (again) in the guest sbios since it has > > > already been run. Although since "the driver needs access to the vbios > > > image in the guest to get device specific configuration details" I > > > should still do '-device > > > vfio-pci,...,romfile=/path/to/vbios-that-was-fixed-up-by-host-sbios', > > > but should patch the guest sbios to avoid running the vbios again in > > > the guest? > > > > > > d) run the drm/amdgpu driver in the guest on the hardware that was > > > initialized in the host sbios > > > > > > Am I getting
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 02:49:28PM +0100, Dave Martin wrote: > On Tue, May 21, 2019 at 03:48:56PM -0300, Jason Gunthorpe wrote: > > On Fri, May 17, 2019 at 03:49:31PM +0100, Catalin Marinas wrote: > > > > > The tagged pointers (whether hwasan or MTE) should ideally be a > > > transparent feature for the application writer but I don't think we can > > > solve it entirely and make it seamless for the multitude of ioctls(). > > > I'd say you only opt in to such feature if you know what you are doing > > > and the user code takes care of specific cases like ioctl(), hence the > > > prctl() proposal even for the hwasan. > > > > I'm not sure such a dire view is warrented.. > > > > The ioctl situation is not so bad, other than a few special cases, > > most drivers just take a 'void __user *' and pass it as an argument to > > some function that accepts a 'void __user *'. sparse et al verify > > this. > > > > As long as the core functions do the right thing the drivers will be > > OK. > > > > The only place things get dicy is if someone casts to unsigned long > > (ie for vma work) but I think that reflects that our driver facing > > APIs for VMAs are compatible with static analysis (ie I have no > > earthly idea why get_user_pages() accepts an unsigned long), not that > > this is too hard. > > If multiple people will care about this, perhaps we should try to > annotate types more explicitly in SYSCALL_DEFINEx() and ABI data > structures. > > For example, we could have a couple of mutually exclusive modifiers > > T __object * > T __vaddr * (or U __vaddr) > > In the first case the pointer points to an object (in the C sense) > that the call may dereference but not use for any other purpose. How would you use these two differently? So far the kernel has worked that __user should tag any pointer that is from userspace and then you can't do anything with it until you transform it into a kernel something > to tell static analysers the real type of pointers smuggled through > UAPI disguised as other types (*cough* KVM, etc.) Yes, that would help alot, we often have to pass pointers through a u64 in the uAPI, and there is no static checker support to make sure they are run through the u64_to_user_ptr() helper. Jason
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 4:03 PM Evgenii Stepanov wrote:
>
> On Wed, May 22, 2019 at 1:47 PM Kees Cook wrote:
> >
> > On Wed, May 22, 2019 at 05:35:27PM +0100, Catalin Marinas wrote:
> > > The two hard requirements I have for supporting any new hardware feature
> > > in Linux are (1) a single kernel image binary continues to run on old
> > > hardware while making use of the new feature if available and (2) old
> > > user space continues to run on new hardware while new user space can
> > > take advantage of the new feature.
> >
> > Agreed! And I think the series meets these requirements, yes?
> >
> > > For MTE, we just can't enable it by default since there are applications
> > > who use the top byte of a pointer and expect it to be ignored rather
> > > than failing with a mismatched tag. Just think of a hwasan compiled
> > > binary where TBI is expected to work and you try to run it with MTE
> > > turned on.
> >
> > Ah! Okay, here's the use-case I wasn't thinking of: the concern is TBI
> > conflicting with MTE. And anything that starts using TBI suddenly can't
> > run in the future because it's being interpreted as MTE bits? (Is that
> > the ABI concern? I feel like we got into the weeds about ioctl()s and
> > one-off bugs...)
> >
> > So there needs to be some way to let the kernel know which of three
> > things it should be doing:
> > 1- leaving userspace addresses as-is (present)
> > 2- wiping the top bits before using (this series)
> > 3- wiping the top bits for most things, but retaining them for MTE as
> >needed (the future)
> >
> > I expect MTE to be the "default" in the future. Once a system's libc has
> > grown support for it, everything will be trying to use MTE. TBI will be
> > the special case (but TBI is effectively a prerequisite).
> >
> > AFAICT, the only difference I see between 2 and 3 will be the tag handling
> > in usercopy (all other places will continue to ignore the top bits). Is
> > that accurate?
> >
> > Is "1" a per-process state we want to keep? (I assume not, but rather it
> > is available via no TBI/MTE CONFIG or a boot-time option, if at all?)
> >
> > To choose between "2" and "3", it seems we need a per-process flag to
> > opt into TBI (and out of MTE). For userspace, how would a future binary
> > choose TBI over MTE? If it's a library issue, we can't use an ELF bit,
> > since the choice may be "late" after ELF load (this implies the need
> > for a prctl().) If it's binary-only ("built with HWKASan") then an ELF
> > bit seems sufficient. And without the marking, I'd expect the kernel to
> > enforce MTE when there are high bits.
> >
> > > I would also expect the C library or dynamic loader to check for the
> > > presence of a HWCAP_MTE bit before starting to tag memory allocations,
> > > otherwise it would get SIGILL on the first MTE instruction it tries to
> > > execute.
> >
> > I've got the same question as Elliot: aren't MTE instructions just NOP
> > to older CPUs? I.e. if the CPU (or kernel) don't support it, it just
> > gets entirely ignored: checking is only needed to satisfy curiosity
> > or behavioral expectations.
>
> MTE instructions are not NOP. Most of them have side effects (changing
> register values, zeroing memory).
no, i meant "they're encoded in a space that was previously no-ops, so
running on MTE code on old hardware doesn't cause SIGILL".
> This only matters for stack tagging, though. Heap tagging is a runtime
> decision in the allocator.
>
> If an image needs to run on old hardware, it will have to do heap tagging
> only.
>
> > To me, the conflict seems to be using TBI in the face of expecting MTE to
> > be the default state of the future. (But the internal changes needed
> > for TBI -- this series -- is a prereq for MTE.)
> >
> > --
> > Kees Cook
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 1:47 PM Kees Cook wrote:
>
> On Wed, May 22, 2019 at 05:35:27PM +0100, Catalin Marinas wrote:
> > The two hard requirements I have for supporting any new hardware feature
> > in Linux are (1) a single kernel image binary continues to run on old
> > hardware while making use of the new feature if available and (2) old
> > user space continues to run on new hardware while new user space can
> > take advantage of the new feature.
>
> Agreed! And I think the series meets these requirements, yes?
>
> > For MTE, we just can't enable it by default since there are applications
> > who use the top byte of a pointer and expect it to be ignored rather
> > than failing with a mismatched tag. Just think of a hwasan compiled
> > binary where TBI is expected to work and you try to run it with MTE
> > turned on.
>
> Ah! Okay, here's the use-case I wasn't thinking of: the concern is TBI
> conflicting with MTE. And anything that starts using TBI suddenly can't
> run in the future because it's being interpreted as MTE bits? (Is that
> the ABI concern? I feel like we got into the weeds about ioctl()s and
> one-off bugs...)
>
> So there needs to be some way to let the kernel know which of three
> things it should be doing:
> 1- leaving userspace addresses as-is (present)
> 2- wiping the top bits before using (this series)
> 3- wiping the top bits for most things, but retaining them for MTE as
>needed (the future)
>
> I expect MTE to be the "default" in the future. Once a system's libc has
> grown support for it, everything will be trying to use MTE. TBI will be
> the special case (but TBI is effectively a prerequisite).
>
> AFAICT, the only difference I see between 2 and 3 will be the tag handling
> in usercopy (all other places will continue to ignore the top bits). Is
> that accurate?
>
> Is "1" a per-process state we want to keep? (I assume not, but rather it
> is available via no TBI/MTE CONFIG or a boot-time option, if at all?)
>
> To choose between "2" and "3", it seems we need a per-process flag to
> opt into TBI (and out of MTE). For userspace, how would a future binary
> choose TBI over MTE? If it's a library issue, we can't use an ELF bit,
> since the choice may be "late" after ELF load (this implies the need
> for a prctl().) If it's binary-only ("built with HWKASan") then an ELF
> bit seems sufficient. And without the marking, I'd expect the kernel to
> enforce MTE when there are high bits.
>
> > I would also expect the C library or dynamic loader to check for the
> > presence of a HWCAP_MTE bit before starting to tag memory allocations,
> > otherwise it would get SIGILL on the first MTE instruction it tries to
> > execute.
>
> I've got the same question as Elliot: aren't MTE instructions just NOP
> to older CPUs? I.e. if the CPU (or kernel) don't support it, it just
> gets entirely ignored: checking is only needed to satisfy curiosity
> or behavioral expectations.
MTE instructions are not NOP. Most of them have side effects (changing
register values, zeroing memory).
This only matters for stack tagging, though. Heap tagging is a runtime
decision in the allocator.
If an image needs to run on old hardware, it will have to do heap tagging only.
> To me, the conflict seems to be using TBI in the face of expecting MTE to
> be the default state of the future. (But the internal changes needed
> for TBI -- this series -- is a prereq for MTE.)
>
> --
> Kees Cook
Re: GPU passthrough support for Stoney [Radeon R2/R3/R4/R5 Graphics]?
On Wed, May 22, 2019 at 1:39 PM Alex Deucher wrote: > > On Tue, May 21, 2019 at 1:46 PM Micah Morton wrote: > > > > On Fri, May 17, 2019 at 9:59 AM Alex Deucher wrote: > > > > > > On Fri, May 17, 2019 at 11:36 AM Micah Morton > > > wrote: > > > > > > > > On Thu, May 16, 2019 at 1:39 PM Alex Deucher > > > > wrote: > > > > > > > > > > On Thu, May 16, 2019 at 4:07 PM Micah Morton > > > > > wrote: > > > > > > > > > > > > On Wed, May 15, 2019 at 7:19 PM Alex Deucher > > > > > > wrote: > > > > > > > > > > > > > > On Wed, May 15, 2019 at 2:26 PM Micah Morton > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > > > > > I'm interested in running a VM on a system with an integrated > > > > > > > > Stoney > > > > > > > > [Radeon R2/R3/R4/R5 Graphics] card and passing through the > > > > > > > > graphics > > > > > > > > card to the VM using the IOMMU. I'm wondering whether this is > > > > > > > > feasible > > > > > > > > and supposed to be doable with the right setup (as opposed to > > > > > > > > passing > > > > > > > > a discrete GPU to the VM, which I think is definitely doable?). > > > > > > > > > > > > > > > > So far, I can do all the qemu/kvm/vfio/iommu stuff to run the > > > > > > > > VM and > > > > > > > > pass the integrated GPU to it, but the drm driver in the VM > > > > > > > > fails > > > > > > > > during amdgpu_device_init(). Specifically, the logs show the > > > > > > > > SMU being > > > > > > > > unresponsive, which leads to a 'SMU firmware load failed' error > > > > > > > > message and kernel panic. I can share VM logs and the > > > > > > > > invocation of > > > > > > > > qemu and such if helpful, but first wanted to know at a high > > > > > > > > level if > > > > > > > > this should be feasible? > > > > > > > > > > > > > > > > P.S.: I'm not initializing the GPU in the host bios or host > > > > > > > > kernel at > > > > > > > > all, so I should be passing a fresh GPU to the VM. Also, I'm > > > > > > > > pretty > > > > > > > > sure I'm running the correct VGA bios for this GPU in the guest > > > > > > > > VM > > > > > > > > bios before guest boot. > > > > > > > > > > > > > > > > Any comments/suggestions would be appreciated! > > > > > > > > > > > > > > It should work in at least once as long as your vm is properly > > > > > > > set up. > > > > > > > > > > > > Is there any reason running coreboot vs UEFI at host boot would > > > > > > make a > > > > > > difference? I was running a modified version of coreboot that avoids > > > > > > doing any GPU initialization in firmware -- so the first POST > > > > > > happens > > > > > > inside the guest. > > > > > > > > > > The GPU on APUs shares a bunch of resources with the CPU. There are a > > > > > bunch of blocks which are shared and need to be initialized on both > > > > > for everything to work properly. > > > > > > > > Interesting. So skipping running the vbios in the host and waiting > > > > until running it for the first time in the guest SeaBIOS is a bad > > > > idea? Would it be better to let APU+CPU initialize normally in the > > > > host and then skip trying to run the vbios in guest SeaBIOS and just > > > > do some kind of reset before the drm driver starts accessing it from > > > > the guest? > > > > > > If you let the sbios initialize things, it should work. The driver > > > will do the right thing to init the card when it loads whether its > > > running on bare metal or in a VM. We've never tested any scenarios > > > where the GPU on APUs is not handled by the sbios. Note that the GPU > > > does not have to be posted per se, it just needs to have been properly > > > taken into account when the sbios comes up so that shared components > > > are initialized correctly. I don't know what your patched system does > > > or doesn't do with respect to the platform initialization. > > > > So it sounds like you are suggesting the following: > > > > a) Run the vbios as part of the host sbios to initialize stuff and > > patch up the vbios > > > > b) Don't run the drm/amdgpu driver in the host, wait until the guest for > > that > > > > c) Avoid running the vbios (again) in the guest sbios since it has > > already been run. Although since "the driver needs access to the vbios > > image in the guest to get device specific configuration details" I > > should still do '-device > > vfio-pci,...,romfile=/path/to/vbios-that-was-fixed-up-by-host-sbios', > > but should patch the guest sbios to avoid running the vbios again in > > the guest? > > > > d) run the drm/amdgpu driver in the guest on the hardware that was > > initialized in the host sbios > > > > Am I getting this right? > > > > I would suggest starting with a standard sbios/vbios. Boot the system > as usual. You can blacklist amdgpu if you don't want gpu driver > loaded on the host. Get a copy of the vbios image. Depending on the > platform, it may be available via the standard vbios shadow location > or it may be at the start of carveout
Re: [PATCH 4/7] drm/amdgpu: Add function to add/remove gws to kfd process
On 2019-05-22 11:51 a.m., Zeng, Oak wrote:
> GWS bo is shared between all kfd processes. Add function to add gws
> to kfd process's bo list so gws can be evicted from and restored
> for process.
>
> Change-Id: I75d74cfdadb7075ff8b2b68634e205deb73dc1ea
> Signed-off-by: Oak Zeng
Reviewed-by: Felix Kuehling
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 103
> +--
> 2 files changed, 100 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index c00c974..f968bf1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -155,6 +155,8 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct kgd_dev *kgd,
> size_t size,
> void amdgpu_amdkfd_free_gtt_mem(struct kgd_dev *kgd, void *mem_obj);
> int amdgpu_amdkfd_alloc_gws(struct kgd_dev *kgd, size_t size, void
> **mem_obj);
> void amdgpu_amdkfd_free_gws(struct kgd_dev *kgd, void *mem_obj);
> +int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem
> **mem);
> +int amdgpu_amdkfd_remove_gws_from_process(void *info, void *mem);
> uint32_t amdgpu_amdkfd_get_fw_version(struct kgd_dev *kgd,
> enum kgd_engine_type type);
> void amdgpu_amdkfd_get_local_mem_info(struct kgd_dev *kgd,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index e1cae4a..87177ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -457,6 +457,17 @@ static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem
> *mem,
> mutex_unlock(_info->lock);
> }
>
> +static void remove_kgd_mem_from_kfd_bo_list(struct kgd_mem *mem,
> + struct amdkfd_process_info *process_info)
> +{
> + struct ttm_validate_buffer *bo_list_entry;
> +
> + bo_list_entry = >validate_list;
> + mutex_lock(_info->lock);
> + list_del(_list_entry->head);
> + mutex_unlock(_info->lock);
> +}
> +
> /* Initializes user pages. It registers the MMU notifier and validates
>* the userptr BO in the GTT domain.
>*
> @@ -1183,12 +1194,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>
> if (user_addr) {
> ret = init_user_pages(*mem, current->mm, user_addr);
> - if (ret) {
> - mutex_lock(>process_info->lock);
> - list_del(&(*mem)->validate_list.head);
> - mutex_unlock(>process_info->lock);
> + if (ret)
> goto allocate_init_user_pages_failed;
> - }
> }
>
> if (offset)
> @@ -1197,6 +1204,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
> return 0;
>
> allocate_init_user_pages_failed:
> + remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
> amdgpu_bo_unref();
> /* Don't unreserve system mem limit twice */
> goto err_reserve_limit;
> @@ -2104,3 +2112,88 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void
> *info, struct dma_fence **ef)
> kfree(pd_bo_list);
> return ret;
> }
> +
> +int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem
> **mem)
> +{
> + struct amdkfd_process_info *process_info = (struct amdkfd_process_info
> *)info;
> + struct amdgpu_bo *gws_bo = (struct amdgpu_bo *)gws;
> + int ret;
> +
> + if (!info || !gws)
> + return -EINVAL;
> +
> + *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
> + if (!*mem)
> + return -EINVAL;
> +
> + mutex_init(&(*mem)->lock);
> + (*mem)->bo = amdgpu_bo_ref(gws_bo);
> + (*mem)->domain = AMDGPU_GEM_DOMAIN_GWS;
> + (*mem)->process_info = process_info;
> + add_kgd_mem_to_kfd_bo_list(*mem, process_info, false);
> + amdgpu_sync_create(&(*mem)->sync);
> +
> +
> + /* Validate gws bo the first time it is added to process */
> + mutex_lock(&(*mem)->process_info->lock);
> + ret = amdgpu_bo_reserve(gws_bo, false);
> + if (unlikely(ret)) {
> + pr_err("Reserve gws bo failed %d\n", ret);
> + goto bo_reservation_failure;
> + }
> +
> + ret = amdgpu_amdkfd_bo_validate(gws_bo, AMDGPU_GEM_DOMAIN_GWS, true);
> + if (ret) {
> + pr_err("GWS BO validate failed %d\n", ret);
> + goto bo_validation_failure;
> + }
> + /* GWS resource is shared b/t amdgpu and amdkfd
> + * Add process eviction fence to bo so they can
> + * evict each other.
> + */
> + amdgpu_bo_fence(gws_bo, _info->eviction_fence->base, true);
> + amdgpu_bo_unreserve(gws_bo);
> + mutex_unlock(&(*mem)->process_info->lock);
> +
> + return ret;
> +
> +bo_validation_failure:
> + amdgpu_bo_unreserve(gws_bo);
> +bo_reservation_failure:
> +
Re: [PATCH 6/7] drm/amdkfd: New IOCTL to allocate queue GWS
On 2019-05-22 11:51 a.m., Zeng, Oak wrote:
> Add a new kfd ioctl to allocate queue GWS. Queue
> GWS is released on queue destroy.
>
> Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
> Signed-off-by: Oak Zeng
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 39
>
> include/uapi/linux/kfd_ioctl.h | 20 +++-
> 2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 38ae53f..828bd66 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1559,6 +1559,43 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file
> *filep,
> return err;
> }
>
> +static int kfd_ioctl_alloc_queue_gws(struct file *filep,
> + struct kfd_process *p, void *data)
> +{
> + int retval;
> + struct kfd_ioctl_alloc_queue_gws_args *args = data;
> + struct kfd_dev *dev = NULL;
> + struct kgd_mem *mem;
> +
> + if (args->num_gws == 0)
> + return -EINVAL;
> +
> + if (!hws_gws_support ||
> + dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
> + return -EINVAL;
> +
> + dev = kfd_device_by_id(args->gpu_id);
> + if (!dev) {
> + pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
> + return -EINVAL;
> + }
> +
> + retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
> + dev->gws, );
Do you want to move this into pqm_set_gws? The corresponding call to
amdgpu_amdkfd_remove_from_gws is handled in pqm (during queue
destruction) too.
Regards,
Felix
> + if (unlikely(retval))
> + return retval;
> +
> + mutex_lock(>mutex);
> + retval = pqm_set_gws(>pqm, args->queue_id, mem);
> + mutex_unlock(>mutex);
> +
> + if (unlikely(retval))
> + amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
> +
> + args->first_gws = 0;
> + return retval;
> +}
> +
> static int kfd_ioctl_get_dmabuf_info(struct file *filep,
> struct kfd_process *p, void *data)
> {
> @@ -1761,6 +1798,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] =
> {
> AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
> kfd_ioctl_import_dmabuf, 0),
>
> + AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
> + kfd_ioctl_alloc_queue_gws, 0),
> };
>
> #define AMDKFD_CORE_IOCTL_COUNT ARRAY_SIZE(amdkfd_ioctls)
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 20917c5..070d1bc 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -410,6 +410,21 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
> __u32 n_success;/* to/from KFD */
> };
>
> +/* Allocate GWS for specific queue
> + *
> + * @gpu_id: device identifier
> + * @queue_id:queue's id that GWS is allocated for
> + * @num_gws: how many GWS to allocate
> + * @first_gws: index of the first GWS allocated.
> + * only support contiguous GWS allocation
> + */
> +struct kfd_ioctl_alloc_queue_gws_args {
> + __u32 gpu_id; /* to KFD */
> + __u32 queue_id; /* to KFD */
> + __u32 num_gws; /* to KFD */
> + __u32 first_gws;/* from KFD */
> +};
> +
> struct kfd_ioctl_get_dmabuf_info_args {
> __u64 size; /* from KFD */
> __u64 metadata_ptr; /* to KFD */
> @@ -529,7 +544,10 @@ enum kfd_mmio_remap {
> #define AMDKFD_IOC_IMPORT_DMABUF\
> AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
>
> +#define AMDKFD_IOC_ALLOC_QUEUE_GWS \
> + AMDKFD_IOWR(0x1E, struct kfd_ioctl_alloc_queue_gws_args)
> +
> #define AMDKFD_COMMAND_START0x01
> -#define AMDKFD_COMMAND_END 0x1E
> +#define AMDKFD_COMMAND_END 0x1F
>
> #endif
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 5/7] drm/amdkfd: Add function to set queue gws
On 2019-05-22 11:51 a.m., Zeng, Oak wrote:
> Add functions in process queue manager to
> set/get queue gws. Also set process's number
> of gws used. Currently only one queue in
> process can use and use all gws.
>
> Change-Id: I03e480c8692db3eabfc3a188cce8904d5d962ab7
> Signed-off-by: Oak Zeng
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 6 +++
> .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 58
> ++
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 5969e37..40a5c67 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -454,6 +454,9 @@ struct queue_properties {
>*
>* @device: The kfd device that created this queue.
>*
> + * @gws: Pointing to gws kgd_mem if this is a gws control queue; NULL
> + * otherwise.
> + *
>* This structure represents user mode compute queues.
>* It contains all the necessary data to handle such queues.
>*
> @@ -475,6 +478,7 @@ struct queue {
>
> struct kfd_process *process;
> struct kfd_dev *device;
> + void *gws;
> };
>
> /*
> @@ -868,6 +872,8 @@ int pqm_update_queue(struct process_queue_manager *pqm,
> unsigned int qid,
> struct queue_properties *p);
> int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
> struct queue_properties *p);
> +int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
> + void *gws);
> struct kernel_queue *pqm_get_kernel_queue(struct process_queue_manager *pqm,
> unsigned int qid);
> int pqm_get_wave_state(struct process_queue_manager *pqm,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> index e652e25..a78494d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -26,6 +26,7 @@
> #include "kfd_device_queue_manager.h"
> #include "kfd_priv.h"
> #include "kfd_kernel_queue.h"
> +#include "amdgpu_amdkfd.h"
>
> static inline struct process_queue_node *get_queue_by_qid(
> struct process_queue_manager *pqm, unsigned int qid)
> @@ -74,6 +75,57 @@ void kfd_process_dequeue_from_device(struct
> kfd_process_device *pdd)
> pdd->already_dequeued = true;
> }
>
> +int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
> + void *gws)
> +{
> + struct kfd_dev *dev = NULL;
> + struct process_queue_node *pqn;
> + struct kfd_process_device *pdd;
> +
> + pqn = get_queue_by_qid(pqm, qid);
> + if (!pqn) {
> + pr_err("Queue id does not match any known queue\n");
> + return -EINVAL;
> + }
> +
> + if (pqn->q)
> + dev = pqn->q->device;
> + if (WARN_ON(!dev))
> + return -ENODEV;
> +
> + pdd = kfd_get_process_device_data(dev, pqm->process);
> + if (!pdd) {
> + pr_err("Process device data doesn't exist\n");
> + return -EINVAL;
> + }
> +
> + /* Only allow one queue per process can have GWS assigned */
> + list_for_each_entry(pqn, >queues, process_queue_list) {
> + if (pqn->q && pqn->q->gws)
> + return -EINVAL;
> + }
> +
> + pqn->q->gws = gws;
> + pdd->qpd.num_gws = gws ? amdgpu_amdkfd_get_num_gws(dev->kgd) : 0;
> +
> + return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
> + pqn->q);
When pqm_set_gws is called during pqm_destroy_queue, this is
unnecessary. It would result in two preemptions and runlist updates
every time a queue is destroyed. See below.
> +}
> +
> +static void *pqm_get_gws(struct process_queue_manager *pqm, unsigned int qid)
> +{
> + struct process_queue_node *pqn;
> +
> + pqn = get_queue_by_qid(pqm, qid);
> + if (!pqn) {
> + pr_debug("No queue %d exists for get gws operation\n", qid);
> + return NULL;
> + }
> +
> + return pqn->q->gws;
> +}
> +
> +
> void kfd_process_dequeue_from_all_devices(struct kfd_process *p)
> {
> struct kfd_process_device *pdd;
> @@ -313,6 +365,12 @@ int pqm_destroy_queue(struct process_queue_manager *pqm,
> unsigned int qid)
> return -1;
> }
>
> + if (pqm_get_gws(pqm, qid)) {
All pqm_get_gws really does an extra lookup of the pqn, which you
already know here. Instead, could you just replace this with:
if (pqn->q->gws) ...
And remove pqm_get_gws.
> +
> amdgpu_amdkfd_remove_gws_from_process(pqm->process->kgd_process_info,
> + pqm_get_gws(pqm, qid));
> + pqm_set_gws(pqm, qid, NULL);
If you call pqm_set_gws here, it will
Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
On Wed, May 22, 2019 at 4:49 AM Catalin Marinas wrote:
>
> On Mon, May 06, 2019 at 06:30:51PM +0200, Andrey Konovalov wrote:
> > This patch is a part of a series that extends arm64 kernel ABI to allow to
> > pass tagged user pointers (with the top byte set to something else other
> > than 0x00) as syscall arguments.
> >
> > This patch allows tagged pointers to be passed to the following memory
> > syscalls: brk, get_mempolicy, madvise, mbind, mincore, mlock, mlock2,
> > mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> > remap_file_pages, shmat and shmdt.
> >
> > This is done by untagging pointers passed to these syscalls in the
> > prologues of their handlers.
>
> I'll go through them one by one to see if we can tighten the expected
> ABI while having the MTE in mind.
>
> > diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c
> > index b44065fb1616..933bb9f3d6ec 100644
> > --- a/arch/arm64/kernel/sys.c
> > +++ b/arch/arm64/kernel/sys.c
> > @@ -35,10 +35,33 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned
> > long, len,
> > {
> > if (offset_in_page(off) != 0)
> > return -EINVAL;
> > -
> > + addr = untagged_addr(addr);
> > return ksys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
> > }
>
> If user passes a tagged pointer to mmap() and the address is honoured
> (or MAP_FIXED is given), what is the expected return pointer? Does it
> need to be tagged with the value from the hint?
For HWASan the most convenient would be to use the tag from the hint.
But since in the TBI (not MTE) mode the kernel has no idea what
meaning userspace assigns to pointer tags, perhaps it should not try
to guess, and should return raw (zero-tagged) address instead.
> With MTE, we may want to use this as a request for the default colour of
> the mapped pages (still under discussion).
I like this - and in that case it would make sense to return the
pointer that can be immediately dereferenced without crashing the
process, i.e. with the matching tag.
> > +SYSCALL_DEFINE6(arm64_mmap_pgoff, unsigned long, addr, unsigned long, len,
> > + unsigned long, prot, unsigned long, flags,
> > + unsigned long, fd, unsigned long, pgoff)
> > +{
> > + addr = untagged_addr(addr);
> > + return ksys_mmap_pgoff(addr, len, prot, flags, fd, pgoff);
> > +}
>
> We don't have __NR_mmap_pgoff on arm64.
>
> > +SYSCALL_DEFINE5(arm64_mremap, unsigned long, addr, unsigned long, old_len,
> > + unsigned long, new_len, unsigned long, flags,
> > + unsigned long, new_addr)
> > +{
> > + addr = untagged_addr(addr);
> > + new_addr = untagged_addr(new_addr);
> > + return ksys_mremap(addr, old_len, new_len, flags, new_addr);
> > +}
>
> Similar comment as for mmap(), do we want the tag from new_addr to be
> preserved? In addition, should we check that the two tags are identical
> or mremap() should become a way to repaint a memory region?
>
> > +SYSCALL_DEFINE2(arm64_munmap, unsigned long, addr, size_t, len)
> > +{
> > + addr = untagged_addr(addr);
> > + return ksys_munmap(addr, len);
> > +}
>
> This looks fine.
>
> > +SYSCALL_DEFINE1(arm64_brk, unsigned long, brk)
> > +{
> > + brk = untagged_addr(brk);
> > + return ksys_brk(brk);
> > +}
>
> I wonder whether brk() should simply not accept tags, and should not
> return them (similar to the prctl(PR_SET_MM) discussion). We could
> document this in the ABI requirements.
>
> > +SYSCALL_DEFINE5(arm64_get_mempolicy, int __user *, policy,
> > + unsigned long __user *, nmask, unsigned long, maxnode,
> > + unsigned long, addr, unsigned long, flags)
> > +{
> > + addr = untagged_addr(addr);
> > + return ksys_get_mempolicy(policy, nmask, maxnode, addr, flags);
> > +}
> > +
> > +SYSCALL_DEFINE3(arm64_madvise, unsigned long, start,
> > + size_t, len_in, int, behavior)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_madvise(start, len_in, behavior);
> > +}
> > +
> > +SYSCALL_DEFINE6(arm64_mbind, unsigned long, start, unsigned long, len,
> > + unsigned long, mode, const unsigned long __user *, nmask,
> > + unsigned long, maxnode, unsigned int, flags)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mbind(start, len, mode, nmask, maxnode, flags);
> > +}
> > +
> > +SYSCALL_DEFINE2(arm64_mlock, unsigned long, start, size_t, len)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mlock(start, len, VM_LOCKED);
> > +}
> > +
> > +SYSCALL_DEFINE2(arm64_mlock2, unsigned long, start, size_t, len)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_mlock(start, len, VM_LOCKED);
> > +}
> > +
> > +SYSCALL_DEFINE2(arm64_munlock, unsigned long, start, size_t, len)
> > +{
> > + start = untagged_addr(start);
> > + return ksys_munlock(start, len);
> > +}
> > +
> > +SYSCALL_DEFINE3(arm64_mprotect, unsigned long, start, size_t, len,
> > +
[pull] amdgpu, amdkfd drm-fixes-5.2
Hi Dave, Daniel, Fixes for 5.2: - Fix for DMCU firmware issues for stable - Add missing polaris10 pci id to kfd - Screen corruption fix on picasso - Fix for driver reload on vega10 - SR-IOV fixes - Locking fix in new SMU code - Compute profile switching fix for KFD The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9: Linux 5.2-rc1 (2019-05-19 15:47:09 -0700) are available in the Git repository at: git://people.freedesktop.org/~agd5f/linux drm-fixes-5.2 for you to fetch changes up to 43d8107f0bdcaa4300e40231cc45ecbd1f77f73f: drm/amdkfd: Fix compute profile switching (2019-05-20 21:22:49 -0500) Alex Deucher (2): drm/amdgpu/soc15: skip reset on init drm/amdgpu/gmc9: set vram_width properly for SR-IOV Dan Carpenter (1): drm/amd/powerplay: fix locking in smu_feature_set_supported() Flora Cui (1): drm/amdgpu: keep stolen memory on picasso Harish Kasiviswanathan (1): drm/amdkfd: Fix compute profile switching Harry Wentland (2): drm/amd/display: Add ASICREV_IS_PICASSO drm/amd/display: Don't load DMCU for Raven 1 Kent Russell (1): drm/amdkfd: Add missing Polaris10 ID Yintian Tao (1): drm/amdgpu: skip fw pri bo alloc for SRIOV drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 17 ++--- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 11 ++- drivers/gpu/drm/amd/amdgpu/soc15.c| 5 + drivers/gpu/drm/amd/amdkfd/kfd_device.c | 17 + drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++- drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 7 +++ drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 ++-- drivers/gpu/drm/amd/display/include/dal_asic_id.h | 7 --- drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 2 +- 9 files changed, 70 insertions(+), 19 deletions(-) ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 05:35:27PM +0100, Catalin Marinas wrote:
> The two hard requirements I have for supporting any new hardware feature
> in Linux are (1) a single kernel image binary continues to run on old
> hardware while making use of the new feature if available and (2) old
> user space continues to run on new hardware while new user space can
> take advantage of the new feature.
Agreed! And I think the series meets these requirements, yes?
> For MTE, we just can't enable it by default since there are applications
> who use the top byte of a pointer and expect it to be ignored rather
> than failing with a mismatched tag. Just think of a hwasan compiled
> binary where TBI is expected to work and you try to run it with MTE
> turned on.
Ah! Okay, here's the use-case I wasn't thinking of: the concern is TBI
conflicting with MTE. And anything that starts using TBI suddenly can't
run in the future because it's being interpreted as MTE bits? (Is that
the ABI concern? I feel like we got into the weeds about ioctl()s and
one-off bugs...)
So there needs to be some way to let the kernel know which of three
things it should be doing:
1- leaving userspace addresses as-is (present)
2- wiping the top bits before using (this series)
3- wiping the top bits for most things, but retaining them for MTE as
needed (the future)
I expect MTE to be the "default" in the future. Once a system's libc has
grown support for it, everything will be trying to use MTE. TBI will be
the special case (but TBI is effectively a prerequisite).
AFAICT, the only difference I see between 2 and 3 will be the tag handling
in usercopy (all other places will continue to ignore the top bits). Is
that accurate?
Is "1" a per-process state we want to keep? (I assume not, but rather it
is available via no TBI/MTE CONFIG or a boot-time option, if at all?)
To choose between "2" and "3", it seems we need a per-process flag to
opt into TBI (and out of MTE). For userspace, how would a future binary
choose TBI over MTE? If it's a library issue, we can't use an ELF bit,
since the choice may be "late" after ELF load (this implies the need
for a prctl().) If it's binary-only ("built with HWKASan") then an ELF
bit seems sufficient. And without the marking, I'd expect the kernel to
enforce MTE when there are high bits.
> I would also expect the C library or dynamic loader to check for the
> presence of a HWCAP_MTE bit before starting to tag memory allocations,
> otherwise it would get SIGILL on the first MTE instruction it tries to
> execute.
I've got the same question as Elliot: aren't MTE instructions just NOP
to older CPUs? I.e. if the CPU (or kernel) don't support it, it just
gets entirely ignored: checking is only needed to satisfy curiosity
or behavioral expectations.
To me, the conflict seems to be using TBI in the face of expecting MTE to
be the default state of the future. (But the internal changes needed
for TBI -- this series -- is a prereq for MTE.)
--
Kees Cook
Re: GPU passthrough support for Stoney [Radeon R2/R3/R4/R5 Graphics]?
On Tue, May 21, 2019 at 1:46 PM Micah Morton wrote: > > On Fri, May 17, 2019 at 9:59 AM Alex Deucher wrote: > > > > On Fri, May 17, 2019 at 11:36 AM Micah Morton wrote: > > > > > > On Thu, May 16, 2019 at 1:39 PM Alex Deucher > > > wrote: > > > > > > > > On Thu, May 16, 2019 at 4:07 PM Micah Morton > > > > wrote: > > > > > > > > > > On Wed, May 15, 2019 at 7:19 PM Alex Deucher > > > > > wrote: > > > > > > > > > > > > On Wed, May 15, 2019 at 2:26 PM Micah Morton > > > > > > wrote: > > > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > > > I'm interested in running a VM on a system with an integrated > > > > > > > Stoney > > > > > > > [Radeon R2/R3/R4/R5 Graphics] card and passing through the > > > > > > > graphics > > > > > > > card to the VM using the IOMMU. I'm wondering whether this is > > > > > > > feasible > > > > > > > and supposed to be doable with the right setup (as opposed to > > > > > > > passing > > > > > > > a discrete GPU to the VM, which I think is definitely doable?). > > > > > > > > > > > > > > So far, I can do all the qemu/kvm/vfio/iommu stuff to run the VM > > > > > > > and > > > > > > > pass the integrated GPU to it, but the drm driver in the VM fails > > > > > > > during amdgpu_device_init(). Specifically, the logs show the SMU > > > > > > > being > > > > > > > unresponsive, which leads to a 'SMU firmware load failed' error > > > > > > > message and kernel panic. I can share VM logs and the invocation > > > > > > > of > > > > > > > qemu and such if helpful, but first wanted to know at a high > > > > > > > level if > > > > > > > this should be feasible? > > > > > > > > > > > > > > P.S.: I'm not initializing the GPU in the host bios or host > > > > > > > kernel at > > > > > > > all, so I should be passing a fresh GPU to the VM. Also, I'm > > > > > > > pretty > > > > > > > sure I'm running the correct VGA bios for this GPU in the guest VM > > > > > > > bios before guest boot. > > > > > > > > > > > > > > Any comments/suggestions would be appreciated! > > > > > > > > > > > > It should work in at least once as long as your vm is properly set > > > > > > up. > > > > > > > > > > Is there any reason running coreboot vs UEFI at host boot would make a > > > > > difference? I was running a modified version of coreboot that avoids > > > > > doing any GPU initialization in firmware -- so the first POST happens > > > > > inside the guest. > > > > > > > > The GPU on APUs shares a bunch of resources with the CPU. There are a > > > > bunch of blocks which are shared and need to be initialized on both > > > > for everything to work properly. > > > > > > Interesting. So skipping running the vbios in the host and waiting > > > until running it for the first time in the guest SeaBIOS is a bad > > > idea? Would it be better to let APU+CPU initialize normally in the > > > host and then skip trying to run the vbios in guest SeaBIOS and just > > > do some kind of reset before the drm driver starts accessing it from > > > the guest? > > > > If you let the sbios initialize things, it should work. The driver > > will do the right thing to init the card when it loads whether its > > running on bare metal or in a VM. We've never tested any scenarios > > where the GPU on APUs is not handled by the sbios. Note that the GPU > > does not have to be posted per se, it just needs to have been properly > > taken into account when the sbios comes up so that shared components > > are initialized correctly. I don't know what your patched system does > > or doesn't do with respect to the platform initialization. > > So it sounds like you are suggesting the following: > > a) Run the vbios as part of the host sbios to initialize stuff and > patch up the vbios > > b) Don't run the drm/amdgpu driver in the host, wait until the guest for that > > c) Avoid running the vbios (again) in the guest sbios since it has > already been run. Although since "the driver needs access to the vbios > image in the guest to get device specific configuration details" I > should still do '-device > vfio-pci,...,romfile=/path/to/vbios-that-was-fixed-up-by-host-sbios', > but should patch the guest sbios to avoid running the vbios again in > the guest? > > d) run the drm/amdgpu driver in the guest on the hardware that was > initialized in the host sbios > > Am I getting this right? > I would suggest starting with a standard sbios/vbios. Boot the system as usual. You can blacklist amdgpu if you don't want gpu driver loaded on the host. Get a copy of the vbios image. Depending on the platform, it may be available via the standard vbios shadow location or it may be at the start of carveout or it may be available via an ACPI interface. The driver has the logic to fetch it. You can dump a copy from the driver via /sys/kernel/debug/dri/X/amdgpu_vbios where X is the number of the dri device of the card in question. Then start VM with the device passed through. Don't worry about running the vbios or not in the host vs.
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 12:21 PM Kees Cook wrote: > > On Wed, May 22, 2019 at 08:30:21AM -0700, enh wrote: > > On Wed, May 22, 2019 at 3:11 AM Catalin Marinas > > wrote: > > > On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote: > > > > I just want to make sure I fully understand your concern about this > > > > being an ABI break, and I work best with examples. The closest situation > > > > I can see would be: > > > > > > > > - some program has no idea about MTE > > > > > > Apart from some libraries like libc (and maybe those that handle > > > specific device ioctls), I think most programs should have no idea about > > > MTE. I wouldn't expect programmers to have to change their app just > > > because we have a new feature that colours heap allocations. > > Right -- things should Just Work from the application perspective. > > > obviously i'm biased as a libc maintainer, but... > > > > i don't think it helps to move this to libc --- now you just have an > > extra dependency where to have a guaranteed working system you need to > > update your kernel and libc together. (or at least update your libc to > > understand new ioctls etc _before_ you can update your kernel.) > > I think (hope?) we've all agreed that we shouldn't pass this off to > userspace. At the very least, it reduces the utility of MTE, and at worst > it complicates userspace when this is clearly a kernel/architecture issue. > > > > > > > - malloc() starts returning MTE-tagged addresses > > > > - program doesn't break from that change > > > > - program uses some syscall that is missing untagged_addr() and fails > > > > - kernel has now broken userspace that used to work > > > > > > That's one aspect though probably more of a case of plugging in a new > > > device (graphics card, network etc.) and the ioctl to the new device > > > doesn't work. > > I think MTE will likely be rather like NX/PXN and SMAP/PAN: there will > be glitches, and we can disable stuff either via CONFIG or (as is more > common now) via a kernel commandline with untagged_addr() containing a > static branch, etc. But I actually don't think we need to go this route > (see below...) > > > > The other is that, assuming we reach a point where the kernel entirely > > > supports this relaxed ABI, can we guarantee that it won't break in the > > > future. Let's say some subsequent kernel change (some refactoring) > > > misses out an untagged_addr(). This renders a previously TBI/MTE-capable > > > syscall unusable. Can we rely only on testing? > > > > > > > The trouble I see with this is that it is largely theoretical and > > > > requires part of userspace to collude to start using a new CPU feature > > > > that tickles a bug in the kernel. As I understand the golden rule, > > > > this is a bug in the kernel (a missed ioctl() or such) to be fixed, > > > > not a global breaking of some userspace behavior. > > > > > > Yes, we should follow the rule that it's a kernel bug but it doesn't > > > help the user that a newly installed kernel causes user space to no > > > longer reach a prompt. Hence the proposal of an opt-in via personality > > > (for MTE we would need an explicit opt-in by the user anyway since the > > > top byte is no longer ignored but checked against the allocation tag). > > > > but realistically would this actually get used in this way? or would > > any given system either be MTE or non-MTE. in which case a kernel > > configuration option would seem to make more sense. (because either > > way, the hypothetical user basically needs to recompile the kernel to > > get back on their feet. or all of userspace.) > > Right: the point is to design things so that we do our best to not break > userspace that is using the new feature (which I think this series has > done well). But supporting MTE/TBI is just like supporting PAN: if someone > refactors a driver and swaps a copy_from_user() to a memcpy(), it's going > to break under PAN. There will be the same long tail of these bugs like > any other, but my sense is that they are small and rare. But I agree: > they're going to be pretty weird bugs to track down. The final result, > however, will be excellent annotation in the kernel for where userspace > addresses get used and people make assumptions about them. > > The sooner we get the series landed and gain QEMU support (or real > hardware), the faster we can hammer out these missed corner-cases. > What's the timeline for either of those things, BTW? > > > > > I feel like I'm missing something about this being seen as an ABI > > > > break. The kernel already fails on userspace addresses that have high > > > > bits set -- are there things that _depend_ on this failure to operate? > > > > > > It's about providing a relaxed ABI which allows non-zero top byte and > > > breaking it later inadvertently without having something better in place > > > to analyse the kernel changes. > > It sounds like the question is how to switch a process in or out of this > ABI (but I don't
Re: [PATCH 10/10] drm/amdgpu: stop removing BOs from the LRU v3
Can you explain how this avoids OOM situations? When is it safe to leave
a reserved BO on the LRU list? Could we do the same thing in
amdgpu_amdkfd_gpuvm.c? And if we did, what would be the expected side
effects or consequences?
Thanks,
Felix
On 2019-05-22 8:59 a.m., Christian König wrote:
> [CAUTION: External Email]
>
> This avoids OOM situations when we have lots of threads
> submitting at the same time.
>
> v3: apply this to the whole driver, not just CS
>
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c| 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c| 4 ++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 20f2955d2a55..3e2da24cd17a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -648,7 +648,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
> *p,
> }
>
> r = ttm_eu_reserve_buffers(>ticket, >validated, true,
> - , true);
> + , false);
> if (unlikely(r != 0)) {
> if (r != -ERESTARTSYS)
> DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> index 06f83cac0d3a..f660628e6af9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
> @@ -79,7 +79,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> list_add(_tv.head, );
> amdgpu_vm_get_pd_bo(vm, , );
>
> - r = ttm_eu_reserve_buffers(, , true, NULL, true);
> + r = ttm_eu_reserve_buffers(, , true, NULL, false);
> if (r) {
> DRM_ERROR("failed to reserve CSA,PD BOs: err=%d\n", r);
> return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index d513a5ad03dd..ed25a4e14404 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -171,7 +171,7 @@ void amdgpu_gem_object_close(struct drm_gem_object *obj,
>
> amdgpu_vm_get_pd_bo(vm, , _pd);
>
> - r = ttm_eu_reserve_buffers(, , false, , true);
> + r = ttm_eu_reserve_buffers(, , false, , false);
> if (r) {
> dev_err(adev->dev, "leaking bo va because "
> "we fail to reserve bo (%d)\n", r);
> @@ -608,7 +608,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void
> *data,
>
> amdgpu_vm_get_pd_bo(>vm, , _pd);
>
> - r = ttm_eu_reserve_buffers(, , true, , true);
> + r = ttm_eu_reserve_buffers(, , true, , false);
> if (r)
> goto error_unref;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index c430e8259038..d60593cc436e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -155,7 +155,7 @@ static inline int amdgpu_bo_reserve(struct amdgpu_bo *bo,
> bool no_intr)
> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> int r;
>
> - r = ttm_bo_reserve(>tbo, !no_intr, false, NULL);
> + r = __ttm_bo_reserve(>tbo, !no_intr, false, NULL);
> if (unlikely(r != 0)) {
> if (r != -ERESTARTSYS)
> dev_err(adev->dev, "%p reserve failed\n", bo);
> --
> 2.17.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 08:30:21AM -0700, enh wrote:
> On Wed, May 22, 2019 at 3:11 AM Catalin Marinas
> wrote:
> > On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote:
> > > I just want to make sure I fully understand your concern about this
> > > being an ABI break, and I work best with examples. The closest situation
> > > I can see would be:
> > >
> > > - some program has no idea about MTE
> >
> > Apart from some libraries like libc (and maybe those that handle
> > specific device ioctls), I think most programs should have no idea about
> > MTE. I wouldn't expect programmers to have to change their app just
> > because we have a new feature that colours heap allocations.
Right -- things should Just Work from the application perspective.
> obviously i'm biased as a libc maintainer, but...
>
> i don't think it helps to move this to libc --- now you just have an
> extra dependency where to have a guaranteed working system you need to
> update your kernel and libc together. (or at least update your libc to
> understand new ioctls etc _before_ you can update your kernel.)
I think (hope?) we've all agreed that we shouldn't pass this off to
userspace. At the very least, it reduces the utility of MTE, and at worst
it complicates userspace when this is clearly a kernel/architecture issue.
>
> > > - malloc() starts returning MTE-tagged addresses
> > > - program doesn't break from that change
> > > - program uses some syscall that is missing untagged_addr() and fails
> > > - kernel has now broken userspace that used to work
> >
> > That's one aspect though probably more of a case of plugging in a new
> > device (graphics card, network etc.) and the ioctl to the new device
> > doesn't work.
I think MTE will likely be rather like NX/PXN and SMAP/PAN: there will
be glitches, and we can disable stuff either via CONFIG or (as is more
common now) via a kernel commandline with untagged_addr() containing a
static branch, etc. But I actually don't think we need to go this route
(see below...)
> > The other is that, assuming we reach a point where the kernel entirely
> > supports this relaxed ABI, can we guarantee that it won't break in the
> > future. Let's say some subsequent kernel change (some refactoring)
> > misses out an untagged_addr(). This renders a previously TBI/MTE-capable
> > syscall unusable. Can we rely only on testing?
> >
> > > The trouble I see with this is that it is largely theoretical and
> > > requires part of userspace to collude to start using a new CPU feature
> > > that tickles a bug in the kernel. As I understand the golden rule,
> > > this is a bug in the kernel (a missed ioctl() or such) to be fixed,
> > > not a global breaking of some userspace behavior.
> >
> > Yes, we should follow the rule that it's a kernel bug but it doesn't
> > help the user that a newly installed kernel causes user space to no
> > longer reach a prompt. Hence the proposal of an opt-in via personality
> > (for MTE we would need an explicit opt-in by the user anyway since the
> > top byte is no longer ignored but checked against the allocation tag).
>
> but realistically would this actually get used in this way? or would
> any given system either be MTE or non-MTE. in which case a kernel
> configuration option would seem to make more sense. (because either
> way, the hypothetical user basically needs to recompile the kernel to
> get back on their feet. or all of userspace.)
Right: the point is to design things so that we do our best to not break
userspace that is using the new feature (which I think this series has
done well). But supporting MTE/TBI is just like supporting PAN: if someone
refactors a driver and swaps a copy_from_user() to a memcpy(), it's going
to break under PAN. There will be the same long tail of these bugs like
any other, but my sense is that they are small and rare. But I agree:
they're going to be pretty weird bugs to track down. The final result,
however, will be excellent annotation in the kernel for where userspace
addresses get used and people make assumptions about them.
The sooner we get the series landed and gain QEMU support (or real
hardware), the faster we can hammer out these missed corner-cases.
What's the timeline for either of those things, BTW?
> > > I feel like I'm missing something about this being seen as an ABI
> > > break. The kernel already fails on userspace addresses that have high
> > > bits set -- are there things that _depend_ on this failure to operate?
> >
> > It's about providing a relaxed ABI which allows non-zero top byte and
> > breaking it later inadvertently without having something better in place
> > to analyse the kernel changes.
It sounds like the question is how to switch a process in or out of this
ABI (but I don't think that's the real issue: I think it's just a matter
of whether or not a process uses tags at all). Doing it at the prctl()
level doesn't make sense to me, except maybe to detect MTE support or
something. ("Should I tag
Re: [PATCH 01/12] dma-buf: add dynamic caching of sg_table
On Wed, May 22, 2019 at 7:28 PM Christian König
wrote:
>
> Am 22.05.19 um 18:17 schrieb Sumit Semwal:
> > Hi Christian,
> >
> > On Sat, 27 Apr 2019 at 05:31, Liam Mark wrote:
> >> On Tue, 16 Apr 2019, Christian König wrote:
> >>
> >>> To allow a smooth transition from pinning buffer objects to dynamic
> >>> invalidation we first start to cache the sg_table for an attachment
> >>> unless the driver explicitly says to not do so.
> >>>
> >>> ---
> >>> drivers/dma-buf/dma-buf.c | 24
> >>> include/linux/dma-buf.h | 11 +++
> >>> 2 files changed, 35 insertions(+)
> >>>
> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> >>> index 7c858020d14b..65161a82d4d5 100644
> >>> --- a/drivers/dma-buf/dma-buf.c
> >>> +++ b/drivers/dma-buf/dma-buf.c
> >>> @@ -573,6 +573,20 @@ struct dma_buf_attachment *dma_buf_attach(struct
> >>> dma_buf *dmabuf,
> >>>list_add(>node, >attachments);
> >>>
> >>>mutex_unlock(>lock);
> >>> +
> >>> + if (!dmabuf->ops->dynamic_sgt_mapping) {
> >>> + struct sg_table *sgt;
> >>> +
> >>> + sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
> >>> + if (!sgt)
> >>> + sgt = ERR_PTR(-ENOMEM);
> >>> + if (IS_ERR(sgt)) {
> >>> + dma_buf_detach(dmabuf, attach);
> >>> + return ERR_CAST(sgt);
> >>> + }
> >>> + attach->sgt = sgt;
> >>> + }
> >>> +
> >>>return attach;
> >>>
> >>> err_attach:
> >>> @@ -595,6 +609,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct
> >>> dma_buf_attachment *attach)
> >>>if (WARN_ON(!dmabuf || !attach))
> >>>return;
> >>>
> >>> + if (attach->sgt)
> >>> + dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
> >>> +DMA_BIDIRECTIONAL);
> >>> +
> >>>mutex_lock(>lock);
> >>>list_del(>node);
> >>>if (dmabuf->ops->detach)
> >>> @@ -630,6 +648,9 @@ struct sg_table *dma_buf_map_attachment(struct
> >>> dma_buf_attachment *attach,
> >>>if (WARN_ON(!attach || !attach->dmabuf))
> >>>return ERR_PTR(-EINVAL);
> >>>
> >>> + if (attach->sgt)
> >>> + return attach->sgt;
> >>> +
> >> I am concerned by this change to make caching the sg_table the default
> >> behavior as this will result in the exporter's map_dma_buf/unmap_dma_buf
> >> calls are no longer being called in
> >> dma_buf_map_attachment/dma_buf_unmap_attachment.
> > Probably this concern from Liam got lost between versions of your
> > patches; could we please request a reply to these points here?
>
> Sorry I indeed never got this mail, but this is actually not an issue
> because Daniel had similar concerns and we didn't made this the default
> in the final version.
>
> >> This seems concerning to me as it appears to ignore the cache maintenance
> >> aspect of the map_dma_buf/unmap_dma_buf calls.
> >> For example won't this potentially cause issues for clients of ION.
> >>
> >> If we had the following
> >> - #1 dma_buf_attach coherent_device
> >> - #2 dma_buf attach non_coherent_device
> >> - #3 dma_buf_map_attachment non_coherent_device
> >> - #4 non_coherent_device writes to buffer
> >> - #5 dma_buf_unmap_attachment non_coherent_device
> >> - #6 dma_buf_map_attachment coherent_device
> >> - #7 coherent_device reads buffer
> >> - #8 dma_buf_unmap_attachment coherent_device
> >>
> >> There wouldn't be any CMO at step #5 anymore (specifically no invalidate)
> >> so now at step #7 the coherent_device could read a stale cache line.
> >>
> >> Also, now by default dma_buf_unmap_attachment no longer removes the
> >> mappings from the iommu, so now by default dma_buf_unmap_attachment is not
> >> doing what I would expect and clients are losing the potential sandboxing
> >> benefits of removing the mappings.
> >> Shouldn't this caching behavior be something that clients opt into instead
> >> of being the default?
>
> Well, it seems you are making incorrect assumptions about the cache
> maintenance of DMA-buf here.
>
> At least for all DRM devices I'm aware of mapping/unmapping an
> attachment does *NOT* have any cache maintenance implications.
>
> E.g. the use case you describe above would certainly fail with amdgpu,
> radeon, nouveau and i915 because mapping a DMA-buf doesn't stop the
> exporter from reading/writing to that buffer (just the opposite actually).
>
> All of them assume perfectly coherent access to the underlying memory.
> As far as I know there is no documented cache maintenance requirements
> for DMA-buf.
I think it is documented. It's just that on x86, we ignore that
because the dma-api pretends there's never a need for cache flushing
on x86, and that everything snoops the cpu caches. Which isn't true
since over 20 ago when AGP happened. The actual rules for x86 dma-buf
are very much ad-hoc (and we occasionally reapply some duct-tape when
cacheline
Re: [PATCH v2 2/2] drm/amd/display: Use new connector state when getting color depth
On 2019-05-22 12:00 p.m., Nicholas Kazlauskas wrote:
> [CAUTION: External Email]
>
> [Why]
> The current state on the connector is queried when getting the max bpc
> rather than the new state. This means that a new max bpc value can only
> currently take effect on the commit *after* it changes.
>
> The new state should be passed in instead.
>
> [How]
> Pass down the dm_state as drm state to where we do color depth lookup.
>
> The passed in state can still be NULL when called from
> amdgpu_dm_connector_mode_valid, so make sure that we have reasonable
> defaults in place. That should probably be addressed at some point.
>
> This change now (correctly) causes a modeset to occur when changing the
> max bpc for a connector.
>
> v2: Drop extra TODO.
>
> Cc: Leo Li
> Cc: Harry Wentland
> Signed-off-by: Nicholas Kazlauskas
> Acked-by: Alex Deucher
Series is
Reviewed-by: Harry Wentland
Harry
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 ++-
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index b8e88209ef5d..fd0421794e0f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3038,13 +3038,13 @@ static void update_stream_scaling_settings(const
> struct drm_display_mode *mode,
> }
>
> static enum dc_color_depth
> -convert_color_depth_from_display_info(const struct drm_connector *connector)
> +convert_color_depth_from_display_info(const struct drm_connector *connector,
> + const struct drm_connector_state *state)
> {
> uint32_t bpc = connector->display_info.bpc;
>
> - /* TODO: Use passed in state instead of the current state. */
> - if (connector->state) {
> - bpc = connector->state->max_bpc;
> + if (state) {
> + bpc = state->max_bpc;
> /* Round down to the nearest even number. */
> bpc = bpc - (bpc & 1);
> }
> @@ -3165,11 +3165,12 @@ static void
> adjust_colour_depth_from_display_info(struct dc_crtc_timing *timing_
>
> }
>
> -static void
> -fill_stream_properties_from_drm_display_mode(struct dc_stream_state *stream,
> -const struct drm_display_mode
> *mode_in,
> -const struct drm_connector
> *connector,
> -const struct dc_stream_state
> *old_stream)
> +static void fill_stream_properties_from_drm_display_mode(
> + struct dc_stream_state *stream,
> + const struct drm_display_mode *mode_in,
> + const struct drm_connector *connector,
> + const struct drm_connector_state *connector_state,
> + const struct dc_stream_state *old_stream)
> {
> struct dc_crtc_timing *timing_out = >timing;
> const struct drm_display_info *info = >display_info;
> @@ -3192,7 +3193,7 @@ fill_stream_properties_from_drm_display_mode(struct
> dc_stream_state *stream,
>
> timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
> timing_out->display_color_depth =
> convert_color_depth_from_display_info(
> - connector);
> + connector, connector_state);
> timing_out->scan_type = SCANNING_TYPE_NODATA;
> timing_out->hdmi_vic = 0;
>
> @@ -3389,6 +3390,8 @@ create_stream_for_sink(struct amdgpu_dm_connector
> *aconnector,
> {
> struct drm_display_mode *preferred_mode = NULL;
> struct drm_connector *drm_connector;
> + const struct drm_connector_state *con_state =
> + dm_state ? _state->base : NULL;
> struct dc_stream_state *stream = NULL;
> struct drm_display_mode mode = *drm_mode;
> bool native_mode_found = false;
> @@ -3461,10 +3464,10 @@ create_stream_for_sink(struct amdgpu_dm_connector
> *aconnector,
> */
> if (!scale || mode_refresh != preferred_refresh)
> fill_stream_properties_from_drm_display_mode(stream,
> - , >base, NULL);
> + , >base, con_state, NULL);
> else
> fill_stream_properties_from_drm_display_mode(stream,
> - , >base, old_stream);
> + , >base, con_state, old_stream);
>
> update_stream_scaling_settings(, dm_state, stream);
>
> --
> 2.17.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 01/12] dma-buf: add dynamic caching of sg_table
Am 22.05.19 um 18:17 schrieb Sumit Semwal:
Hi Christian,
On Sat, 27 Apr 2019 at 05:31, Liam Mark wrote:
On Tue, 16 Apr 2019, Christian König wrote:
To allow a smooth transition from pinning buffer objects to dynamic
invalidation we first start to cache the sg_table for an attachment
unless the driver explicitly says to not do so.
---
drivers/dma-buf/dma-buf.c | 24
include/linux/dma-buf.h | 11 +++
2 files changed, 35 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 7c858020d14b..65161a82d4d5 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -573,6 +573,20 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf
*dmabuf,
list_add(>node, >attachments);
mutex_unlock(>lock);
+
+ if (!dmabuf->ops->dynamic_sgt_mapping) {
+ struct sg_table *sgt;
+
+ sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
+ if (!sgt)
+ sgt = ERR_PTR(-ENOMEM);
+ if (IS_ERR(sgt)) {
+ dma_buf_detach(dmabuf, attach);
+ return ERR_CAST(sgt);
+ }
+ attach->sgt = sgt;
+ }
+
return attach;
err_attach:
@@ -595,6 +609,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct
dma_buf_attachment *attach)
if (WARN_ON(!dmabuf || !attach))
return;
+ if (attach->sgt)
+ dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
+DMA_BIDIRECTIONAL);
+
mutex_lock(>lock);
list_del(>node);
if (dmabuf->ops->detach)
@@ -630,6 +648,9 @@ struct sg_table *dma_buf_map_attachment(struct
dma_buf_attachment *attach,
if (WARN_ON(!attach || !attach->dmabuf))
return ERR_PTR(-EINVAL);
+ if (attach->sgt)
+ return attach->sgt;
+
I am concerned by this change to make caching the sg_table the default
behavior as this will result in the exporter's map_dma_buf/unmap_dma_buf
calls are no longer being called in
dma_buf_map_attachment/dma_buf_unmap_attachment.
Probably this concern from Liam got lost between versions of your
patches; could we please request a reply to these points here?
Sorry I indeed never got this mail, but this is actually not an issue
because Daniel had similar concerns and we didn't made this the default
in the final version.
This seems concerning to me as it appears to ignore the cache maintenance
aspect of the map_dma_buf/unmap_dma_buf calls.
For example won't this potentially cause issues for clients of ION.
If we had the following
- #1 dma_buf_attach coherent_device
- #2 dma_buf attach non_coherent_device
- #3 dma_buf_map_attachment non_coherent_device
- #4 non_coherent_device writes to buffer
- #5 dma_buf_unmap_attachment non_coherent_device
- #6 dma_buf_map_attachment coherent_device
- #7 coherent_device reads buffer
- #8 dma_buf_unmap_attachment coherent_device
There wouldn't be any CMO at step #5 anymore (specifically no invalidate)
so now at step #7 the coherent_device could read a stale cache line.
Also, now by default dma_buf_unmap_attachment no longer removes the
mappings from the iommu, so now by default dma_buf_unmap_attachment is not
doing what I would expect and clients are losing the potential sandboxing
benefits of removing the mappings.
Shouldn't this caching behavior be something that clients opt into instead
of being the default?
Well, it seems you are making incorrect assumptions about the cache
maintenance of DMA-buf here.
At least for all DRM devices I'm aware of mapping/unmapping an
attachment does *NOT* have any cache maintenance implications.
E.g. the use case you describe above would certainly fail with amdgpu,
radeon, nouveau and i915 because mapping a DMA-buf doesn't stop the
exporter from reading/writing to that buffer (just the opposite actually).
All of them assume perfectly coherent access to the underlying memory.
As far as I know there is no documented cache maintenance requirements
for DMA-buf.
The IOMMU concern on the other hand is certainly valid and I perfectly
agree that keeping the mapping time as short as possible is desirable.
Regards,
Christian.
Liam
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Best,
Sumit.
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 9:35 AM Catalin Marinas wrote: > > On Wed, May 22, 2019 at 08:30:21AM -0700, enh wrote: > > On Wed, May 22, 2019 at 3:11 AM Catalin Marinas > > wrote: > > > On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote: > > > > I just want to make sure I fully understand your concern about this > > > > being an ABI break, and I work best with examples. The closest situation > > > > I can see would be: > > > > > > > > - some program has no idea about MTE > > > > > > Apart from some libraries like libc (and maybe those that handle > > > specific device ioctls), I think most programs should have no idea about > > > MTE. I wouldn't expect programmers to have to change their app just > > > because we have a new feature that colours heap allocations. > > > > obviously i'm biased as a libc maintainer, but... > > > > i don't think it helps to move this to libc --- now you just have an > > extra dependency where to have a guaranteed working system you need to > > update your kernel and libc together. (or at least update your libc to > > understand new ioctls etc _before_ you can update your kernel.) > > That's not what I meant (or I misunderstood you). If we have a relaxed > ABI in the kernel and a libc that returns tagged pointers on malloc() I > wouldn't expect the programmer to do anything different in the > application code like explicit untagging. Basically the program would > continue to run unmodified irrespective of whether you use an old libc > without tagged pointers or a new one which tags heap allocations. > > What I do expect is that the libc checks for the presence of the relaxed > ABI, currently proposed as an AT_FLAGS bit (for MTE we'd have a > HWCAP_MTE), and only tag the malloc() pointers if the kernel supports > the relaxed ABI. As you said, you shouldn't expect that the C library > and kernel are upgraded together, so they should be able to work in any > new/old version combination. yes, that part makes sense. i do think we'd use the AT_FLAGS bit, for exactly this. i was questioning the argument about the ioctl issues, and saying that from my perspective, untagging bugs are not really any different than any other kind of kernel bug. > > > > The trouble I see with this is that it is largely theoretical and > > > > requires part of userspace to collude to start using a new CPU feature > > > > that tickles a bug in the kernel. As I understand the golden rule, > > > > this is a bug in the kernel (a missed ioctl() or such) to be fixed, > > > > not a global breaking of some userspace behavior. > > > > > > Yes, we should follow the rule that it's a kernel bug but it doesn't > > > help the user that a newly installed kernel causes user space to no > > > longer reach a prompt. Hence the proposal of an opt-in via personality > > > (for MTE we would need an explicit opt-in by the user anyway since the > > > top byte is no longer ignored but checked against the allocation tag). > > > > but realistically would this actually get used in this way? or would > > any given system either be MTE or non-MTE. in which case a kernel > > configuration option would seem to make more sense. (because either > > way, the hypothetical user basically needs to recompile the kernel to > > get back on their feet. or all of userspace.) > > The two hard requirements I have for supporting any new hardware feature > in Linux are (1) a single kernel image binary continues to run on old > hardware while making use of the new feature if available and (2) old > user space continues to run on new hardware while new user space can > take advantage of the new feature. > > The distro user space usually has a hard requirement that it continues > to run on (certain) old hardware. We can't enforce this in the kernel > but we offer the option to user space developers of checking feature > availability through HWCAP bits. > > The Android story may be different as you have more control about which > kernel configurations are deployed on specific SoCs. I'm looking more > from a Linux distro angle where you just get an off-the-shelf OS image > and install it on your hardware, either taking advantage of new features > or just not using them if the software was not updated. Or, if updated > software is installed on old hardware, it would just run. > > For MTE, we just can't enable it by default since there are applications > who use the top byte of a pointer and expect it to be ignored rather > than failing with a mismatched tag. Just think of a hwasan compiled > binary where TBI is expected to work and you try to run it with MTE > turned on. > > I would also expect the C library or dynamic loader to check for the > presence of a HWCAP_MTE bit before starting to tag memory allocations, > otherwise it would get SIGILL on the first MTE instruction it tries to > execute. (a bit off-topic, but i thought the MTE instructions were encoded in the no-op space, to avoid this?) > > i'm not sure i see this new way for a kernel
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 08:30:21AM -0700, enh wrote: > On Wed, May 22, 2019 at 3:11 AM Catalin Marinas > wrote: > > On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote: > > > I just want to make sure I fully understand your concern about this > > > being an ABI break, and I work best with examples. The closest situation > > > I can see would be: > > > > > > - some program has no idea about MTE > > > > Apart from some libraries like libc (and maybe those that handle > > specific device ioctls), I think most programs should have no idea about > > MTE. I wouldn't expect programmers to have to change their app just > > because we have a new feature that colours heap allocations. > > obviously i'm biased as a libc maintainer, but... > > i don't think it helps to move this to libc --- now you just have an > extra dependency where to have a guaranteed working system you need to > update your kernel and libc together. (or at least update your libc to > understand new ioctls etc _before_ you can update your kernel.) That's not what I meant (or I misunderstood you). If we have a relaxed ABI in the kernel and a libc that returns tagged pointers on malloc() I wouldn't expect the programmer to do anything different in the application code like explicit untagging. Basically the program would continue to run unmodified irrespective of whether you use an old libc without tagged pointers or a new one which tags heap allocations. What I do expect is that the libc checks for the presence of the relaxed ABI, currently proposed as an AT_FLAGS bit (for MTE we'd have a HWCAP_MTE), and only tag the malloc() pointers if the kernel supports the relaxed ABI. As you said, you shouldn't expect that the C library and kernel are upgraded together, so they should be able to work in any new/old version combination. > > > The trouble I see with this is that it is largely theoretical and > > > requires part of userspace to collude to start using a new CPU feature > > > that tickles a bug in the kernel. As I understand the golden rule, > > > this is a bug in the kernel (a missed ioctl() or such) to be fixed, > > > not a global breaking of some userspace behavior. > > > > Yes, we should follow the rule that it's a kernel bug but it doesn't > > help the user that a newly installed kernel causes user space to no > > longer reach a prompt. Hence the proposal of an opt-in via personality > > (for MTE we would need an explicit opt-in by the user anyway since the > > top byte is no longer ignored but checked against the allocation tag). > > but realistically would this actually get used in this way? or would > any given system either be MTE or non-MTE. in which case a kernel > configuration option would seem to make more sense. (because either > way, the hypothetical user basically needs to recompile the kernel to > get back on their feet. or all of userspace.) The two hard requirements I have for supporting any new hardware feature in Linux are (1) a single kernel image binary continues to run on old hardware while making use of the new feature if available and (2) old user space continues to run on new hardware while new user space can take advantage of the new feature. The distro user space usually has a hard requirement that it continues to run on (certain) old hardware. We can't enforce this in the kernel but we offer the option to user space developers of checking feature availability through HWCAP bits. The Android story may be different as you have more control about which kernel configurations are deployed on specific SoCs. I'm looking more from a Linux distro angle where you just get an off-the-shelf OS image and install it on your hardware, either taking advantage of new features or just not using them if the software was not updated. Or, if updated software is installed on old hardware, it would just run. For MTE, we just can't enable it by default since there are applications who use the top byte of a pointer and expect it to be ignored rather than failing with a mismatched tag. Just think of a hwasan compiled binary where TBI is expected to work and you try to run it with MTE turned on. I would also expect the C library or dynamic loader to check for the presence of a HWCAP_MTE bit before starting to tag memory allocations, otherwise it would get SIGILL on the first MTE instruction it tries to execute. > i'm not sure i see this new way for a kernel update to break my system > and need to be fixed forward/rolled back as any different from any of > the existing ways in which this can happen :-) as an end-user i have > to rely on whoever's sending me software updates to test adequately > enough that they find the problems. as an end user, there isn't any > difference between "my phone rebooted when i tried to take a photo > because of a kernel/driver leak", say, and "my phone rebooted when i > tried to take a photo because of missing untagging of a pointer passed > via ioctl". > > i suspect you and i have very
Re: [PATCH 01/12] dma-buf: add dynamic caching of sg_table
Hi Christian,
On Sat, 27 Apr 2019 at 05:31, Liam Mark wrote:
>
> On Tue, 16 Apr 2019, Christian König wrote:
>
> > To allow a smooth transition from pinning buffer objects to dynamic
> > invalidation we first start to cache the sg_table for an attachment
> > unless the driver explicitly says to not do so.
> >
> > ---
> > drivers/dma-buf/dma-buf.c | 24
> > include/linux/dma-buf.h | 11 +++
> > 2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index 7c858020d14b..65161a82d4d5 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -573,6 +573,20 @@ struct dma_buf_attachment *dma_buf_attach(struct
> > dma_buf *dmabuf,
> > list_add(>node, >attachments);
> >
> > mutex_unlock(>lock);
> > +
> > + if (!dmabuf->ops->dynamic_sgt_mapping) {
> > + struct sg_table *sgt;
> > +
> > + sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
> > + if (!sgt)
> > + sgt = ERR_PTR(-ENOMEM);
> > + if (IS_ERR(sgt)) {
> > + dma_buf_detach(dmabuf, attach);
> > + return ERR_CAST(sgt);
> > + }
> > + attach->sgt = sgt;
> > + }
> > +
> > return attach;
> >
> > err_attach:
> > @@ -595,6 +609,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct
> > dma_buf_attachment *attach)
> > if (WARN_ON(!dmabuf || !attach))
> > return;
> >
> > + if (attach->sgt)
> > + dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
> > +DMA_BIDIRECTIONAL);
> > +
> > mutex_lock(>lock);
> > list_del(>node);
> > if (dmabuf->ops->detach)
> > @@ -630,6 +648,9 @@ struct sg_table *dma_buf_map_attachment(struct
> > dma_buf_attachment *attach,
> > if (WARN_ON(!attach || !attach->dmabuf))
> > return ERR_PTR(-EINVAL);
> >
> > + if (attach->sgt)
> > + return attach->sgt;
> > +
>
> I am concerned by this change to make caching the sg_table the default
> behavior as this will result in the exporter's map_dma_buf/unmap_dma_buf
> calls are no longer being called in
> dma_buf_map_attachment/dma_buf_unmap_attachment.
Probably this concern from Liam got lost between versions of your
patches; could we please request a reply to these points here?
>
> This seems concerning to me as it appears to ignore the cache maintenance
> aspect of the map_dma_buf/unmap_dma_buf calls.
> For example won't this potentially cause issues for clients of ION.
>
> If we had the following
> - #1 dma_buf_attach coherent_device
> - #2 dma_buf attach non_coherent_device
> - #3 dma_buf_map_attachment non_coherent_device
> - #4 non_coherent_device writes to buffer
> - #5 dma_buf_unmap_attachment non_coherent_device
> - #6 dma_buf_map_attachment coherent_device
> - #7 coherent_device reads buffer
> - #8 dma_buf_unmap_attachment coherent_device
>
> There wouldn't be any CMO at step #5 anymore (specifically no invalidate)
> so now at step #7 the coherent_device could read a stale cache line.
>
> Also, now by default dma_buf_unmap_attachment no longer removes the
> mappings from the iommu, so now by default dma_buf_unmap_attachment is not
> doing what I would expect and clients are losing the potential sandboxing
> benefits of removing the mappings.
> Shouldn't this caching behavior be something that clients opt into instead
> of being the default?
>
> Liam
>
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Best,
Sumit.
[PATCH v2 1/2] drm/amd/display: Switch the custom "max bpc" property to the DRM prop
[Why]
The custom "max bpc" property was added to limit color depth while the
DRM one was still being merged. It's been a few kernel versions since
then and this TODO was still sticking around.
[How]
Attach the DRM max bpc property to the connector and drop all of our
custom property management. Set the max bpc to 8 by default since
DRM defaults to the max in the range which would be 16 in this case.
No behavioral changes are intended with this patch, it should just be
a refactor.
Cc: Leo Li
Cc: Harry Wentland
Signed-off-by: Nicholas Kazlauskas
Acked-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 2 --
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 29 ---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
4 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 4e944737b708..767ee6991ef4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -631,10 +631,6 @@ int amdgpu_display_modeset_create_props(struct
amdgpu_device *adev)
amdgpu_dither_enum_list, sz);
if (amdgpu_device_has_dc_support(adev)) {
- adev->mode_info.max_bpc_property =
- drm_property_create_range(adev->ddev, 0, "max bpc", 8,
16);
- if (!adev->mode_info.max_bpc_property)
- return -ENOMEM;
adev->mode_info.abm_level_property =
drm_property_create_range(adev->ddev, 0,
"abm level", 0, 4);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index c7940af42f76..8bda00ce8816 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -332,8 +332,6 @@ struct amdgpu_mode_info {
struct drm_property *audio_property;
/* FMT dithering */
struct drm_property *dither_property;
- /* maximum number of bits per channel for monitor color */
- struct drm_property *max_bpc_property;
/* Adaptive Backlight Modulation (power feature) */
struct drm_property *abm_level_property;
/* it is used to allow enablement of freesync mode */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 4a1755bce96c..b8e88209ef5d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3040,14 +3040,14 @@ static void update_stream_scaling_settings(const struct
drm_display_mode *mode,
static enum dc_color_depth
convert_color_depth_from_display_info(const struct drm_connector *connector)
{
- struct dm_connector_state *dm_conn_state =
- to_dm_connector_state(connector->state);
uint32_t bpc = connector->display_info.bpc;
- /* TODO: Remove this when there's support for max_bpc in drm */
- if (dm_conn_state && bpc > dm_conn_state->max_bpc)
- /* Round down to nearest even number. */
- bpc = dm_conn_state->max_bpc - (dm_conn_state->max_bpc & 1);
+ /* TODO: Use passed in state instead of the current state. */
+ if (connector->state) {
+ bpc = connector->state->max_bpc;
+ /* Round down to the nearest even number. */
+ bpc = bpc - (bpc & 1);
+ }
switch (bpc) {
case 0:
@@ -3689,9 +3689,6 @@ int amdgpu_dm_connector_atomic_set_property(struct
drm_connector *connector,
} else if (property == adev->mode_info.underscan_property) {
dm_new_state->underscan_enable = val;
ret = 0;
- } else if (property == adev->mode_info.max_bpc_property) {
- dm_new_state->max_bpc = val;
- ret = 0;
} else if (property == adev->mode_info.abm_level_property) {
dm_new_state->abm_level = val;
ret = 0;
@@ -3743,9 +3740,6 @@ int amdgpu_dm_connector_atomic_get_property(struct
drm_connector *connector,
} else if (property == adev->mode_info.underscan_property) {
*val = dm_state->underscan_enable;
ret = 0;
- } else if (property == adev->mode_info.max_bpc_property) {
- *val = dm_state->max_bpc;
- ret = 0;
} else if (property == adev->mode_info.abm_level_property) {
*val = dm_state->abm_level;
ret = 0;
@@ -3808,7 +3802,6 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector
*connector)
state->underscan_enable = false;
state->underscan_hborder = 0;
state->underscan_vborder = 0;
- state->max_bpc = 8;
[PATCH v2 2/2] drm/amd/display: Use new connector state when getting color depth
[Why]
The current state on the connector is queried when getting the max bpc
rather than the new state. This means that a new max bpc value can only
currently take effect on the commit *after* it changes.
The new state should be passed in instead.
[How]
Pass down the dm_state as drm state to where we do color depth lookup.
The passed in state can still be NULL when called from
amdgpu_dm_connector_mode_valid, so make sure that we have reasonable
defaults in place. That should probably be addressed at some point.
This change now (correctly) causes a modeset to occur when changing the
max bpc for a connector.
v2: Drop extra TODO.
Cc: Leo Li
Cc: Harry Wentland
Signed-off-by: Nicholas Kazlauskas
Acked-by: Alex Deucher
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 ++-
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index b8e88209ef5d..fd0421794e0f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3038,13 +3038,13 @@ static void update_stream_scaling_settings(const struct
drm_display_mode *mode,
}
static enum dc_color_depth
-convert_color_depth_from_display_info(const struct drm_connector *connector)
+convert_color_depth_from_display_info(const struct drm_connector *connector,
+ const struct drm_connector_state *state)
{
uint32_t bpc = connector->display_info.bpc;
- /* TODO: Use passed in state instead of the current state. */
- if (connector->state) {
- bpc = connector->state->max_bpc;
+ if (state) {
+ bpc = state->max_bpc;
/* Round down to the nearest even number. */
bpc = bpc - (bpc & 1);
}
@@ -3165,11 +3165,12 @@ static void
adjust_colour_depth_from_display_info(struct dc_crtc_timing *timing_
}
-static void
-fill_stream_properties_from_drm_display_mode(struct dc_stream_state *stream,
-const struct drm_display_mode
*mode_in,
-const struct drm_connector
*connector,
-const struct dc_stream_state
*old_stream)
+static void fill_stream_properties_from_drm_display_mode(
+ struct dc_stream_state *stream,
+ const struct drm_display_mode *mode_in,
+ const struct drm_connector *connector,
+ const struct drm_connector_state *connector_state,
+ const struct dc_stream_state *old_stream)
{
struct dc_crtc_timing *timing_out = >timing;
const struct drm_display_info *info = >display_info;
@@ -3192,7 +3193,7 @@ fill_stream_properties_from_drm_display_mode(struct
dc_stream_state *stream,
timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
timing_out->display_color_depth = convert_color_depth_from_display_info(
- connector);
+ connector, connector_state);
timing_out->scan_type = SCANNING_TYPE_NODATA;
timing_out->hdmi_vic = 0;
@@ -3389,6 +3390,8 @@ create_stream_for_sink(struct amdgpu_dm_connector
*aconnector,
{
struct drm_display_mode *preferred_mode = NULL;
struct drm_connector *drm_connector;
+ const struct drm_connector_state *con_state =
+ dm_state ? _state->base : NULL;
struct dc_stream_state *stream = NULL;
struct drm_display_mode mode = *drm_mode;
bool native_mode_found = false;
@@ -3461,10 +3464,10 @@ create_stream_for_sink(struct amdgpu_dm_connector
*aconnector,
*/
if (!scale || mode_refresh != preferred_refresh)
fill_stream_properties_from_drm_display_mode(stream,
- , >base, NULL);
+ , >base, con_state, NULL);
else
fill_stream_properties_from_drm_display_mode(stream,
- , >base, old_stream);
+ , >base, con_state, old_stream);
update_stream_scaling_settings(, dm_state, stream);
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm/amd/display: Switch the custom "max bpc" property to the DRM prop
On 5/22/19 11:11 AM, Nicholas Kazlauskas wrote:
> [CAUTION: External Email]
>
> [Why]
> The custom "max bpc" property was added to limit color depth while the
> DRM one was still being merged. It's been a few kernel versions since
> then and this TODO was still sticking around.
>
> [How]
> Attach the DRM max bpc property to the connector and drop all of our
> custom property management. Set the max bpc to 8 by default since
> DRM defaults to the max in the range which would be 16 in this case.
>
> No behavioral changes are intended with this patch, it should just be
> a refactor.
>
> Cc: Leo Li
> Cc: Harry Wentland
> Signed-off-by: Nicholas Kazlauskas
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 2 --
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
> 4 files changed, 13 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 4e944737b708..767ee6991ef4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -631,10 +631,6 @@ int amdgpu_display_modeset_create_props(struct
> amdgpu_device *adev)
> amdgpu_dither_enum_list, sz);
>
> if (amdgpu_device_has_dc_support(adev)) {
> - adev->mode_info.max_bpc_property =
> - drm_property_create_range(adev->ddev, 0, "max bpc",
> 8, 16);
> - if (!adev->mode_info.max_bpc_property)
> - return -ENOMEM;
> adev->mode_info.abm_level_property =
> drm_property_create_range(adev->ddev, 0,
> "abm level", 0, 4);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index c7940af42f76..8bda00ce8816 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -332,8 +332,6 @@ struct amdgpu_mode_info {
> struct drm_property *audio_property;
> /* FMT dithering */
> struct drm_property *dither_property;
> - /* maximum number of bits per channel for monitor color */
> - struct drm_property *max_bpc_property;
> /* Adaptive Backlight Modulation (power feature) */
> struct drm_property *abm_level_property;
> /* it is used to allow enablement of freesync mode */
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4a1755bce96c..a7a9e4d81a17 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3040,14 +3040,14 @@ static void update_stream_scaling_settings(const
> struct drm_display_mode *mode,
> static enum dc_color_depth
> convert_color_depth_from_display_info(const struct drm_connector *connector)
> {
> - struct dm_connector_state *dm_conn_state =
> - to_dm_connector_state(connector->state);
> - uint32_t bpc = connector->display_info.bpc;
> + uint32_t bpc = 8;
Actually, there is a behavioral state change here. This should still be:
bpc = connector->display_info.bpc;
I'll fix up this and drop that extra TODO left over in patch 2 and post
a v2.
Nicholas Kazlauskas
>
> - /* TODO: Remove this when there's support for max_bpc in drm */
> - if (dm_conn_state && bpc > dm_conn_state->max_bpc)
> - /* Round down to nearest even number. */
> - bpc = dm_conn_state->max_bpc - (dm_conn_state->max_bpc & 1);
> + /* TODO: Use passed in state instead of the current state. */
> + if (connector->state) {
> + bpc = connector->state->max_bpc;
> + /* Round down to the nearest even number. */
> + bpc = bpc - (bpc & 1);
> + }
>
> switch (bpc) {
> case 0:
> @@ -3689,9 +3689,6 @@ int amdgpu_dm_connector_atomic_set_property(struct
> drm_connector *connector,
> } else if (property == adev->mode_info.underscan_property) {
> dm_new_state->underscan_enable = val;
> ret = 0;
> - } else if (property == adev->mode_info.max_bpc_property) {
> - dm_new_state->max_bpc = val;
> - ret = 0;
> } else if (property == adev->mode_info.abm_level_property) {
> dm_new_state->abm_level = val;
> ret = 0;
> @@ -3743,9 +3740,6 @@ int amdgpu_dm_connector_atomic_get_property(struct
> drm_connector *connector,
> } else if (property == adev->mode_info.underscan_property) {
> *val = dm_state->underscan_enable;
> ret = 0;
> - } else if (property == adev->mode_info.max_bpc_property) {
[PATCH 6/7] drm/amdkfd: New IOCTL to allocate queue GWS
Add a new kfd ioctl to allocate queue GWS. Queue
GWS is released on queue destroy.
Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
Signed-off-by: Oak Zeng
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 39
include/uapi/linux/kfd_ioctl.h | 20 +++-
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 38ae53f..828bd66 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1559,6 +1559,43 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file
*filep,
return err;
}
+static int kfd_ioctl_alloc_queue_gws(struct file *filep,
+ struct kfd_process *p, void *data)
+{
+ int retval;
+ struct kfd_ioctl_alloc_queue_gws_args *args = data;
+ struct kfd_dev *dev = NULL;
+ struct kgd_mem *mem;
+
+ if (args->num_gws == 0)
+ return -EINVAL;
+
+ if (!hws_gws_support ||
+ dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
+ return -EINVAL;
+
+ dev = kfd_device_by_id(args->gpu_id);
+ if (!dev) {
+ pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
+ return -EINVAL;
+ }
+
+ retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
+ dev->gws, );
+ if (unlikely(retval))
+ return retval;
+
+ mutex_lock(>mutex);
+ retval = pqm_set_gws(>pqm, args->queue_id, mem);
+ mutex_unlock(>mutex);
+
+ if (unlikely(retval))
+ amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
+
+ args->first_gws = 0;
+ return retval;
+}
+
static int kfd_ioctl_get_dmabuf_info(struct file *filep,
struct kfd_process *p, void *data)
{
@@ -1761,6 +1798,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
kfd_ioctl_import_dmabuf, 0),
+ AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
+ kfd_ioctl_alloc_queue_gws, 0),
};
#define AMDKFD_CORE_IOCTL_COUNTARRAY_SIZE(amdkfd_ioctls)
diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 20917c5..070d1bc 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -410,6 +410,21 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
__u32 n_success;/* to/from KFD */
};
+/* Allocate GWS for specific queue
+ *
+ * @gpu_id: device identifier
+ * @queue_id:queue's id that GWS is allocated for
+ * @num_gws: how many GWS to allocate
+ * @first_gws: index of the first GWS allocated.
+ * only support contiguous GWS allocation
+ */
+struct kfd_ioctl_alloc_queue_gws_args {
+ __u32 gpu_id; /* to KFD */
+ __u32 queue_id; /* to KFD */
+ __u32 num_gws; /* to KFD */
+ __u32 first_gws;/* from KFD */
+};
+
struct kfd_ioctl_get_dmabuf_info_args {
__u64 size; /* from KFD */
__u64 metadata_ptr; /* to KFD */
@@ -529,7 +544,10 @@ enum kfd_mmio_remap {
#define AMDKFD_IOC_IMPORT_DMABUF \
AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
+#define AMDKFD_IOC_ALLOC_QUEUE_GWS \
+ AMDKFD_IOWR(0x1E, struct kfd_ioctl_alloc_queue_gws_args)
+
#define AMDKFD_COMMAND_START 0x01
-#define AMDKFD_COMMAND_END 0x1E
+#define AMDKFD_COMMAND_END 0x1F
#endif
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 7/7] drm/amdkfd: PM4 packets change to support GWS
Add a field in map_queues packet to indicate whether
this is a gws control queue. Only one queue per process
can be gws control queue. Change num_gws field in
map_process packet to 7 bits
Change-Id: I0db91d99c6962d14f9202b2eb950f8e7e497b79e
Signed-off-by: Oak Zeng
Reviewed-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c | 1 +
drivers/gpu/drm/amd/amdkfd/kfd_pm4_headers_ai.h | 7 ---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
index 3dd731c..07f02f8e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
@@ -159,6 +159,7 @@ static int pm_map_queues_v9(struct packet_manager *pm,
uint32_t *buffer,
packet->bitfields2.engine_sel =
engine_sel__mes_map_queues__compute_vi;
+ packet->bitfields2.gws_control_queue = q->gws ? 1 : 0;
packet->bitfields2.queue_type =
queue_type__mes_map_queues__normal_compute_vi;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_pm4_headers_ai.h
b/drivers/gpu/drm/amd/amdkfd/kfd_pm4_headers_ai.h
index 0661339..49ab66b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_pm4_headers_ai.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_pm4_headers_ai.h
@@ -176,8 +176,7 @@ struct pm4_mes_map_process {
union {
struct {
- uint32_t num_gws:6;
- uint32_t reserved7:1;
+ uint32_t num_gws:7;
uint32_t sdma_enable:1;
uint32_t num_oac:4;
uint32_t reserved8:4;
@@ -272,7 +271,9 @@ struct pm4_mes_map_queues {
struct {
uint32_t reserved1:4;
enum mes_map_queues_queue_sel_enum queue_sel:2;
- uint32_t reserved2:15;
+ uint32_t reserved5:6;
+ uint32_t gws_control_queue:1;
+ uint32_t reserved2:8;
enum mes_map_queues_queue_type_enum queue_type:3;
uint32_t reserved3:2;
enum mes_map_queues_engine_sel_enum engine_sel:3;
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 5/7] drm/amdkfd: Add function to set queue gws
Add functions in process queue manager to
set/get queue gws. Also set process's number
of gws used. Currently only one queue in
process can use and use all gws.
Change-Id: I03e480c8692db3eabfc3a188cce8904d5d962ab7
Signed-off-by: Oak Zeng
---
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 6 +++
.../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 58 ++
2 files changed, 64 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 5969e37..40a5c67 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -454,6 +454,9 @@ struct queue_properties {
*
* @device: The kfd device that created this queue.
*
+ * @gws: Pointing to gws kgd_mem if this is a gws control queue; NULL
+ * otherwise.
+ *
* This structure represents user mode compute queues.
* It contains all the necessary data to handle such queues.
*
@@ -475,6 +478,7 @@ struct queue {
struct kfd_process *process;
struct kfd_dev *device;
+ void *gws;
};
/*
@@ -868,6 +872,8 @@ int pqm_update_queue(struct process_queue_manager *pqm,
unsigned int qid,
struct queue_properties *p);
int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
struct queue_properties *p);
+int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
+ void *gws);
struct kernel_queue *pqm_get_kernel_queue(struct process_queue_manager *pqm,
unsigned int qid);
int pqm_get_wave_state(struct process_queue_manager *pqm,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
index e652e25..a78494d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -26,6 +26,7 @@
#include "kfd_device_queue_manager.h"
#include "kfd_priv.h"
#include "kfd_kernel_queue.h"
+#include "amdgpu_amdkfd.h"
static inline struct process_queue_node *get_queue_by_qid(
struct process_queue_manager *pqm, unsigned int qid)
@@ -74,6 +75,57 @@ void kfd_process_dequeue_from_device(struct
kfd_process_device *pdd)
pdd->already_dequeued = true;
}
+int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
+ void *gws)
+{
+ struct kfd_dev *dev = NULL;
+ struct process_queue_node *pqn;
+ struct kfd_process_device *pdd;
+
+ pqn = get_queue_by_qid(pqm, qid);
+ if (!pqn) {
+ pr_err("Queue id does not match any known queue\n");
+ return -EINVAL;
+ }
+
+ if (pqn->q)
+ dev = pqn->q->device;
+ if (WARN_ON(!dev))
+ return -ENODEV;
+
+ pdd = kfd_get_process_device_data(dev, pqm->process);
+ if (!pdd) {
+ pr_err("Process device data doesn't exist\n");
+ return -EINVAL;
+ }
+
+ /* Only allow one queue per process can have GWS assigned */
+ list_for_each_entry(pqn, >queues, process_queue_list) {
+ if (pqn->q && pqn->q->gws)
+ return -EINVAL;
+ }
+
+ pqn->q->gws = gws;
+ pdd->qpd.num_gws = gws ? amdgpu_amdkfd_get_num_gws(dev->kgd) : 0;
+
+ return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
+ pqn->q);
+}
+
+static void *pqm_get_gws(struct process_queue_manager *pqm, unsigned int qid)
+{
+ struct process_queue_node *pqn;
+
+ pqn = get_queue_by_qid(pqm, qid);
+ if (!pqn) {
+ pr_debug("No queue %d exists for get gws operation\n", qid);
+ return NULL;
+ }
+
+ return pqn->q->gws;
+}
+
+
void kfd_process_dequeue_from_all_devices(struct kfd_process *p)
{
struct kfd_process_device *pdd;
@@ -313,6 +365,12 @@ int pqm_destroy_queue(struct process_queue_manager *pqm,
unsigned int qid)
return -1;
}
+ if (pqm_get_gws(pqm, qid)) {
+
amdgpu_amdkfd_remove_gws_from_process(pqm->process->kgd_process_info,
+ pqm_get_gws(pqm, qid));
+ pqm_set_gws(pqm, qid, NULL);
+ }
+
if (pqn->kq) {
/* destroy kernel queue (DIQ) */
dqm = pqn->kq->dev->dqm;
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 3/7] drm/amdkfd: Allocate gws on device initialization
On device initialization, KFD allocates all (64) gws which
is shared by all KFD processes.
Change-Id: I1f1274b8d4f6a8ad08785e2791a9a79def75e913
Signed-off-by: Oak Zeng
Reviewed-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdkfd/kfd_device.c | 14 +-
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 +++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 7b4ea24..9d1b026 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -553,6 +553,13 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
} else
kfd->max_proc_per_quantum = hws_max_conc_proc;
+ /* Allocate global GWS that is shared by all KFD processes */
+ if (hws_gws_support && amdgpu_amdkfd_alloc_gws(kfd->kgd,
+ amdgpu_amdkfd_get_num_gws(kfd->kgd), >gws)) {
+ dev_err(kfd_device, "Could not allocate %d gws\n",
+ amdgpu_amdkfd_get_num_gws(kfd->kgd));
+ goto out;
+ }
/* calculate max size of mqds needed for queues */
size = max_num_of_queues_per_device *
kfd->device_info->mqd_size_aligned;
@@ -576,7 +583,7 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>gtt_start_gpu_addr, >gtt_start_cpu_ptr,
false)) {
dev_err(kfd_device, "Could not allocate %d bytes\n", size);
- goto out;
+ goto alloc_gtt_mem_failure;
}
dev_info(kfd_device, "Allocated %d bytes on gart\n", size);
@@ -646,6 +653,9 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
kfd_gtt_sa_fini(kfd);
kfd_gtt_sa_init_error:
amdgpu_amdkfd_free_gtt_mem(kfd->kgd, kfd->gtt_mem);
+alloc_gtt_mem_failure:
+ if (hws_gws_support)
+ amdgpu_amdkfd_free_gws(kfd->kgd, kfd->gws);
dev_err(kfd_device,
"device %x:%x NOT added due to errors\n",
kfd->pdev->vendor, kfd->pdev->device);
@@ -663,6 +673,8 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
kfd_doorbell_fini(kfd);
kfd_gtt_sa_fini(kfd);
amdgpu_amdkfd_free_gtt_mem(kfd->kgd, kfd->gtt_mem);
+ if (hws_gws_support)
+ amdgpu_amdkfd_free_gws(kfd->kgd, kfd->gws);
}
kfree(kfd);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 338fb07..5969e37 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -288,6 +288,9 @@ struct kfd_dev {
/* Compute Profile ref. count */
atomic_t compute_profile;
+
+ /* Global GWS resource shared b/t processes*/
+ void *gws;
};
enum kfd_mempool {
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/7] drm/amdgpu: Add function to add/remove gws to kfd process
GWS bo is shared between all kfd processes. Add function to add gws
to kfd process's bo list so gws can be evicted from and restored
for process.
Change-Id: I75d74cfdadb7075ff8b2b68634e205deb73dc1ea
Signed-off-by: Oak Zeng
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 2 +
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 103 +--
2 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index c00c974..f968bf1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -155,6 +155,8 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct kgd_dev *kgd, size_t
size,
void amdgpu_amdkfd_free_gtt_mem(struct kgd_dev *kgd, void *mem_obj);
int amdgpu_amdkfd_alloc_gws(struct kgd_dev *kgd, size_t size, void **mem_obj);
void amdgpu_amdkfd_free_gws(struct kgd_dev *kgd, void *mem_obj);
+int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem
**mem);
+int amdgpu_amdkfd_remove_gws_from_process(void *info, void *mem);
uint32_t amdgpu_amdkfd_get_fw_version(struct kgd_dev *kgd,
enum kgd_engine_type type);
void amdgpu_amdkfd_get_local_mem_info(struct kgd_dev *kgd,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e1cae4a..87177ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -457,6 +457,17 @@ static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem *mem,
mutex_unlock(_info->lock);
}
+static void remove_kgd_mem_from_kfd_bo_list(struct kgd_mem *mem,
+ struct amdkfd_process_info *process_info)
+{
+ struct ttm_validate_buffer *bo_list_entry;
+
+ bo_list_entry = >validate_list;
+ mutex_lock(_info->lock);
+ list_del(_list_entry->head);
+ mutex_unlock(_info->lock);
+}
+
/* Initializes user pages. It registers the MMU notifier and validates
* the userptr BO in the GTT domain.
*
@@ -1183,12 +1194,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
if (user_addr) {
ret = init_user_pages(*mem, current->mm, user_addr);
- if (ret) {
- mutex_lock(>process_info->lock);
- list_del(&(*mem)->validate_list.head);
- mutex_unlock(>process_info->lock);
+ if (ret)
goto allocate_init_user_pages_failed;
- }
}
if (offset)
@@ -1197,6 +1204,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
return 0;
allocate_init_user_pages_failed:
+ remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
amdgpu_bo_unref();
/* Don't unreserve system mem limit twice */
goto err_reserve_limit;
@@ -2104,3 +2112,88 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info,
struct dma_fence **ef)
kfree(pd_bo_list);
return ret;
}
+
+int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem
**mem)
+{
+ struct amdkfd_process_info *process_info = (struct amdkfd_process_info
*)info;
+ struct amdgpu_bo *gws_bo = (struct amdgpu_bo *)gws;
+ int ret;
+
+ if (!info || !gws)
+ return -EINVAL;
+
+ *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
+ if (!*mem)
+ return -EINVAL;
+
+ mutex_init(&(*mem)->lock);
+ (*mem)->bo = amdgpu_bo_ref(gws_bo);
+ (*mem)->domain = AMDGPU_GEM_DOMAIN_GWS;
+ (*mem)->process_info = process_info;
+ add_kgd_mem_to_kfd_bo_list(*mem, process_info, false);
+ amdgpu_sync_create(&(*mem)->sync);
+
+
+ /* Validate gws bo the first time it is added to process */
+ mutex_lock(&(*mem)->process_info->lock);
+ ret = amdgpu_bo_reserve(gws_bo, false);
+ if (unlikely(ret)) {
+ pr_err("Reserve gws bo failed %d\n", ret);
+ goto bo_reservation_failure;
+ }
+
+ ret = amdgpu_amdkfd_bo_validate(gws_bo, AMDGPU_GEM_DOMAIN_GWS, true);
+ if (ret) {
+ pr_err("GWS BO validate failed %d\n", ret);
+ goto bo_validation_failure;
+ }
+ /* GWS resource is shared b/t amdgpu and amdkfd
+* Add process eviction fence to bo so they can
+* evict each other.
+*/
+ amdgpu_bo_fence(gws_bo, _info->eviction_fence->base, true);
+ amdgpu_bo_unreserve(gws_bo);
+ mutex_unlock(&(*mem)->process_info->lock);
+
+ return ret;
+
+bo_validation_failure:
+ amdgpu_bo_unreserve(gws_bo);
+bo_reservation_failure:
+ mutex_unlock(&(*mem)->process_info->lock);
+ amdgpu_sync_free(&(*mem)->sync);
+ remove_kgd_mem_from_kfd_bo_list(*mem, process_info);
+ amdgpu_bo_unref(_bo);
+ mutex_destroy(&(*mem)->lock);
+ kfree(*mem);
+ *mem = NULL;
+ return
[PATCH 2/7] drm/amdgpu: Add interface to alloc gws from amdgpu
Add amdgpu_amdkfd interface to alloc and free gws
from amdgpu
Change-Id: I4eb418356e5a6051aa09c5e2c4a454263631d6ab
Signed-off-by: Oak Zeng
Reviewed-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 34 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 2 ++
2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index a4780d5..4af3989 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -339,6 +339,40 @@ void amdgpu_amdkfd_free_gtt_mem(struct kgd_dev *kgd, void
*mem_obj)
amdgpu_bo_unref(&(bo));
}
+int amdgpu_amdkfd_alloc_gws(struct kgd_dev *kgd, size_t size,
+ void **mem_obj)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)kgd;
+ struct amdgpu_bo *bo = NULL;
+ struct amdgpu_bo_param bp;
+ int r;
+
+ memset(, 0, sizeof(bp));
+ bp.size = size;
+ bp.byte_align = 1;
+ bp.domain = AMDGPU_GEM_DOMAIN_GWS;
+ bp.flags = AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
+ bp.type = ttm_bo_type_device;
+ bp.resv = NULL;
+
+ r = amdgpu_bo_create(adev, , );
+ if (r) {
+ dev_err(adev->dev,
+ "failed to allocate gws BO for amdkfd (%d)\n", r);
+ return r;
+ }
+
+ *mem_obj = bo;
+ return 0;
+}
+
+void amdgpu_amdkfd_free_gws(struct kgd_dev *kgd, void *mem_obj)
+{
+ struct amdgpu_bo *bo = (struct amdgpu_bo *)mem_obj;
+
+ amdgpu_bo_unref();
+}
+
uint32_t amdgpu_amdkfd_get_fw_version(struct kgd_dev *kgd,
enum kgd_engine_type type)
{
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 5700643..c00c974 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -153,6 +153,8 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct kgd_dev *kgd, size_t
size,
void **mem_obj, uint64_t *gpu_addr,
void **cpu_ptr, bool mqd_gfx9);
void amdgpu_amdkfd_free_gtt_mem(struct kgd_dev *kgd, void *mem_obj);
+int amdgpu_amdkfd_alloc_gws(struct kgd_dev *kgd, size_t size, void **mem_obj);
+void amdgpu_amdkfd_free_gws(struct kgd_dev *kgd, void *mem_obj);
uint32_t amdgpu_amdkfd_get_fw_version(struct kgd_dev *kgd,
enum kgd_engine_type type);
void amdgpu_amdkfd_get_local_mem_info(struct kgd_dev *kgd,
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/7] drm/amdkfd: Add gws number to kfd topology node properties
Add amdgpu_amdkfd interface to get num_gws and add num_gws
to /sys/class/kfd/kfd/topology/nodes/x/properties. Only report
num_gws if MEC FW support GWS barriers. Currently it is
determined by a module parameter which will be replaced
with MEC FW version check when firmware is ready.
Change-Id: Ie0d00fb20a37ef2856860dbecbe1ad0ca1ef09f7
Signed-off-by: Oak Zeng
Reviewed-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 7 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 10 ++
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 5 +
drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 5 +
drivers/gpu/drm/amd/amdkfd/kfd_topology.h | 1 +
6 files changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 98326e3b..a4780d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -544,6 +544,13 @@ uint64_t amdgpu_amdkfd_get_mmio_remap_phys_addr(struct
kgd_dev *kgd)
return adev->rmmio_remap.bus_addr;
}
+uint32_t amdgpu_amdkfd_get_num_gws(struct kgd_dev *kgd)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)kgd;
+
+ return adev->gds.gws_size;
+}
+
int amdgpu_amdkfd_submit_ib(struct kgd_dev *kgd, enum kgd_engine_type engine,
uint32_t vmid, uint64_t gpu_addr,
uint32_t *ib_cmd, uint32_t ib_len)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index f57f297..5700643 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -169,6 +169,7 @@ int amdgpu_amdkfd_get_dmabuf_info(struct kgd_dev *kgd, int
dma_buf_fd,
uint64_t amdgpu_amdkfd_get_vram_usage(struct kgd_dev *kgd);
uint64_t amdgpu_amdkfd_get_hive_id(struct kgd_dev *kgd);
uint64_t amdgpu_amdkfd_get_mmio_remap_phys_addr(struct kgd_dev *kgd);
+uint32_t amdgpu_amdkfd_get_num_gws(struct kgd_dev *kgd);
uint8_t amdgpu_amdkfd_get_xgmi_hops_count(struct kgd_dev *dst, struct kgd_dev
*src);
#define read_user_wptr(mmptr, wptr, dst) \
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index a334d3b..3a03c2b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -666,6 +666,16 @@ MODULE_PARM_DESC(noretry,
int halt_if_hws_hang;
module_param(halt_if_hws_hang, int, 0644);
MODULE_PARM_DESC(halt_if_hws_hang, "Halt if HWS hang is detected (0 = off
(default), 1 = on)");
+
+/**
+ * DOC: hws_gws_support(bool)
+ * Whether HWS support gws barriers. Default value: false (not supported)
+ * This will be replaced with a MEC firmware version check once firmware
+ * is ready
+ */
+bool hws_gws_support;
+module_param(hws_gws_support, bool, 0444);
+MODULE_PARM_DESC(hws_gws_support, "MEC FW support gws barriers (false = not
supported (Default), true = supported)");
#endif
/**
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 8f02d78..338fb07 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -160,6 +160,11 @@ extern int noretry;
*/
extern int halt_if_hws_hang;
+/*
+ * Whether MEC FW support GWS barriers
+ */
+extern bool hws_gws_support;
+
enum cache_policy {
cache_policy_coherent,
cache_policy_noncoherent
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 2c06d6c..128c72c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -454,6 +454,8 @@ static ssize_t node_show(struct kobject *kobj, struct
attribute *attr,
dev->node_props.lds_size_in_kb);
sysfs_show_32bit_prop(buffer, "gds_size_in_kb",
dev->node_props.gds_size_in_kb);
+ sysfs_show_32bit_prop(buffer, "num_gws",
+ dev->node_props.num_gws);
sysfs_show_32bit_prop(buffer, "wave_front_size",
dev->node_props.wave_front_size);
sysfs_show_32bit_prop(buffer, "array_count",
@@ -1290,6 +1292,9 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
dev->node_props.num_sdma_engines = gpu->device_info->num_sdma_engines;
dev->node_props.num_sdma_xgmi_engines =
gpu->device_info->num_xgmi_sdma_engines;
+ dev->node_props.num_gws = (hws_gws_support &&
+ dev->gpu->dqm->sched_policy != KFD_SCHED_POLICY_NO_HWS) ?
+ amdgpu_amdkfd_get_num_gws(dev->gpu->kgd) : 0;
kfd_fill_mem_clk_max_info(dev);
kfd_fill_iolink_non_crat_info(dev);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
index 949e885..276354a 100644
---
Re: [PATCH 2/2] drm/amd/display: Use new connector state when getting color depth
Series is
Reviewed-by: Harry Wentland
Harry
On 2019-05-22 11:11 a.m., Nicholas Kazlauskas wrote:
> [CAUTION: External Email]
>
> [Why]
> The current state on the connector is queried when getting the max bpc
> rather than the new state. This means that a new max bpc value can only
> currently take effect on the commit *after* it changes.
>
> The new state should be passed in instead.
>
> [How]
> Pass down the dm_state as drm state to where we do color depth lookup.
>
> The passed in state can still be NULL when called from
> amdgpu_dm_connector_mode_valid, so make sure that we have reasonable
> defaults in place. That should probably be addressed at some point.
>
> This change now (correctly) causes a modeset to occur when changing the
> max bpc for a connector.
>
> Cc: Leo Li
> Cc: Harry Wentland
> Signed-off-by: Nicholas Kazlauskas
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +++
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index a7a9e4d81a17..580c324891fd 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3038,13 +3038,14 @@ static void update_stream_scaling_settings(const
> struct drm_display_mode *mode,
> }
>
> static enum dc_color_depth
> -convert_color_depth_from_display_info(const struct drm_connector *connector)
> +convert_color_depth_from_display_info(const struct drm_connector *connector,
> + const struct drm_connector_state *state)
> {
> uint32_t bpc = 8;
>
> /* TODO: Use passed in state instead of the current state. */
> - if (connector->state) {
> - bpc = connector->state->max_bpc;
> + if (state) {
> + bpc = state->max_bpc;
> /* Round down to the nearest even number. */
> bpc = bpc - (bpc & 1);
> }
> @@ -3165,11 +3166,12 @@ static void
> adjust_colour_depth_from_display_info(struct dc_crtc_timing *timing_
>
> }
>
> -static void
> -fill_stream_properties_from_drm_display_mode(struct dc_stream_state *stream,
> -const struct drm_display_mode
> *mode_in,
> -const struct drm_connector
> *connector,
> -const struct dc_stream_state
> *old_stream)
> +static void fill_stream_properties_from_drm_display_mode(
> + struct dc_stream_state *stream,
> + const struct drm_display_mode *mode_in,
> + const struct drm_connector *connector,
> + const struct drm_connector_state *connector_state,
> + const struct dc_stream_state *old_stream)
> {
> struct dc_crtc_timing *timing_out = >timing;
> const struct drm_display_info *info = >display_info;
> @@ -3192,7 +3194,7 @@ fill_stream_properties_from_drm_display_mode(struct
> dc_stream_state *stream,
>
> timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
> timing_out->display_color_depth =
> convert_color_depth_from_display_info(
> - connector);
> + connector, connector_state);
> timing_out->scan_type = SCANNING_TYPE_NODATA;
> timing_out->hdmi_vic = 0;
>
> @@ -3389,6 +3391,8 @@ create_stream_for_sink(struct amdgpu_dm_connector
> *aconnector,
> {
> struct drm_display_mode *preferred_mode = NULL;
> struct drm_connector *drm_connector;
> + const struct drm_connector_state *con_state =
> + dm_state ? _state->base : NULL;
> struct dc_stream_state *stream = NULL;
> struct drm_display_mode mode = *drm_mode;
> bool native_mode_found = false;
> @@ -3461,10 +3465,10 @@ create_stream_for_sink(struct amdgpu_dm_connector
> *aconnector,
> */
> if (!scale || mode_refresh != preferred_refresh)
> fill_stream_properties_from_drm_display_mode(stream,
> - , >base, NULL);
> + , >base, con_state, NULL);
> else
> fill_stream_properties_from_drm_display_mode(stream,
> - , >base, old_stream);
> + , >base, con_state, old_stream);
>
> update_stream_scaling_settings(, dm_state, stream);
>
> --
> 2.17.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Wed, May 22, 2019 at 3:11 AM Catalin Marinas wrote: > > Hi Kees, > > Thanks for joining the thread ;). > > On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote: > > On Tue, May 21, 2019 at 07:29:33PM +0100, Catalin Marinas wrote: > > > On Mon, May 20, 2019 at 04:53:07PM -0700, Evgenii Stepanov wrote: > > > > On Fri, May 17, 2019 at 7:49 AM Catalin Marinas > > > > wrote: > > > > > IMO (RFC for now), I see two ways forward: > > > > > [...] > > > > > 2. Similar shim to the above libc wrapper but inside the kernel > > > > >(arch/arm64 only; most pointer arguments could be covered with an > > > > >__SC_CAST similar to the s390 one). There are two differences from > > > > >what we've discussed in the past: > > > > > > > > > >a) this is an opt-in by the user which would have to explicitly > > > > > call > > > > > prctl(). If it returns -ENOTSUPP etc., the user won't be allowed > > > > > to pass tagged pointers to the kernel. This would probably be > > > > > the > > > > > responsibility of the C lib to make sure it doesn't tag heap > > > > > allocations. If the user did not opt-in, the syscalls are routed > > > > > through the normal path (no untagging address shim). > > > > > > > > > >b) ioctl() and other blacklisted syscalls (prctl) will not accept > > > > > tagged pointers (to be documented in Vicenzo's ABI patches). > > > > > > > > The way I see it, a patch that breaks handling of tagged pointers is > > > > not that different from, say, a patch that adds a wild pointer > > > > dereference. Both are bugs; the difference is that (a) the former > > > > breaks a relatively uncommon target and (b) it's arguably an easier > > > > mistake to make. If MTE adoption goes well, (a) will not be the case > > > > for long. > > > > > > It's also the fact such patch would go unnoticed for a long time until > > > someone exercises that code path. And when they do, the user would be > > > pretty much in the dark trying to figure what what went wrong, why a > > > SIGSEGV or -EFAULT happened. What's worse, we can't even say we fixed > > > all the places where it matters in the current kernel codebase (ignoring > > > future patches). > > > > So, looking forward a bit, this isn't going to be an ARM-specific issue > > for long. > > I do hope so. > > > In fact, I think we shouldn't have arm-specific syscall wrappers > > in this series: I think untagged_addr() should likely be added at the > > top-level and have it be a no-op for other architectures. > > That's what the current patchset does, so we have this as a starting > point. Kostya raised another potential issue with the syscall wrappers: > with MTE the kernel will be forced to enable the match-all (wildcard) > pointers for user space accesses since copy_from_user() would only get a > 0 tag. So it has wider implications than just uaccess routines not > checking the colour. > > > So given this becoming a kernel-wide multi-architecture issue (under > > the assumption that x86, RISC-V, and others will gain similar TBI or > > MTE things), we should solve it in a way that we can re-use. > > Can we do any better to aid the untagged_addr() placement (e.g. better > type annotations, better static analysis)? We have to distinguish > between user pointers that may be dereferenced by the kernel (I think > almost fully covered with this patchset) and user addresses represented > as ulong that may: > > a) be converted to a user pointer and dereferenced; I think that's the >case for many overloaded ulong/u64 arguments > > b) used for address space management, rbtree look-ups etc. where the tag >is no longer relevant and it even gets in the way > > We tried last year to identify void __user * casts to unsigned long > using sparse on the assumption that pointers can be tagged while ulong > is about address space management and needs to lose such tag. I think we > could have pushed this further. For example, get_user_pages() takes an > unsigned long but it is perfectly capable of untagging the address > itself. Shall we change its first argument to void __user * (together > with all its callers)? > > find_vma(), OTOH, could untag the address but it doesn't help since > vm_start/end don't have such information (that's more about the content > or type that the user decided) and the callers check against it. > > Are there any other places where this matters? These patches tracked > down find_vma() as some heuristics but we may need better static > analysis to identify other cases. > > > We need something that is going to work everywhere. And it needs to be > > supported by the kernel for the simple reason that the kernel needs to > > do MTE checks during copy_from_user(): having that information stripped > > means we lose any userspace-assigned MTE protections if they get handled > > by the kernel, which is a total non-starter, IMO. > > Such feedback is welcomed ;). > > > As an aside: I think Sparc ADI support in Linux
Re: [PATCH 2/2] drm/amd/display: Use new connector state when getting color depth
Series is:
Acked-by: Alex Deucher
From: amd-gfx on behalf of Nicholas
Kazlauskas
Sent: Wednesday, May 22, 2019 11:11 AM
To: amd-gfx@lists.freedesktop.org
Cc: Li, Sun peng (Leo); Wentland, Harry; Kazlauskas, Nicholas
Subject: [PATCH 2/2] drm/amd/display: Use new connector state when getting
color depth
[CAUTION: External Email]
[Why]
The current state on the connector is queried when getting the max bpc
rather than the new state. This means that a new max bpc value can only
currently take effect on the commit *after* it changes.
The new state should be passed in instead.
[How]
Pass down the dm_state as drm state to where we do color depth lookup.
The passed in state can still be NULL when called from
amdgpu_dm_connector_mode_valid, so make sure that we have reasonable
defaults in place. That should probably be addressed at some point.
This change now (correctly) causes a modeset to occur when changing the
max bpc for a connector.
Cc: Leo Li
Cc: Harry Wentland
Signed-off-by: Nicholas Kazlauskas
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +++
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a7a9e4d81a17..580c324891fd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3038,13 +3038,14 @@ static void update_stream_scaling_settings(const struct
drm_display_mode *mode,
}
static enum dc_color_depth
-convert_color_depth_from_display_info(const struct drm_connector *connector)
+convert_color_depth_from_display_info(const struct drm_connector *connector,
+ const struct drm_connector_state *state)
{
uint32_t bpc = 8;
/* TODO: Use passed in state instead of the current state. */
- if (connector->state) {
- bpc = connector->state->max_bpc;
+ if (state) {
+ bpc = state->max_bpc;
/* Round down to the nearest even number. */
bpc = bpc - (bpc & 1);
}
@@ -3165,11 +3166,12 @@ static void
adjust_colour_depth_from_display_info(struct dc_crtc_timing *timing_
}
-static void
-fill_stream_properties_from_drm_display_mode(struct dc_stream_state *stream,
-const struct drm_display_mode
*mode_in,
-const struct drm_connector
*connector,
-const struct dc_stream_state
*old_stream)
+static void fill_stream_properties_from_drm_display_mode(
+ struct dc_stream_state *stream,
+ const struct drm_display_mode *mode_in,
+ const struct drm_connector *connector,
+ const struct drm_connector_state *connector_state,
+ const struct dc_stream_state *old_stream)
{
struct dc_crtc_timing *timing_out = >timing;
const struct drm_display_info *info = >display_info;
@@ -3192,7 +3194,7 @@ fill_stream_properties_from_drm_display_mode(struct
dc_stream_state *stream,
timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
timing_out->display_color_depth = convert_color_depth_from_display_info(
- connector);
+ connector, connector_state);
timing_out->scan_type = SCANNING_TYPE_NODATA;
timing_out->hdmi_vic = 0;
@@ -3389,6 +3391,8 @@ create_stream_for_sink(struct amdgpu_dm_connector
*aconnector,
{
struct drm_display_mode *preferred_mode = NULL;
struct drm_connector *drm_connector;
+ const struct drm_connector_state *con_state =
+ dm_state ? _state->base : NULL;
struct dc_stream_state *stream = NULL;
struct drm_display_mode mode = *drm_mode;
bool native_mode_found = false;
@@ -3461,10 +3465,10 @@ create_stream_for_sink(struct amdgpu_dm_connector
*aconnector,
*/
if (!scale || mode_refresh != preferred_refresh)
fill_stream_properties_from_drm_display_mode(stream,
- , >base, NULL);
+ , >base, con_state, NULL);
else
fill_stream_properties_from_drm_display_mode(stream,
- , >base, old_stream);
+ , >base, con_state, old_stream);
update_stream_scaling_settings(, dm_state, stream);
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/2] drm/amd/display: Use new connector state when getting color depth
[Why]
The current state on the connector is queried when getting the max bpc
rather than the new state. This means that a new max bpc value can only
currently take effect on the commit *after* it changes.
The new state should be passed in instead.
[How]
Pass down the dm_state as drm state to where we do color depth lookup.
The passed in state can still be NULL when called from
amdgpu_dm_connector_mode_valid, so make sure that we have reasonable
defaults in place. That should probably be addressed at some point.
This change now (correctly) causes a modeset to occur when changing the
max bpc for a connector.
Cc: Leo Li
Cc: Harry Wentland
Signed-off-by: Nicholas Kazlauskas
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +++
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a7a9e4d81a17..580c324891fd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3038,13 +3038,14 @@ static void update_stream_scaling_settings(const struct
drm_display_mode *mode,
}
static enum dc_color_depth
-convert_color_depth_from_display_info(const struct drm_connector *connector)
+convert_color_depth_from_display_info(const struct drm_connector *connector,
+ const struct drm_connector_state *state)
{
uint32_t bpc = 8;
/* TODO: Use passed in state instead of the current state. */
- if (connector->state) {
- bpc = connector->state->max_bpc;
+ if (state) {
+ bpc = state->max_bpc;
/* Round down to the nearest even number. */
bpc = bpc - (bpc & 1);
}
@@ -3165,11 +3166,12 @@ static void
adjust_colour_depth_from_display_info(struct dc_crtc_timing *timing_
}
-static void
-fill_stream_properties_from_drm_display_mode(struct dc_stream_state *stream,
-const struct drm_display_mode
*mode_in,
-const struct drm_connector
*connector,
-const struct dc_stream_state
*old_stream)
+static void fill_stream_properties_from_drm_display_mode(
+ struct dc_stream_state *stream,
+ const struct drm_display_mode *mode_in,
+ const struct drm_connector *connector,
+ const struct drm_connector_state *connector_state,
+ const struct dc_stream_state *old_stream)
{
struct dc_crtc_timing *timing_out = >timing;
const struct drm_display_info *info = >display_info;
@@ -3192,7 +3194,7 @@ fill_stream_properties_from_drm_display_mode(struct
dc_stream_state *stream,
timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
timing_out->display_color_depth = convert_color_depth_from_display_info(
- connector);
+ connector, connector_state);
timing_out->scan_type = SCANNING_TYPE_NODATA;
timing_out->hdmi_vic = 0;
@@ -3389,6 +3391,8 @@ create_stream_for_sink(struct amdgpu_dm_connector
*aconnector,
{
struct drm_display_mode *preferred_mode = NULL;
struct drm_connector *drm_connector;
+ const struct drm_connector_state *con_state =
+ dm_state ? _state->base : NULL;
struct dc_stream_state *stream = NULL;
struct drm_display_mode mode = *drm_mode;
bool native_mode_found = false;
@@ -3461,10 +3465,10 @@ create_stream_for_sink(struct amdgpu_dm_connector
*aconnector,
*/
if (!scale || mode_refresh != preferred_refresh)
fill_stream_properties_from_drm_display_mode(stream,
- , >base, NULL);
+ , >base, con_state, NULL);
else
fill_stream_properties_from_drm_display_mode(stream,
- , >base, old_stream);
+ , >base, con_state, old_stream);
update_stream_scaling_settings(, dm_state, stream);
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/2] drm/amd/display: Switch the custom "max bpc" property to the DRM prop
[Why]
The custom "max bpc" property was added to limit color depth while the
DRM one was still being merged. It's been a few kernel versions since
then and this TODO was still sticking around.
[How]
Attach the DRM max bpc property to the connector and drop all of our
custom property management. Set the max bpc to 8 by default since
DRM defaults to the max in the range which would be 16 in this case.
No behavioral changes are intended with this patch, it should just be
a refactor.
Cc: Leo Li
Cc: Harry Wentland
Signed-off-by: Nicholas Kazlauskas
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 2 --
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 ---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
4 files changed, 13 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 4e944737b708..767ee6991ef4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -631,10 +631,6 @@ int amdgpu_display_modeset_create_props(struct
amdgpu_device *adev)
amdgpu_dither_enum_list, sz);
if (amdgpu_device_has_dc_support(adev)) {
- adev->mode_info.max_bpc_property =
- drm_property_create_range(adev->ddev, 0, "max bpc", 8,
16);
- if (!adev->mode_info.max_bpc_property)
- return -ENOMEM;
adev->mode_info.abm_level_property =
drm_property_create_range(adev->ddev, 0,
"abm level", 0, 4);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index c7940af42f76..8bda00ce8816 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -332,8 +332,6 @@ struct amdgpu_mode_info {
struct drm_property *audio_property;
/* FMT dithering */
struct drm_property *dither_property;
- /* maximum number of bits per channel for monitor color */
- struct drm_property *max_bpc_property;
/* Adaptive Backlight Modulation (power feature) */
struct drm_property *abm_level_property;
/* it is used to allow enablement of freesync mode */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 4a1755bce96c..a7a9e4d81a17 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3040,14 +3040,14 @@ static void update_stream_scaling_settings(const struct
drm_display_mode *mode,
static enum dc_color_depth
convert_color_depth_from_display_info(const struct drm_connector *connector)
{
- struct dm_connector_state *dm_conn_state =
- to_dm_connector_state(connector->state);
- uint32_t bpc = connector->display_info.bpc;
+ uint32_t bpc = 8;
- /* TODO: Remove this when there's support for max_bpc in drm */
- if (dm_conn_state && bpc > dm_conn_state->max_bpc)
- /* Round down to nearest even number. */
- bpc = dm_conn_state->max_bpc - (dm_conn_state->max_bpc & 1);
+ /* TODO: Use passed in state instead of the current state. */
+ if (connector->state) {
+ bpc = connector->state->max_bpc;
+ /* Round down to the nearest even number. */
+ bpc = bpc - (bpc & 1);
+ }
switch (bpc) {
case 0:
@@ -3689,9 +3689,6 @@ int amdgpu_dm_connector_atomic_set_property(struct
drm_connector *connector,
} else if (property == adev->mode_info.underscan_property) {
dm_new_state->underscan_enable = val;
ret = 0;
- } else if (property == adev->mode_info.max_bpc_property) {
- dm_new_state->max_bpc = val;
- ret = 0;
} else if (property == adev->mode_info.abm_level_property) {
dm_new_state->abm_level = val;
ret = 0;
@@ -3743,9 +3740,6 @@ int amdgpu_dm_connector_atomic_get_property(struct
drm_connector *connector,
} else if (property == adev->mode_info.underscan_property) {
*val = dm_state->underscan_enable;
ret = 0;
- } else if (property == adev->mode_info.max_bpc_property) {
- *val = dm_state->max_bpc;
- ret = 0;
} else if (property == adev->mode_info.abm_level_property) {
*val = dm_state->abm_level;
ret = 0;
@@ -3808,7 +3802,6 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector
*connector)
state->underscan_enable = false;
state->underscan_hborder = 0;
state->underscan_vborder = 0;
- state->max_bpc = 8;
Re: [PATCH v15 17/17] selftests, arm64: add a selftest for passing tagged pointers to kernel
On Mon, May 06, 2019 at 06:31:03PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch adds a simple test, that calls the uname syscall with a
> tagged user pointer as an argument. Without the kernel accepting tagged
> user pointers the test fails with EFAULT.
That's probably sufficient for a simple example. Something we could add
to Documentation maybe is a small library that can be LD_PRELOAD'ed so
that you can run a lot more tests like LTP.
We could add this to selftests but I think it's too glibc specific.
8<
#include
#define TAG_SHIFT (56)
#define TAG_MASK(0xffUL << TAG_SHIFT)
void *__libc_malloc(size_t size);
void __libc_free(void *ptr);
void *__libc_realloc(void *ptr, size_t size);
void *__libc_calloc(size_t nmemb, size_t size);
static void *tag_ptr(void *ptr)
{
unsigned long tag = rand() & 0xff;
if (!ptr)
return ptr;
return (void *)((unsigned long)ptr | (tag << TAG_SHIFT));
}
static void *untag_ptr(void *ptr)
{
return (void *)((unsigned long)ptr & ~TAG_MASK);
}
void *malloc(size_t size)
{
return tag_ptr(__libc_malloc(size));
}
void free(void *ptr)
{
__libc_free(untag_ptr(ptr));
}
void *realloc(void *ptr, size_t size)
{
return tag_ptr(__libc_realloc(untag_ptr(ptr), size));
}
void *calloc(size_t nmemb, size_t size)
{
return tag_ptr(__libc_calloc(nmemb, size));
}
Re: [PATCH] drm/sched: Fix static checker warning for potential NULL ptr
Am 22.05.19 um 15:57 schrieb Andrey Grodzovsky:
Signed-off-by: Andrey Grodzovsky
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_main.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 90d7a82..ec7faca 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -286,16 +286,17 @@ static void drm_sched_job_timedout(struct work_struct
*work)
job = list_first_entry_or_null(>ring_mirror_list,
struct drm_sched_job, node);
- if (job)
+ if (job) {
job->sched->ops->timedout_job(job);
- /*
-* Guilty job did complete and hence needs to be manually removed
-* See drm_sched_stop doc.
-*/
- if (sched->free_guilty) {
- job->sched->ops->free_job(job);
- sched->free_guilty = false;
+ /*
+* Guilty job did complete and hence needs to be manually
removed
+* See drm_sched_stop doc.
+*/
+ if (sched->free_guilty) {
+ job->sched->ops->free_job(job);
+ sched->free_guilty = false;
+ }
}
spin_lock_irqsave(>job_list_lock, flags);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/sched: Fix static checker warning for potential NULL ptr
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/scheduler/sched_main.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 90d7a82..ec7faca 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -286,16 +286,17 @@ static void drm_sched_job_timedout(struct work_struct
*work)
job = list_first_entry_or_null(>ring_mirror_list,
struct drm_sched_job, node);
- if (job)
+ if (job) {
job->sched->ops->timedout_job(job);
- /*
-* Guilty job did complete and hence needs to be manually removed
-* See drm_sched_stop doc.
-*/
- if (sched->free_guilty) {
- job->sched->ops->free_job(job);
- sched->free_guilty = false;
+ /*
+* Guilty job did complete and hence needs to be manually
removed
+* See drm_sched_stop doc.
+*/
+ if (sched->free_guilty) {
+ job->sched->ops->free_job(job);
+ sched->free_guilty = false;
+ }
}
spin_lock_irqsave(>job_list_lock, flags);
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Tue, May 21, 2019 at 03:48:56PM -0300, Jason Gunthorpe wrote: > On Fri, May 17, 2019 at 03:49:31PM +0100, Catalin Marinas wrote: > > > The tagged pointers (whether hwasan or MTE) should ideally be a > > transparent feature for the application writer but I don't think we can > > solve it entirely and make it seamless for the multitude of ioctls(). > > I'd say you only opt in to such feature if you know what you are doing > > and the user code takes care of specific cases like ioctl(), hence the > > prctl() proposal even for the hwasan. > > I'm not sure such a dire view is warrented.. > > The ioctl situation is not so bad, other than a few special cases, > most drivers just take a 'void __user *' and pass it as an argument to > some function that accepts a 'void __user *'. sparse et al verify > this. > > As long as the core functions do the right thing the drivers will be > OK. > > The only place things get dicy is if someone casts to unsigned long > (ie for vma work) but I think that reflects that our driver facing > APIs for VMAs are compatible with static analysis (ie I have no > earthly idea why get_user_pages() accepts an unsigned long), not that > this is too hard. If multiple people will care about this, perhaps we should try to annotate types more explicitly in SYSCALL_DEFINEx() and ABI data structures. For example, we could have a couple of mutually exclusive modifiers T __object * T __vaddr * (or U __vaddr) In the first case the pointer points to an object (in the C sense) that the call may dereference but not use for any other purpose. In the latter case the pointer (or other type) is a virtual address that the call does not dereference but my do other things with. Also U __really(T) to tell static analysers the real type of pointers smuggled through UAPI disguised as other types (*cough* KVM, etc.) We could gradually make sparse more strict about the presence of annotations and allowed conversions, add get/put_user() variants that demand explicit annotation, etc. find_vma() wouldn't work with a __object pointer, for example. A get_user_pages_for_dereference() might be needed for __object pointers (embodying a promise from the caller that only the object will be dereferenced within the mapped pages). Thoughts? This kind of thing would need widespread buy-in in order to be viable. Cheers ---Dave
[PATCH 09/10] drm/amdgpu: create GDS, GWS and OA in system domain
And only move them in on validation. This allows for better control when multiple processes are fighting over those resources. Signed-off-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 93b2c5a48a71..30493429851e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -495,7 +495,11 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, #endif bo->tbo.bdev = >mman.bdev; - amdgpu_bo_placement_from_domain(bo, bp->domain); + if (bp->domain & (AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA | + AMDGPU_GEM_DOMAIN_GDS)) + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU); + else + amdgpu_bo_placement_from_domain(bo, bp->domain); if (bp->type == ttm_bo_type_kernel) bo->tbo.priority = 1; -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 07/10] drm/amd/display: use ttm_eu_reserve_buffers instead of amdgpu_bo_reserve v2
From: Chunming Zhou
add ticket for display bo, so that it can preempt busy bo.
v2: fix stupid rebase error
Change-Id: I9f031cdcc8267de00e819ae303baa0a52df8ebb9
Signed-off-by: Chunming Zhou
Reviewed-by: Christian König
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 21 ++-
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 4a1755bce96c..56f320f3fd72 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4182,6 +4182,9 @@ static int dm_plane_helper_prepare_fb(struct drm_plane
*plane,
struct amdgpu_device *adev;
struct amdgpu_bo *rbo;
struct dm_plane_state *dm_plane_state_new, *dm_plane_state_old;
+ struct list_head list;
+ struct ttm_validate_buffer tv;
+ struct ww_acquire_ctx ticket;
uint64_t tiling_flags;
uint32_t domain;
int r;
@@ -4198,9 +4201,17 @@ static int dm_plane_helper_prepare_fb(struct drm_plane
*plane,
obj = new_state->fb->obj[0];
rbo = gem_to_amdgpu_bo(obj);
adev = amdgpu_ttm_adev(rbo->tbo.bdev);
- r = amdgpu_bo_reserve(rbo, false);
- if (unlikely(r != 0))
+ INIT_LIST_HEAD();
+
+ tv.bo = >tbo;
+ tv.num_shared = 1;
+ list_add(, );
+
+ r = ttm_eu_reserve_buffers(, , false, NULL, true);
+ if (r) {
+ dev_err(adev->dev, "fail to reserve bo (%d)\n", r);
return r;
+ }
if (plane->type != DRM_PLANE_TYPE_CURSOR)
domain = amdgpu_display_supported_domains(adev);
@@ -4211,21 +4222,21 @@ static int dm_plane_helper_prepare_fb(struct drm_plane
*plane,
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS)
DRM_ERROR("Failed to pin framebuffer with error %d\n",
r);
- amdgpu_bo_unreserve(rbo);
+ ttm_eu_backoff_reservation(, );
return r;
}
r = amdgpu_ttm_alloc_gart(>tbo);
if (unlikely(r != 0)) {
amdgpu_bo_unpin(rbo);
- amdgpu_bo_unreserve(rbo);
+ ttm_eu_backoff_reservation(, );
DRM_ERROR("%p bind failed\n", rbo);
return r;
}
amdgpu_bo_get_tiling_flags(rbo, _flags);
- amdgpu_bo_unreserve(rbo);
+ ttm_eu_backoff_reservation(, );
afb->address = amdgpu_bo_gpu_offset(rbo);
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 06/10] drm/ttm: fix busy memory to fail other user v10
BOs on the LRU might be blocked during command submission
and cause OOM situations.
Avoid this by blocking for the first busy BO not locked by
the same ticket as the BO we are searching space for.
v10: completely start over with the patch since we didn't
handled a whole bunch of corner cases.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 77 ++--
1 file changed, 66 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 4c6389d849ed..861facac33d4 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -771,32 +771,72 @@ EXPORT_SYMBOL(ttm_bo_eviction_valuable);
* b. Otherwise, trylock it.
*/
static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
- struct ttm_operation_ctx *ctx, bool *locked)
+ struct ttm_operation_ctx *ctx, bool *locked, bool *busy)
{
bool ret = false;
- *locked = false;
if (bo->resv == ctx->resv) {
reservation_object_assert_held(bo->resv);
if (ctx->flags & TTM_OPT_FLAG_ALLOW_RES_EVICT
|| !list_empty(>ddestroy))
ret = true;
+ *locked = false;
+ if (busy)
+ *busy = false;
} else {
- *locked = reservation_object_trylock(bo->resv);
- ret = *locked;
+ ret = reservation_object_trylock(bo->resv);
+ *locked = ret;
+ if (busy)
+ *busy = !ret;
}
return ret;
}
+/**
+ * ttm_mem_evict_wait_busy - wait for a busy BO to become available
+ *
+ * @busy_bo: BO which couldn't be locked with trylock
+ * @ctx: operation context
+ * @ticket: acquire ticket
+ *
+ * Try to lock a busy buffer object to avoid failing eviction.
+ */
+static int ttm_mem_evict_wait_busy(struct ttm_buffer_object *busy_bo,
+ struct ttm_operation_ctx *ctx,
+ struct ww_acquire_ctx *ticket)
+{
+ int r;
+
+ if (!busy_bo || !ticket)
+ return -EBUSY;
+
+ if (ctx->interruptible)
+ r = reservation_object_lock_interruptible(busy_bo->resv,
+ ticket);
+ else
+ r = reservation_object_lock(busy_bo->resv, ticket);
+
+ /*
+* TODO: It would be better to keep the BO locked until allocation is at
+* least tried one more time, but that would mean a much larger rework
+* of TTM.
+*/
+ if (!r)
+ reservation_object_unlock(busy_bo->resv);
+
+ return r == -EDEADLK ? -EAGAIN : r;
+}
+
static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
uint32_t mem_type,
const struct ttm_place *place,
- struct ttm_operation_ctx *ctx)
+ struct ttm_operation_ctx *ctx,
+ struct ww_acquire_ctx *ticket)
{
+ struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
struct ttm_bo_global *glob = bdev->glob;
struct ttm_mem_type_manager *man = >man[mem_type];
- struct ttm_buffer_object *bo = NULL;
bool locked = false;
unsigned i;
int ret;
@@ -804,8 +844,15 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
spin_lock(>lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
list_for_each_entry(bo, >lru[i], lru) {
- if (!ttm_bo_evict_swapout_allowable(bo, ctx, ))
+ bool busy;
+
+ if (!ttm_bo_evict_swapout_allowable(bo, ctx, ,
+ )) {
+ if (busy && !busy_bo &&
+ bo->resv->lock.ctx != ticket)
+ busy_bo = bo;
continue;
+ }
if (place && !bdev->driver->eviction_valuable(bo,
place)) {
@@ -824,8 +871,13 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
}
if (!bo) {
+ if (busy_bo)
+ ttm_bo_get(busy_bo);
spin_unlock(>lru_lock);
- return -EBUSY;
+ ret = ttm_mem_evict_wait_busy(busy_bo, ctx, ticket);
+ if (busy_bo)
+ ttm_bo_put(busy_bo);
+ return ret;
}
kref_get(>list_kref);
@@ -911,7 +963,8 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object
*bo,
return ret;
if (mem->mm_node)
break;
- ret =
[PATCH 10/10] drm/amdgpu: stop removing BOs from the LRU v3
This avoids OOM situations when we have lots of threads
submitting at the same time.
v3: apply this to the whole driver, not just CS
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c| 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c| 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 20f2955d2a55..3e2da24cd17a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -648,7 +648,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
}
r = ttm_eu_reserve_buffers(>ticket, >validated, true,
- , true);
+ , false);
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS)
DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
index 06f83cac0d3a..f660628e6af9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
@@ -79,7 +79,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct
amdgpu_vm *vm,
list_add(_tv.head, );
amdgpu_vm_get_pd_bo(vm, , );
- r = ttm_eu_reserve_buffers(, , true, NULL, true);
+ r = ttm_eu_reserve_buffers(, , true, NULL, false);
if (r) {
DRM_ERROR("failed to reserve CSA,PD BOs: err=%d\n", r);
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index d513a5ad03dd..ed25a4e14404 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -171,7 +171,7 @@ void amdgpu_gem_object_close(struct drm_gem_object *obj,
amdgpu_vm_get_pd_bo(vm, , _pd);
- r = ttm_eu_reserve_buffers(, , false, , true);
+ r = ttm_eu_reserve_buffers(, , false, , false);
if (r) {
dev_err(adev->dev, "leaking bo va because "
"we fail to reserve bo (%d)\n", r);
@@ -608,7 +608,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
amdgpu_vm_get_pd_bo(>vm, , _pd);
- r = ttm_eu_reserve_buffers(, , true, , true);
+ r = ttm_eu_reserve_buffers(, , true, , false);
if (r)
goto error_unref;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index c430e8259038..d60593cc436e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -155,7 +155,7 @@ static inline int amdgpu_bo_reserve(struct amdgpu_bo *bo,
bool no_intr)
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
int r;
- r = ttm_bo_reserve(>tbo, !no_intr, false, NULL);
+ r = __ttm_bo_reserve(>tbo, !no_intr, false, NULL);
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS)
dev_err(adev->dev, "%p reserve failed\n", bo);
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 08/10] drm/amdgpu: drop some validation failure messages
The messages about amdgpu_cs_list_validate are duplicated because the
caller will complain into the logs as well and we can also get
interrupted by a signal here.
Also fix the the caller to not report -EAGAIN from validation.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index fff558cf385b..20f2955d2a55 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -671,16 +671,12 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
*p,
}
r = amdgpu_cs_list_validate(p, );
- if (r) {
- DRM_ERROR("amdgpu_cs_list_validate(duplicates) failed.\n");
+ if (r)
goto error_validate;
- }
r = amdgpu_cs_list_validate(p, >validated);
- if (r) {
- DRM_ERROR("amdgpu_cs_list_validate(validated) failed.\n");
+ if (r)
goto error_validate;
- }
amdgpu_cs_report_moved_bytes(p->adev, p->bytes_moved,
p->bytes_moved_vis);
@@ -1383,7 +1379,7 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data,
struct drm_file *filp)
if (r) {
if (r == -ENOMEM)
DRM_ERROR("Not enough memory for command
submission!\n");
- else if (r != -ERESTARTSYS)
+ else if (r != -ERESTARTSYS && r != -EAGAIN)
DRM_ERROR("Failed to process the buffer list %d!\n", r);
goto out;
}
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 03/10] drm/ttm: remove manual placement preference
If drivers don't prefer a system memory placement
they should not but it into the placement list first.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 4336893cb35e..b29799aedb71 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1011,8 +1011,12 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
ttm_flag_masked(_flags, place->flags,
~TTM_PL_MASK_MEMTYPE);
- if (mem_type == TTM_PL_SYSTEM)
- break;
+ if (mem_type == TTM_PL_SYSTEM) {
+ mem->mem_type = mem_type;
+ mem->placement = cur_flags;
+ mem->mm_node = NULL;
+ return 0;
+ }
ret = (*man->func->get_node)(man, bo, place, mem);
if (unlikely(ret))
@@ -1024,16 +1028,12 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
(*man->func->put_node)(man, mem);
return ret;
}
- break;
+ mem->mem_type = mem_type;
+ mem->placement = cur_flags;
+ return 0;
}
}
- if ((type_ok && (mem_type == TTM_PL_SYSTEM)) || mem->mm_node) {
- mem->mem_type = mem_type;
- mem->placement = cur_flags;
- return 0;
- }
-
for (i = 0; i < placement->num_busy_placement; ++i) {
const struct ttm_place *place = >busy_placement[i];
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 02/10] drm/ttm: return immediately in case of a signal
When a signal arrives we should return immediately for
handling it and not try other placements first.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 2845fceb2fbd..4336893cb35e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -978,7 +978,6 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
uint32_t cur_flags = 0;
bool type_found = false;
bool type_ok = false;
- bool has_erestartsys = false;
int i, ret;
ret = reservation_object_reserve_shared(bo->resv, 1);
@@ -1069,8 +1068,8 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
mem->placement = cur_flags;
return 0;
}
- if (ret == -ERESTARTSYS)
- has_erestartsys = true;
+ if (ret && ret != -EBUSY)
+ return ret;
}
if (!type_found) {
@@ -1078,7 +1077,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
return -EINVAL;
}
- return (has_erestartsys) ? -ERESTARTSYS : -ENOMEM;
+ return -ENOMEM;
}
EXPORT_SYMBOL(ttm_bo_mem_space);
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 01/10] drm/ttm: Make LRU removal optional.
We are already doing this for DMA-buf imports and also for
amdgpu VM BOs for quite a while now.
If this doesn't run into any problems we are probably going
to stop removing BOs from the LRU altogether.
Signed-off-by: Christian König
---
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 9 +
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c| 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++--
drivers/gpu/drm/qxl/qxl_release.c | 2 +-
drivers/gpu/drm/radeon/radeon_gem.c | 2 +-
drivers/gpu/drm/radeon/radeon_object.c| 2 +-
drivers/gpu/drm/ttm/ttm_execbuf_util.c| 20 +++
drivers/gpu/drm/virtio/virtgpu_ioctl.c| 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 3 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h| 2 +-
include/drm/ttm/ttm_bo_driver.h | 5 -
include/drm/ttm/ttm_execbuf_util.h| 3 ++-
13 files changed, 34 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e1cae4a37113..647e18f9e136 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -574,7 +574,7 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
amdgpu_vm_get_pd_bo(vm, >list, >vm_pd[0]);
ret = ttm_eu_reserve_buffers(>ticket, >list,
-false, >duplicates);
+false, >duplicates, true);
if (!ret)
ctx->reserved = true;
else {
@@ -647,7 +647,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
}
ret = ttm_eu_reserve_buffers(>ticket, >list,
-false, >duplicates);
+false, >duplicates, true);
if (!ret)
ctx->reserved = true;
else
@@ -1800,7 +1800,8 @@ static int validate_invalid_user_pages(struct
amdkfd_process_info *process_info)
}
/* Reserve all BOs and page tables for validation */
- ret = ttm_eu_reserve_buffers(, _list, false, );
+ ret = ttm_eu_reserve_buffers(, _list, false, ,
+true);
WARN(!list_empty(), "Duplicates should be empty");
if (ret)
goto out_free;
@@ -2006,7 +2007,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info,
struct dma_fence **ef)
}
ret = ttm_eu_reserve_buffers(, ,
-false, _save);
+false, _save, true);
if (ret) {
pr_debug("Memory eviction: TTM Reserve Failed. Try again\n");
goto ttm_reserve_fail;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index d72cc583ebd1..fff558cf385b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -648,7 +648,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
}
r = ttm_eu_reserve_buffers(>ticket, >validated, true,
- );
+ , true);
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS)
DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
index 54dd02a898b9..06f83cac0d3a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
@@ -79,7 +79,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct
amdgpu_vm *vm,
list_add(_tv.head, );
amdgpu_vm_get_pd_bo(vm, , );
- r = ttm_eu_reserve_buffers(, , true, NULL);
+ r = ttm_eu_reserve_buffers(, , true, NULL, true);
if (r) {
DRM_ERROR("failed to reserve CSA,PD BOs: err=%d\n", r);
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 7b840367004c..d513a5ad03dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -171,7 +171,7 @@ void amdgpu_gem_object_close(struct drm_gem_object *obj,
amdgpu_vm_get_pd_bo(vm, , _pd);
- r = ttm_eu_reserve_buffers(, , false, );
+ r = ttm_eu_reserve_buffers(, , false, , true);
if (r) {
dev_err(adev->dev, "leaking bo va because "
"we fail to reserve bo (%d)\n", r);
@@ -608,7 +608,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
amdgpu_vm_get_pd_bo(>vm, , _pd);
- r = ttm_eu_reserve_buffers(, , true, );
+ r = ttm_eu_reserve_buffers(, , true, , true);
if (r)
goto error_unref;
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
[PATCH 04/10] drm/ttm: cleanup ttm_bo_mem_space
We tried this once before, but that turned out to be more
complicated than thought. With all the right prerequisites
it looks like we can do this now.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 127 ++-
1 file changed, 66 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index b29799aedb71..fd6dbebea430 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -892,13 +892,12 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object
*bo,
* space, or we've evicted everything and there isn't enough space.
*/
static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
- uint32_t mem_type,
- const struct ttm_place *place,
- struct ttm_mem_reg *mem,
- struct ttm_operation_ctx *ctx)
+ const struct ttm_place *place,
+ struct ttm_mem_reg *mem,
+ struct ttm_operation_ctx *ctx)
{
struct ttm_bo_device *bdev = bo->bdev;
- struct ttm_mem_type_manager *man = >man[mem_type];
+ struct ttm_mem_type_manager *man = >man[mem->mem_type];
int ret;
do {
@@ -907,11 +906,11 @@ static int ttm_bo_mem_force_space(struct
ttm_buffer_object *bo,
return ret;
if (mem->mm_node)
break;
- ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
+ ret = ttm_mem_evict_first(bdev, mem->mem_type, place, ctx);
if (unlikely(ret != 0))
return ret;
} while (1);
- mem->mem_type = mem_type;
+
return ttm_bo_add_move_fence(bo, man, mem);
}
@@ -959,6 +958,51 @@ static bool ttm_bo_mt_compatible(struct
ttm_mem_type_manager *man,
return true;
}
+/**
+ * ttm_bo_mem_placement - check if placement is compatible
+ * @bo: BO to find memory for
+ * @place: where to search
+ * @mem: the memory object to fill in
+ * @ctx: operation context
+ *
+ * Check if placement is compatible and fill in mem structure.
+ * Returns -EBUSY if placement won't work or negative error code.
+ * 0 when placement can be used.
+ */
+static int ttm_bo_mem_placement(struct ttm_buffer_object *bo,
+ const struct ttm_place *place,
+ struct ttm_mem_reg *mem,
+ struct ttm_operation_ctx *ctx)
+{
+ struct ttm_bo_device *bdev = bo->bdev;
+ uint32_t mem_type = TTM_PL_SYSTEM;
+ struct ttm_mem_type_manager *man;
+ uint32_t cur_flags = 0;
+ int ret;
+
+ ret = ttm_mem_type_from_place(place, _type);
+ if (ret)
+ return ret;
+
+ man = >man[mem_type];
+ if (!man->has_type || !man->use_type)
+ return -EBUSY;
+
+ if (!ttm_bo_mt_compatible(man, mem_type, place, _flags))
+ return -EBUSY;
+
+ cur_flags = ttm_bo_select_caching(man, bo->mem.placement, cur_flags);
+ /*
+* Use the access and other non-mapping-related flag bits from
+* the memory placement flags to the current flags
+*/
+ ttm_flag_masked(_flags, place->flags, ~TTM_PL_MASK_MEMTYPE);
+
+ mem->mem_type = mem_type;
+ mem->placement = cur_flags;
+ return 0;
+}
+
/**
* Creates space for memory region @mem according to its type.
*
@@ -973,11 +1017,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
struct ttm_operation_ctx *ctx)
{
struct ttm_bo_device *bdev = bo->bdev;
- struct ttm_mem_type_manager *man;
- uint32_t mem_type = TTM_PL_SYSTEM;
- uint32_t cur_flags = 0;
bool type_found = false;
- bool type_ok = false;
int i, ret;
ret = reservation_object_reserve_shared(bo->resv, 1);
@@ -987,37 +1027,20 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
mem->mm_node = NULL;
for (i = 0; i < placement->num_placement; ++i) {
const struct ttm_place *place = >placement[i];
+ struct ttm_mem_type_manager *man;
- ret = ttm_mem_type_from_place(place, _type);
+ ret = ttm_bo_mem_placement(bo, place, mem, ctx);
+ if (ret == -EBUSY)
+ continue;
if (ret)
return ret;
- man = >man[mem_type];
- if (!man->has_type || !man->use_type)
- continue;
-
- type_ok = ttm_bo_mt_compatible(man, mem_type, place,
- _flags);
-
- if (!type_ok)
- continue;
type_found = true;
- cur_flags = ttm_bo_select_caching(man, bo->mem.placement,
-
[PATCH 05/10] drm/ttm: immediately move BOs to the new LRU v2
Move BOs which are currently in a lower domain to the new
LRU before allocating backing space while validating.
This makes sure that we always have enough entries on the
LRU to allow for other processes to wait for an operation
to complete.
v2: generalize the test
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 45 ++--
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index fd6dbebea430..4c6389d849ed 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -166,17 +166,17 @@ static void ttm_bo_release_list(struct kref *list_kref)
ttm_mem_global_free(bdev->glob->mem_glob, acc_size);
}
-void ttm_bo_add_to_lru(struct ttm_buffer_object *bo)
+static void ttm_bo_add_mem_to_lru(struct ttm_buffer_object *bo,
+ struct ttm_mem_reg *mem)
{
struct ttm_bo_device *bdev = bo->bdev;
struct ttm_mem_type_manager *man;
reservation_object_assert_held(bo->resv);
+ BUG_ON(!list_empty(>lru));
- if (!(bo->mem.placement & TTM_PL_FLAG_NO_EVICT)) {
- BUG_ON(!list_empty(>lru));
-
- man = >man[bo->mem.mem_type];
+ if (!(mem->placement & TTM_PL_FLAG_NO_EVICT)) {
+ man = >man[mem->mem_type];
list_add_tail(>lru, >lru[bo->priority]);
kref_get(>list_kref);
@@ -188,6 +188,11 @@ void ttm_bo_add_to_lru(struct ttm_buffer_object *bo)
}
}
}
+
+void ttm_bo_add_to_lru(struct ttm_buffer_object *bo)
+{
+ ttm_bo_add_mem_to_lru(bo, >mem);
+}
EXPORT_SYMBOL(ttm_bo_add_to_lru);
static void ttm_bo_ref_bug(struct kref *list_kref)
@@ -1000,6 +1005,14 @@ static int ttm_bo_mem_placement(struct ttm_buffer_object
*bo,
mem->mem_type = mem_type;
mem->placement = cur_flags;
+
+ if (bo->mem.mem_type < mem_type && !list_empty(>lru)) {
+ spin_lock(>bdev->glob->lru_lock);
+ ttm_bo_del_from_lru(bo);
+ ttm_bo_add_mem_to_lru(bo, mem);
+ spin_unlock(>bdev->glob->lru_lock);
+ }
+
return 0;
}
@@ -1033,7 +1046,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
if (ret == -EBUSY)
continue;
if (ret)
- return ret;
+ goto error;
type_found = true;
mem->mm_node = NULL;
@@ -1043,13 +1056,13 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
man = >man[mem->mem_type];
ret = (*man->func->get_node)(man, bo, place, mem);
if (unlikely(ret))
- return ret;
+ goto error;
if (mem->mm_node) {
ret = ttm_bo_add_move_fence(bo, man, mem);
if (unlikely(ret)) {
(*man->func->put_node)(man, mem);
- return ret;
+ goto error;
}
return 0;
}
@@ -1062,7 +1075,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
if (ret == -EBUSY)
continue;
if (ret)
- return ret;
+ goto error;
type_found = true;
mem->mm_node = NULL;
@@ -1074,15 +1087,23 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
return 0;
if (ret && ret != -EBUSY)
- return ret;
+ goto error;
}
+ ret = -ENOMEM;
if (!type_found) {
pr_err(TTM_PFX "No compatible memory type found\n");
- return -EINVAL;
+ ret = -EINVAL;
}
- return -ENOMEM;
+error:
+ if (bo->mem.mem_type == TTM_PL_SYSTEM && !list_empty(>lru)) {
+ spin_lock(>bdev->glob->lru_lock);
+ ttm_bo_move_to_lru_tail(bo, NULL);
+ spin_unlock(>bdev->glob->lru_lock);
+ }
+
+ return ret;
}
EXPORT_SYMBOL(ttm_bo_mem_space);
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v15 09/17] fs, arm64: untag user pointers in copy_mount_options
On Mon, May 06, 2019 at 06:30:55PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > In copy_mount_options a user address is being subtracted from TASK_SIZE. > If the address is lower than TASK_SIZE, the size is calculated to not > allow the exact_copy_from_user() call to cross TASK_SIZE boundary. > However if the address is tagged, then the size will be calculated > incorrectly. > > Untag the address before subtracting. > > Signed-off-by: Andrey Konovalov Reviewed-by: Catalin Marinas
Re: [PATCH v15 07/17] mm, arm64: untag user pointers in mm/gup.c
On Mon, May 06, 2019 at 06:30:53PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > mm/gup.c provides a kernel interface that accepts user addresses and > manipulates user pages directly (for example get_user_pages, that is used > by the futex syscall). Since a user can provided tagged addresses, we need > to handle this case. > > Add untagging to gup.c functions that use user addresses for vma lookups. > > Signed-off-by: Andrey Konovalov Reviewed-by: Catalin Marinas
Re: [PATCH v15 06/17] mm: untag user pointers in do_pages_move
On Mon, May 06, 2019 at 06:30:52PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > do_pages_move() is used in the implementation of the move_pages syscall. > > Untag user pointers in this function. > > Signed-off-by: Andrey Konovalov Reviewed-by: Catalin Marinas
Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
On Mon, May 06, 2019 at 06:30:51PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch allows tagged pointers to be passed to the following memory
> syscalls: brk, get_mempolicy, madvise, mbind, mincore, mlock, mlock2,
> mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> remap_file_pages, shmat and shmdt.
>
> This is done by untagging pointers passed to these syscalls in the
> prologues of their handlers.
I'll go through them one by one to see if we can tighten the expected
ABI while having the MTE in mind.
> diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c
> index b44065fb1616..933bb9f3d6ec 100644
> --- a/arch/arm64/kernel/sys.c
> +++ b/arch/arm64/kernel/sys.c
> @@ -35,10 +35,33 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long,
> len,
> {
> if (offset_in_page(off) != 0)
> return -EINVAL;
> -
> + addr = untagged_addr(addr);
> return ksys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
> }
If user passes a tagged pointer to mmap() and the address is honoured
(or MAP_FIXED is given), what is the expected return pointer? Does it
need to be tagged with the value from the hint?
With MTE, we may want to use this as a request for the default colour of
the mapped pages (still under discussion).
> +SYSCALL_DEFINE6(arm64_mmap_pgoff, unsigned long, addr, unsigned long, len,
> + unsigned long, prot, unsigned long, flags,
> + unsigned long, fd, unsigned long, pgoff)
> +{
> + addr = untagged_addr(addr);
> + return ksys_mmap_pgoff(addr, len, prot, flags, fd, pgoff);
> +}
We don't have __NR_mmap_pgoff on arm64.
> +SYSCALL_DEFINE5(arm64_mremap, unsigned long, addr, unsigned long, old_len,
> + unsigned long, new_len, unsigned long, flags,
> + unsigned long, new_addr)
> +{
> + addr = untagged_addr(addr);
> + new_addr = untagged_addr(new_addr);
> + return ksys_mremap(addr, old_len, new_len, flags, new_addr);
> +}
Similar comment as for mmap(), do we want the tag from new_addr to be
preserved? In addition, should we check that the two tags are identical
or mremap() should become a way to repaint a memory region?
> +SYSCALL_DEFINE2(arm64_munmap, unsigned long, addr, size_t, len)
> +{
> + addr = untagged_addr(addr);
> + return ksys_munmap(addr, len);
> +}
This looks fine.
> +SYSCALL_DEFINE1(arm64_brk, unsigned long, brk)
> +{
> + brk = untagged_addr(brk);
> + return ksys_brk(brk);
> +}
I wonder whether brk() should simply not accept tags, and should not
return them (similar to the prctl(PR_SET_MM) discussion). We could
document this in the ABI requirements.
> +SYSCALL_DEFINE5(arm64_get_mempolicy, int __user *, policy,
> + unsigned long __user *, nmask, unsigned long, maxnode,
> + unsigned long, addr, unsigned long, flags)
> +{
> + addr = untagged_addr(addr);
> + return ksys_get_mempolicy(policy, nmask, maxnode, addr, flags);
> +}
> +
> +SYSCALL_DEFINE3(arm64_madvise, unsigned long, start,
> + size_t, len_in, int, behavior)
> +{
> + start = untagged_addr(start);
> + return ksys_madvise(start, len_in, behavior);
> +}
> +
> +SYSCALL_DEFINE6(arm64_mbind, unsigned long, start, unsigned long, len,
> + unsigned long, mode, const unsigned long __user *, nmask,
> + unsigned long, maxnode, unsigned int, flags)
> +{
> + start = untagged_addr(start);
> + return ksys_mbind(start, len, mode, nmask, maxnode, flags);
> +}
> +
> +SYSCALL_DEFINE2(arm64_mlock, unsigned long, start, size_t, len)
> +{
> + start = untagged_addr(start);
> + return ksys_mlock(start, len, VM_LOCKED);
> +}
> +
> +SYSCALL_DEFINE2(arm64_mlock2, unsigned long, start, size_t, len)
> +{
> + start = untagged_addr(start);
> + return ksys_mlock(start, len, VM_LOCKED);
> +}
> +
> +SYSCALL_DEFINE2(arm64_munlock, unsigned long, start, size_t, len)
> +{
> + start = untagged_addr(start);
> + return ksys_munlock(start, len);
> +}
> +
> +SYSCALL_DEFINE3(arm64_mprotect, unsigned long, start, size_t, len,
> + unsigned long, prot)
> +{
> + start = untagged_addr(start);
> + return ksys_mprotect_pkey(start, len, prot, -1);
> +}
> +
> +SYSCALL_DEFINE3(arm64_msync, unsigned long, start, size_t, len, int, flags)
> +{
> + start = untagged_addr(start);
> + return ksys_msync(start, len, flags);
> +}
> +
> +SYSCALL_DEFINE3(arm64_mincore, unsigned long, start, size_t, len,
> + unsigned char __user *, vec)
> +{
> + start = untagged_addr(start);
> + return ksys_mincore(start, len, vec);
> +}
These look fine.
> +SYSCALL_DEFINE5(arm64_remap_file_pages, unsigned long, start,
> + unsigned long, size, unsigned long, prot,
> + unsigned long, pgoff, unsigned long, flags)
>
Re: [PATCH v15 04/17] mm: add ksys_ wrappers to memory syscalls
On Mon, May 06, 2019 at 06:30:50PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > This patch adds ksys_ wrappers to the following memory syscalls: > > brk, get_mempolicy (renamed kernel_get_mempolicy -> ksys_get_mempolicy), > madvise, mbind (renamed kernel_mbind -> ksys_mbind), mincore, > mlock (renamed do_mlock -> ksys_mlock), mlock2, mmap_pgoff, > mprotect (renamed do_mprotect_pkey -> ksys_mprotect_pkey), mremap, msync, > munlock, munmap, remap_file_pages, shmat, shmdt. > > The next patch in this series will add a custom implementation for these > syscalls that makes them accept tagged pointers on arm64. > > Signed-off-by: Andrey Konovalov Reviewed-by: Catalin Marinas
Re: [PATCH v15 03/17] lib, arm64: untag user pointers in strn*_user
On Mon, May 06, 2019 at 06:30:49PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > strncpy_from_user and strnlen_user accept user addresses as arguments, and > do not go through the same path as copy_from_user and others, so here we > need to handle the case of tagged user addresses separately. > > Untag user pointers passed to these functions. > > Note, that this patch only temporarily untags the pointers to perform > validity checks, but then uses them as is to perform user memory accesses. > > Signed-off-by: Andrey Konovalov Just to keep track of where I am with the reviews while the ABI discussion continues: Reviewed-by: Catalin Marinas
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
Hi Kees, Thanks for joining the thread ;). On Tue, May 21, 2019 at 05:04:39PM -0700, Kees Cook wrote: > On Tue, May 21, 2019 at 07:29:33PM +0100, Catalin Marinas wrote: > > On Mon, May 20, 2019 at 04:53:07PM -0700, Evgenii Stepanov wrote: > > > On Fri, May 17, 2019 at 7:49 AM Catalin Marinas > > > wrote: > > > > IMO (RFC for now), I see two ways forward: > > > > [...] > > > > 2. Similar shim to the above libc wrapper but inside the kernel > > > >(arch/arm64 only; most pointer arguments could be covered with an > > > >__SC_CAST similar to the s390 one). There are two differences from > > > >what we've discussed in the past: > > > > > > > >a) this is an opt-in by the user which would have to explicitly call > > > > prctl(). If it returns -ENOTSUPP etc., the user won't be allowed > > > > to pass tagged pointers to the kernel. This would probably be the > > > > responsibility of the C lib to make sure it doesn't tag heap > > > > allocations. If the user did not opt-in, the syscalls are routed > > > > through the normal path (no untagging address shim). > > > > > > > >b) ioctl() and other blacklisted syscalls (prctl) will not accept > > > > tagged pointers (to be documented in Vicenzo's ABI patches). > > > > > > The way I see it, a patch that breaks handling of tagged pointers is > > > not that different from, say, a patch that adds a wild pointer > > > dereference. Both are bugs; the difference is that (a) the former > > > breaks a relatively uncommon target and (b) it's arguably an easier > > > mistake to make. If MTE adoption goes well, (a) will not be the case > > > for long. > > > > It's also the fact such patch would go unnoticed for a long time until > > someone exercises that code path. And when they do, the user would be > > pretty much in the dark trying to figure what what went wrong, why a > > SIGSEGV or -EFAULT happened. What's worse, we can't even say we fixed > > all the places where it matters in the current kernel codebase (ignoring > > future patches). > > So, looking forward a bit, this isn't going to be an ARM-specific issue > for long. I do hope so. > In fact, I think we shouldn't have arm-specific syscall wrappers > in this series: I think untagged_addr() should likely be added at the > top-level and have it be a no-op for other architectures. That's what the current patchset does, so we have this as a starting point. Kostya raised another potential issue with the syscall wrappers: with MTE the kernel will be forced to enable the match-all (wildcard) pointers for user space accesses since copy_from_user() would only get a 0 tag. So it has wider implications than just uaccess routines not checking the colour. > So given this becoming a kernel-wide multi-architecture issue (under > the assumption that x86, RISC-V, and others will gain similar TBI or > MTE things), we should solve it in a way that we can re-use. Can we do any better to aid the untagged_addr() placement (e.g. better type annotations, better static analysis)? We have to distinguish between user pointers that may be dereferenced by the kernel (I think almost fully covered with this patchset) and user addresses represented as ulong that may: a) be converted to a user pointer and dereferenced; I think that's the case for many overloaded ulong/u64 arguments b) used for address space management, rbtree look-ups etc. where the tag is no longer relevant and it even gets in the way We tried last year to identify void __user * casts to unsigned long using sparse on the assumption that pointers can be tagged while ulong is about address space management and needs to lose such tag. I think we could have pushed this further. For example, get_user_pages() takes an unsigned long but it is perfectly capable of untagging the address itself. Shall we change its first argument to void __user * (together with all its callers)? find_vma(), OTOH, could untag the address but it doesn't help since vm_start/end don't have such information (that's more about the content or type that the user decided) and the callers check against it. Are there any other places where this matters? These patches tracked down find_vma() as some heuristics but we may need better static analysis to identify other cases. > We need something that is going to work everywhere. And it needs to be > supported by the kernel for the simple reason that the kernel needs to > do MTE checks during copy_from_user(): having that information stripped > means we lose any userspace-assigned MTE protections if they get handled > by the kernel, which is a total non-starter, IMO. Such feedback is welcomed ;). > As an aside: I think Sparc ADI support in Linux actually side-stepped > this[1] (i.e. chose "solution 1"): "All addresses passed to kernel must > be non-ADI tagged addresses." (And sadly, "Kernel does not enable ADI > for kernel code.") I think this was a mistake we should not repeat for > arm64 (we do
[PATCH] drm/amdgpu: Need to set the baco cap before baco reset
For passthrough, after rebooted the VM, driver will do
a baco reset before doing other driver initialization during loading
driver. For doing the baco reset, it will first
check the baco reset capability. So first need to set the
cap from the vbios information or baco reset won't be
enabled.
Signed-off-by: Emily Deng
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8
drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 16
drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 +
.../amd/powerplay/hwmgr/vega10_processpptables.c | 22 ++
.../amd/powerplay/hwmgr/vega10_processpptables.h | 1 +
drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 1 +
7 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index d6286ed..14415b3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2606,6 +2606,14 @@ int amdgpu_device_init(struct amdgpu_device *adev,
* E.g., driver was not cleanly unloaded previously, etc.
*/
if (!amdgpu_sriov_vf(adev) && amdgpu_asic_need_reset_on_init(adev)) {
+ if (adev->powerplay.pp_funcs &&
adev->powerplay.pp_funcs->set_asic_baco_cap) {
+ r =
adev->powerplay.pp_funcs->set_asic_baco_cap(adev->powerplay.pp_handle);
+ if (r) {
+ dev_err(adev->dev, "set baco capability
failed\n");
+ goto failed;
+ }
+ }
+
r = amdgpu_asic_reset(adev);
if (r) {
dev_err(adev->dev, "asic reset on init failed\n");
diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
index 2b579ba..0dcc18d 100644
--- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
@@ -285,6 +285,7 @@ struct amd_pm_funcs {
int (*set_hard_min_fclk_by_freq)(void *handle, uint32_t clock);
int (*set_min_deep_sleep_dcefclk)(void *handle, uint32_t clock);
int (*get_asic_baco_capability)(void *handle, bool *cap);
+ int (*set_asic_baco_cap)(void *handle);
int (*get_asic_baco_state)(void *handle, int *state);
int (*set_asic_baco_state)(void *handle, int state);
int (*get_ppfeature_status)(void *handle, char *buf);
diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
index bea1587..9856760 100644
--- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
+++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
@@ -1404,6 +1404,21 @@ static int pp_set_active_display_count(void *handle,
uint32_t count)
return ret;
}
+static int pp_set_asic_baco_cap(void *handle)
+{
+ struct pp_hwmgr *hwmgr = handle;
+
+ if (!hwmgr)
+ return -EINVAL;
+
+ if (!hwmgr->pm_en || !hwmgr->hwmgr_func->set_asic_baco_cap)
+ return 0;
+
+ hwmgr->hwmgr_func->set_asic_baco_cap(hwmgr);
+
+ return 0;
+}
+
static int pp_get_asic_baco_capability(void *handle, bool *cap)
{
struct pp_hwmgr *hwmgr = handle;
@@ -1546,6 +1561,7 @@ static const struct amd_pm_funcs pp_dpm_funcs = {
.set_hard_min_dcefclk_by_freq = pp_set_hard_min_dcefclk_by_freq,
.set_hard_min_fclk_by_freq = pp_set_hard_min_fclk_by_freq,
.get_asic_baco_capability = pp_get_asic_baco_capability,
+ .set_asic_baco_cap = pp_set_asic_baco_cap,
.get_asic_baco_state = pp_get_asic_baco_state,
.set_asic_baco_state = pp_set_asic_baco_state,
.get_ppfeature_status = pp_get_ppfeature_status,
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
index ed6c638..8dc23eb 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
@@ -5171,6 +5171,7 @@ static const struct pp_hwmgr_func vega10_hwmgr_funcs = {
.odn_edit_dpm_table = vega10_odn_edit_dpm_table,
.get_performance_level = vega10_get_performance_level,
.get_asic_baco_capability = smu9_baco_get_capability,
+ .set_asic_baco_cap = vega10_baco_set_cap,
.get_asic_baco_state = smu9_baco_get_state,
.set_asic_baco_state = vega10_baco_set_state,
.enable_mgpu_fan_boost = vega10_enable_mgpu_fan_boost,
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
index b6767d7..8fdeb23 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_processpptables.c
@@ -1371,3 +1371,25 @@ int vega10_get_powerplay_table_entry(struct pp_hwmgr
*hwmgr,
return
