From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 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 312c543b258f..dabef307a74f 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3415,21 +3415,12 @@ static bool modeset_required(struct drm_crtc_state *crtc_state, struct dc_stream_state *new_stream, struct dc_stream_state *old_stream) { - if (!drm_atomic_crtc_needs_modeset(crtc_state)) - return false; - - if (!crtc_state->enable) - return false; - - return crtc_state->active; + return crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state); }
static bool modereset_required(struct drm_crtc_state *crtc_state) { - if (!drm_atomic_crtc_needs_modeset(crtc_state)) - return false; - - return !crtc_state->enable || !crtc_state->active; + return !crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state); }
static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder) @@ -8108,8 +8099,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm, * We want to do dc stream updates that do not require a * full modeset below. */ - if (!(enable && aconnector && new_crtc_state->enable && - new_crtc_state->active)) + if (!(enable && aconnector && new_crtc_state->active)) return 0; /* * Given above conditions, the dc state cannot be NULL because:
On Wed, Jul 22, 2020 at 2:38 PM Michel Dänzer michel@daenzer.net wrote:
From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com
modeset vs modereset is a bit an inglorious name choice ... since this seems to be glue code and not part of core dc, maybe rename to enable_required/disable_required to keep it consistent with the wording atomic helpers use? DC also seems to use reset for a lot of other things already (state reset, like atomic, or gpu reset like drm/scheduler's td_r_), so I think this would also help clarity from a DC perspective.
Patch itself is good, above just an idea for another patch on top.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 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 312c543b258f..dabef307a74f 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3415,21 +3415,12 @@ static bool modeset_required(struct drm_crtc_state *crtc_state, struct dc_stream_state *new_stream, struct dc_stream_state *old_stream) {
if (!drm_atomic_crtc_needs_modeset(crtc_state))return false;if (!crtc_state->enable)return false;return crtc_state->active;
return crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state);}
static bool modereset_required(struct drm_crtc_state *crtc_state) {
if (!drm_atomic_crtc_needs_modeset(crtc_state))return false;return !crtc_state->enable || !crtc_state->active;
return !crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state);}
static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder) @@ -8108,8 +8099,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm, * We want to do dc stream updates that do not require a * full modeset below. */
if (!(enable && aconnector && new_crtc_state->enable &&new_crtc_state->active))
if (!(enable && aconnector && new_crtc_state->active)) return 0; /* * Given above conditions, the dc state cannot be NULL because:-- 2.28.0.rc0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2020-07-22 8:51 a.m., Daniel Vetter wrote:
On Wed, Jul 22, 2020 at 2:38 PM Michel Dänzer michel@daenzer.net wrote:
From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Looks fine to me. The check is sufficiently old enough that I don't mind relying on the core for this either.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
modeset vs modereset is a bit an inglorious name choice ... since this seems to be glue code and not part of core dc, maybe rename to enable_required/disable_required to keep it consistent with the wording atomic helpers use? DC also seems to use reset for a lot of other things already (state reset, like atomic, or gpu reset like drm/scheduler's td_r_), so I think this would also help clarity from a DC perspective.
Patch itself is good, above just an idea for another patch on top.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
That sounds like a reasonable idea to me. These are used more as a stream_changed / stream_removed flag, but I don't think these helpers really need to exist at all.
That could come as a follow up patch.
Regards, Nicholas Kazlauskas
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 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 312c543b258f..dabef307a74f 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3415,21 +3415,12 @@ static bool modeset_required(struct drm_crtc_state *crtc_state, struct dc_stream_state *new_stream, struct dc_stream_state *old_stream) {
if (!drm_atomic_crtc_needs_modeset(crtc_state))return false;if (!crtc_state->enable)return false;return crtc_state->active;
return crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state);}
static bool modereset_required(struct drm_crtc_state *crtc_state) {
if (!drm_atomic_crtc_needs_modeset(crtc_state))return false;return !crtc_state->enable || !crtc_state->active;
return !crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state);}
static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
@@ -8108,8 +8099,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm, * We want to do dc stream updates that do not require a * full modeset below. */
if (!(enable && aconnector && new_crtc_state->enable &&new_crtc_state->active))
if (!(enable && aconnector && new_crtc_state->active)) return 0; /* * Given above conditions, the dc state cannot be NULL because:-- 2.28.0.rc0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2020-07-22 3:10 p.m., Kazlauskas, Nicholas wrote:
On 2020-07-22 8:51 a.m., Daniel Vetter wrote:
On Wed, Jul 22, 2020 at 2:38 PM Michel Dänzer michel@daenzer.net wrote:
From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Looks fine to me. The check is sufficiently old enough that I don't mind relying on the core for this either.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
modeset vs modereset is a bit an inglorious name choice ... since this seems to be glue code and not part of core dc, maybe rename to enable_required/disable_required to keep it consistent with the wording atomic helpers use? DC also seems to use reset for a lot of other things already (state reset, like atomic, or gpu reset like drm/scheduler's td_r_), so I think this would also help clarity from a DC perspective.
Patch itself is good, above just an idea for another patch on top.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks for the reviews! I assume this will get picked up by a DC developer or Alex/Christian.
That sounds like a reasonable idea to me. These are used more as a stream_changed / stream_removed flag, but I don't think these helpers really need to exist at all.
That could come as a follow up patch.
Yeah, I'm leaving that to you guys. :)
On Wed, Jul 22, 2020 at 4:25 PM Michel Dänzer michel@daenzer.net wrote:
On 2020-07-22 3:10 p.m., Kazlauskas, Nicholas wrote:
On 2020-07-22 8:51 a.m., Daniel Vetter wrote:
On Wed, Jul 22, 2020 at 2:38 PM Michel Dänzer michel@daenzer.net wrote:
From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Looks fine to me. The check is sufficiently old enough that I don't mind relying on the core for this either.
"active implies enabled" has been a hard assumption of atomic from day 1. So should work anywhere you have atomic. -Daniel
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
modeset vs modereset is a bit an inglorious name choice ... since this seems to be glue code and not part of core dc, maybe rename to enable_required/disable_required to keep it consistent with the wording atomic helpers use? DC also seems to use reset for a lot of other things already (state reset, like atomic, or gpu reset like drm/scheduler's td_r_), so I think this would also help clarity from a DC perspective.
Patch itself is good, above just an idea for another patch on top.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks for the reviews! I assume this will get picked up by a DC developer or Alex/Christian.
That sounds like a reasonable idea to me. These are used more as a stream_changed / stream_removed flag, but I don't think these helpers really need to exist at all.
That could come as a follow up patch.
Yeah, I'm leaving that to you guys. :)
-- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer
On Wed, Jul 22, 2020 at 10:25 AM Michel Dänzer michel@daenzer.net wrote:
On 2020-07-22 3:10 p.m., Kazlauskas, Nicholas wrote:
On 2020-07-22 8:51 a.m., Daniel Vetter wrote:
On Wed, Jul 22, 2020 at 2:38 PM Michel Dänzer michel@daenzer.net wrote:
From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Looks fine to me. The check is sufficiently old enough that I don't mind relying on the core for this either.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
modeset vs modereset is a bit an inglorious name choice ... since this seems to be glue code and not part of core dc, maybe rename to enable_required/disable_required to keep it consistent with the wording atomic helpers use? DC also seems to use reset for a lot of other things already (state reset, like atomic, or gpu reset like drm/scheduler's td_r_), so I think this would also help clarity from a DC perspective.
Patch itself is good, above just an idea for another patch on top.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks for the reviews! I assume this will get picked up by a DC developer or Alex/Christian.
Applied. Thanks!
Alex
That sounds like a reasonable idea to me. These are used more as a stream_changed / stream_removed flag, but I don't think these helpers really need to exist at all.
That could come as a follow up patch.
Yeah, I'm leaving that to you guys. :)
-- 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
On 2020-07-22 7:12 p.m., Alex Deucher wrote:
On Wed, Jul 22, 2020 at 10:25 AM Michel Dänzer michel@daenzer.net wrote:
On 2020-07-22 3:10 p.m., Kazlauskas, Nicholas wrote:
On 2020-07-22 8:51 a.m., Daniel Vetter wrote:
On Wed, Jul 22, 2020 at 2:38 PM Michel Dänzer michel@daenzer.net wrote:
From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Looks fine to me. The check is sufficiently old enough that I don't mind relying on the core for this either.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
modeset vs modereset is a bit an inglorious name choice ... since this seems to be glue code and not part of core dc, maybe rename to enable_required/disable_required to keep it consistent with the wording atomic helpers use? DC also seems to use reset for a lot of other things already (state reset, like atomic, or gpu reset like drm/scheduler's td_r_), so I think this would also help clarity from a DC perspective.
Patch itself is good, above just an idea for another patch on top.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks for the reviews! I assume this will get picked up by a DC developer or Alex/Christian.
Applied. Thanks!
Thank you. Can't see it in the DRM changes for 5.9 though.
On Wed, Aug 19, 2020 at 5:08 AM Michel Dänzer michel@daenzer.net wrote:
On 2020-07-22 7:12 p.m., Alex Deucher wrote:
On Wed, Jul 22, 2020 at 10:25 AM Michel Dänzer michel@daenzer.net wrote:
On 2020-07-22 3:10 p.m., Kazlauskas, Nicholas wrote:
On 2020-07-22 8:51 a.m., Daniel Vetter wrote:
On Wed, Jul 22, 2020 at 2:38 PM Michel Dänzer michel@daenzer.net wrote:
From: Michel Dänzer mdaenzer@redhat.com
drm_atomic_crtc_check enforces that ::active can only be true if ::enable is as well.
Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Looks fine to me. The check is sufficiently old enough that I don't mind relying on the core for this either.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
modeset vs modereset is a bit an inglorious name choice ... since this seems to be glue code and not part of core dc, maybe rename to enable_required/disable_required to keep it consistent with the wording atomic helpers use? DC also seems to use reset for a lot of other things already (state reset, like atomic, or gpu reset like drm/scheduler's td_r_), so I think this would also help clarity from a DC perspective.
Patch itself is good, above just an idea for another patch on top.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks for the reviews! I assume this will get picked up by a DC developer or Alex/Christian.
Applied. Thanks!
Thank you. Can't see it in the DRM changes for 5.9 though.
Will show up for 5.10 as it didn't seem critical for 5.9.
Alex
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Kazlauskas, Nicholas -
Michel Dänzer