Message-ID: <CAF6AEGtzfEFpr610c8TdLAH_zEYEtXzt61ywQTcFbvCOj-G9-A@mail.gmail.com>
Date:   Fri, 25 Aug 2023 11:15:26 -0700
From:   Rob Clark <robdclark@...il.com>
To:     Helen Mae Koike Fornazier <helen.koike@...labora.com>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Vignesh Raman <vignesh.raman@...labora.com>,
        dri-devel@...ts.freedesktop.org, emma@...olt.net,
        linux-doc@...r.kernel.org, david.heidelberg@...labora.com,
        linux-amlogic@...ts.infradead.org, jbrunet@...libre.com,
        robdclark@...gle.com, corbet@....net, khilman@...libre.com,
        sergi.blanch.torne@...labora.com, gustavo.padovan@...labora.com,
        linux-rockchip@...ts.infradead.org, daniels@...labora.com,
        martin.blumenstingl@...glemail.com, robclark@...edesktop.org,
        anholt@...gle.com, linux-mediatek@...ts.infradead.org,
        mripard@...nel.org, matthias.bgg@...il.com,
        linux-arm-kernel@...ts.infradead.org,
        angelogioacchino.delregno@...labora.com, neil.armstrong@...aro.org,
        guilherme.gallo@...labora.com, linux-kernel@...r.kernel.org,
        tzimmermann@...e.de
Subject: Re: [PATCH 2/6] drm: ci: Force db410c to host mode

On Fri, Aug 25, 2023 at 8:06 AM Helen Mae Koike Fornazier
<helen.koike@...labora.com> wrote:
>
> On Friday, August 25, 2023 11:41 -03, Rob Clark <robdclark@...il.com> wrote:
>
> > On Fri, Aug 25, 2023 at 7:34 AM Helen Mae Koike Fornazier
> > <helen.koike@...labora.com> wrote:
> > >
> > > On Friday, August 25, 2023 11:30 -03, Rob Clark <robdclark@...il.com> wrote:
> > >
> > > > On Fri, Aug 25, 2023 at 6:56 AM Jani Nikula <jani.nikula@...ux.intel.com> wrote:
> > > > >
> > > > > On Fri, 25 Aug 2023, Vignesh Raman <vignesh.raman@...labora.com> wrote:
> > > > > > Force db410c to host mode to fix network issue which results in failure
> > > > > > to mount root fs via NFS.
> > > > > > See https://gitlab.freedesktop.org/gfx-ci/linux/-/commit/cb72a629b8c15c80a54dda510743cefd1c4b65b8
> > > > > >
> > > > > > Since this fix is not sent upstream, add it to build.sh script
> > > > > > before building the kernel and dts. Better approach would be
> > > > > > to use devicetree overlays.
> > > > > >
> > > > > > Signed-off-by: Vignesh Raman <vignesh.raman@...labora.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/ci/build.sh | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
> > > > > > index 7b014287a041..c39834bd6bd7 100644
> > > > > > --- a/drivers/gpu/drm/ci/build.sh
> > > > > > +++ b/drivers/gpu/drm/ci/build.sh
> > > > > > @@ -70,6 +70,10 @@ if [ -z "$CI_MERGE_REQUEST_PROJECT_PATH" ]; then
> > > > > >      fi
> > > > > >  fi
> > > > > >
> > > > > > +# Force db410c to host mode to fix network issue which results in failure to mount root fs via NFS.
> > > > > > +# See https://gitlab.freedesktop.org/gfx-ci/linux/-/commit/cb72a629b8c15c80a54dda510743cefd1c4b65b8
> > > > > > +sed -i '/&usb {/,/status = "okay";/s/status = "okay";/&\n\tdr_mode = "host";/' arch/arm64/boot/dts/qcom/apq8016-sbc.dts
> > > > > > +
> > > > >
> > > > > It seems like a really bad idea to me to have the CI build modify the
> > > > > source tree before building.
> > > > >
> > > > > The kernel being built will have a dirty git repo, and the localversion
> > > > > will have -dirty in it.
> > > > >
> > > > > I think it would be better to do out-of-tree builds and assume the
> > > > > source is read-only.
> > > >
> > > > We have the ${target_branch}-external-fixes mechanism to merge
> > > > necessary changes before building the kernel for CI.  Which is
> > > > necessary for a couple of reasons:
> > >
> > > Should we create an official topic/drm-ci-external-fixes branch ?
> >
> > Hmm, maybe.. I guess as we expand this to more driver trees, and want
> > to be able to re-run CI in the drm tree after merges to
> > drm-next/drm-fixes, we maybe want to have central
> > drm-next-external-fixes and drm-fixes-external-fixes.  I guess we can
> > keep those based on drm-next and drm-fixes?  And if there would be
> > conflicts because, say, ${driver}-next is behind drm-next, then
> > ${driver}-next could be rebased on drm-next?
> >
>
> tbh this is one of the reasons I would prefer in-code fixes instead of
> commits on a -external-fixes branch, since it seems things start to become
> complex to manage all different trees for people executing ci tests
> on different history points, but I don't oppose going for -external-fixes
> either.

If by in-code you mean in the same branch that we are running CI on, I
think that will be difficult to do without having force-pushes later
to remove the unrelated patch.

It doesn't happen every release, but sometimes there is an issue
requiring a fix outside of drm, which really shouldn't land via
drm-next.  In some cases even, we might need a temporary hack fix when
a proper solution is still being worked out.  Since the kernel
development is unlike mesa, where we have a single main branch and CI
runs on every change entering that main branch, we are going to have
to deal occasionally with breakage that comes in via another tree that
isn't running CI.  But IME so far with msm-next-external-fixes, it
hasn't been so bad.. I haven't even had to rebase it every kernel
cycle.  (If the fix cherry-picked into -external-fixes from a previous
release cycle already exists in msm-next then the merge of that commit
is a no-op.)

BR,
-R

> Regards,
> Helen
>
> > BR,
> > -R
> >
> > > Regards,
> > > Helen
> > >
> > > >
> > > > 1) patches like this which aren't appropriate upstream but necessary
> > > > due to the CI lab setup
> > > > 2) target branch if often based on an early -rc, and it isn't unheard
> > > > of to need some fix for some board or another which isn't appropriate
> > > > to land via drm-next
> > > >
> > > > We should use the -external-fixes branch mechanism for patches like this one.
> > > >
> > > > BR,
> > > > -R
> > > >
> > > > > >  for opt in $ENABLE_KCONFIGS; do
> > > > > >    echo CONFIG_$opt=y >> drivers/gpu/drm/ci/${KERNEL_ARCH}.config
> > > > > >  done
> > > > >
> > > > > Ditto for the config changes in the context here. Those are files in
> > > > > git, don't change them.
> > > > >
> > > > > Shouldn't this use something like 'scripts/config --enable' or
> > > > > 'scripts/config --disable' on the .config file to be used for building
> > > > > instead?
> > > > >
> > > > >
> > > > > BR,
> > > > > Jani.
> > > > >
> > > > >
> > > > > --
> > > > > Jani Nikula, Intel Open Source Graphics Center
> > >
>

Powered by blists - more mailing lists