[PATCH v2 0/2] drm/msm/gen_header: allow skipping the validation

[PATCH v2 0/2] drm/msm/gen_header: allow skipping the validation

[Dmitry Baryshkov]

In order to remove pointless messages regarding missing lxml, skip
validation of MSM register files against the schema. Only the driver
developers really care and/or can fix the files.

Keep the validation enabled during one of DRM CI stages, so that we
still catch errors, introduced by mistake.

To: Rob Clark <robdclark@gmail.com>
To: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Sean Paul <sean@poorly.run>
To: Marijn Suijten <marijn.suijten@somainline.org>
To: David Airlie <airlied@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
To: Helen Koike <helen.koike@collabora.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: linux-arm-msm@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedreno@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Changes in v2:
- added validation of XML files agains schema in in DRM CI
- Link to v1: https://lore.kernel.org/r/20240409-fd-fix-lxml-v1-1-e5c300d6c1c8@linaro.org

---
Dmitry Baryshkov (2):
      drm/msm/gen_header: allow skipping the validation
      drm/ci: validate drm/msm XML register files against schema

 drivers/gpu/drm/ci/build.sh                 |  3 +++
 drivers/gpu/drm/ci/build.yml                |  1 +
 drivers/gpu/drm/msm/Kconfig                 |  8 ++++++++
 drivers/gpu/drm/msm/Makefile                |  9 ++++++++-
 drivers/gpu/drm/msm/registers/gen_header.py | 14 +++++++++++---
 5 files changed, 31 insertions(+), 4 deletions(-)
---
base-commit: 104e548a7c97da24224b375632fca0fc8b64c0db
change-id: 20240409-fd-fix-lxml-eecc6949f64e

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v2 1/2] drm/msm/gen_header: allow skipping the validation

[Dmitry Baryshkov]

We don't need to run the validation of the XML files if we are just
compiling the kernel. Skip the validation unless the user enables
corresponding Kconfig option. This removes a warning from gen_header.py
about lxml being not installed.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/all/20240409120108.2303d0bd@canb.auug.org.au/
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/Kconfig                 |  8 ++++++++
 drivers/gpu/drm/msm/Makefile                |  9 ++++++++-
 drivers/gpu/drm/msm/registers/gen_header.py | 14 +++++++++++---
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index f202f26adab2..4c9bf237d4a2 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -54,6 +54,14 @@ config DRM_MSM_GPU_SUDO
 	  Only use this if you are a driver developer.  This should *not*
 	  be enabled for production kernels.  If unsure, say N.
 
+config DRM_MSM_VALIDATE_XML
+	bool "Validate XML register files against schema"
+	depends on DRM_MSM && EXPERT
+	depends on $(success,$(PYTHON3) -c "import lxml")
+	help
+	  Validate XML files with register definitions against rules-fd schema.
+	  This option is mostly targeting DRM MSM developers. If unsure, say N.
+
 config DRM_MSM_MDSS
 	bool
 	depends on DRM_MSM
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index c861de58286c..718968717ad5 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -156,8 +156,15 @@ msm-y += $(adreno-y) $(msm-display-y)
 
 obj-$(CONFIG_DRM_MSM)	+= msm.o
 
+ifeq (y,$(CONFIG_DRM_MSM_VALIDATE_XML))
+	headergen-opts += --validate
+else
+	headergen-opts += --no-validate
+endif
+
 quiet_cmd_headergen = GENHDR  $@
-      cmd_headergen = mkdir -p $(obj)/generated && $(PYTHON3) $(srctree)/$(src)/registers/gen_header.py --rnn $(srctree)/$(src)/registers --xml $< c-defines > $@
+      cmd_headergen = mkdir -p $(obj)/generated && $(PYTHON3) $(srctree)/$(src)/registers/gen_header.py \
+		      $(headergen-opts) --rnn $(srctree)/$(src)/registers --xml $< c-defines > $@
 
 $(obj)/generated/%.xml.h: $(src)/registers/adreno/%.xml \
 		$(src)/registers/adreno/adreno_common.xml \
diff --git a/drivers/gpu/drm/msm/registers/gen_header.py b/drivers/gpu/drm/msm/registers/gen_header.py
index 90d5c2991d05..fc3bfdc991d2 100644
--- a/drivers/gpu/drm/msm/registers/gen_header.py
+++ b/drivers/gpu/drm/msm/registers/gen_header.py
@@ -538,6 +538,9 @@ class Parser(object):
 		self.variants.add(reg.domain)
 
 	def do_validate(self, schemafile):
+		if self.validate == False:
+			return
+
 		try:
 			from lxml import etree
 
@@ -567,7 +570,10 @@ class Parser(object):
 			if not xmlschema.validate(xml_doc):
 				error_str = str(xmlschema.error_log.filter_from_errors()[0])
 				raise self.error("Schema validation failed for: " + filename + "\n" + error_str)
-		except ImportError:
+		except ImportError as e:
+			if self.validate:
+				raise e
+
 			print("lxml not found, skipping validation", file=sys.stderr)
 
 	def do_parse(self, filename):
@@ -586,9 +592,10 @@ class Parser(object):
 		self.stack.pop()
 		file.close()
 
-	def parse(self, rnn_path, filename):
+	def parse(self, rnn_path, filename, validate):
 		self.path = rnn_path
 		self.stack = []
+		self.validate = validate
 		self.do_parse(filename)
 
 	def parse_reg(self, attrs, bit_size):
@@ -853,7 +860,7 @@ def dump_c(args, guard, func):
 	p = Parser()
 
 	try:
-		p.parse(args.rnn, args.xml)
+		p.parse(args.rnn, args.xml, args.validate)
 	except Error as e:
 		print(e, file=sys.stderr)
 		exit(1)
@@ -941,6 +948,7 @@ def main():
 	parser = argparse.ArgumentParser()
 	parser.add_argument('--rnn', type=str, required=True)
 	parser.add_argument('--xml', type=str, required=True)
+	parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
 
 	subparsers = parser.add_subparsers()
 	subparsers.required = True

-- 
2.39.2

Re: [PATCH v2 1/2] drm/msm/gen_header: allow skipping the validation

[Abhinav Kumar]

On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote:
> We don't need to run the validation of the XML files if we are just
> compiling the kernel. Skip the validation unless the user enables
> corresponding Kconfig option. This removes a warning from gen_header.py
> about lxml being not installed.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Closes: https://lore.kernel.org/all/20240409120108.2303d0bd@canb.auug.org.au/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/Kconfig                 |  8 ++++++++
>   drivers/gpu/drm/msm/Makefile                |  9 ++++++++-
>   drivers/gpu/drm/msm/registers/gen_header.py | 14 +++++++++++---
>   3 files changed, 27 insertions(+), 4 deletions(-)
> 

Looks reasonable to me, only developers need to worry about or fix the 
xml files

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH v2 1/2] drm/msm/gen_header: allow skipping the validation

[Doug Anderson]

Hi,

On Fri, May 3, 2024 at 11:15 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> @@ -941,6 +948,7 @@ def main():
>         parser = argparse.ArgumentParser()
>         parser.add_argument('--rnn', type=str, required=True)
>         parser.add_argument('--xml', type=str, required=True)
> +       parser.add_argument('--validate', action=argparse.BooleanOptionalAction)

FWIW, the above (argparse.BooleanOptionalAction) appears to be a
python 3.9 thing. My own build environment happens to have python3
default to python 3.8 and thus I get a build error related to this. I
have no idea what the kernel usually assumes for a baseline, but
others might get build errors too. I don't even see python listed in:

https://docs.kernel.org/process/changes.html

...in any case, if it's easy to change this to not require python3.9
that would at least help for my build environment. :-P

-Doug

Re: [PATCH v2 1/2] drm/msm/gen_header: allow skipping the validation

[Abhinav Kumar]

On 5/8/2024 3:41 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 3, 2024 at 11:15 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> @@ -941,6 +948,7 @@ def main():
>>          parser = argparse.ArgumentParser()
>>          parser.add_argument('--rnn', type=str, required=True)
>>          parser.add_argument('--xml', type=str, required=True)
>> +       parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
> 
> FWIW, the above (argparse.BooleanOptionalAction) appears to be a
> python 3.9 thing. My own build environment happens to have python3
> default to python 3.8 and thus I get a build error related to this. I
> have no idea what the kernel usually assumes for a baseline, but
> others might get build errors too. I don't even see python listed in:
> 
> https://docs.kernel.org/process/changes.html
> 
> ...in any case, if it's easy to change this to not require python3.9
> that would at least help for my build environment. :-P
> 

Yes, I had posted this y'day as I also ran into this

https://patchwork.freedesktop.org/patch/593057/


> -Doug

[PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema

[Dmitry Baryshkov]

In order to validate drm/msm register definition files against schema,
reuse the nodebugfs build step. The validation entry is guarded by
the EXPERT Kconfig option and we don't want to enable that option for
all the builds.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/ci/build.sh  | 3 +++
 drivers/gpu/drm/ci/build.yml | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
index 106f2d40d222..28a495c0c39c 100644
--- a/drivers/gpu/drm/ci/build.sh
+++ b/drivers/gpu/drm/ci/build.sh
@@ -12,6 +12,9 @@ rm -rf .git/rebase-apply
 apt-get update
 apt-get install -y libssl-dev
 
+# for msm header validation
+apt-get install -y python3-lxml
+
 if [[ "$KERNEL_ARCH" = "arm64" ]]; then
     GCC_ARCH="aarch64-linux-gnu"
     DEBIAN_ARCH="arm64"
diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml
index 17ab38304885..9c198239033d 100644
--- a/drivers/gpu/drm/ci/build.yml
+++ b/drivers/gpu/drm/ci/build.yml
@@ -106,6 +106,7 @@ build-nodebugfs:arm64:
   extends: .build:arm64
   variables:
     DISABLE_KCONFIGS: "DEBUG_FS"
+    ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML"
 
 build:x86_64:
   extends: .build:x86_64

-- 
2.39.2

Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema

[Abhinav Kumar]

On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote:
> In order to validate drm/msm register definition files against schema,
> reuse the nodebugfs build step. The validation entry is guarded by
> the EXPERT Kconfig option and we don't want to enable that option for
> all the builds.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/ci/build.sh  | 3 +++
>   drivers/gpu/drm/ci/build.yml | 1 +
>   2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
> index 106f2d40d222..28a495c0c39c 100644
> --- a/drivers/gpu/drm/ci/build.sh
> +++ b/drivers/gpu/drm/ci/build.sh
> @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply
>   apt-get update
>   apt-get install -y libssl-dev
>   
> +# for msm header validation
> +apt-get install -y python3-lxml
> +
>   if [[ "$KERNEL_ARCH" = "arm64" ]]; then
>       GCC_ARCH="aarch64-linux-gnu"
>       DEBIAN_ARCH="arm64"
> diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml
> index 17ab38304885..9c198239033d 100644
> --- a/drivers/gpu/drm/ci/build.yml
> +++ b/drivers/gpu/drm/ci/build.yml
> @@ -106,6 +106,7 @@ build-nodebugfs:arm64:
>     extends: .build:arm64
>     variables:
>       DISABLE_KCONFIGS: "DEBUG_FS"
> +    ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML"
>   

Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device.

Cant we make this build rule msm specific?

Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema

[Dmitry Baryshkov]

On Fri, 3 May 2024 at 22:42, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote:
> > In order to validate drm/msm register definition files against schema,
> > reuse the nodebugfs build step. The validation entry is guarded by
> > the EXPERT Kconfig option and we don't want to enable that option for
> > all the builds.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/ci/build.sh  | 3 +++
> >   drivers/gpu/drm/ci/build.yml | 1 +
> >   2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
> > index 106f2d40d222..28a495c0c39c 100644
> > --- a/drivers/gpu/drm/ci/build.sh
> > +++ b/drivers/gpu/drm/ci/build.sh
> > @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply
> >   apt-get update
> >   apt-get install -y libssl-dev
> >
> > +# for msm header validation
> > +apt-get install -y python3-lxml
> > +
> >   if [[ "$KERNEL_ARCH" = "arm64" ]]; then
> >       GCC_ARCH="aarch64-linux-gnu"
> >       DEBIAN_ARCH="arm64"
> > diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml
> > index 17ab38304885..9c198239033d 100644
> > --- a/drivers/gpu/drm/ci/build.yml
> > +++ b/drivers/gpu/drm/ci/build.yml
> > @@ -106,6 +106,7 @@ build-nodebugfs:arm64:
> >     extends: .build:arm64
> >     variables:
> >       DISABLE_KCONFIGS: "DEBUG_FS"
> > +    ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML"
> >
>
> Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device.
>
> Cant we make this build rule msm specific?

No need to. We just need to validate the files at least once during
the whole pipeline build.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema

[Abhinav Kumar]

On 5/3/2024 1:20 PM, Dmitry Baryshkov wrote:
> On Fri, 3 May 2024 at 22:42, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote:
>>> In order to validate drm/msm register definition files against schema,
>>> reuse the nodebugfs build step. The validation entry is guarded by
>>> the EXPERT Kconfig option and we don't want to enable that option for
>>> all the builds.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/ci/build.sh  | 3 +++
>>>    drivers/gpu/drm/ci/build.yml | 1 +
>>>    2 files changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
>>> index 106f2d40d222..28a495c0c39c 100644
>>> --- a/drivers/gpu/drm/ci/build.sh
>>> +++ b/drivers/gpu/drm/ci/build.sh
>>> @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply
>>>    apt-get update
>>>    apt-get install -y libssl-dev
>>>
>>> +# for msm header validation
>>> +apt-get install -y python3-lxml
>>> +
>>>    if [[ "$KERNEL_ARCH" = "arm64" ]]; then
>>>        GCC_ARCH="aarch64-linux-gnu"
>>>        DEBIAN_ARCH="arm64"
>>> diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml
>>> index 17ab38304885..9c198239033d 100644
>>> --- a/drivers/gpu/drm/ci/build.yml
>>> +++ b/drivers/gpu/drm/ci/build.yml
>>> @@ -106,6 +106,7 @@ build-nodebugfs:arm64:
>>>      extends: .build:arm64
>>>      variables:
>>>        DISABLE_KCONFIGS: "DEBUG_FS"
>>> +    ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML"
>>>
>>
>> Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device.
>>
>> Cant we make this build rule msm specific?
> 
> No need to. We just need to validate the files at least once during
> the whole pipeline build.
>

ah okay, today the arm64 config anyway sets all arm64 vendor drm configs 
to y.

A couple of more questions:

1) Why is this enabled only for no-debugfs option?
2) Will there be any concerns from other vendors to enable CONFIG_EXPERT 
in their CI runs as the arm64 config is shared across all arm64 vendors.

Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema

[Dmitry Baryshkov]

On Sat, 4 May 2024 at 01:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/3/2024 1:20 PM, Dmitry Baryshkov wrote:
> > On Fri, 3 May 2024 at 22:42, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote:
> >>> In order to validate drm/msm register definition files against schema,
> >>> reuse the nodebugfs build step. The validation entry is guarded by
> >>> the EXPERT Kconfig option and we don't want to enable that option for
> >>> all the builds.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>    drivers/gpu/drm/ci/build.sh  | 3 +++
> >>>    drivers/gpu/drm/ci/build.yml | 1 +
> >>>    2 files changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
> >>> index 106f2d40d222..28a495c0c39c 100644
> >>> --- a/drivers/gpu/drm/ci/build.sh
> >>> +++ b/drivers/gpu/drm/ci/build.sh
> >>> @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply
> >>>    apt-get update
> >>>    apt-get install -y libssl-dev
> >>>
> >>> +# for msm header validation
> >>> +apt-get install -y python3-lxml
> >>> +
> >>>    if [[ "$KERNEL_ARCH" = "arm64" ]]; then
> >>>        GCC_ARCH="aarch64-linux-gnu"
> >>>        DEBIAN_ARCH="arm64"
> >>> diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml
> >>> index 17ab38304885..9c198239033d 100644
> >>> --- a/drivers/gpu/drm/ci/build.yml
> >>> +++ b/drivers/gpu/drm/ci/build.yml
> >>> @@ -106,6 +106,7 @@ build-nodebugfs:arm64:
> >>>      extends: .build:arm64
> >>>      variables:
> >>>        DISABLE_KCONFIGS: "DEBUG_FS"
> >>> +    ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML"
> >>>
> >>
> >> Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device.
> >>
> >> Cant we make this build rule msm specific?
> >
> > No need to. We just need to validate the files at least once during
> > the whole pipeline build.
> >
>
> ah okay, today the arm64 config anyway sets all arm64 vendor drm configs
> to y.
>
> A couple of more questions:
>
> 1) Why is this enabled only for no-debugfs option?
> 2) Will there be any concerns from other vendors to enable CONFIG_EXPERT
> in their CI runs as the arm64 config is shared across all arm64 vendors.

I don't get the second question. This option is only enabled for
no-debugfs, which isn't used for execution.

I didn't want to add an extra build stage, just for the sake of
validating regs against the schema, nor did I want EXPERT to find its
way into the actual running kernels.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema

[Abhinav Kumar]

On 5/3/2024 5:02 PM, Dmitry Baryshkov wrote:
> On Sat, 4 May 2024 at 01:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/3/2024 1:20 PM, Dmitry Baryshkov wrote:
>>> On Fri, 3 May 2024 at 22:42, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote:
>>>>> In order to validate drm/msm register definition files against schema,
>>>>> reuse the nodebugfs build step. The validation entry is guarded by
>>>>> the EXPERT Kconfig option and we don't want to enable that option for
>>>>> all the builds.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/ci/build.sh  | 3 +++
>>>>>     drivers/gpu/drm/ci/build.yml | 1 +
>>>>>     2 files changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
>>>>> index 106f2d40d222..28a495c0c39c 100644
>>>>> --- a/drivers/gpu/drm/ci/build.sh
>>>>> +++ b/drivers/gpu/drm/ci/build.sh
>>>>> @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply
>>>>>     apt-get update
>>>>>     apt-get install -y libssl-dev
>>>>>
>>>>> +# for msm header validation
>>>>> +apt-get install -y python3-lxml
>>>>> +
>>>>>     if [[ "$KERNEL_ARCH" = "arm64" ]]; then
>>>>>         GCC_ARCH="aarch64-linux-gnu"
>>>>>         DEBIAN_ARCH="arm64"
>>>>> diff --git a/drivers/gpu/drm/ci/build.yml b/drivers/gpu/drm/ci/build.yml
>>>>> index 17ab38304885..9c198239033d 100644
>>>>> --- a/drivers/gpu/drm/ci/build.yml
>>>>> +++ b/drivers/gpu/drm/ci/build.yml
>>>>> @@ -106,6 +106,7 @@ build-nodebugfs:arm64:
>>>>>       extends: .build:arm64
>>>>>       variables:
>>>>>         DISABLE_KCONFIGS: "DEBUG_FS"
>>>>> +    ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML"
>>>>>
>>>>
>>>> Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 device.
>>>>
>>>> Cant we make this build rule msm specific?
>>>
>>> No need to. We just need to validate the files at least once during
>>> the whole pipeline build.
>>>
>>
>> ah okay, today the arm64 config anyway sets all arm64 vendor drm configs
>> to y.
>>
>> A couple of more questions:
>>
>> 1) Why is this enabled only for no-debugfs option?
>> 2) Will there be any concerns from other vendors to enable CONFIG_EXPERT
>> in their CI runs as the arm64 config is shared across all arm64 vendors.
> 
> I don't get the second question. This option is only enabled for
> no-debugfs, which isn't used for execution.
> 

Ah I see, makes sense.

> I didn't want to add an extra build stage, just for the sake of
> validating regs against the schema, nor did I want EXPERT to find its
> way into the actual running kernels.
> 

This answered my second question actually. That basically I didnt also 
want EXPERT to find its way into actual running kernels.

Hence, I am fine with this change now

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

But, I will wait to hear from helen, vignesh about what they think of this.

Re: [PATCH v2 2/2] drm/ci: validate drm/msm XML register files against schema

[Helen Koike]

On 03/05/2024 21:07, Abhinav Kumar wrote:
> 
> 
> On 5/3/2024 5:02 PM, Dmitry Baryshkov wrote:
>> On Sat, 4 May 2024 at 01:38, Abhinav Kumar <quic_abhinavk@quicinc.com> 
>> wrote:
>>>
>>>
>>>
>>> On 5/3/2024 1:20 PM, Dmitry Baryshkov wrote:
>>>> On Fri, 3 May 2024 at 22:42, Abhinav Kumar 
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 5/3/2024 11:15 AM, Dmitry Baryshkov wrote:
>>>>>> In order to validate drm/msm register definition files against 
>>>>>> schema,
>>>>>> reuse the nodebugfs build step. The validation entry is guarded by
>>>>>> the EXPERT Kconfig option and we don't want to enable that option for
>>>>>> all the builds.
>>>>>>
>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> ---
>>>>>>     drivers/gpu/drm/ci/build.sh  | 3 +++
>>>>>>     drivers/gpu/drm/ci/build.yml | 1 +
>>>>>>     2 files changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/ci/build.sh 
>>>>>> b/drivers/gpu/drm/ci/build.sh
>>>>>> index 106f2d40d222..28a495c0c39c 100644
>>>>>> --- a/drivers/gpu/drm/ci/build.sh
>>>>>> +++ b/drivers/gpu/drm/ci/build.sh
>>>>>> @@ -12,6 +12,9 @@ rm -rf .git/rebase-apply
>>>>>>     apt-get update
>>>>>>     apt-get install -y libssl-dev
>>>>>>
>>>>>> +# for msm header validation
>>>>>> +apt-get install -y python3-lxml
>>>>>> +
>>>>>>     if [[ "$KERNEL_ARCH" = "arm64" ]]; then
>>>>>>         GCC_ARCH="aarch64-linux-gnu"
>>>>>>         DEBIAN_ARCH="arm64"
>>>>>> diff --git a/drivers/gpu/drm/ci/build.yml 
>>>>>> b/drivers/gpu/drm/ci/build.yml
>>>>>> index 17ab38304885..9c198239033d 100644
>>>>>> --- a/drivers/gpu/drm/ci/build.yml
>>>>>> +++ b/drivers/gpu/drm/ci/build.yml
>>>>>> @@ -106,6 +106,7 @@ build-nodebugfs:arm64:
>>>>>>       extends: .build:arm64
>>>>>>       variables:
>>>>>>         DISABLE_KCONFIGS: "DEBUG_FS"
>>>>>> +    ENABLE_KCONFIGS: "EXPERT DRM_MSM_VALIDATE_XML"
>>>>>>
>>>>>
>>>>> Wouldnt this end up enabling DRM_MSM_VALIDATE_XML for any arm64 
>>>>> device.
>>>>>
>>>>> Cant we make this build rule msm specific?
>>>>
>>>> No need to. We just need to validate the files at least once during
>>>> the whole pipeline build.
>>>>
>>>
>>> ah okay, today the arm64 config anyway sets all arm64 vendor drm configs
>>> to y.
>>>
>>> A couple of more questions:
>>>
>>> 1) Why is this enabled only for no-debugfs option?
>>> 2) Will there be any concerns from other vendors to enable CONFIG_EXPERT
>>> in their CI runs as the arm64 config is shared across all arm64 vendors.
>>
>> I don't get the second question. This option is only enabled for
>> no-debugfs, which isn't used for execution.
>>
> 
> Ah I see, makes sense.
> 
>> I didn't want to add an extra build stage, just for the sake of
>> validating regs against the schema, nor did I want EXPERT to find its
>> way into the actual running kernels.
>>
> 
> This answered my second question actually. That basically I didnt also 
> want EXPERT to find its way into actual running kernels.
> 
> Hence, I am fine with this change now
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> But, I will wait to hear from helen, vignesh about what they think of this.


lgfm

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

Re: [PATCH v2 0/2] drm/msm/gen_header: allow skipping the validation

[Dmitry Baryshkov]

On Fri, 3 May 2024 at 21:15, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> In order to remove pointless messages regarding missing lxml, skip
> validation of MSM register files against the schema. Only the driver
> developers really care and/or can fix the files.
>
> Keep the validation enabled during one of DRM CI stages, so that we
> still catch errors, introduced by mistake.

Helen, could you please ack merging the second patch through drm/msm tree?

> ---
> Dmitry Baryshkov (2):
>       drm/msm/gen_header: allow skipping the validation
>       drm/ci: validate drm/msm XML register files against schema

-- 
With best wishes
Dmitry

Re: [PATCH v2 0/2] drm/msm/gen_header: allow skipping the validation

[Helen Koike]

On 03/05/2024 17:23, Dmitry Baryshkov wrote:
> On Fri, 3 May 2024 at 21:15, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> In order to remove pointless messages regarding missing lxml, skip
>> validation of MSM register files against the schema. Only the driver
>> developers really care and/or can fix the files.
>>
>> Keep the validation enabled during one of DRM CI stages, so that we
>> still catch errors, introduced by mistake.
> 
> Helen, could you please ack merging the second patch through drm/msm tree?

Done.

> 
>> ---
>> Dmitry Baryshkov (2):
>>        drm/msm/gen_header: allow skipping the validation
>>        drm/ci: validate drm/msm XML register files against schema
>