[1/8] drm/amd/amdgpu: Added asic_type as ACP DMA driver platform data

Commit Message

Alex Deucher June 23, 2017, 4:34 p.m. UTC

From: Vijendar Mukunda <[e-mail]>

asic_type information is passed to ACP DMA Driver as platform data.
We need this to determine whether the asic is Carrizo (CZ) or
Stoney (ST) in the acp sound driver.

Reviewed-by: Alex Deucher <[e-mail]>
Signed-off-by: Vijendar Mukunda <[e-mail]>
Signed-off-by: Alex Deucher <[e-mail]>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 4 ++++
 sound/soc/amd/acp-pcm-dma.c             | 8 ++------
 sound/soc/amd/acp.h                     | 7 +++++++
 3 files changed, 13 insertions(+), 6 deletions(-)

Comments

Christian König June 23, 2017, 4:43 p.m. UTC | #1

Am 23.06.2017 um 18:34 schrieb Alex Deucher:
> From: Vijendar Mukunda <[e-mail]>
>
> asic_type information is passed to ACP DMA Driver as platform data.
> We need this to determine whether the asic is Carrizo (CZ) or
> Stoney (ST) in the acp sound driver.
>
> Reviewed-by: Alex Deucher <[e-mail]>
> Signed-off-by: Vijendar Mukunda <[e-mail]>
> Signed-off-by: Alex Deucher <[e-mail]>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 4 ++++
>   sound/soc/amd/acp-pcm-dma.c             | 8 ++------
>   sound/soc/amd/acp.h                     | 7 +++++++
>   3 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> index 06879d1..7a2a765 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> @@ -262,6 +262,7 @@ static int acp_hw_init(void *handle)
>   	uint64_t acp_base;
>   	struct device *dev;
>   	struct i2s_platform_data *i2s_pdata;
> +	u32 asic_type;
>   
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> @@ -271,6 +272,7 @@ static int acp_hw_init(void *handle)
>   	if (!ip_block)
>   		return -EINVAL;
>   
> +	asic_type = adev->asic_type;
>   	r = amd_acp_hw_init(adev->acp.cgs_device,
>   			    ip_block->version->major, ip_block->version->minor);
>   	/* -ENODEV means board uses AZ rather than ACP */
> @@ -355,6 +357,8 @@ static int acp_hw_init(void *handle)
>   	adev->acp.acp_cell[0].name = "acp_audio_dma";
>   	adev->acp.acp_cell[0].num_resources = 4;
>   	adev->acp.acp_cell[0].resources = &adev->acp.acp_res[0];
> +	adev->acp.acp_cell[0].platform_data = &asic_type;
> +	adev->acp.acp_cell[0].pdata_size = sizeof(asic_type);

Have the painkillers jellyfied my brain or are you setting a pointer in 
the amdgpu device structure to some variable on the stack?

If it's just for initialization then that's probably ok, but I would 
reset the pointer at the end of the function.

Otherwise somebody might have the idea to dereference it later on and 
that can only lead to trouble.

Christian.

>   
>   	adev->acp.acp_cell[1].name = "designware-i2s";
>   	adev->acp.acp_cell[1].num_resources = 1;
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index 08b1399..dcbf997 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -73,12 +73,6 @@ static const struct snd_pcm_hardware acp_pcm_hardware_capture = {
>   	.periods_max = CAPTURE_MAX_NUM_PERIODS,
>   };
>   
> -struct audio_drv_data {
> -	struct snd_pcm_substream *play_stream;
> -	struct snd_pcm_substream *capture_stream;
> -	void __iomem *acp_mmio;
> -};
> -
>   static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
>   {
>   	return readl(acp_mmio + (reg * 4));
> @@ -916,6 +910,7 @@ static int acp_audio_probe(struct platform_device *pdev)
>   	int status;
>   	struct audio_drv_data *audio_drv_data;
>   	struct resource *res;
> +	const u32 *pdata = pdev->dev.platform_data;
>   
>   	audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct audio_drv_data),
>   					GFP_KERNEL);
> @@ -932,6 +927,7 @@ static int acp_audio_probe(struct platform_device *pdev)
>   
>   	audio_drv_data->play_stream = NULL;
>   	audio_drv_data->capture_stream = NULL;
> +	audio_drv_data->asic_type =  *pdata;
>   
>   	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>   	if (!res) {
> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
> index 330832e..28cf914 100644
> --- a/sound/soc/amd/acp.h
> +++ b/sound/soc/amd/acp.h
> @@ -84,6 +84,13 @@ struct audio_substream_data {
>   	void __iomem *acp_mmio;
>   };
>   
> +struct audio_drv_data {
> +	struct snd_pcm_substream *play_stream;
> +	struct snd_pcm_substream *capture_stream;
> +	void __iomem *acp_mmio;
> +	u32 asic_type;
> +};
> +
>   enum {
>   	ACP_TILE_P1 = 0,
>   	ACP_TILE_P2,

Alex Deucher June 23, 2017, 5:05 p.m. UTC | #2

On Fri, Jun 23, 2017 at 12:43 PM, Christian König
<[e-mail]> wrote:
> Am 23.06.2017 um 18:34 schrieb Alex Deucher:
>>
>> From: Vijendar Mukunda <[e-mail]>
>>
>> asic_type information is passed to ACP DMA Driver as platform data.
>> We need this to determine whether the asic is Carrizo (CZ) or
>> Stoney (ST) in the acp sound driver.
>>
>> Reviewed-by: Alex Deucher <[e-mail]>
>> Signed-off-by: Vijendar Mukunda <[e-mail]>
>> Signed-off-by: Alex Deucher <[e-mail]>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 4 ++++
>>   sound/soc/amd/acp-pcm-dma.c             | 8 ++------
>>   sound/soc/amd/acp.h                     | 7 +++++++
>>   3 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> index 06879d1..7a2a765 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> @@ -262,6 +262,7 @@ static int acp_hw_init(void *handle)
>>         uint64_t acp_base;
>>         struct device *dev;
>>         struct i2s_platform_data *i2s_pdata;
>> +       u32 asic_type;
>>         struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>   @@ -271,6 +272,7 @@ static int acp_hw_init(void *handle)
>>         if (!ip_block)
>>                 return -EINVAL;
>>   +     asic_type = adev->asic_type;
>>         r = amd_acp_hw_init(adev->acp.cgs_device,
>>                             ip_block->version->major,
>> ip_block->version->minor);
>>         /* -ENODEV means board uses AZ rather than ACP */
>> @@ -355,6 +357,8 @@ static int acp_hw_init(void *handle)
>>         adev->acp.acp_cell[0].name = "acp_audio_dma";
>>         adev->acp.acp_cell[0].num_resources = 4;
>>         adev->acp.acp_cell[0].resources = &adev->acp.acp_res[0];
>> +       adev->acp.acp_cell[0].platform_data = &asic_type;
>> +       adev->acp.acp_cell[0].pdata_size = sizeof(asic_type);
>
>
> Have the painkillers jellyfied my brain or are you setting a pointer in the
> amdgpu device structure to some variable on the stack?
>
> If it's just for initialization then that's probably ok, but I would reset
> the pointer at the end of the function.
>
> Otherwise somebody might have the idea to dereference it later on and that
> can only lead to trouble.

I think it's ok because the hotplug of the audio device is triggered
from this function, so the audio device should be probed by the time
this variable goes out of scope.  That said, it would probably be
better to use the asic_type from the GPU driver instance directly
rather than a stack variable.

Alex


>
> Christian.
>
>
>>         adev->acp.acp_cell[1].name = "designware-i2s";
>>         adev->acp.acp_cell[1].num_resources = 1;
>> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
>> index 08b1399..dcbf997 100644
>> --- a/sound/soc/amd/acp-pcm-dma.c
>> +++ b/sound/soc/amd/acp-pcm-dma.c
>> @@ -73,12 +73,6 @@ static const struct snd_pcm_hardware
>> acp_pcm_hardware_capture = {
>>         .periods_max = CAPTURE_MAX_NUM_PERIODS,
>>   };
>>   -struct audio_drv_data {
>> -       struct snd_pcm_substream *play_stream;
>> -       struct snd_pcm_substream *capture_stream;
>> -       void __iomem *acp_mmio;
>> -};
>> -
>>   static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
>>   {
>>         return readl(acp_mmio + (reg * 4));
>> @@ -916,6 +910,7 @@ static int acp_audio_probe(struct platform_device
>> *pdev)
>>         int status;
>>         struct audio_drv_data *audio_drv_data;
>>         struct resource *res;
>> +       const u32 *pdata = pdev->dev.platform_data;
>>         audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct
>> audio_drv_data),
>>                                         GFP_KERNEL);
>> @@ -932,6 +927,7 @@ static int acp_audio_probe(struct platform_device
>> *pdev)
>>         audio_drv_data->play_stream = NULL;
>>         audio_drv_data->capture_stream = NULL;
>> +       audio_drv_data->asic_type =  *pdata;
>>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>         if (!res) {
>> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
>> index 330832e..28cf914 100644
>> --- a/sound/soc/amd/acp.h
>> +++ b/sound/soc/amd/acp.h
>> @@ -84,6 +84,13 @@ struct audio_substream_data {
>>         void __iomem *acp_mmio;
>>   };
>>   +struct audio_drv_data {
>> +       struct snd_pcm_substream *play_stream;
>> +       struct snd_pcm_substream *capture_stream;
>> +       void __iomem *acp_mmio;
>> +       u32 asic_type;
>> +};
>> +
>>   enum {
>>         ACP_TILE_P1 = 0,
>>         ACP_TILE_P2,
>
>
>

Christian König June 26, 2017, 1:29 p.m. UTC | #3

Am 23.06.2017 um 19:05 schrieb Alex Deucher:
> On Fri, Jun 23, 2017 at 12:43 PM, Christian König
> <[e-mail]> wrote:
>> Am 23.06.2017 um 18:34 schrieb Alex Deucher:
>>> From: Vijendar Mukunda <[e-mail]>
>>>
>>> asic_type information is passed to ACP DMA Driver as platform data.
>>> We need this to determine whether the asic is Carrizo (CZ) or
>>> Stoney (ST) in the acp sound driver.
>>>
>>> Reviewed-by: Alex Deucher <[e-mail]>
>>> Signed-off-by: Vijendar Mukunda <[e-mail]>
>>> Signed-off-by: Alex Deucher <[e-mail]>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 4 ++++
>>>    sound/soc/amd/acp-pcm-dma.c             | 8 ++------
>>>    sound/soc/amd/acp.h                     | 7 +++++++
>>>    3 files changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> index 06879d1..7a2a765 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> @@ -262,6 +262,7 @@ static int acp_hw_init(void *handle)
>>>          uint64_t acp_base;
>>>          struct device *dev;
>>>          struct i2s_platform_data *i2s_pdata;
>>> +       u32 asic_type;
>>>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>    @@ -271,6 +272,7 @@ static int acp_hw_init(void *handle)
>>>          if (!ip_block)
>>>                  return -EINVAL;
>>>    +     asic_type = adev->asic_type;
>>>          r = amd_acp_hw_init(adev->acp.cgs_device,
>>>                              ip_block->version->major,
>>> ip_block->version->minor);
>>>          /* -ENODEV means board uses AZ rather than ACP */
>>> @@ -355,6 +357,8 @@ static int acp_hw_init(void *handle)
>>>          adev->acp.acp_cell[0].name = "acp_audio_dma";
>>>          adev->acp.acp_cell[0].num_resources = 4;
>>>          adev->acp.acp_cell[0].resources = &adev->acp.acp_res[0];
>>> +       adev->acp.acp_cell[0].platform_data = &asic_type;
>>> +       adev->acp.acp_cell[0].pdata_size = sizeof(asic_type);
>>
>> Have the painkillers jellyfied my brain or are you setting a pointer in the
>> amdgpu device structure to some variable on the stack?
>>
>> If it's just for initialization then that's probably ok, but I would reset
>> the pointer at the end of the function.
>>
>> Otherwise somebody might have the idea to dereference it later on and that
>> can only lead to trouble.
> I think it's ok because the hotplug of the audio device is triggered
> from this function, so the audio device should be probed by the time
> this variable goes out of scope.  That said, it would probably be
> better to use the asic_type from the GPU driver instance directly
> rather than a stack variable.

Yeah, agree. But keeping a stale reference in to a stack variable in the 
driver struct is still ugly like hell.

At bare minimum set this to NULL at the end of the function, or even 
better use a intptr_t to encode the asic type into the pointer.

Christian.

>
> Alex
>
>
>> Christian.
>>
>>
>>>          adev->acp.acp_cell[1].name = "designware-i2s";
>>>          adev->acp.acp_cell[1].num_resources = 1;
>>> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
>>> index 08b1399..dcbf997 100644
>>> --- a/sound/soc/amd/acp-pcm-dma.c
>>> +++ b/sound/soc/amd/acp-pcm-dma.c
>>> @@ -73,12 +73,6 @@ static const struct snd_pcm_hardware
>>> acp_pcm_hardware_capture = {
>>>          .periods_max = CAPTURE_MAX_NUM_PERIODS,
>>>    };
>>>    -struct audio_drv_data {
>>> -       struct snd_pcm_substream *play_stream;
>>> -       struct snd_pcm_substream *capture_stream;
>>> -       void __iomem *acp_mmio;
>>> -};
>>> -
>>>    static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
>>>    {
>>>          return readl(acp_mmio + (reg * 4));
>>> @@ -916,6 +910,7 @@ static int acp_audio_probe(struct platform_device
>>> *pdev)
>>>          int status;
>>>          struct audio_drv_data *audio_drv_data;
>>>          struct resource *res;
>>> +       const u32 *pdata = pdev->dev.platform_data;
>>>          audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct
>>> audio_drv_data),
>>>                                          GFP_KERNEL);
>>> @@ -932,6 +927,7 @@ static int acp_audio_probe(struct platform_device
>>> *pdev)
>>>          audio_drv_data->play_stream = NULL;
>>>          audio_drv_data->capture_stream = NULL;
>>> +       audio_drv_data->asic_type =  *pdata;
>>>          res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>          if (!res) {
>>> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
>>> index 330832e..28cf914 100644
>>> --- a/sound/soc/amd/acp.h
>>> +++ b/sound/soc/amd/acp.h
>>> @@ -84,6 +84,13 @@ struct audio_substream_data {
>>>          void __iomem *acp_mmio;
>>>    };
>>>    +struct audio_drv_data {
>>> +       struct snd_pcm_substream *play_stream;
>>> +       struct snd_pcm_substream *capture_stream;
>>> +       void __iomem *acp_mmio;
>>> +       u32 asic_type;
>>> +};
>>> +
>>>    enum {
>>>          ACP_TILE_P1 = 0,
>>>          ACP_TILE_P2,
>>
>>

Mark Brown June 28, 2017, 5:54 p.m. UTC | #4

On Fri, Jun 23, 2017 at 01:05:37PM -0400, Alex Deucher wrote:
> On Fri, Jun 23, 2017 at 12:43 PM, Christian König

> > Have the painkillers jellyfied my brain or are you setting a pointer in the
> > amdgpu device structure to some variable on the stack?

> > If it's just for initialization then that's probably ok, but I would reset
> > the pointer at the end of the function.

> > Otherwise somebody might have the idea to dereference it later on and that
> > can only lead to trouble.

> I think it's ok because the hotplug of the audio device is triggered
> from this function, so the audio device should be probed by the time
> this variable goes out of scope.  That said, it would probably be

No, that's in no way safe.  There is no guarantee that probe is going to
happen on any particular schedule, if the other driver isn't registered
yet it'll need to wait for that to happen for example.  You will often
get away with it but that's at best going to be vulnerable to framework
changes (what if someone decides to kick off the probe on a separate
CPU in an effort to reduce boot time?).

> better to use the asic_type from the GPU driver instance directly
> rather than a stack variable.

Yes.

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index 06879d1..7a2a765 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -262,6 +262,7 @@  static int acp_hw_init(void *handle)
 	uint64_t acp_base;
 	struct device *dev;
 	struct i2s_platform_data *i2s_pdata;
+	u32 asic_type;
 
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
@@ -271,6 +272,7 @@  static int acp_hw_init(void *handle)
 	if (!ip_block)
 		return -EINVAL;
 
+	asic_type = adev->asic_type;
 	r = amd_acp_hw_init(adev->acp.cgs_device,
 			    ip_block->version->major, ip_block->version->minor);
 	/* -ENODEV means board uses AZ rather than ACP */
@@ -355,6 +357,8 @@  static int acp_hw_init(void *handle)
 	adev->acp.acp_cell[0].name = "acp_audio_dma";
 	adev->acp.acp_cell[0].num_resources = 4;
 	adev->acp.acp_cell[0].resources = &adev->acp.acp_res[0];
+	adev->acp.acp_cell[0].platform_data = &asic_type;
+	adev->acp.acp_cell[0].pdata_size = sizeof(asic_type);
 
 	adev->acp.acp_cell[1].name = "designware-i2s";
 	adev->acp.acp_cell[1].num_resources = 1;
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 08b1399..dcbf997 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -73,12 +73,6 @@  static const struct snd_pcm_hardware acp_pcm_hardware_capture = {
 	.periods_max = CAPTURE_MAX_NUM_PERIODS,
 };
 
-struct audio_drv_data {
-	struct snd_pcm_substream *play_stream;
-	struct snd_pcm_substream *capture_stream;
-	void __iomem *acp_mmio;
-};
-
 static u32 acp_reg_read(void __iomem *acp_mmio, u32 reg)
 {
 	return readl(acp_mmio + (reg * 4));
@@ -916,6 +910,7 @@  static int acp_audio_probe(struct platform_device *pdev)
 	int status;
 	struct audio_drv_data *audio_drv_data;
 	struct resource *res;
+	const u32 *pdata = pdev->dev.platform_data;
 
 	audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct audio_drv_data),
 					GFP_KERNEL);
@@ -932,6 +927,7 @@  static int acp_audio_probe(struct platform_device *pdev)
 
 	audio_drv_data->play_stream = NULL;
 	audio_drv_data->capture_stream = NULL;
+	audio_drv_data->asic_type =  *pdata;
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!res) {
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 330832e..28cf914 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -84,6 +84,13 @@  struct audio_substream_data {
 	void __iomem *acp_mmio;
 };
 
+struct audio_drv_data {
+	struct snd_pcm_substream *play_stream;
+	struct snd_pcm_substream *capture_stream;
+	void __iomem *acp_mmio;
+	u32 asic_type;
+};
+
 enum {
 	ACP_TILE_P1 = 0,
 	ACP_TILE_P2,