Re: [PATCH 07/14] drm/amdgpu/si,cik,vi: Verify IP block when querying video codecs

["Timur Kristóf" <timur.kristof@gmail.com>]

On Wed, 2025-10-29 at 11:35 +0100, Christian König wrote:
> 
> 
> On 10/28/25 23:06, Timur Kristóf wrote:
> > Some harvested chips may not have any IP blocks,
> > or we may not have the firmware for the IP blocks.
> > In these cases, the query should return that no video
> > codec is supported.
> > 
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++-
> >  drivers/gpu/drm/amd/amdgpu/cik.c        | 6 ++++++
> >  drivers/gpu/drm/amd/amdgpu/si.c         | 6 ++++++
> >  drivers/gpu/drm/amd/amdgpu/vi.c         | 6 ++++++
> >  4 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index b3e6b3fcdf2c..42b5da59d00f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -1263,7 +1263,8 @@ int amdgpu_info_ioctl(struct drm_device *dev,
> > void *data, struct drm_file *filp)
> >  			-EFAULT : 0;
> >  	}
> >  	case AMDGPU_INFO_VIDEO_CAPS: {
> > -		const struct amdgpu_video_codecs *codecs;
> > +		static const struct amdgpu_video_codecs no_codecs
> > = {0};
> 
> No zero init for static variables please, that will raise you a
> constant checker warning.
> 
> > +		const struct amdgpu_video_codecs *codecs =
> > &no_codecs;
> >  		struct drm_amdgpu_info_video_caps *caps;
> >  		int r;
> >  
> > diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
> > b/drivers/gpu/drm/amd/amdgpu/cik.c
> > index 9cd63b4177bf..b755238c2c3d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> > @@ -130,6 +130,12 @@ static const struct amdgpu_video_codecs
> > cik_video_codecs_decode =
> >  static int cik_query_video_codecs(struct amdgpu_device *adev, bool
> > encode,
> >  				  const struct amdgpu_video_codecs
> > **codecs)
> >  {
> > +	const enum amd_ip_block_type ip =
> > +		encode ? AMD_IP_BLOCK_TYPE_VCE :
> > AMD_IP_BLOCK_TYPE_UVD;
> > +
> > +	if (!amdgpu_device_ip_is_valid(adev, ip))
> > +		return 0;
> 
> I'm wondering if returning EOPNOTSUPP is not more appropriate here
> than returning an empty cappability list.

I don't think so.

Returning EOPNOTSUPP would indicate that the operation of querying the
codec support is not supported, and not that the list of supported
codecs is empty.

> 
> Anyway setting the codecs list to empty in the caller is rather bad
> coding style.

Sure, I'll come up with a better way to do this.

> 
> Regards,
> Christian.
> 
> > +
> >  	switch (adev->asic_type) {
> >  	case CHIP_BONAIRE:
> >  	case CHIP_HAWAII:
> > diff --git a/drivers/gpu/drm/amd/amdgpu/si.c
> > b/drivers/gpu/drm/amd/amdgpu/si.c
> > index e0f139de7991..9468c03bdb1b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/si.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/si.c
> > @@ -1003,6 +1003,12 @@ static const struct amdgpu_video_codecs
> > hainan_video_codecs_decode =
> >  static int si_query_video_codecs(struct amdgpu_device *adev, bool
> > encode,
> >  				 const struct amdgpu_video_codecs
> > **codecs)
> >  {
> > +	const enum amd_ip_block_type ip =
> > +		encode ? AMD_IP_BLOCK_TYPE_VCE :
> > AMD_IP_BLOCK_TYPE_UVD;
> > +
> > +	if (!amdgpu_device_ip_is_valid(adev, ip))
> > +		return 0;
> > +
> >  	switch (adev->asic_type) {
> >  	case CHIP_VERDE:
> >  	case CHIP_TAHITI:
> > diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> > b/drivers/gpu/drm/amd/amdgpu/vi.c
> > index a611a7345125..f0e4193cf722 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> > @@ -256,6 +256,12 @@ static const struct amdgpu_video_codecs
> > cz_video_codecs_decode =
> >  static int vi_query_video_codecs(struct amdgpu_device *adev, bool
> > encode,
> >  				 const struct amdgpu_video_codecs
> > **codecs)
> >  {
> > +	const enum amd_ip_block_type ip =
> > +		encode ? AMD_IP_BLOCK_TYPE_VCE :
> > AMD_IP_BLOCK_TYPE_UVD;
> > +
> > +	if (!amdgpu_device_ip_is_valid(adev, ip))
> > +		return 0;
> > +
> >  	switch (adev->asic_type) {
> >  	case CHIP_TOPAZ:
> >  		if (encode)