Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs

[Alex Deucher <alexdeucher@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5860 bytes --]

On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka <jan.kiszka@web.de> wrote:
> >
> > On 10.09.20 20:18, Deucher, Alexander wrote:
> > > [AMD Public Use]
> > >
> > >
> > >
> > >> -----Original Message-----
> > >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > >> Daniel Vetter
> > >> Sent: Monday, September 7, 2020 3:15 AM
> > >> To: Jan Kiszka <jan.kiszka@web.de>; amd-gfx list <amd-
> > >> gfx@lists.freedesktop.org>; Wentland, Harry <Harry.Wentland@amd.com>;
> > >> Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
> > >> Cc: dri-devel <dri-devel@lists.freedesktop.org>; intel-gfx <intel-
> > >> gfx@lists.freedesktop.org>; Thomas Zimmermann
> > >> <tzimmermann@suse.de>; Ville Syrjala <ville.syrjala@linux.intel.com>
> > >> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> > >>
> > >> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka <jan.kiszka@web.de> wrote:
> > >>>
> > >>> On 11.02.20 18:04, Daniel Vetter wrote:
> > >>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> > >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>>>
> > >>>>> WARN if the encoder possible_crtcs is effectively empty or contains
> > >>>>> bits for non-existing crtcs.
> > >>>>>
> > >>>>> v2: Move to drm_mode_config_validate() (Daniel)
> > >>>>>     Make the docs say we WARN when this is wrong (Daniel)
> > >>>>>     Extract full_crtc_mask()
> > >>>>>
> > >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > >>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>>
> > >>>> When pushing the fixup needs to be applied before the validation
> > >>>> patch here, because we don't want to anger the bisect gods.
> > >>>>
> > >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >>>>
> > >>>> I think with the fixup we should be good enough with the existing
> > >>>> nonsense in drivers. Fingers crossed.
> > >>>> -Daniel
> > >>>>
> > >>>>
> > >>>>> ---
> > >>>>>  drivers/gpu/drm/drm_mode_config.c | 27
> > >> ++++++++++++++++++++++++++-
> > >>>>>  include/drm/drm_encoder.h         |  2 +-
> > >>>>>  2 files changed, 27 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
> > >>>>> b/drivers/gpu/drm/drm_mode_config.c
> > >>>>> index afc91447293a..4c1b350ddb95 100644
> > >>>>> --- a/drivers/gpu/drm/drm_mode_config.c
> > >>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
> > >>>>> @@ -581,6 +581,29 @@ static void
> > >> validate_encoder_possible_clones(struct drm_encoder *encoder)
> > >>>>>           encoder->possible_clones, encoder_mask);  }
> > >>>>>
> > >>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
> > >>>>> +    struct drm_crtc *crtc;
> > >>>>> +    u32 crtc_mask = 0;
> > >>>>> +
> > >>>>> +    drm_for_each_crtc(crtc, dev)
> > >>>>> +            crtc_mask |= drm_crtc_mask(crtc);
> > >>>>> +
> > >>>>> +    return crtc_mask;
> > >>>>> +}
> > >>>>> +
> > >>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
> > >>>>> +*encoder) {
> > >>>>> +    u32 crtc_mask = full_crtc_mask(encoder->dev);
> > >>>>> +
> > >>>>> +    WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> > >>>>> +         (encoder->possible_crtcs & ~crtc_mask) != 0,
> > >>>>> +         "Bogus possible_crtcs: "
> > >>>>> +         "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> > >>>>> +         encoder->base.id, encoder->name,
> > >>>>> +         encoder->possible_crtcs, crtc_mask); }
> > >>>>> +
> > >>>>>  void drm_mode_config_validate(struct drm_device *dev)  {
> > >>>>>      struct drm_encoder *encoder;
> > >>>>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
> > >> drm_device *dev)
> > >>>>>      drm_for_each_encoder(encoder, dev)
> > >>>>>              fixup_encoder_possible_clones(encoder);
> > >>>>>
> > >>>>> -    drm_for_each_encoder(encoder, dev)
> > >>>>> +    drm_for_each_encoder(encoder, dev) {
> > >>>>>              validate_encoder_possible_clones(encoder);
> > >>>>> +            validate_encoder_possible_crtcs(encoder);
> > >>>>> +    }
> > >>>>>  }
> > >>>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > >>>>> index 3741963b9587..b236269f41ac 100644
> > >>>>> --- a/include/drm/drm_encoder.h
> > >>>>> +++ b/include/drm/drm_encoder.h
> > >>>>> @@ -142,7 +142,7 @@ struct drm_encoder {
> > >>>>>       * the bits for all &drm_crtc objects this encoder can be connected to
> > >>>>>       * before calling drm_dev_register().
> > >>>>>       *
> > >>>>> -     * In reality almost every driver gets this wrong.
> > >>>>> +     * You will get a WARN if you get this wrong in the driver.
> > >>>>>       *
> > >>>>>       * Note that since CRTC objects can't be hotplugged the assigned
> > >> indices
> > >>>>>       * are stable and hence known before registering all objects.
> > >>>>> --
> > >>>>> 2.24.1
> > >>>>>
> > >>>>
> > >>>
> > >>> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
> > >>
> > >> Adding amdgpu display folks.
> > >
> > > I took a quick look at this and it looks like we limit the number of crtcs later in the mode init process if the number of physical displays can't actually use more crtcs.  E.g., the physical board configuration would only allow for 3 active displays even if the hardware technically supports 4 crtcs.  I presume that way we can just leave the additional hardware power gated all the time.
> > >
> >
> > So, will this be fixed any time soon? I don't feel qualified writing
> > such a patch but I would obviously be happy to test one.
>
> It's harmless, but I'll send out a patch soon.

I believe the attached patch should fix this.

Alex

[-- Attachment #2: 0001-drm-amdgpu-disply-set-num_crtc-earlier.patch --]
[-- Type: text/x-patch, Size: 2027 bytes --]

From 441d23a9700ce2d03d8d89686a95a9a6500d866d Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@amd.com>
Date: Thu, 3 Dec 2020 16:06:26 -0500
Subject: [PATCH] drm/amdgpu/disply: set num_crtc earlier

To avoid a recently added warning:
 Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf (full crtc mask=0x7)
 WARNING: CPU: 3 PID: 439 at drivers/gpu/drm/drm_mode_config.c:617 drm_mode_config_validate+0x178/0x200 [drm]
In this case the warning is harmless, but confusing to users.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 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 313501cc39fc..1ec57bc798e2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1130,9 +1130,6 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 		goto error;
 	}
 
-	/* Update the actual used number of crtc */
-	adev->mode_info.num_crtc = adev->dm.display_indexes_num;
-
 	/* create fake encoders for MST */
 	dm_dp_create_fake_mst_encoders(adev);
 
@@ -3364,6 +3361,10 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
 	enum dc_connection_type new_connection_type = dc_connection_none;
 	const struct dc_plane_cap *plane;
 
+	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;
+
 	link_cnt = dm->dc->caps.max_links;
 	if (amdgpu_dm_mode_config_init(dm->adev)) {
 		DRM_ERROR("DM: Failed to initialize mode config\n");
@@ -3425,8 +3426,6 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
 			goto fail;
 		}
 
-	dm->display_indexes_num = dm->dc->caps.max_streams;
-
 	/* loops over all connectors on the board */
 	for (i = 0; i < link_cnt; i++) {
 		struct dc_link *link = NULL;
-- 
2.25.4


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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