[PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Lyude Paul]

While I thought I had this correct (since it actually did reject modes
like I expected during testing), Ville Syrjala from Intel pointed out
that the logic here isn't correct. max_clock refers to the max symbol
rate supported by the encoder, so limiting clock to ds_clock using max()
doesn't make sense. Additionally, we want to check against 6bpc for the
time being since that's the minimum possible bpc here, not the reported
bpc from the connector. See:

https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html

For more info.

So, let's rewrite this using Ville's advice.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock limits for mode validation")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 7b640e05bd4cd..24c81e423d349 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector *connector,
 		   const struct drm_display_mode *mode,
 		   unsigned *out_clock)
 {
-	const unsigned min_clock = 25000;
-	unsigned max_clock, ds_clock, clock;
+	const unsigned int min_clock = 25000;
+	unsigned int max_clock, ds_clock, clock;
+	const u8 bpp = 18; /* 6 bpc */
 	enum drm_mode_status ret;
 
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp->caps.dp_interlace)
 		return MODE_NO_INTERLACE;
 
 	max_clock = outp->dp.link_nr * outp->dp.link_bw;
-	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd,
-						  outp->dp.downstream_ports);
-	if (ds_clock)
-		max_clock = min(max_clock, ds_clock);
-
-	clock = mode->clock * (connector->display_info.bpc * 3) / 10;
-	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
-					    &clock);
+	clock = mode->clock * bpp / 8;
+	if (clock > max_clock)
+		return MODE_CLOCK_HIGH;
+
+	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp->dp.downstream_ports);
+	if (ds_clock && mode->clock > ds_clock)
+		return MODE_CLOCK_HIGH;
+
+	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock, &clock);
 	if (out_clock)
 		*out_clock = clock;
+
 	return ret;
 }
-- 
2.26.2

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Ilia Mirkin]

Can we use 6bpc on arbitrary DP monitors, or is there a capability for
it? Maybe only use 6bpc if display_info.bpc == 6 and otherwise use 8?

On Tue, Sep 22, 2020 at 5:06 PM Lyude Paul <lyude@redhat.com> wrote:
>
> While I thought I had this correct (since it actually did reject modes
> like I expected during testing), Ville Syrjala from Intel pointed out
> that the logic here isn't correct. max_clock refers to the max symbol
> rate supported by the encoder, so limiting clock to ds_clock using max()
> doesn't make sense. Additionally, we want to check against 6bpc for the
> time being since that's the minimum possible bpc here, not the reported
> bpc from the connector. See:
>
> https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html
>
> For more info.
>
> So, let's rewrite this using Ville's advice.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock limits for mode validation")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index 7b640e05bd4cd..24c81e423d349 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector *connector,
>                    const struct drm_display_mode *mode,
>                    unsigned *out_clock)
>  {
> -       const unsigned min_clock = 25000;
> -       unsigned max_clock, ds_clock, clock;
> +       const unsigned int min_clock = 25000;
> +       unsigned int max_clock, ds_clock, clock;
> +       const u8 bpp = 18; /* 6 bpc */
>         enum drm_mode_status ret;
>
>         if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp->caps.dp_interlace)
>                 return MODE_NO_INTERLACE;
>
>         max_clock = outp->dp.link_nr * outp->dp.link_bw;
> -       ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd,
> -                                                 outp->dp.downstream_ports);
> -       if (ds_clock)
> -               max_clock = min(max_clock, ds_clock);
> -
> -       clock = mode->clock * (connector->display_info.bpc * 3) / 10;
> -       ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> -                                           &clock);
> +       clock = mode->clock * bpp / 8;
> +       if (clock > max_clock)
> +               return MODE_CLOCK_HIGH;
> +
> +       ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp->dp.downstream_ports);
> +       if (ds_clock && mode->clock > ds_clock)
> +               return MODE_CLOCK_HIGH;
> +
> +       ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock, &clock);
>         if (out_clock)
>                 *out_clock = clock;
> +
>         return ret;
>  }
> --
> 2.26.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Lyude Paul]

On Tue, 2020-09-22 at 17:10 -0400, Ilia Mirkin wrote:
> Can we use 6bpc on arbitrary DP monitors, or is there a capability for
> it? Maybe only use 6bpc if display_info.bpc == 6 and otherwise use 8?

I don't think that display_info.bpc actually implies a minimum bpc, only a
maximum bpc iirc (Ville would know the answer to this one). The other thing to
note here is that we want to assume the lowest possible bpc here since we're
only concerned if the mode passed to ->mode_valid can be set under -any-
conditions (including those that require lowering the bpc beyond it's maximum
value), so we definitely do want to always use 6bpc here even once we get
support for optimizing the bpc based on the available display bandwidth.
> 
> On Tue, Sep 22, 2020 at 5:06 PM Lyude Paul <lyude@redhat.com> wrote:
> > While I thought I had this correct (since it actually did reject modes
> > like I expected during testing), Ville Syrjala from Intel pointed out
> > that the logic here isn't correct. max_clock refers to the max symbol
> > rate supported by the encoder, so limiting clock to ds_clock using max()
> > doesn't make sense. Additionally, we want to check against 6bpc for the
> > time being since that's the minimum possible bpc here, not the reported
> > bpc from the connector. See:
> > 
> > https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html
> > 
> > For more info.
> > 
> > So, let's rewrite this using Ville's advice.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock
> > limits for mode validation")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > index 7b640e05bd4cd..24c81e423d349 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > @@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector *connector,
> >                    const struct drm_display_mode *mode,
> >                    unsigned *out_clock)
> >  {
> > -       const unsigned min_clock = 25000;
> > -       unsigned max_clock, ds_clock, clock;
> > +       const unsigned int min_clock = 25000;
> > +       unsigned int max_clock, ds_clock, clock;
> > +       const u8 bpp = 18; /* 6 bpc */
> >         enum drm_mode_status ret;
> > 
> >         if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp-
> > >caps.dp_interlace)
> >                 return MODE_NO_INTERLACE;
> > 
> >         max_clock = outp->dp.link_nr * outp->dp.link_bw;
> > -       ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd,
> > -                                                 outp-
> > >dp.downstream_ports);
> > -       if (ds_clock)
> > -               max_clock = min(max_clock, ds_clock);
> > -
> > -       clock = mode->clock * (connector->display_info.bpc * 3) / 10;
> > -       ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> > -                                           &clock);
> > +       clock = mode->clock * bpp / 8;
> > +       if (clock > max_clock)
> > +               return MODE_CLOCK_HIGH;
> > +
> > +       ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp-
> > >dp.downstream_ports);
> > +       if (ds_clock && mode->clock > ds_clock)
> > +               return MODE_CLOCK_HIGH;
> > +
> > +       ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> > &clock);
> >         if (out_clock)
> >                 *out_clock = clock;
> > +
> >         return ret;
> >  }
> > --
> > 2.26.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Cheers,
	Lyude Paul (she/her)
	Software Engineer at Red Hat

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Ilia Mirkin]

On Tue, Sep 22, 2020 at 5:14 PM Lyude Paul <lyude@redhat.com> wrote:
>
> On Tue, 2020-09-22 at 17:10 -0400, Ilia Mirkin wrote:
> > Can we use 6bpc on arbitrary DP monitors, or is there a capability for
> > it? Maybe only use 6bpc if display_info.bpc == 6 and otherwise use 8?
>
> I don't think that display_info.bpc actually implies a minimum bpc, only a
> maximum bpc iirc (Ville would know the answer to this one). The other thing to
> note here is that we want to assume the lowest possible bpc here since we're
> only concerned if the mode passed to ->mode_valid can be set under -any-
> conditions (including those that require lowering the bpc beyond it's maximum
> value), so we definitely do want to always use 6bpc here even once we get
> support for optimizing the bpc based on the available display bandwidth.

Yeah, display_info is the max bpc. But would an average monitor
support 6bpc? And if it does, does the current link training code even
try that when display_info.bpc != 6?

  -ilia

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Lyude Paul]

On Tue, 2020-09-22 at 17:22 -0400, Ilia Mirkin wrote:
> On Tue, Sep 22, 2020 at 5:14 PM Lyude Paul <lyude@redhat.com> wrote:
> > On Tue, 2020-09-22 at 17:10 -0400, Ilia Mirkin wrote:
> > > Can we use 6bpc on arbitrary DP monitors, or is there a capability for
> > > it? Maybe only use 6bpc if display_info.bpc == 6 and otherwise use 8?
> > 
> > I don't think that display_info.bpc actually implies a minimum bpc, only a
> > maximum bpc iirc (Ville would know the answer to this one). The other thing
> > to
> > note here is that we want to assume the lowest possible bpc here since we're
> > only concerned if the mode passed to ->mode_valid can be set under -any-
> > conditions (including those that require lowering the bpc beyond it's
> > maximum
> > value), so we definitely do want to always use 6bpc here even once we get
> > support for optimizing the bpc based on the available display bandwidth.
> 
> Yeah, display_info is the max bpc. But would an average monitor
> support 6bpc? And if it does, does the current link training code even
> try that when display_info.bpc != 6?

So I did confirm that 6bpc support is mandatory for DP, so yes-6 bpc will always
work.

But also, your second comment doesn't really apply here. So: to be clear, we're
not really concerned here about whether nouveau will actually use 6bpc or not. 
In truth I'm not actually sure either if we have any code that uses 6bpc (iirc
we don't), since we don't current optimize bpc. I think it's very possible for
us to use 6bpc for eDP displays if I recall though, but I'm not sure on that.

But that's also not the point of this code. ->mode_valid() is only used in two
situations in DRM modesetting: when probing connector modes, and when checking
if a mode is valid or not during the atomic check for atomic modesetting. Its
purpose is only to reject display modes that are physically impossible to set in
hardware due to static hardware constraints. Put another way, we only check the
given mode against constraints which will always remain constant regardless of
the rest of the display state. An example of a static constraint would be the
max pixel clock supported by the hardware, since on sensible hardware this never
changes. A dynamic constraint would be something like how much bandwidth is
currently unused on an MST topology, since that value is entirely dependent on
the rest of the display state.

So - with that said, bpc is technically a dynamic constraint because while a
sink and source both likely have their own bpc limits, any bpc which is equal or
below that limit can be used depending on what the driver decides - which will
be based on the max_bpc property, and additionally for MST displays it will also
depend on the available bandwidth on the topology. The only non-dynamic thing
about bpc is that at a minimum, it will be 6 - so any mode that doesn't fit on
the link with a bpc of 6 is guaranteed to be a mode that we'll never be able to
set and therefore want to prune.

So, even if we're not using 6 in the majority of situations, I'm fairly
confident it's the right value here. It's also what i915 does as well (and they
previously had to fix a bug that was the result of assuming a minimum of 8bpc
instead of 6).

> 
>   -ilia
> 
-- 
Sincerely,
      Lyude Paul (she/her)
      Software Engineer at Red Hat

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Ilia Mirkin]

On Fri, Sep 25, 2020 at 6:08 PM Lyude Paul <lyude@redhat.com> wrote:
>
> On Tue, 2020-09-22 at 17:22 -0400, Ilia Mirkin wrote:
> > On Tue, Sep 22, 2020 at 5:14 PM Lyude Paul <lyude@redhat.com> wrote:
> > > On Tue, 2020-09-22 at 17:10 -0400, Ilia Mirkin wrote:
> > > > Can we use 6bpc on arbitrary DP monitors, or is there a capability for
> > > > it? Maybe only use 6bpc if display_info.bpc == 6 and otherwise use 8?
> > >
> > > I don't think that display_info.bpc actually implies a minimum bpc, only a
> > > maximum bpc iirc (Ville would know the answer to this one). The other thing
> > > to
> > > note here is that we want to assume the lowest possible bpc here since we're
> > > only concerned if the mode passed to ->mode_valid can be set under -any-
> > > conditions (including those that require lowering the bpc beyond it's
> > > maximum
> > > value), so we definitely do want to always use 6bpc here even once we get
> > > support for optimizing the bpc based on the available display bandwidth.
> >
> > Yeah, display_info is the max bpc. But would an average monitor
> > support 6bpc? And if it does, does the current link training code even
> > try that when display_info.bpc != 6?
>
> So I did confirm that 6bpc support is mandatory for DP, so yes-6 bpc will always
> work.
>
> But also, your second comment doesn't really apply here. So: to be clear, we're
> not really concerned here about whether nouveau will actually use 6bpc or not.
> In truth I'm not actually sure either if we have any code that uses 6bpc (iirc
> we don't), since we don't current optimize bpc. I think it's very possible for
> us to use 6bpc for eDP displays if I recall though, but I'm not sure on that.
>
> But that's also not the point of this code. ->mode_valid() is only used in two
> situations in DRM modesetting: when probing connector modes, and when checking
> if a mode is valid or not during the atomic check for atomic modesetting. Its
> purpose is only to reject display modes that are physically impossible to set in
> hardware due to static hardware constraints. Put another way, we only check the
> given mode against constraints which will always remain constant regardless of
> the rest of the display state. An example of a static constraint would be the
> max pixel clock supported by the hardware, since on sensible hardware this never
> changes. A dynamic constraint would be something like how much bandwidth is
> currently unused on an MST topology, since that value is entirely dependent on
> the rest of the display state.
>
> So - with that said, bpc is technically a dynamic constraint because while a
> sink and source both likely have their own bpc limits, any bpc which is equal or
> below that limit can be used depending on what the driver decides - which will
> be based on the max_bpc property, and additionally for MST displays it will also
> depend on the available bandwidth on the topology. The only non-dynamic thing
> about bpc is that at a minimum, it will be 6 - so any mode that doesn't fit on
> the link with a bpc of 6 is guaranteed to be a mode that we'll never be able to
> set and therefore want to prune.
>
> So, even if we're not using 6 in the majority of situations, I'm fairly
> confident it's the right value here. It's also what i915 does as well (and they
> previously had to fix a bug that was the result of assuming a minimum of 8bpc
> instead of 6).

Here's the situation I'm trying to avoid:

1. Mode is considered "OK" from a bandwidth perspective @6bpc
2. Modesetting logic never tries 6bpc and the modeset fails

As long as the two bits of logic agree on what is settable, I'm happy.

Cheers,

  -ilia

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Lyude Paul]

On Fri, 2020-09-25 at 19:53 -0400, Ilia Mirkin wrote:
> On Fri, Sep 25, 2020 at 6:08 PM Lyude Paul <lyude@redhat.com> wrote:
> > On Tue, 2020-09-22 at 17:22 -0400, Ilia Mirkin wrote:
> > > On Tue, Sep 22, 2020 at 5:14 PM Lyude Paul <lyude@redhat.com> wrote:
> > > > On Tue, 2020-09-22 at 17:10 -0400, Ilia Mirkin wrote:
> > > > > Can we use 6bpc on arbitrary DP monitors, or is there a capability
> > > > > for
> > > > > it? Maybe only use 6bpc if display_info.bpc == 6 and otherwise use
> > > > > 8?
> > > > 
> > > > I don't think that display_info.bpc actually implies a minimum bpc,
> > > > only a
> > > > maximum bpc iirc (Ville would know the answer to this one). The other
> > > > thing
> > > > to
> > > > note here is that we want to assume the lowest possible bpc here since
> > > > we're
> > > > only concerned if the mode passed to ->mode_valid can be set under
> > > > -any-
> > > > conditions (including those that require lowering the bpc beyond it's
> > > > maximum
> > > > value), so we definitely do want to always use 6bpc here even once we
> > > > get
> > > > support for optimizing the bpc based on the available display
> > > > bandwidth.
> > > 
> > > Yeah, display_info is the max bpc. But would an average monitor
> > > support 6bpc? And if it does, does the current link training code even
> > > try that when display_info.bpc != 6?
> > 
> > So I did confirm that 6bpc support is mandatory for DP, so yes-6 bpc will
> > always
> > work.
> > 
> > But also, your second comment doesn't really apply here. So: to be clear,
> > we're
> > not really concerned here about whether nouveau will actually use 6bpc or
> > not.
> > In truth I'm not actually sure either if we have any code that uses 6bpc
> > (iirc
> > we don't), since we don't current optimize bpc. I think it's very possible
> > for
> > us to use 6bpc for eDP displays if I recall though, but I'm not sure on
> > that.
> > 
> > But that's also not the point of this code. ->mode_valid() is only used in
> > two
> > situations in DRM modesetting: when probing connector modes, and when
> > checking
> > if a mode is valid or not during the atomic check for atomic modesetting.
> > Its
> > purpose is only to reject display modes that are physically impossible to
> > set in
> > hardware due to static hardware constraints. Put another way, we only
> > check the
> > given mode against constraints which will always remain constant
> > regardless of
> > the rest of the display state. An example of a static constraint would be
> > the
> > max pixel clock supported by the hardware, since on sensible hardware this
> > never
> > changes. A dynamic constraint would be something like how much bandwidth
> > is
> > currently unused on an MST topology, since that value is entirely
> > dependent on
> > the rest of the display state.
> > 
> > So - with that said, bpc is technically a dynamic constraint because while
> > a
> > sink and source both likely have their own bpc limits, any bpc which is
> > equal or
> > below that limit can be used depending on what the driver decides - which
> > will
> > be based on the max_bpc property, and additionally for MST displays it
> > will also
> > depend on the available bandwidth on the topology. The only non-dynamic
> > thing
> > about bpc is that at a minimum, it will be 6 - so any mode that doesn't
> > fit on
> > the link with a bpc of 6 is guaranteed to be a mode that we'll never be
> > able to
> > set and therefore want to prune.
> > 
> > So, even if we're not using 6 in the majority of situations, I'm fairly
> > confident it's the right value here. It's also what i915 does as well (and
> > they
> > previously had to fix a bug that was the result of assuming a minimum of
> > 8bpc
> > instead of 6).
> 
> Here's the situation I'm trying to avoid:
> 
> 1. Mode is considered "OK" from a bandwidth perspective @6bpc
> 2. Modesetting logic never tries 6bpc and the modeset fails
> 
> As long as the two bits of logic agree on what is settable, I'm happy.

I gotcha-I guess I was just confused because this is already possible with the
current code we have (and it was also possible before we started adding these
checks into ->mode_valid, which is why I need to get to the max_bpc
thing soon). I guess I'll just use the connector's reported bpc for now until
we add support for that
> 
> Cheers,
> 
>   -ilia
> 
-- 
Cheers,
	Lyude Paul (she/her)
	Software Engineer at Red Hat

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Ville Syrjälä]

On Tue, Sep 22, 2020 at 05:05:10PM -0400, Lyude Paul wrote:
> While I thought I had this correct (since it actually did reject modes
> like I expected during testing), Ville Syrjala from Intel pointed out
> that the logic here isn't correct. max_clock refers to the max symbol
> rate supported by the encoder, so limiting clock to ds_clock using max()
> doesn't make sense. Additionally, we want to check against 6bpc for the
> time being since that's the minimum possible bpc here, not the reported
> bpc from the connector. See:
> 
> https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html
> 
> For more info.
> 
> So, let's rewrite this using Ville's advice.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock limits for mode validation")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index 7b640e05bd4cd..24c81e423d349 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector *connector,
>  		   const struct drm_display_mode *mode,
>  		   unsigned *out_clock)
>  {
> -	const unsigned min_clock = 25000;
> -	unsigned max_clock, ds_clock, clock;
> +	const unsigned int min_clock = 25000;
> +	unsigned int max_clock, ds_clock, clock;
> +	const u8 bpp = 18; /* 6 bpc */

AFAICS nv50_outp_atomic_check() and nv50_msto_atomic_check()
just blindly use connector->display_info.bpc without any fallback
logic to lower the bpc. So Ilia's concerns seem well founded.
Without that logic I guess you should just use
connector->display_info.bpc here as well.

>  	enum drm_mode_status ret;
>  
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp->caps.dp_interlace)
>  		return MODE_NO_INTERLACE;
>  
>  	max_clock = outp->dp.link_nr * outp->dp.link_bw;
> -	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd,
> -						  outp->dp.downstream_ports);
> -	if (ds_clock)
> -		max_clock = min(max_clock, ds_clock);
> -
> -	clock = mode->clock * (connector->display_info.bpc * 3) / 10;
> -	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> -					    &clock);
> +	clock = mode->clock * bpp / 8;
> +	if (clock > max_clock)
> +		return MODE_CLOCK_HIGH;

This stuff vs. nouveau_conn_mode_clock_valid() still seems a bit messy.
The max_clock you pass to nouveau_conn_mode_clock_valid() is the max
symbol clock, but nouveau_conn_mode_clock_valid() checks it against the
dotclock. Also only nouveau_conn_mode_clock_valid() has any kind of
stereo 3D handling, but AFAICS stereo_allowed is also set for DP?

> +
> +	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp->dp.downstream_ports);
> +	if (ds_clock && mode->clock > ds_clock)
> +		return MODE_CLOCK_HIGH;
> +
> +	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock, &clock);
>  	if (out_clock)
>  		*out_clock = clock;
> +
>  	return ret;
>  }
> -- 
> 2.26.2

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Lyude Paul]

On Mon, 2020-09-28 at 16:01 +0300, Ville Syrjälä wrote:
> On Tue, Sep 22, 2020 at 05:05:10PM -0400, Lyude Paul wrote:
> > While I thought I had this correct (since it actually did reject modes
> > like I expected during testing), Ville Syrjala from Intel pointed out
> > that the logic here isn't correct. max_clock refers to the max symbol
> > rate supported by the encoder, so limiting clock to ds_clock using max()
> > doesn't make sense. Additionally, we want to check against 6bpc for the
> > time being since that's the minimum possible bpc here, not the reported
> > bpc from the connector. See:
> > 
> > https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html
> > 
> > For more info.
> > 
> > So, let's rewrite this using Ville's advice.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock
> > limits for mode validation")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > index 7b640e05bd4cd..24c81e423d349 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > @@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector *connector,
> >  		   const struct drm_display_mode *mode,
> >  		   unsigned *out_clock)
> >  {
> > -	const unsigned min_clock = 25000;
> > -	unsigned max_clock, ds_clock, clock;
> > +	const unsigned int min_clock = 25000;
> > +	unsigned int max_clock, ds_clock, clock;
> > +	const u8 bpp = 18; /* 6 bpc */
> 
> AFAICS nv50_outp_atomic_check() and nv50_msto_atomic_check()
> just blindly use connector->display_info.bpc without any fallback
> logic to lower the bpc. So Ilia's concerns seem well founded.
> Without that logic I guess you should just use
> connector->display_info.bpc here as well.
> 
> >  	enum drm_mode_status ret;
> >  
> >  	if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp->caps.dp_interlace)
> >  		return MODE_NO_INTERLACE;
> >  
> >  	max_clock = outp->dp.link_nr * outp->dp.link_bw;
> > -	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd,
> > -						  outp->dp.downstream_ports);
> > -	if (ds_clock)
> > -		max_clock = min(max_clock, ds_clock);
> > -
> > -	clock = mode->clock * (connector->display_info.bpc * 3) / 10;
> > -	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> > -					    &clock);
> > +	clock = mode->clock * bpp / 8;
> > +	if (clock > max_clock)
> > +		return MODE_CLOCK_HIGH;
> 
> This stuff vs. nouveau_conn_mode_clock_valid() still seems a bit messy.
> The max_clock you pass to nouveau_conn_mode_clock_valid() is the max
> symbol clock, but nouveau_conn_mode_clock_valid() checks it against the
> dotclock. Also only nouveau_conn_mode_clock_valid() has any kind of
> stereo 3D handling, but AFAICS stereo_allowed is also set for DP?

...not sure I'm following you here, it's set to true for DP so don't we want
to check it and adjust the pixel clock we output accordingly?

> 
> > +
> > +	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp-
> > >dp.downstream_ports);
> > +	if (ds_clock && mode->clock > ds_clock)
> > +		return MODE_CLOCK_HIGH;
> > +
> > +	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> > &clock);
> >  	if (out_clock)
> >  		*out_clock = clock;
> > +
> >  	return ret;
> >  }
> > -- 
> > 2.26.2
-- 
Cheers,
	Lyude Paul (she/her)
	Software Engineer at Red Hat

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Ville Syrjälä]

On Tue, Sep 29, 2020 at 01:54:13PM -0400, Lyude Paul wrote:
> On Mon, 2020-09-28 at 16:01 +0300, Ville Syrjälä wrote:
> > On Tue, Sep 22, 2020 at 05:05:10PM -0400, Lyude Paul wrote:
> > > While I thought I had this correct (since it actually did reject modes
> > > like I expected during testing), Ville Syrjala from Intel pointed out
> > > that the logic here isn't correct. max_clock refers to the max symbol
> > > rate supported by the encoder, so limiting clock to ds_clock using max()
> > > doesn't make sense. Additionally, we want to check against 6bpc for the
> > > time being since that's the minimum possible bpc here, not the reported
> > > bpc from the connector. See:
> > > 
> > > https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html
> > > 
> > > For more info.
> > > 
> > > So, let's rewrite this using Ville's advice.
> > > 
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock
> > > limits for mode validation")
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > ---
> > >  drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++----------
> > >  1 file changed, 13 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > index 7b640e05bd4cd..24c81e423d349 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > @@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector *connector,
> > >  		   const struct drm_display_mode *mode,
> > >  		   unsigned *out_clock)
> > >  {
> > > -	const unsigned min_clock = 25000;
> > > -	unsigned max_clock, ds_clock, clock;
> > > +	const unsigned int min_clock = 25000;
> > > +	unsigned int max_clock, ds_clock, clock;
> > > +	const u8 bpp = 18; /* 6 bpc */
> > 
> > AFAICS nv50_outp_atomic_check() and nv50_msto_atomic_check()
> > just blindly use connector->display_info.bpc without any fallback
> > logic to lower the bpc. So Ilia's concerns seem well founded.
> > Without that logic I guess you should just use
> > connector->display_info.bpc here as well.
> > 
> > >  	enum drm_mode_status ret;
> > >  
> > >  	if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp->caps.dp_interlace)
> > >  		return MODE_NO_INTERLACE;
> > >  
> > >  	max_clock = outp->dp.link_nr * outp->dp.link_bw;
> > > -	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd,
> > > -						  outp->dp.downstream_ports);
> > > -	if (ds_clock)
> > > -		max_clock = min(max_clock, ds_clock);
> > > -
> > > -	clock = mode->clock * (connector->display_info.bpc * 3) / 10;
> > > -	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> > > -					    &clock);
> > > +	clock = mode->clock * bpp / 8;
> > > +	if (clock > max_clock)
> > > +		return MODE_CLOCK_HIGH;
> > 
> > This stuff vs. nouveau_conn_mode_clock_valid() still seems a bit messy.
> > The max_clock you pass to nouveau_conn_mode_clock_valid() is the max
> > symbol clock, but nouveau_conn_mode_clock_valid() checks it against the
> > dotclock. Also only nouveau_conn_mode_clock_valid() has any kind of
> > stereo 3D handling, but AFAICS stereo_allowed is also set for DP?
> 
> ...not sure I'm following you here, it's set to true for DP so don't we want
> to check it and adjust the pixel clock we output accordingly?

Yes, but then you need to also double your your pixel clock
derived values in this function. Ie. all the mode->clock
needs to become mode->clock*2 when dealing with a 3D frame
packing mode.

> 
> > 
> > > +
> > > +	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp-
> > > >dp.downstream_ports);
> > > +	if (ds_clock && mode->clock > ds_clock)
> > > +		return MODE_CLOCK_HIGH;
> > > +
> > > +	ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock,
> > > &clock);
> > >  	if (out_clock)
> > >  		*out_clock = clock;
> > > +
> > >  	return ret;
> > >  }
> > > -- 
> > > 2.26.2
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Software Engineer at Red Hat

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Lyude Paul]

On Tue, 2020-09-29 at 21:09 +0300, Ville Syrjälä wrote:
> On Tue, Sep 29, 2020 at 01:54:13PM -0400, Lyude Paul wrote:
> > On Mon, 2020-09-28 at 16:01 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 22, 2020 at 05:05:10PM -0400, Lyude Paul wrote:
> > > > While I thought I had this correct (since it actually did reject modes
> > > > like I expected during testing), Ville Syrjala from Intel pointed out
> > > > that the logic here isn't correct. max_clock refers to the max symbol
> > > > rate supported by the encoder, so limiting clock to ds_clock using
> > > > max()
> > > > doesn't make sense. Additionally, we want to check against 6bpc for
> > > > the
> > > > time being since that's the minimum possible bpc here, not the
> > > > reported
> > > > bpc from the connector. See:
> > > > 
> > > > https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html
> > > > 
> > > > For more info.
> > > > 
> > > > So, let's rewrite this using Ville's advice.
> > > > 
> > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock
> > > > limits for mode validation")
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > > ---
> > > >  drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++----------
> > > >  1 file changed, 13 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > > b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > > index 7b640e05bd4cd..24c81e423d349 100644
> > > > --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > > +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> > > > @@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector
> > > > *connector,
> > > >  		   const struct drm_display_mode *mode,
> > > >  		   unsigned *out_clock)
> > > >  {
> > > > -	const unsigned min_clock = 25000;
> > > > -	unsigned max_clock, ds_clock, clock;
> > > > +	const unsigned int min_clock = 25000;
> > > > +	unsigned int max_clock, ds_clock, clock;
> > > > +	const u8 bpp = 18; /* 6 bpc */
> > > 
> > > AFAICS nv50_outp_atomic_check() and nv50_msto_atomic_check()
> > > just blindly use connector->display_info.bpc without any fallback
> > > logic to lower the bpc. So Ilia's concerns seem well founded.
> > > Without that logic I guess you should just use
> > > connector->display_info.bpc here as well.
> > > 
> > > >  	enum drm_mode_status ret;
> > > >  
> > > >  	if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp-
> > > > >caps.dp_interlace)
> > > >  		return MODE_NO_INTERLACE;
> > > >  
> > > >  	max_clock = outp->dp.link_nr * outp->dp.link_bw;
> > > > -	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd,
> > > > -						  outp-
> > > > >dp.downstream_ports);
> > > > -	if (ds_clock)
> > > > -		max_clock = min(max_clock, ds_clock);
> > > > -
> > > > -	clock = mode->clock * (connector->display_info.bpc * 3) / 10;
> > > > -	ret = nouveau_conn_mode_clock_valid(mode, min_clock,
> > > > max_clock,
> > > > -					    &clock);
> > > > +	clock = mode->clock * bpp / 8;
> > > > +	if (clock > max_clock)
> > > > +		return MODE_CLOCK_HIGH;
> > > 
> > > This stuff vs. nouveau_conn_mode_clock_valid() still seems a bit messy.
> > > The max_clock you pass to nouveau_conn_mode_clock_valid() is the max
> > > symbol clock, but nouveau_conn_mode_clock_valid() checks it against the
> > > dotclock. Also only nouveau_conn_mode_clock_valid() has any kind of
> > > stereo 3D handling, but AFAICS stereo_allowed is also set for DP?
> > 
> > ...not sure I'm following you here, it's set to true for DP so don't we
> > want
> > to check it and adjust the pixel clock we output accordingly?
> 
> Yes, but then you need to also double your your pixel clock
> derived values in this function. Ie. all the mode->clock
> needs to become mode->clock*2 when dealing with a 3D frame
> packing mode.

oh this is a good point, thanks for noticing. I guess I'll get to changing
this around once I start working on the rest of the bpp stuff, since I'd
rather get this fixed first (I doubt many folks are using nouveau for 3D right
now, but if other nouveau folks disagree i'm happy to change my mind)
> 
> > > > +
> > > > +	ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp-
> > > > > dp.downstream_ports);
> > > > +	if (ds_clock && mode->clock > ds_clock)
> > > > +		return MODE_CLOCK_HIGH;
> > > > +
> > > > +	ret = nouveau_conn_mode_clock_valid(mode, min_clock,
> > > > max_clock,
> > > > &clock);
> > > >  	if (out_clock)
> > > >  		*out_clock = clock;
> > > > +
> > > >  	return ret;
> > > >  }
> > > > -- 
> > > > 2.26.2
> > -- 
> > Cheers,
> > 	Lyude Paul (she/her)
> > 	Software Engineer at Red Hat
-- 
Cheers,
	Lyude Paul (she/her)
	Software Engineer at Red Hat