Re: [PATCH v3 033/105] drm/vc4: crtc: Assign output to channel automatically

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