Re: [PATCH] drm/amd/display: Align num_crtc to max_streams
[AMD Official Use Only - General] Thank you very much Harry and Hamza. You are right, adev->mode_info.num_crtc has already been updated, the variables that really need updating are adev->mode_info.num_crtc, adev->vline0_irq.num_types, adev->vupdate_irq.num_types and adev->pageflip_irq.num_types. I have made a new patch with title "drm/amd/display: fix dm irq error message in gpu recover", please help to review. Regards, Rico From: Wentland, Harry Sent: Wednesday, February 8, 2023 0:19 To: Mahfooz, Hamza ; Yin, Tianci (Rico) ; amd-gfx@lists.freedesktop.org Cc: Wang, Yu (Charlie) ; Siqueira, Rodrigo ; Pillai, Aurabindo Subject: Re: [PATCH] drm/amd/display: Align num_crtc to max_streams On 2/7/23 09:41, Hamza Mahfooz wrote: > > On 2/7/23 09:31, Harry Wentland wrote: >> >> >> On 2/7/23 08:00, Hamza Mahfooz wrote: >>> >>> On 2/6/23 23:05, Tianci Yin wrote: >>>> From: tiancyin >>>> >>>> [Why] >>>> Display pipe might be harvested on some SKUs, that cause the >>>> adev->mode_info.num_crtc mismatch with the usable crtc number, >>>> then below error dmesgs observed after GPU recover. >>>> >>>> *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 >>>> *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 >>>> >>>> [How] >>>> The max_streams is limited number after pipe fuse, align num_crtc >>>> to max_streams to eliminate the error logs. >>>> >>>> Signed-off-by: tiancyin >>>> --- >>>>drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ >>>>1 file changed, 3 insertions(+) >>>> >>>> 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 b31cfda30ff9..87ec2574cc09 100644 >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> @@ -4285,6 +4285,9 @@ static int amdgpu_dm_initialize_drm_device(struct >>>> amdgpu_device *adev) >>>>break; >>>>} >>>>+/* Adjust the crtc number according to the DCN pipe fuse. */ >>>> +adev->mode_info.num_crtc = dm->dc->caps.max_streams; >>> >>> This would introduce array-out-bounds issues, since there are arrays of >>> size AMDGPU_MAX_CRTCS that use num_crtc as a bounds check. >> >> From what I can tell max_streams is always <= AMDGPU_MAX_CRTCS (6), >> though we're not checking. Maybe it'd be good to check and print a >> DRM_ERROR here if that's ever not the case (e.g., if we ever add >> any new ASIC that has more streams). > > I have had UBSAN warns before commit d633b7a25fbe ("drm/amd/display: fix > possible buffer overflow relating to secure display") was applied, so it > seems to already be the case, maybe due to virtual streams. > Interesting. On closer look I'm not sure why this patch is needed. We already do exactly what this patch does at the beginning of amdgpu_dm_initialize_drm_device: >dm->display_indexes_num = dm->dc->caps.max_streams; >/* Update the actual used number of crtc */ >adev->mode_info.num_crtc = adev->dm.display_indexes_num; Harry >> >> Harry >> >>> >>>> + >>>>for (i = 0; i < dm->dc->caps.max_streams; i++) >>>>if (amdgpu_dm_crtc_init(dm, mode_info->planes[i], i)) { >>>>DRM_ERROR("KMS: Failed to initialize crtc\n"); >>> >> >
Re: [PATCH] drm/amd/display: Align num_crtc to max_streams
On 2/7/23 09:41, Hamza Mahfooz wrote: > > On 2/7/23 09:31, Harry Wentland wrote: >> >> >> On 2/7/23 08:00, Hamza Mahfooz wrote: >>> >>> On 2/6/23 23:05, Tianci Yin wrote: From: tiancyin [Why] Display pipe might be harvested on some SKUs, that cause the adev->mode_info.num_crtc mismatch with the usable crtc number, then below error dmesgs observed after GPU recover. *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 [How] The max_streams is limited number after pipe fuse, align num_crtc to max_streams to eliminate the error logs. Signed-off-by: tiancyin --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ 1 file changed, 3 insertions(+) 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 b31cfda30ff9..87ec2574cc09 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4285,6 +4285,9 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) break; } + /* Adjust the crtc number according to the DCN pipe fuse. */ + adev->mode_info.num_crtc = dm->dc->caps.max_streams; >>> >>> This would introduce array-out-bounds issues, since there are arrays of >>> size AMDGPU_MAX_CRTCS that use num_crtc as a bounds check. >> >> From what I can tell max_streams is always <= AMDGPU_MAX_CRTCS (6), >> though we're not checking. Maybe it'd be good to check and print a >> DRM_ERROR here if that's ever not the case (e.g., if we ever add >> any new ASIC that has more streams). > > I have had UBSAN warns before commit d633b7a25fbe ("drm/amd/display: fix > possible buffer overflow relating to secure display") was applied, so it > seems to already be the case, maybe due to virtual streams. > Interesting. On closer look I'm not sure why this patch is needed. We already do exactly what this patch does at the beginning of amdgpu_dm_initialize_drm_device: > dm->display_indexes_num = dm->dc->caps.max_streams; > /* Update the actual used number of crtc */ > adev->mode_info.num_crtc = adev->dm.display_indexes_num; Harry >> >> Harry >> >>> + for (i = 0; i < dm->dc->caps.max_streams; i++) if (amdgpu_dm_crtc_init(dm, mode_info->planes[i], i)) { DRM_ERROR("KMS: Failed to initialize crtc\n"); >>> >> >
Re: [PATCH] drm/amd/display: Align num_crtc to max_streams
On 2/7/23 09:31, Harry Wentland wrote: On 2/7/23 08:00, Hamza Mahfooz wrote: On 2/6/23 23:05, Tianci Yin wrote: From: tiancyin [Why] Display pipe might be harvested on some SKUs, that cause the adev->mode_info.num_crtc mismatch with the usable crtc number, then below error dmesgs observed after GPU recover. *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 [How] The max_streams is limited number after pipe fuse, align num_crtc to max_streams to eliminate the error logs. Signed-off-by: tiancyin --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ 1 file changed, 3 insertions(+) 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 b31cfda30ff9..87ec2574cc09 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4285,6 +4285,9 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) break; } + /* Adjust the crtc number according to the DCN pipe fuse. */ + adev->mode_info.num_crtc = dm->dc->caps.max_streams; This would introduce array-out-bounds issues, since there are arrays of size AMDGPU_MAX_CRTCS that use num_crtc as a bounds check. From what I can tell max_streams is always <= AMDGPU_MAX_CRTCS (6), though we're not checking. Maybe it'd be good to check and print a DRM_ERROR here if that's ever not the case (e.g., if we ever add any new ASIC that has more streams). I have had UBSAN warns before commit d633b7a25fbe ("drm/amd/display: fix possible buffer overflow relating to secure display") was applied, so it seems to already be the case, maybe due to virtual streams. Harry + for (i = 0; i < dm->dc->caps.max_streams; i++) if (amdgpu_dm_crtc_init(dm, mode_info->planes[i], i)) { DRM_ERROR("KMS: Failed to initialize crtc\n"); -- Hamza
Re: [PATCH] drm/amd/display: Align num_crtc to max_streams
On 2/7/23 08:00, Hamza Mahfooz wrote: > > On 2/6/23 23:05, Tianci Yin wrote: >> From: tiancyin >> >> [Why] >> Display pipe might be harvested on some SKUs, that cause the >> adev->mode_info.num_crtc mismatch with the usable crtc number, >> then below error dmesgs observed after GPU recover. >> >> *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 >> *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 >> >> [How] >> The max_streams is limited number after pipe fuse, align num_crtc >> to max_streams to eliminate the error logs. >> >> Signed-off-by: tiancyin >> --- >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> 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 b31cfda30ff9..87ec2574cc09 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -4285,6 +4285,9 @@ static int amdgpu_dm_initialize_drm_device(struct >> amdgpu_device *adev) >> break; >> } >> + /* Adjust the crtc number according to the DCN pipe fuse. */ >> + adev->mode_info.num_crtc = dm->dc->caps.max_streams; > > This would introduce array-out-bounds issues, since there are arrays of > size AMDGPU_MAX_CRTCS that use num_crtc as a bounds check. >From what I can tell max_streams is always <= AMDGPU_MAX_CRTCS (6), though we're not checking. Maybe it'd be good to check and print a DRM_ERROR here if that's ever not the case (e.g., if we ever add any new ASIC that has more streams). Harry > >> + >> for (i = 0; i < dm->dc->caps.max_streams; i++) >> if (amdgpu_dm_crtc_init(dm, mode_info->planes[i], i)) { >> DRM_ERROR("KMS: Failed to initialize crtc\n"); >
Re: [PATCH] drm/amd/display: Align num_crtc to max_streams
On 2/6/23 23:05, Tianci Yin wrote: From: tiancyin [Why] Display pipe might be harvested on some SKUs, that cause the adev->mode_info.num_crtc mismatch with the usable crtc number, then below error dmesgs observed after GPU recover. *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 [How] The max_streams is limited number after pipe fuse, align num_crtc to max_streams to eliminate the error logs. Signed-off-by: tiancyin --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ 1 file changed, 3 insertions(+) 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 b31cfda30ff9..87ec2574cc09 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4285,6 +4285,9 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) break; } + /* Adjust the crtc number according to the DCN pipe fuse. */ + adev->mode_info.num_crtc = dm->dc->caps.max_streams; This would introduce array-out-bounds issues, since there are arrays of size AMDGPU_MAX_CRTCS that use num_crtc as a bounds check. + for (i = 0; i < dm->dc->caps.max_streams; i++) if (amdgpu_dm_crtc_init(dm, mode_info->planes[i], i)) { DRM_ERROR("KMS: Failed to initialize crtc\n"); -- Hamza
[PATCH] drm/amd/display: Align num_crtc to max_streams
From: tiancyin [Why] Display pipe might be harvested on some SKUs, that cause the adev->mode_info.num_crtc mismatch with the usable crtc number, then below error dmesgs observed after GPU recover. *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_crtc_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_pflip_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 *ERROR* amdgpu_dm_set_vupdate_irq_state: crtc is NULL at id :3 [How] The max_streams is limited number after pipe fuse, align num_crtc to max_streams to eliminate the error logs. Signed-off-by: tiancyin --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ 1 file changed, 3 insertions(+) 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 b31cfda30ff9..87ec2574cc09 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4285,6 +4285,9 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) break; } + /* Adjust the crtc number according to the DCN pipe fuse. */ + adev->mode_info.num_crtc = dm->dc->caps.max_streams; + for (i = 0; i < dm->dc->caps.max_streams; i++) if (amdgpu_dm_crtc_init(dm, mode_info->planes[i], i)) { DRM_ERROR("KMS: Failed to initialize crtc\n"); -- 2.34.1