Hi Eric,

On Wed, May 27, 2020 at 10:23:23AM -0700, Eric Anholt wrote:
> On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >  static int
> >  vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> >  {
> > -       int ret;
> > +       unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0);
> > +       struct drm_crtc_state *crtc_state;
> > +       struct drm_crtc *crtc;
> > +       int i, ret;
> > +
> > +       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > +               struct vc4_crtc_state *vc4_crtc_state =
> > +                       to_vc4_crtc_state(crtc_state);
> > +               struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> > +               bool is_assigned = false;
> > +               unsigned int channel;
> > +
> > +               if (!crtc_state->active)
> > +                       continue;
> > +
> > +               /*
> > +                * The problem we have to solve here is that we have
> > +                * up to 7 encoders, connected to up to 6 CRTCs.
> > +                *
> > +                * Those CRTCs, depending on the instance, can be
> > +                * routed to 1, 2 or 3 HVS FIFOs, and we need to set
> > +                * the change the muxing between FIFOs and outputs in
> > +                * the HVS accordingly.
> > +                *
> > +                * It would be pretty hard to come up with an
> > +                * algorithm that would generically solve
> > +                * this. However, the current routing trees we support
> > +                * allow us to simplify a bit the problem.
> > +                *
> > +                * Indeed, with the current supported layouts, if we
> > +                * try to assign in the ascending crtc index order the
> > +                * FIFOs, we can't fall into the situation where an
> > +                * earlier CRTC that had multiple routes is assigned
> > +                * one that was the only option for a later CRTC.
> > +                *
> > +                * If the layout changes and doesn't give us that in
> > +                * the future, we will need to have something smarter,
> > +                * but it works so far.
> > +                */
> > +               for_each_set_bit(channel, &unassigned_channels,
> > +                                sizeof(unassigned_channels)) {
> > +
> > +                       if (!(BIT(channel) & vc4_crtc->data->hvs_available_channels))
> > +                               continue;
> > +
> > +                       vc4_crtc_state->assigned_channel = channel;
> > +                       unassigned_channels &= ~BIT(channel);
> > +                       is_assigned = true;
> > +                       break;
> > +               }
> > +
> > +               if (!is_assigned)
> > +                       return -EINVAL;
> 
> I think this logic is just
> 
> int matching_channels = unassigned_channels &
> vc4_crtc->data->hvs_available_channels;
> if (matching_channels) {
>   vc4_crtc_state->assigned_channel = ffs(matching_channels) - 1;
>   unassigned_channels &= ~BIT(channel);
> } else {
>   return -EINVAL;
> }

Thanks for that suggestion (and the others), it indeed works as expected.

> If you're changing the assignment of a channel, I think you're going
> to need to set state->mode_changed or something to trigger a full
> modeset, so we don't try to just rewrite the channel of an existing
> CRTC while scanning out.

Since we won't have any CRTC configuration done outside of atomic_enable
/ atomic_disable, can we really have a case where we would reallocate a
new channel to a CRTC without that CRTC being disabled / enabled?

Maxime