Hi Maxime

On Thu, 17 Sep 2020 at 13:16, Maxime Ripard maxime@cerno.tech wrote:

The vc4 display engine has a first controller called the HVS that will perform the composition of the planes. That HVS has 3 FIFOs and can therefore compose planes for up to three outputs. The timings part is generated through a component called the Pixel Valve, and the BCM2711 has 6 of them.

Thus, the HVS has some bits to control which FIFO gets output to which Pixel Valve. The current code supports that muxing by looking at all the CRTCs in a new DRM atomic state in atomic_check, and given the set of contraints that we have, assigns FIFOs to CRTCs or reject the mode

s/contraints/constraints

entirely. The actual muxing will occur during atomic_commit.

However, that doesn't work if only a fraction of the CRTCs' state is updated in that state, since it will ignore the CRTCs that are kept running unmodified, and will thus unassign its associated FIFO, and later disable it.

Yup, that would surely mess things up :)

In order to make the code work as expected, let's pull the CRTC state of all the enabled CRTC in our atomic_check so that we can operate on all the running CRTCs, no matter whether they are affected by the new state or not.

Cc: Hoegeun Kwon hoegeun.kwon@samsung.com Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard maxime@cerno.tech

Tested-by: Dave Stevenson dave.stevenson@raspberrypi.com Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com

Thanks Dave

---

drivers/gpu/drm/vc4/vc4_kms.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 16e233e1406e..af3ee3dcdab6 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -620,6 +620,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) struct drm_crtc *crtc; int i, ret;

•   /*
  

•    * Since the HVS FIFOs are shared across all the pixelvalves and
  

•    * the TXP (and thus all the CRTCs), we need to pull the current
  

•    * state of all the enabled CRTCs so that an update to a single
  

•    * CRTC still keeps the previous FIFOs enabled and assigned to
  

•    * the same CRTCs, instead of evaluating only the CRTC being
  

•    * modified.
  

•    */
  

•   list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
  

•           if (!crtc->state->enable)
  

•                   continue;
  

•           crtc_state = drm_atomic_get_crtc_state(state, crtc);
  

•           if (IS_ERR(crtc_state))
  

•                   return PTR_ERR(crtc_state);
  

•   }
  

•   for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
            struct vc4_crtc_state *vc4_crtc_state =
                    to_vc4_crtc_state(crtc_state);
  

-- 2.26.2