[PATCH] drm/amd/display: check fb of primary plane

[PATCH] drm/amd/display: check fb of primary plane

[Sefa Eyeoglu]

Sometimes the primary plane might not be initialized (yet), which
causes dm_check_crtc_cursor to divide by zero.
Apparently a weird state before a S3-suspend causes the aforementioned
divide-by-zero error when resuming from S3.  This was explained in
bug 212293 on Bugzilla.

To avoid this divide-by-zero error we check if the primary plane's fb
isn't NULL.  If it's NULL the src_w and src_h attributes will be 0,
which would cause a divide-by-zero.

This fixes Bugzilla report 212293
https://bugzilla.kernel.org/show_bug.cgi?id=212293

Fixes: 12f4849a1cfd69f3 ("drm/amd/display: check cursor scaling")
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 573cf17262da4e11..fbb1ac223ccbb62a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9267,7 +9267,8 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
 
 	new_cursor_state = drm_atomic_get_new_plane_state(state, crtc->cursor);
 	new_primary_state = drm_atomic_get_new_plane_state(state, crtc->primary);
-	if (!new_cursor_state || !new_primary_state || !new_cursor_state->fb) {
+	if (!new_cursor_state || !new_primary_state ||
+		!new_cursor_state->fb || !new_primary_state->fb) {
 		return 0;
 	}
 
-- 
2.31.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check fb of primary plane

[Simon Ser]

On Tuesday, March 16th, 2021 at 10:50 PM, Sefa Eyeoglu <contact@scrumplex.net> wrote:

> Sometimes the primary plane might not be initialized (yet), which
> causes dm_check_crtc_cursor to divide by zero.
> Apparently a weird state before a S3-suspend causes the aforementioned
> divide-by-zero error when resuming from S3.  This was explained in
> bug 212293 on Bugzilla.
>
> To avoid this divide-by-zero error we check if the primary plane's fb
> isn't NULL.  If it's NULL the src_w and src_h attributes will be 0,
> which would cause a divide-by-zero.
>
> This fixes Bugzilla report 212293
> https://bugzilla.kernel.org/show_bug.cgi?id=212293
>
> Fixes: 12f4849a1cfd69f3 ("drm/amd/display: check cursor scaling")
> Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>

Thanks for the fix! In theory we should return -EINVAL here, because we can't
enable the cursor plane without the primary plane. But that would break the
legacy API translation layer in DRM core, which expects that planes can always
be disabled individually.

So this is:

Reviewed-by: Simon Ser <contact@emersion.fr>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check fb of primary plane

[Michel Dänzer]

On 2021-03-17 9:29 a.m., Simon Ser wrote:
> On Tuesday, March 16th, 2021 at 10:50 PM, Sefa Eyeoglu <contact@scrumplex.net> wrote:
> 
>> Sometimes the primary plane might not be initialized (yet), which
>> causes dm_check_crtc_cursor to divide by zero.
>> Apparently a weird state before a S3-suspend causes the aforementioned
>> divide-by-zero error when resuming from S3.  This was explained in
>> bug 212293 on Bugzilla.
>>
>> To avoid this divide-by-zero error we check if the primary plane's fb
>> isn't NULL.  If it's NULL the src_w and src_h attributes will be 0,
>> which would cause a divide-by-zero.
>>
>> This fixes Bugzilla report 212293
>> https://bugzilla.kernel.org/show_bug.cgi?id=212293
>>
>> Fixes: 12f4849a1cfd69f3 ("drm/amd/display: check cursor scaling")
>> Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
> 
> Thanks for the fix! In theory we should return -EINVAL here, because we can't
> enable the cursor plane without the primary plane. But that would break the
> legacy API translation layer in DRM core, which expects that planes can always
> be disabled individually.

The core DRM code can deal with being unable to enable the CRTC while the primary plane is disabled. If you have evidence to the contrary, I'd like to see it.


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check fb of primary plane

[Harry Wentland]

On 2021-03-16 5:50 p.m., Sefa Eyeoglu wrote:
> Sometimes the primary plane might not be initialized (yet), which
> causes dm_check_crtc_cursor to divide by zero.
> Apparently a weird state before a S3-suspend causes the aforementioned
> divide-by-zero error when resuming from S3.  This was explained in
> bug 212293 on Bugzilla.
> 
> To avoid this divide-by-zero error we check if the primary plane's fb
> isn't NULL.  If it's NULL the src_w and src_h attributes will be 0,
> which would cause a divide-by-zero.
> 
> This fixes Bugzilla report 212293
> https://bugzilla.kernel.org/show_bug.cgi?id=212293>> 
> Fixes: 12f4849a1cfd69f3 ("drm/amd/display: check cursor scaling")
> Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>

Thanks for your patch.

It is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 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 573cf17262da4e11..fbb1ac223ccbb62a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9267,7 +9267,8 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>   
>   	new_cursor_state = drm_atomic_get_new_plane_state(state, crtc->cursor);
>   	new_primary_state = drm_atomic_get_new_plane_state(state, crtc->primary);
> -	if (!new_cursor_state || !new_primary_state || !new_cursor_state->fb) {
> +	if (!new_cursor_state || !new_primary_state ||
> +		!new_cursor_state->fb || !new_primary_state->fb) {
>   		return 0;
>   	}
>   
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: check fb of primary plane

[Alex Deucher]

On Wed, Mar 17, 2021 at 10:05 AM Harry Wentland <harry.wentland@amd.com> wrote:
>
>
>
> On 2021-03-16 5:50 p.m., Sefa Eyeoglu wrote:
> > Sometimes the primary plane might not be initialized (yet), which
> > causes dm_check_crtc_cursor to divide by zero.
> > Apparently a weird state before a S3-suspend causes the aforementioned
> > divide-by-zero error when resuming from S3.  This was explained in
> > bug 212293 on Bugzilla.
> >
> > To avoid this divide-by-zero error we check if the primary plane's fb
> > isn't NULL.  If it's NULL the src_w and src_h attributes will be 0,
> > which would cause a divide-by-zero.
> >
> > This fixes Bugzilla report 212293
> > https://bugzilla.kernel.org/show_bug.cgi?id=212293>>
> > Fixes: 12f4849a1cfd69f3 ("drm/amd/display: check cursor scaling")
> > Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
>
> Thanks for your patch.
>
> It is
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Applied.  Thanks!

Alex

>
> Harry
>
> > ---
> >   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > 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 573cf17262da4e11..fbb1ac223ccbb62a 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -9267,7 +9267,8 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
> >
> >       new_cursor_state = drm_atomic_get_new_plane_state(state, crtc->cursor);
> >       new_primary_state = drm_atomic_get_new_plane_state(state, crtc->primary);
> > -     if (!new_cursor_state || !new_primary_state || !new_cursor_state->fb) {
> > +     if (!new_cursor_state || !new_primary_state ||
> > +             !new_cursor_state->fb || !new_primary_state->fb) {
> >               return 0;
> >       }
> >
> >
>
> _______________________________________________
> 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