05:39 MoeIcenowy: karolherbst: congratulation for being a quantum physicist ;-)
09:29 jclsn: Hi, I would like to submit my first patch to the kernel. I was trying out this patch https://patchew.org/linux/20260805153416.8459-1-royalnet026@gmail.com/ on my Rock 5B this morning and realized that the color space is not mappend properly when HDR is enabled.
09:30 jclsn: I found a patch for drm_hdmi_state_helper.c and HDR is now enabled correctly :)
09:31 jclsn: From what I read drm-misc-next is always open, so I can just send in the patch and it's getting merged in the next available merge window?
09:32 jclsn: Should I send in the merge requests via email or use the freedesktop Gitlab?
11:17 CounterPillow: https://docs.kernel.org/process/submitting-patches.html
11:18 CounterPillow: also that patch you linked is the most obviously non-LLM-disclosing-LLM-patch ever, including the way v1 didn't even use git format-patch to produce the diff but had clod produce it (???)
11:25 jclsn: CounterPillow: So you think it is not going to be accepted? It works though
11:26 llyyr: do you understand the code?
11:27 jclsn: Well, it connects the hdmi colorspace property to the connector
11:27 jclsn: I don't understand the whole DRM system though no
11:28 jclsn: I mean if there is a better way to do it, I would hope that maintainers would point to it and not just rejecte the patch
11:28 jclsn: The mentioned patch also hasn't received a reply yet
11:29 llyyr: then report a bug, not submit a patch you don't understand
11:37 jclsn: I rather like to try to understand
11:40 jclsn: I always wanted to understand kernel development and read a few books about it partially, but then I never had a reason to dig deeper, because all devices I had worked
11:41 jclsn: Submitting a small patch is a better start than a whole driver
11:43 jclsn: What about an RFC patch?
11:55 javierm: jclsn: if you tested the mentioned patch, you could provide your Tested-by tag. It might help reviewers to know that it works
11:56 javierm: or did you mean that this patch need other changes in order to work ? If that's the case, you could report to the patch author
11:58 jclsn: javierm: To be clear. I am not the submitter of the above patch. I merely tested it and it successfully enables KDE to offer the HDR toggle. When you toggle HDR on though, the image becomes washed out like behind a white veil.
11:58 jclsn: I then found this addional patch was required to have my monitor correctly display HDR https://pastebin.com/raw/wm42iF1H
11:59 jclsn: Sure, I can add a tested by, but if the above patch is already so controversial I am not sure if this makes sense.
11:59 jclsn: I mean I found mine with Claude too, but I would submit it manually and am open for suggestions on how to improve it.
11:59 jclsn: I also just bought the Bootlin course about the Linux graphics stack. Happy to learn more
12:00 jclsn: It is just a one-liner though really. I would like to make sure though that it doesn't break anything anywhere else.
12:06 jclsn: What I don't understand is why this global hdmi_state_helper needs a patch. Other drivers properly support HDR as well, so I could imagine that this should rather be moved into the driver directly.
12:14 javierm: jclsn: vc4 for example does the check in its struct drm_connector_helper_funcs.atomic_check handler, before calling to the helper you are changing
12:14 javierm: jclsn: https://elixir.bootlin.com/linux/v7.3-rc5/source/drivers/gpu/drm/vc4/vc4_hdmi.c#L495
12:17 javierm: jclsn: which makes sense I think since neither colorspace nor hdr_output_metadata are properties of struct hdmi (which is what the helper checks)
12:25 javierm: jclsn: in other words, I don't think your patch is correct since drm_atomic_helper_connector_hdmi_check() is meant to only check conn_state->hdmi
12:26 jclsn: I understand. Thanks for clearing that up. I am currently searching for the right place in dw_hdmi_rockhip.c, but colorspace or color_space are not mentioned anywhere
12:27 jclsn: It's part of the drm_connector_state though
12:27 jclsn: https://elixir.bootlin.com/linux/v7.3-rc5/source/include/drm/drm_connector.h#L1242
12:28 javierm: jclsn: yes I know, my point was that the helper is scoped to struct hdmi so it was the wrong layer to add the check
12:39 jclsn: javierm: The whole connector_atomit_check() doesn't exist. There is only encoder_atomic_check() which the connector is not being passed to. I could probably get it somehow with container_of(), but I guess it's the wrong function in the first place
12:41 javierm: jclsn: the right place won't be the rockchip driver either I think, since that is just platform glue code
12:41 javierm: jclsn: the driver uses the drivers/gpu/drm/bridge/synopsys/ and that is exposed by the bridge framework
12:42 javierm: so *I think* the correct place to do this would be in the drm_bridge_connector_atomic_check() helper. That's what calls to drm_atomic_helper_connector_hdmi_check()
12:43 javierm: so then it would be similar to what vc4 does
12:43 javierm: jclsn: same for Igor's patch you mentioned. I don't think dw_hdmi_qp_rockchip_bind() is the correct way to attach the colorspace property
12:45 jclsn: javierm: Thanks, let me have a look
12:48 javierm: jclsn: maybe drm_connector_attach_colorspace_property() can be called from drm_bridge_connector_init() ?
12:48 javierm: jclsn: other connector properties are attached there, for example https://elixir.bootlin.com/linux/v7.3-rc5/source/drivers/gpu/drm/display/drm_bridge_connector.c#L1090
12:49 javierm: jclsn: so yeah, I'm not an expert on bridges but AFAICT that will likely be the correct layer to make those changes
12:49 javierm: jclsn: you can post as RFC but please a) make a disclaimer that you were assisted by an LLM and b) try to understand the patches
12:50 javierm: in particular make sure to explain *why* the changes are needed instead of what the patches do
12:55 jclsn: Here seems to be the right place https://elixir.bootlin.com/linux/v7.3-rc5/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2551
12:55 jclsn: I am going to add the check from the vc4 driver there and see if that does the trick
12:56 jclsn: Sure, I am open about this and I am going to use git format-patch for sure or git email
12:56 jclsn: I will try to see if I can find the right place for the other patch as well
12:57 jclsn: I've been waiting for the RK3588 mainline support to be ready for some time now, because the vendor driver quality is of low quality. This is the last fix I need for this to replace my Android TV
12:59 javierm: jclsn: yeah, that seems to be the correct place indeed and the colorspace property attach in https://elixir.bootlin.com/linux/v7.3-rc5/source/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c#L2582
13:02 jclsn: So it's the right place. I just booted the recompiled kernel. Will now see if you are right about the colorspace property :)
13:02 CounterPillow: dw-hdmi is not even used on RK3588 is it?
13:03 CounterPillow: unless I'm confusing where else the qp split is
13:03 jclsn: CounterPillow: Well, the driver source file is called dw_hdmi_rockchip.c
13:03 jclsn: So would be weird if it wasn't
13:04 CounterPillow: yes there is a Rockchip SoC that uses non-qp dw_hdmi
13:04 CounterPillow: RK3568 does for example
13:04 CounterPillow: but RK3588 uses dw_hdmi_qp
13:04 jclsn: Okay great
13:04 javierm: jclsn: look at the of_device_id table
13:05 javierm: so CounterPillow is correct and is for other rockchip SoCs
13:06 javierm: jclsn: anyways, you get the idea. Figure out where is the correct place to make those changes and is OK to post patches if you are sure about the changes
13:07 javierm: what is not OK in my opinion is to post patches (even as RFC) because were made by an LLM and the person posting them has no clue if are correct or not
13:07 javierm: that is a waste of time for everyone involved
13:08 CounterPillow: also search https://lore.kernel.org/all/ to see if someone already posted a patch for this
13:09 javierm: indeed
13:09 jclsn: Okay thanks, will do
13:11 jclsn: I know the amount of LLM patches are overwhelming. We have the same issues in the Mixxx project. The number of pending PRs has gone up form ~200 to ~400 int he last two years. At some point you have to use LLMs yourself to counter them, but we are still reluctant to do so.
13:11 jclsn: To be fair, they can help you get to the solution faster. It is also less enjoyable when you do though.
13:22 jclsn: CounterPillow: There doesn't seem to be a patch for this already
13:23 jclsn: I am doing a clean rebuild now and if everything is good, I am going to file a patch
13:23 jclsn: Should I reply to the already existing one?
13:25 CounterPillow: usually for patch submission, no.
13:27 CounterPillow: if you use tooling like b4 <https://b4.docs.kernel.org/en/latest/contributor/overview.html> (cw: slop logo) you'd declare any dependencies on other patches in b4 prep --edit-deps, but if it doesn't have a "it requires this thing to apply or build" kind dependency you don't need to declare it
13:51 jclsn: CounterPillow: I mean I should still somewhow reference Igor's patch if mine supersedes it, do I not?
13:52 CounterPillow: if yours supersedes it, then yeah you can mention it in the cover letter (the thing after the --- if using format-patch, or b4 prep --edit-cover if using b4)
13:58 jclsn: Okay got it
13:59 jclsn: The Bootlin course is really good
14:00 jclsn: He's starting with 2D Fourier Transforms. Interesting
16:26 jclsn: So I found the right places (I think). The colorspace property had to be attached in the dw_rockhip_qp driver's bind function though
16:26 jclsn: https://pastebin.com/raw/KGeTRM1N
16:26 jclsn: Think is is good to send? I kept the commit messages short. They reflect my limited understanding.
16:27 jclsn: CounterPillow: javierm: Let me know what you think
16:36 jclsn: Ah no, nevermind this is wrong
16:36 jclsn: Now you still have the HDR colorspace when it's disabled
19:41 mareko: GLCTS with zink is the new VKCTS
19:46 HdkR: Two CTS for the price of one
20:43 jclsn: So I found the proper way to implement this I think https://pastebin.com/raw/0tB9yfH4
20:43 jclsn: I am creating a connector_atomic_check and am adding it to the bind
20:44 jclsn: Works as expected now
20:44 jclsn: Not sure if this is better than adding the colorspace check to the helper directly, but at least everything stays in the rockchip driver
20:45 jclsn: Gonna write a proper commit message tomorrow and submit the patch