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

[Christian König]

Am 23.06.2017 um 19:05 schrieb Alex Deucher:
> On Fri, Jun 23, 2017 at 12:43 PM, Christian König
> <deathsimple at vodafone.de> wrote:
>> Am 23.06.2017 um 18:34 schrieb Alex Deucher:
>>> From: Vijendar Mukunda <Vijendar.Mukunda at amd.com>
>>>
>>> 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 <alexander.deucher at amd.com>
>>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda at amd.com>
>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>> ---
>>>    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,
>>
>>