[PATCH] drm/amd/display: Fix BUG_ON during CRTC atomic check update

[PATCH] drm/amd/display: Fix BUG_ON during CRTC atomic check update

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

For cases where the CRTC is inactive (DPMS off), where a modeset is not
required, yet the CRTC is still in the atomic state, we should not
attempt to update anything on it.

Previously, we were relying on the modereset_required() helper to check
the above condition. However, the function returns false immediatly if
a modeset is not required, ignoring the CRTC's enable/active state
flags. The correct way to filter is by looking at these flags instead.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

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 72f9a46..16d0bf7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4823,15 +4823,16 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 		 * We want to do dc stream updates that do not require a
 		 * full modeset below.
 		 */
-		if (!enable || !aconnector || modereset_required(new_crtc_state))
+		if (!(enable && aconnector && new_crtc_state->enable &&
+		      new_crtc_state->active))
 			continue;
 		/*
 		 * Given above conditions, the dc state cannot be NULL because:
-		 * 1. We're attempting to enable a CRTC. Which has a...
-		 * 2. Valid connector attached, and
-		 * 3. User does not want to reset it (disable or mark inactive,
-		 *    which can happen on a CRTC that's already disabled).
-		 * => It currently exists.
+		 * 1. We're in the process of enabling CRTCs (just been added
+		 *    to the dc context, or already is on the context)
+		 * 2. Has a valid connector attached, and
+		 * 3. Is currently active and enabled.
+		 * => The dc stream state currently exists.
 		 */
 		BUG_ON(dm_new_crtc_state->stream == NULL);
 
-- 
2.7.4

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

Re: [PATCH] drm/amd/display: Fix BUG_ON during CRTC atomic check update

[Michel Dänzer]

On 2018-05-28 05:06 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> For cases where the CRTC is inactive (DPMS off), where a modeset is not
> required, yet the CRTC is still in the atomic state, we should not
> attempt to update anything on it.
> 
> Previously, we were relying on the modereset_required() helper to check

This should say "modeset_required()", right?


> the above condition. However, the function returns false immediatly if

"immediately" (missing "e")

> a modeset is not required, ignoring the CRTC's enable/active state
> flags. The correct way to filter is by looking at these flags instead.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Please add the following tags to the commit log:

Fixes: e277adc5a06c "drm/amd/display: Hookup color management functions"
Tested-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.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: Fix BUG_ON during CRTC atomic check update

[Leo Li]

On 2018-05-28 11:20 AM, Michel Dänzer wrote:
> On 2018-05-28 05:06 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> For cases where the CRTC is inactive (DPMS off), where a modeset is not
>> required, yet the CRTC is still in the atomic state, we should not
>> attempt to update anything on it.
>>
>> Previously, we were relying on the modereset_required() helper to check
> 
> This should say "modeset_required()", right?
> 

modereset_required() here refers to an amdgpu_dm specific helper, which 
was previously being used. It essentially returns true if the CRTC is 
*being* disabled or made inactive, not if it's currently disabled or 
inactive.

> 
>> the above condition. However, the function returns false immediatly if
> 
> "immediately" (missing "e") >
>> a modeset is not required, ignoring the CRTC's enable/active state
>> flags. The correct way to filter is by looking at these flags instead.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> 
> Please add the following tags to the commit log:
> 
> Fixes: e277adc5a06c "drm/amd/display: Hookup color management functions"
> Tested-by: Michel Dänzer <michel.daenzer@amd.com>
> 

Will do.

Leo

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

[PATCH v2] drm/amd/display: Fix BUG_ON during CRTC atomic check update

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

For cases where the CRTC is inactive (DPMS off), where a modeset is not
required, yet the CRTC is still in the atomic state, we should not
attempt to update anything on it.

Previously, we were relying on the modereset_required() helper to check
the above condition. However, the function returns false immediately if
a modeset is not required, ignoring the CRTC's enable/active state
flags. The correct way to filter is by looking at these flags instead.

Fixes: e277adc5a06c "drm/amd/display: Hookup color management functions"

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Tested-by: Michel Dänzer <michel.daenzer@amd.com>
---

v2: Fix commit message typo (immediatly -> immediately)
    Add Fixes line and Michel's Tested-by

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

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 72f9a46..16d0bf7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4823,15 +4823,16 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 		 * We want to do dc stream updates that do not require a
 		 * full modeset below.
 		 */
-		if (!enable || !aconnector || modereset_required(new_crtc_state))
+		if (!(enable && aconnector && new_crtc_state->enable &&
+		      new_crtc_state->active))
 			continue;
 		/*
 		 * Given above conditions, the dc state cannot be NULL because:
-		 * 1. We're attempting to enable a CRTC. Which has a...
-		 * 2. Valid connector attached, and
-		 * 3. User does not want to reset it (disable or mark inactive,
-		 *    which can happen on a CRTC that's already disabled).
-		 * => It currently exists.
+		 * 1. We're in the process of enabling CRTCs (just been added
+		 *    to the dc context, or already is on the context)
+		 * 2. Has a valid connector attached, and
+		 * 3. Is currently active and enabled.
+		 * => The dc stream state currently exists.
 		 */
 		BUG_ON(dm_new_crtc_state->stream == NULL);
 
-- 
2.7.4

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

Re: [PATCH v2] drm/amd/display: Fix BUG_ON during CRTC atomic check update

[Michel Dänzer]

On 2018-05-28 08:00 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> For cases where the CRTC is inactive (DPMS off), where a modeset is not
> required, yet the CRTC is still in the atomic state, we should not
> attempt to update anything on it.
> 
> Previously, we were relying on the modereset_required() helper to check
> the above condition. However, the function returns false immediately if
> a modeset is not required, ignoring the CRTC's enable/active state
> flags. The correct way to filter is by looking at these flags instead.
> 
> Fixes: e277adc5a06c "drm/amd/display: Hookup color management functions"

Might also want to add

Bugzilla: https://bugs.freedesktop.org/106194

for good measure.


> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> Tested-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
> 
> v2: Fix commit message typo (immediatly -> immediately)
>     Add Fixes line and Michel's Tested-by

Other than that looks good, thanks!


-- 
Earthling Michel Dänzer               |               http://www.amd.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

[PATCH v3] drm/amd/display: Fix BUG_ON during CRTC atomic check update

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

For cases where the CRTC is inactive (DPMS off), where a modeset is not
required, yet the CRTC is still in the atomic state, we should not
attempt to update anything on it.

Previously, we were relying on the modereset_required() helper to check
the above condition. However, the function returns false immediately if
a modeset is not required, ignoring the CRTC's enable/active state
flags. The correct way to filter is by looking at these flags instead.

Fixes: e277adc5a06c "drm/amd/display: Hookup color management functions"
Bugzilla: https://bugs.freedesktop.org/106194

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Tested-by: Michel Dänzer <michel.daenzer@amd.com>
---

v2: Fix commit message typo (immediatly -> immediately)
    Add Fixes line and Michel's Tested-by
v3: Add Bugzilla link

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

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 72f9a46..16d0bf7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4823,15 +4823,16 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 		 * We want to do dc stream updates that do not require a
 		 * full modeset below.
 		 */
-		if (!enable || !aconnector || modereset_required(new_crtc_state))
+		if (!(enable && aconnector && new_crtc_state->enable &&
+		      new_crtc_state->active))
 			continue;
 		/*
 		 * Given above conditions, the dc state cannot be NULL because:
-		 * 1. We're attempting to enable a CRTC. Which has a...
-		 * 2. Valid connector attached, and
-		 * 3. User does not want to reset it (disable or mark inactive,
-		 *    which can happen on a CRTC that's already disabled).
-		 * => It currently exists.
+		 * 1. We're in the process of enabling CRTCs (just been added
+		 *    to the dc context, or already is on the context)
+		 * 2. Has a valid connector attached, and
+		 * 3. Is currently active and enabled.
+		 * => The dc stream state currently exists.
 		 */
 		BUG_ON(dm_new_crtc_state->stream == NULL);
 
-- 
2.7.4

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