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@xxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
