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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200616150449.xkgyxd6bwcp3cfkb@gilmour.lan>
Date:   Tue, 16 Jun 2020 17:04:49 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Eric Anholt <eric@...olt.net>
Cc:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        linux-rpi-kernel@...ts.infradead.org,
        bcm-kernel-feedback-list@...adcom.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Tim Gover <tim.gover@...pberrypi.com>,
        Phil Elwell <phil@...pberrypi.com>
Subject: 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@...no.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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)