Re: [PATCH] drm/amdgpu/dc: Simplify drm_crtc_state::active checks

["Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>]

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
> 
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel