03:39 agd5f_: ajax, I think the display hardware on those chips can even theoretically support it, although I'm not sure if it's been validated
03:47 imirkin: karolherbst: 80 chars is great for density imo. 2x 80 char also happens to fit perfectly on my screen.
03:48 imirkin: otherwise you end up with CTS-style density, where each line is indented by 80 chars and i end up wondering "why did they double-space their code"
04:05 AndrewR: https://github.com/Randrianasulu/MPBenchmarks - I think I fixed them ..... or at least added some useful for me features
07:05 tzimmermann: danvet_, i think what irks me is that the rules for pci and usb devices are different
07:06 danvet_: tzimmermann, yeah, pci is special
07:06 danvet_: for hysterical raisins mostly
07:06 danvet_: tzimmermann, the rules are also different for platform devices
07:06 tzimmermann: ok, lol
07:07 danvet_: so the reason is the old shadow attach stuff from before kms
07:07 tzimmermann: but using an upcast for usb devices is ok
07:07 danvet_: which only worked for pci
07:07 danvet_: so essentially drm core handleded the binding for you
07:07 tzimmermann: oh yeah, legacy. i think i expected that
07:07 tzimmermann: :/
07:07 danvet_: then we extended that midlayer to usb and platform using drm_bus abstraction
07:08 danvet_: and eventually I managed to demidlayer that all
07:08 danvet_: ->load/unload callback also comes from this
07:08 danvet_: except pci is still hanging in there
07:08 tzimmermann: oh
07:09 tzimmermann: would it be viable to convert all drivers to use drm_device.dev and declare the pdev field as legacy? (not that i'd volunteer)
07:10 danvet_: I think, but I'm not sure
07:10 danvet_: I'd first try to move core drm code away from it
07:10 tzimmermann: ok, np
07:10 danvet_: with the runtime check using dev_is_pci
07:11 danvet_: since I'm not sure we're not using it as some kind of flag too
07:11 danvet_: or whether a driver has a ->dev pointer that doesn't match ->pdev
07:11 tzimmermann: that check exists? :o
07:11 tzimmermann: i was looking for something like this
07:11 danvet_: tzimmermann, yeah see my latest reply in the thread for the right name
07:12 danvet_: we could remove pdev completely I think with that
07:12 tzimmermann: sounds like it's worth trying at least
07:12 danvet_: well for !CONFIG_DRM_LEGACY
07:13 danvet_: I don't think it's worth the bother at all for legacy driver
07:14 tzimmermann: oh, i didn't see that reply
07:14 tzimmermann: i'll send out another iteration of these patches and add a todo item
07:18 tzimmermann: sravn, danvet_, i sent out the next iteration of the fbdev-iomem patchset. the final patch still needs a review, if you have a minute to spare. i moved the while fb_read/fb_write code into the helpers, which makes the code much more pleasent overall
07:18 tzimmermann: s/while/whole
07:33 MrCooper: DPA: can't etnaviv avoid the problem based on PIPE_BIND_SCANOUT?
07:40 danvet_: tzimmermann, do we have something to test that fbdev read/write stuff?
07:42 tzimmermann: danvet_. idk, but was wondering myself. running weston with fbdev backend might be an option for a quick smoke test.
07:42 tzimmermann: in terms of automated test cases; no idea
07:47 danvet_: tzimmermann, igt has a very minimal fbdev testcase
07:48 danvet_: but it doesn't have read/write support
07:48 danvet_: and pretty sure weston doesn't use that either
07:48 tzimmermann: danvet, tests/fbdev.c
07:48 tzimmermann: danvet_ ^
07:49 danvet_: yeah that one
07:50 danvet_: maybe add some read/write tests there just to see whether it oopses right away or not :-)
07:50 tzimmermann: danvet_, i haven't done much with igt yet. if i add an igt_subtest("rw"), that should do it, basically?
07:50 danvet_: yeah
07:50 tzimmermann: ok, thanks
07:50 danvet_: I'd do two, one "read" one "write" and then do a few things in there
07:51 danvet_: there's a bunch of igt_assert functions too
07:51 MrCooper: FWIW, http://linux-fbdev.cvs.sourceforge.net/ lists an fbtest CVS repo
07:52 tzimmermann: danvet_, i have to see. maybe i'll do a single test for writing and reading back
07:52 danvet_: tzimmermann, ah yeah
07:52 danvet_: I was more thinking about corner cases
07:52 danvet_: to make sure the error handling isn't too broken
07:52 tzimmermann: it would be nice to integrate this with mmap as well
08:01 danvet_: MrCooper, tzimmermann that cvs server seems dead
08:01 danvet_: and google doesn't find any other fbtest that looks like it's a this old one
08:09 danvet_: ah I think I found it, but it's underwhelming
08:09 danvet_: thus far just a few visual checks, nothing that really goes into corners and makes sure stuff works and which you could just run unattended
08:10 danvet_: https://kernel.googlesource.com/pub/scm/linux/kernel/git/geert/fbtest/
08:10 danvet_: also it's gpl, igt is mit
08:20 danvet_: tzimmermann, I'm kinda wondering whether we could get away with just no read/write support :-)
08:22 tzimmermann: i know what you want to say, but it's been around for ages. i'm pretty sure, someone's using it
08:22 tzimmermann: i'd be more interested if we could get away with no fbdev support ;)
08:31 DPA: MrCooper: I think dcss can deal with etnavivs modifiers. So I think forcing linear there would be wrong.
08:31 DPA: Can't I just fix it for mxsfb and call it a day? If other drivers have a similar problem, it can still be fixed once it actually starts causing real world problems for those.
08:32 MrCooper: can etnaviv select a scanout compatible modifier when PIPE_BIND_SCANOUT is set?
08:36 danvet_: tzimmermann, we can dream :-)
08:44 DPA: MrCooper: I don't know, but I certainly won't add more hacks & assumptions to etnaviv there than there already are. I don't know what impact on performace and other things that would have.
08:46 DPA: Also, what's wrong with just fixing mxsfb?
08:49 lynxeye: DPA: actually it's a valid assumption to default to linear with PIPE_BIND_SCANOUT without modifiers. I think we already did this in the past and it might have been lost in all the reworks of the layout code.
08:50 lynxeye: The base assumption in etnaviv should be: no fancy layouts (i.e anything but linear) for scanout unless explicitly asked for
08:51 lynxeye: tiling between GPU and scanout will only possibly work with etnaviv if you support modifiers (as we don't have a covert side-channel like the older DRM drivers)
08:54 DPA: Ok, I'll take a look at that then.
09:08 jadahl: can a blob set to a drm property be released, assuming the same property is set to a new blob in the same cycle, or should one always wait with release until after the next flip event after it was replaced?
09:18 danvet_: jadahl, with atomic that's all taken care of by the framework
09:18 danvet_: or well, should be
09:18 danvet_: since we don't do driver private uapi for kms anymore
09:18 danvet_: summary is that the lifetime of the blob should be tied to the right state structure
09:18 danvet_: which is correctly lifetime managed already
09:18 jadahl: so whats the responsibility of userspace using the drm api?
09:19 jadahl: i can release it acter commit?
09:19 jadahl: *after
09:19 emersion: i think yes
09:19 danvet_: oh for userspace
09:19 danvet_: yes
09:19 danvet_: right away after the atomic ioctl call
09:19 danvet_: kernel refcounts this stuff
09:19 jadahl: ah, neat
09:20 jadahl:removes code
09:20 danvet_: jadahl, maybe we need to improve the kernel doc a bit?
09:20 danvet_: emersion, ^^
09:21 jadahl: honestly the docs for KMS I read are the same for the kernel driver and the user space so it's a bit hard to navigate. (I usually end up looking at the page on 01.org)
09:23 emersion: yeah, this is a known issue
09:23 emersion: same doc for drivers and user-space isn't great
09:24 emersion: i don't even know where i'd document that, danvet_
09:24 emersion: jadahl: which page on 01.org?
09:25 emersion: https://dri.freedesktop.org/docs/drm/gpu/ should have the same content
09:25 emersion: for some reason 01.org is better referenced by search engines…
09:25 jadahl: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html
09:26 emersion: right, same as https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html
09:30 danvet_: emersion, maybe in include/uapi/drm/drm_mode.h next to the structures for blob ioctls?
09:30 danvet_: that's very we started having some very shy first kerneldoc for ioctls and uapi ...
09:31 danvet_: https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#userspace-api-structures
09:31 danvet_: emersion, it's not great but at least something
09:32 emersion: ok, looks like this would be a reasonable place
09:32 danvet_: in the glorious future we might have some nice intro/overview pages that tie it all together for how this is used
09:36 vsyrjala: i guess props are still a problematic from the uapi docs pov
09:36 vsyrjala: where do you put the docs for those in a way that doesn't mixup the internals and uapi side?
09:39 emersion: maybe only document the uAPI side in the list of props, add a "See XX_prop_create() for driver developers." note, and document the internal bits there?
09:40 emersion: well, even a clearly separate "For driver developers" paragraph for each prop would be better than the status quo
09:42 emersion: thinking about it, the kernel already has a "list of props"
09:44 emersion: hmm, where was that again
09:46 emersion: drm_plane has some drm_property fields
09:47 emersion: hm, i guess only a few are there
10:22 hakzsam: MrCooper: https://gitlab.freedesktop.org/mesa/mesa/-/jobs/5136045 it's complaining about the CTS test name ...
10:25 MrCooper: see https://gitlab.freedesktop.org/freedesktop/ci-templates/-/merge_requests/46
10:26 hakzsam: a CTS test name isn't a URL and not a tag
10:27 MrCooper: read the commets as well
10:28 hakzsam: I think this should only check the commit title, as you suggested
10:30 MrCooper: meanwhile, maybe pass --textwidth <large enough number> to ci-fairy check-commits
10:32 MrCooper: or maybe just disable the "check commits" job for now
10:33 hakzsam: yeah
10:34 hakzsam: pendingchaos: ^
10:54 sravn: tzimmermann: review of the fbdev stuff may have to wait, busy with some work stuff. From first look the naming itches me a bit, likes danvet iomem proposal. I will also find time to test it again using qemu and sparc64
10:55 tzimmermann: sravn, no problem. you don't have to test again. the code you tested didn't change
11:03 pendingchaos: I'll disable the job for now
12:23 karolherbst: ehhh
12:23 karolherbst: pendingchaos: I figured out the problem :/
12:24 karolherbst: CL allows applications to cache stuff on their own
12:24 karolherbst: so if nir changes, things can just break randomly
12:26 karolherbst: curro: we need to version the binaries :/
12:27 karolherbst: or does clover do that already?
12:27 karolherbst: maybe we need to add the git hash then or something
12:28 karolherbst: aghh yeah.. /tmp/kernel_cache/LUXCORE_1.5/
12:28 karolherbst: what a path
12:28 karolherbst: keys were Mesa/llvmpipe (LLVM 10.0.1, 256 bits) and NV137
12:28 karolherbst: mhh
13:05 danvet_: seanpaul, robclark https://patchwork.freedesktop.org/series/82927/ I think review on this from cros perspective would be really good
13:05 danvet_: hwentlan, ^^ I think this came up from amdgpu originally
13:05 danvet_: mripard, ^^
13:16 mripard: danvet_: I know nothing about the legacy cursor, so I'm not sure it's worth anything, but a-b :)
13:17 danvet_: mripard, I guess I'm fine with just the a-b on the vc4 patch :-)
13:17 danvet_: can you put that on the m-l pls?
13:19 mripard: yep
13:19 danvet_: thx
13:19 mripard: done
13:28 mripard: danvet_: while we're at it, do you know if https://lore.kernel.org/dri-devel/20200925130044.574220-1-maxime@cerno.tech/ is the proper way to deal with it?
14:18 sravn: danvet_: to continue your answer to Guido. When committing patches would it then be acceptable practice to add an r-b or an a-b if I think I have stared enough on a patch to warrant such?
14:19 sravn: So far I have considered it more or less implicit by the s-o-b that I add (dim does for me).
14:19 sravn: I can imagine the answer is "shrug" :-)
14:35 karolherbst: curro: "fun", apparently we run out of stack by using the API in some way: https://gist.githubusercontent.com/karolherbst/ebf213c4c66feea029c5f4106296b592/raw/7c62f31cae4246dec05cbb1ff86af947fa061f8e/gistfile1.txt
14:35 karolherbst: no clue what the application was doing
14:35 jekstrand: karolherbst: Are event waits recursing?
14:35 karolherbst: ohh.. I can press c
14:36 karolherbst: no clue
14:36 karolherbst: ahh
14:36 karolherbst: 104753
14:36 karolherbst: quite some stack
14:36 karolherbst: clFinish
14:36 karolherbst: mhhh
14:36 jekstrand: That looks like infinite recursion to me
14:37 karolherbst: or we have that many events
14:37 karolherbst: jekstrand: see how the this pointer is always different
14:37 jekstrand: that could be too
14:37 danvet_: sravn, your own sob implies the ack
14:37 karolherbst: with clFinish we create an even depending on all others
14:37 karolherbst: and I guess it can lead to a weird chain? dunno
14:37 danvet_: but you can also add your explicit r-b
14:37 danvet_: the ack is very much implied when you push a patch to drm-misc
14:37 jekstrand: karolherbst: Hrm...
14:38 karolherbst: ahh yeah
14:38 karolherbst: seems like we have a linear chain tree
14:38 karolherbst: each event has a list of deps it depends on
14:38 karolherbst: and they wait on each of those
14:38 karolherbst: jekstrand: it's one of the image tests doing bazllions of subtests
14:39 jekstrand: karolherbst: Right.
14:40 jekstrand: karolherbst: Maybe we need something where we prune the tree every once in a while?
14:40 karolherbst: no clue
14:40 jekstrand: That or have an actual list of events rather than singly linked with recursion
14:40 karolherbst: I suspect it's something dumb
14:40 jekstrand: Most things are
14:42 karolherbst: I bet we just depend on the previous queued thing
14:42 karolherbst: mhhh
14:49 jekstrand: karolherbst: It looks like event::trigger_self() drops the chain references
14:51 jekstrand: karolherbst: But maybe we want to drop deps too?
14:57 karolherbst: mhh. let's see
15:00 karolherbst: yeah.. no clue, maybe curro has an idea
15:01 jekstrand: karolherbst: Yeah, I think I see what's probably going wrong.
15:01 karolherbst: mhh.. CL_INTENSITY is a bit broken
15:02 jekstrand: Ugh.... event::wait() is const
15:02 karolherbst: yeah
15:02 karolherbst: :)
15:02 karolherbst: I think we had a similiar idea
15:04 jekstrand: karolherbst: How about this: https://paste.centos.org/view/7a62cbb1
15:05 karolherbst: mhhh
15:05 karolherbst: it is still a waste of memory :/
15:05 jekstrand: It is
15:05 jekstrand: I don't like it for that reason
15:06 jekstrand: Yeah, it's no good for that reason. We don't want an infinite leak either
15:06 karolherbst: ohh cool, those image tests are quite nice btw
15:06 jekstrand: ?
15:06 karolherbst: "test_image_streams 2D read int CL_FILTER_NEAREST UNNORMALIZED CL_ADDRESS_CLAMP CL_LUMINANCE CL_SIGNED_INT8" :)
15:06 karolherbst: you can filter by just appending what you care about
15:07 karolherbst: okay.. let's see
15:07 jekstrand: karolherbst: Yeah, we really need to clear out event::deps after we've waited on them
15:07 karolherbst: yep
15:07 karolherbst: I guess we rewait quite a lot
15:07 karolherbst: let's see...
15:07 jekstrand: karolherbst: I'm a little afraid to make deps mutable since it's protected not private
15:07 karolherbst: I have a dirty trick
15:07 jekstrand: But this is all internal clover apis so meh
15:08 karolherbst: jekstrand: "const_cast<event*>(this)->deps.clear();"
15:08 karolherbst: :p
15:08 jekstrand: karolherbst: No
15:08 jekstrand: karolherbst: Just make it mutable at that point
15:08 karolherbst: I know
15:08 karolherbst: just want to try it out
15:08 karolherbst: maybe it's a different issue :p
15:10 jekstrand: Ugh... soft_event uses it
15:10 jekstrand: without taking a lock
15:10 karolherbst: uhh
15:13 karolherbst: jekstrand: so clearing the dep list doens't do anything
15:14 jekstrand: Grr
15:14 karolherbst: I think the issue is when you enqueue bazillions of things and just wait once
15:14 karolherbst: without explicit deps
15:14 karolherbst: so you have this implicit dep chain
15:15 jekstrand: Could be
15:15 karolherbst: well, all the dep list I checked had exactly one entry
15:25 karolherbst: jekstrand: I think I found a bug in the nir_lower_cl_images_to_tex pass
15:26 karolherbst: intrinsic image_deref_size (ssa_0, ssa_10) (16) /* access=16 */ -> (uint32)txs ssa_0 (texture_deref), ssa_10 (lod), 0 (sampler)
15:26 karolherbst: shouldn't it be using a sampler_deref?
15:27 karolherbst: mhh.. but the image_deref_size uses a constant sampler.. mhh
15:28 karolherbst: ah yeah.. nvm
15:28 karolherbst: thre is no sampler anyway
15:29 karolherbst: and also shouldn't matter for txs
16:11 jekstrand: karolherbst: No, txs doesn't need a sampler.
16:11 jekstrand: Neither does txf
16:12 karolherbst: debugging image issues are pain with the CTS :D
16:12 karolherbst: it doesn't dump nice pngs
16:23 karolherbst: is there anything special about luminance textures?
16:24 jekstrand: Why is OpenCL using luminance textures?!?
16:24 karolherbst: it supports those
16:24 karolherbst: we could also just not support them
16:24 karolherbst: I think they are purely optional
16:24 jekstrand: Yeah, and they're pretty stupid
16:24 karolherbst: mhh, also broken with llvmpipe
16:25 jekstrand: I say we shouldn't support luminance or intensity unless there's an app we care about that requires them
16:25 karolherbst: let's see what is actually required
16:25 karolherbst: jekstrand: there is also CL_DEPTH, but I think that makes kind of sense
16:25 karolherbst: also, CL_DEPTH is super limited
16:25 jekstrand: Again, WHY?!?
16:26 karolherbst: because CL_DEPTH is required
16:26 karolherbst: UNORM_INT16 and FLOAT are for DEPTH
16:26 karolherbst: but I think that's CL 2.0
16:27 karolherbst: but you get away by only reading from CL_DEPTH
16:27 karolherbst: it also works, so not really an issue
16:27 jekstrand: I guess it's ok but it's only really useful for CL<->GL interop
16:28 karolherbst: sure
16:28 karolherbst: but I think it's natural we do support it at some point
16:29 karolherbst: sadly BGRA is also required
16:29 karolherbst: but unorm_int8 is enough
16:30 karolherbst: sBGRA is also reuiqred... *sigh*
16:30 karolherbst: but yeah.. those are all CL 2.0 features
16:31 karolherbst: mhh CL 3.0 has weaker rules indeed
16:31 karolherbst: but the difference is really just CL_DEPTH
16:31 karolherbst: and sRGB
16:31 karolherbst: both of which just works
16:36 karolherbst: ehh.. well, at runtime it requires CLC 2.0 support because of function overloads.. *sigh* :D
16:36 karolherbst: oh well..
16:36 karolherbst: disabling optional formats is probably easier now
16:38 karolherbst: okay..
16:38 karolherbst: "2D CL_R CL_SNORM_INT8 CL_FILTER_LINEAR CL_ADDRESS_CLAMP_TO_EDGE UNNORMALIZED float read" is broken on nouveau
16:38 karolherbst: which is...
16:38 karolherbst: required
16:39 karolherbst: huh
16:39 karolherbst: but not for 1.2
16:39 karolherbst: CL_UNORM_INT8 is though
16:39 karolherbst: ehhh..
16:39 karolherbst: I think I know what's up
16:40 karolherbst: unorm broken with llvmpipe though
17:00 karolherbst: jekstrand: I think I know how to fix the even stuff: we want a list of queued events per command_queue and only deps if those were explicitly specified and then we can flush the list once it's requested.. or just remove entries while the events are being worked on
17:41 AndrewR: is there possibility !6946 (inf performance patch from Marek ) actually break something on i686 ? I think I applied it on top of my hacks and a lot of stuff got broken (but may be I must blame only of my VERY hackish hacks for nv50 instruction emitter ... will try to retest, but I don't have fastest machine around ...)