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

2020-12-15 Thread Jan Kiszka 
On 03.12.20 22:30, Alex Deucher wrote:
> On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher  wrote:
>>
>> On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka  wrote:
>>>
>>> On 10.09.20 20:18, Deucher, Alexander wrote:
 [AMD Public Use]



> -Original Message-
> From: amd-gfx  On Behalf Of
> Daniel Vetter
> Sent: Monday, September 7, 2020 3:15 AM
> To: Jan Kiszka ; amd-gfx list  g...@lists.freedesktop.org>; Wentland, Harry ;
> Kazlauskas, Nicholas 
> Cc: dri-devel ; intel-gfx  g...@lists.freedesktop.org>; Thomas Zimmermann
> ; Ville Syrjala 
> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
>
> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka  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ä 

 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 
 Cc: Daniel Vetter 
 Signed-off-by: Ville Syrjälä 
>>>
>>> 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 
>>>
>>> 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 _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

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

2020-12-09 Thread Daniel Vetter 
On Thu, Dec 3, 2020 at 10:31 PM Alex Deucher  wrote:
>
> On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher  wrote:
> >
> > On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka  wrote:
> > >
> > > On 10.09.20 20:18, Deucher, Alexander wrote:
> > > > [AMD Public Use]
> > > >
> > > >
> > > >
> > > >> -Original Message-
> > > >> From: amd-gfx  On Behalf Of
> > > >> Daniel Vetter
> > > >> Sent: Monday, September 7, 2020 3:15 AM
> > > >> To: Jan Kiszka ; amd-gfx list  > > >> g...@lists.freedesktop.org>; Wentland, Harry ;
> > > >> Kazlauskas, Nicholas 
> > > >> Cc: dri-devel ; intel-gfx  > > >> g...@lists.freedesktop.org>; Thomas Zimmermann
> > > >> ; Ville Syrjala 
> > > >> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> > > >>
> > > >> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka  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ä 
> > > >
> > > > 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 
> > > > Cc: Daniel Vetter 
> > > > Signed-off-by: Ville Syrjälä 
> > > 
> > >  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 
> > > 
> > >  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 _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

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

2020-12-03 Thread Alex Deucher 
On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher  wrote:
>
> On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka  wrote:
> >
> > On 10.09.20 20:18, Deucher, Alexander wrote:
> > > [AMD Public Use]
> > >
> > >
> > >
> > >> -Original Message-
> > >> From: amd-gfx  On Behalf Of
> > >> Daniel Vetter
> > >> Sent: Monday, September 7, 2020 3:15 AM
> > >> To: Jan Kiszka ; amd-gfx list  > >> g...@lists.freedesktop.org>; Wentland, Harry ;
> > >> Kazlauskas, Nicholas 
> > >> Cc: dri-devel ; intel-gfx  > >> g...@lists.freedesktop.org>; Thomas Zimmermann
> > >> ; Ville Syrjala 
> > >> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> > >>
> > >> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka  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ä 
> > >
> > > 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 
> > > Cc: Daniel Vetter 
> > > Signed-off-by: Ville Syrjälä 
> > 
> >  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 
> > 
> >  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 _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

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

2020-09-29 Thread Alex Deucher 
On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka  wrote:
>
> On 10.09.20 20:18, Deucher, Alexander wrote:
> > [AMD Public Use]
> >
> >
> >
> >> -Original Message-
> >> From: amd-gfx  On Behalf Of
> >> Daniel Vetter
> >> Sent: Monday, September 7, 2020 3:15 AM
> >> To: Jan Kiszka ; amd-gfx list  >> g...@lists.freedesktop.org>; Wentland, Harry ;
> >> Kazlauskas, Nicholas 
> >> Cc: dri-devel ; intel-gfx  >> g...@lists.freedesktop.org>; Thomas Zimmermann
> >> ; Ville Syrjala 
> >> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> >>
> >> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka  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ä 
> >
> > 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 
> > Cc: Daniel Vetter 
> > Signed-off-by: Ville Syrjälä 
> 
>  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 
> 
>  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 _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

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

2020-09-29 Thread Jan Kiszka 
On 10.09.20 20:18, Deucher, Alexander wrote:
> [AMD Public Use]
>
>
>
>> -Original Message-
>> From: amd-gfx  On Behalf Of
>> Daniel Vetter
>> Sent: Monday, September 7, 2020 3:15 AM
>> To: Jan Kiszka ; amd-gfx list > g...@lists.freedesktop.org>; Wentland, Harry ;
>> Kazlauskas, Nicholas 
>> Cc: dri-devel ; intel-gfx > g...@lists.freedesktop.org>; Thomas Zimmermann
>> ; Ville Syrjala 
>> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
>>
>> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka  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ä 
>
> 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 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 

 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 

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

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

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

2020-09-10 Thread Deucher, Alexander 
[AMD Public Use]



> -Original Message-
> From: amd-gfx  On Behalf Of
> Daniel Vetter
> Sent: Monday, September 7, 2020 3:15 AM
> To: Jan Kiszka ; amd-gfx list  g...@lists.freedesktop.org>; Wentland, Harry ;
> Kazlauskas, Nicholas 
> Cc: dri-devel ; intel-gfx  g...@lists.freedesktop.org>; Thomas Zimmermann
> ; Ville Syrjala 
> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
> 
> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka  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ä 
> > >>
> > >> 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 
> > >> Cc: Daniel Vetter 
> > >> Signed-off-by: Ville Syrjälä 
> > >
> > > 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 
> > >
> > > 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 _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.

Alex


> -Daniel
> 
> >
> > [   14.033246] [ cut here ]
> > [   14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65]
> possible_crtcs=0xf (full crtc mask=0x7)
> > [   14.033279] WARNING: CPU: 0 PID: 282 at
> ../drivers/gpu/drm/drm_mode_config.c:622
> drm_mode_config_validate+0x17d/0x200 [drm]
> > [   14.033279] Modules linked in: amdgpu(E+) mfd_core(E)
> snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E)
>

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

2020-09-08 Thread Jan Kiszka 
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ä 
>>
>> 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 
>> Cc: Daniel Vetter 
>> Signed-off-by: Ville Syrjälä 
> 
> 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 
> 
> 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 _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):

[   14.033246] [ cut here ]
[   14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf 
(full crtc mask=0x7)
[   14.033279] WARNING: CPU: 0 PID: 282 at 
../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 
[drm]
[   14.033279] Modules linked in: amdgpu(E+) mfd_core(E) 
snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) 
snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) 
drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E) 
snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E) 
ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E) 
soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E) 
efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E) 
button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E) 
efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) 
crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E) 
crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E) 
r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E) 
crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
[   14.033306]  crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
[   14.033310] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: GE 
5.9.0-rc3+ #2
[   14.033311] Hardware name: Default string Default string/Default string, 
BIOS 5.0.1.4 02/14/2020
[   14.033324] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
[   14.033327] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49 
8b 54 24 38

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

2020-09-07 Thread Daniel Vetter 
On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka  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ä 
> >>
> >> 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 
> >> Cc: Daniel Vetter 
> >> Signed-off-by: Ville Syrjälä 
> >
> > 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 
> >
> > 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 _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.
-Daniel

>
> [   14.033246] [ cut here ]
> [   14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf 
> (full crtc mask=0x7)
> [   14.033279] WARNING: CPU: 0 PID: 282 at 
> ../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200 
> [drm]
> [   14.033279] Modules linked in: amdgpu(E+) mfd_core(E) 
> snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E) 
> snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) 
> drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) 
> cec(E) snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) 
> snd_pcm(E) ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) 
> ccp(E) soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) 
> k10temp(E) efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) 
> efivars(E) video(E) button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) 
> nct6775(E) hwmon_vid(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) 
> ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) 
> crc_t10dif(E) crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) 
> xhci_pci(E) r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) 
> i2c_amd_mp2_pci(E) crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
> [   14.033306]  crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
> [   14.033310] CPU: 0 PID: 282

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

2020-02-11 Thread Daniel Vetter 
On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> 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 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 

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 

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2020-02-11 Thread Ville Syrjala 
From: Ville Syrjälä 

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 
Cc: Daniel Vetter 
Signed-off-by: Ville Syrjälä 
---
 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 _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

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