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 [off-list ref] wrote:

quoted

 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