crabbedhaloablut has quit [Read error: Connection reset by peer]
crabbedhaloablut has joined #dri-devel
MayeulC has joined #dri-devel
kts has quit [Remote host closed the connection]
kts has joined #dri-devel
uajain_ has quit []
uajain has joined #dri-devel
kts has quit [Ping timeout: 480 seconds]
Newbyte has joined #dri-devel
aravind has quit [Ping timeout: 480 seconds]
aravind has joined #dri-devel
kts has joined #dri-devel
Duke`` has joined #dri-devel
KunalAgarwal[m][m] has joined #dri-devel
kts has quit [Ping timeout: 480 seconds]
egalli has joined #dri-devel
DemiMarie has joined #dri-devel
DemiMarie is now known as Guest9236
paulk-bis has joined #dri-devel
paulk has quit [Read error: Connection reset by peer]
fab_ has joined #dri-devel
fab_ is now known as Guest9238
jasuarez has joined #dri-devel
CADIndie has quit [Ping timeout: 480 seconds]
Duke`` has quit [Ping timeout: 480 seconds]
publicface has quit [Ping timeout: 480 seconds]
Guest9238 has quit []
itoral has joined #dri-devel
mripard has quit [Remote host closed the connection]
tomba has joined #dri-devel
Emantor has quit [Ping timeout: 480 seconds]
rasterman has joined #dri-devel
Emantor has joined #dri-devel
mripard has joined #dri-devel
mripard has quit [Remote host closed the connection]
sima has joined #dri-devel
fab has joined #dri-devel
kzd has quit [Ping timeout: 480 seconds]
fab has quit [Quit: fab]
fab has joined #dri-devel
fab has quit []
fab has joined #dri-devel
fab has quit []
fab has joined #dri-devel
fab has quit []
fab has joined #dri-devel
aravind has quit [Ping timeout: 480 seconds]
aravind has joined #dri-devel
sghuge has quit [Remote host closed the connection]
Wallbraker has joined #dri-devel
sghuge has joined #dri-devel
macromorgan_ has joined #dri-devel
macromorgan has quit [Read error: Connection reset by peer]
macromorgan__ has joined #dri-devel
macromorgan_ has quit [Read error: Connection reset by peer]
macromorgan_ has joined #dri-devel
Quinten[m] has joined #dri-devel
mripard has joined #dri-devel
tzimmermann has joined #dri-devel
jsa has joined #dri-devel
fkassabri[m] has joined #dri-devel
macromorgan__ has quit [Ping timeout: 480 seconds]
rgallaispou has joined #dri-devel
i509vcb has quit [Quit: Connection closed for inactivity]
derRicha1d has quit []
heat_ has joined #dri-devel
znullptr[m] has joined #dri-devel
Haaninjo has joined #dri-devel
vliaskov has joined #dri-devel
cmeissl[m] has joined #dri-devel
tursulin has joined #dri-devel
Venemo_ is now known as Venemo
BilalElmoussaoui[m] has joined #dri-devel
glennk has joined #dri-devel
kusma has joined #dri-devel
frieder has joined #dri-devel
sima is now known as Guest9260
frankbinns1 is now known as frankbinns
sima has joined #dri-devel
jcristau_ is now known as jcristau
fab has quit [Quit: fab]
tshikaboom has joined #dri-devel
pcercuei has joined #dri-devel
AlaaEmad[m] has joined #dri-devel
fab has joined #dri-devel
pushqrdx[m] has joined #dri-devel
sima has quit [Ping timeout: 480 seconds]
frieder has quit [Ping timeout: 480 seconds]
fab has quit []
egbert is now known as Guest9264
egbert has joined #dri-devel
fab has joined #dri-devel
fab has quit []
heat_ has quit [Remote host closed the connection]
frieder has joined #dri-devel
ohadsharabi[m] has joined #dri-devel
lemonzest has quit [Quit: WeeChat 4.1.1]
Guest9264 has quit [Ping timeout: 480 seconds]
flynnjiang has joined #dri-devel
frieder has quit [Ping timeout: 480 seconds]
fab has joined #dri-devel
lemonzest has joined #dri-devel
YuGiOhJCJ has quit [Quit: YuGiOhJCJ]
fab has quit [Remote host closed the connection]
fab has joined #dri-devel
flynnjiang has quit [Quit: flynnjiang]
hansg has joined #dri-devel
pixelcluster_ has quit []
pixelcluster has joined #dri-devel
frieder has joined #dri-devel
YHNdnzj[moz] has joined #dri-devel
donaldrobson has joined #dri-devel
apinheiro has joined #dri-devel
cmichael has joined #dri-devel
donaldrobson has quit [Ping timeout: 480 seconds]
donaldrobson has joined #dri-devel
mvlad has joined #dri-devel
Company has joined #dri-devel
robmur01_ is now known as robmur01
Guest9260 has quit []
sima has joined #dri-devel
idr_ has joined #dri-devel
idr has quit [Ping timeout: 480 seconds]
jani_ is now known as jani
Guest9172 is now known as shadeslayer
f11f12 has joined #dri-devel
<mripard>
sima: following up on here and mastodon, I guess I'll just drop the patch
<mripard>
it's still an implicit panic, which is inconsistent over similar calls, but that's really not the point of the series
vliaskov has quit [Remote host closed the connection]
vliaskov has joined #dri-devel
yyds has quit []
<javierm>
mripard: I still don't understand why the check is controversial... is not that drm_connector_init() is in a hot path
<javierm>
but yeah, not the point of the series so you could drop it if that would help focusing on the actual changes rather than bikeshedding how paranoid helpers functions should be
<pq>
Pre-conditions vs. legit argument values.
tjaalton_ is now known as tjaalton
<emersion>
normal runtime errors vs. programmer mistakes
<javierm>
pq, emersion: the problem is that C doesn't have a way to check that
<emersion>
a way to check what?
<javierm>
so whether pre-conditions / programmer mistakes or not, it will lead to a panic
<emersion>
no matter how complicated the type system may be, there will always be cases where the type system won't be enough
<pq>
C does have assert(), and the kernel has BUG() and friend.
<emersion>
there will always be cases where a panic/abort is the only way to indicate a programmer mistake
<emersion>
maybe my function has two arguments, and one can only be supplied when the other not, etc
<emersion>
or a string needs to have a specific format
<emersion>
or a number greater/smaller than 42
<emersion>
Rust cannot encode these things
<emersion>
(and IMHO trying to encode complicated constraints in type systems results in developers writing types, instead of writing actual useful code :P )
<javierm>
emersion: no, but we are talking about preventing a NULL pointer dereference here
<pq>
It's also a question of definition: who is required to validate the arguments or pre-conditions. The one who does not bear that responsibility can explode when pre-conditions are broken.
<emersion>
javierm: to me, they're the same kinds of errors: programmer mistake
<pq>
javierm, yes, we are talking about exactly that.
hansg has quit [Quit: Leaving]
<javierm>
emersion, pq: Ok. So is more controversial than I thought then
<pq>
If the pre-condition is that this function must be called with a non-NULL argument, and someone calls it with NULL, then it should explode. But if that pre-condition does not exist, then the function may as well return a graceful error if it cannot work.
<emersion>
my take is that it's a good thing to panic/WARN/abort on programmer error
<emersion>
way easier to spot the mistake and debug
<javierm>
pq, emersion: my take is that core kernel code should be paranoid and try very hard to not lead to a panic
<pq>
Indeed, unexpected failures should explode hard and on the spot (in debug builds).
<emersion>
a panic isn't such a big deal
<emersion>
sorry, i mean a WARN
<emersion>
it's not like it takes down the whole kernel or anything
<pq>
javierm, that's true depending on what we're talking about. If arguments come from userspace, then totally yeah.
<emersion>
invalid args coming from user-space isn't a kernel programming mistake
<mripard>
emersion: Rust can do that if you use the newtype pattern
<javierm>
emersion: it could with CONFIG_PANIC_ON_OOPS=y but yeah, probably not enabled in production builds
<mripard>
but back to the topic
<emersion>
mripard: do what?
<mripard>
all of that discussion about panic vs error check is also missing an important point
<emersion>
ah, use a newtype to enforce that the arg has a given property?
<mripard>
a) we never document that *anywhere* b) it's implicit c) it's not consistent across similar calls
<emersion>
that makes it the caller's responsibility to check, which may not be better
<mripard>
and that cannot be explained by "well panic is just better"
<mripard>
those three things are bad, no matter which side of the fence you're on
<javierm>
mripard: agreed
<emersion>
documenting pre-conditions is indeed important
<pq>
indeed, and if you have a if (!arg) return -EFAIL;, then it documents that there is no pre-condition saying arg must not be NULL, because NULL is gracefully handled.
<mripard>
so I'm sending a separate series adding a bunch of BUG_ON, improving the documentation and making that consistent across calls
<pq>
so you're making the policy by adding the check
<mripard>
would that satisfy everyone?
<emersion>
i think that's the right thing to do
<pq>
mripard, that sounds awesome to me.
<mripard>
s/so I'm sending/if I'm sending/
<mripard>
great
<mripard>
I'll do that then :)
<mripard>
topic closed, can we move to the rest of the series ? :)
<pq>
essentially the debate is about wether "arg is not NULL" is a pre-condition or not.
<pq>
If you add BUG_ON(!foo), that documents it's a pre-condition.
<pq>
If you add if (!foo) return -EINVAL; that documents it is not a pre-condition.
<emersion>
yea
<pq>
For a casual code reader, not having a pre-condition documented means that the opposite is probably normal operation and needs to be handled as well as possible, which might prompt them to write a lot of code to deal with it. Sometimes that's good, sometimes bad, depending.
<pq>
Simply dereferencing a pointer can be seen as a pre-condition "is not NULL'ish", because NULL pointer dereference is expected to crash immediately. Problem are cases where it does not crash, or somehow allows a compiler to generate nonsense code.
<javierm>
just wanted to mention that the BUG_ON() kernel-doc says that should be used only as a last resort and if there's really no way out
<javierm>
I understand your point from a theoretical PoV but in practice I believe that can cause a lot of harm to not be paranoid about check if the pre-conditions are meet
<pq>
depends on the probabilities
<javierm>
regardless I agree with mripard that making the pre-condition explicit with a BUG_ON() rather than implicit is progress
<pq>
Pre-conditions should definitely be checked at least in debug builds / CI for sure. What to do when it fails is another question. But this is also orthogonal to defining or not defining something as a pre-condition.
<mripard>
could we just stop discussion a one-liner and focus on the rest of the series that actually provide some notable features?
<pq>
and whether something is a pre-condition or not is totally arbitrary
<mripard>
pleaaaase :)
<pq>
sorry, philosophy is much more interesting than work :-p
<javierm>
pq :D
tuxayo has joined #dri-devel
tristianc6704 has quit [Ping timeout: 480 seconds]
alyssa has joined #dri-devel
kugel has quit [Ping timeout: 480 seconds]
tristianc6704 has joined #dri-devel
Danct12 has quit [Quit: WeeChat 4.1.1]
<sima>
mripard, I think you get the crown for "caused the best bikeshed of the year"
<mripard>
if that means it's an automatic ack for the rest of the series, then I'm glad I did :-D
tlwoerner_ has joined #dri-devel
tlwoerner has quit [Ping timeout: 480 seconds]
simon-perretta-img has quit [Ping timeout: 480 seconds]
simon-perretta-img has joined #dri-devel
yuq825 has left #dri-devel [#dri-devel]
dabrain34[m] has joined #dri-devel
DodoGTA has quit [Quit: DodoGTA]
DodoGTA has joined #dri-devel
kts has joined #dri-devel
Amber_Harmonia has quit [Read error: Network is unreachable]
fab has quit [Remote host closed the connection]
vliaskov_ has joined #dri-devel
vliaskov has quit [Ping timeout: 480 seconds]
itoral has quit [Remote host closed the connection]
tshikaboom has quit []
docelwilif^ has quit [Remote host closed the connection]
<karolherbst>
yeah.. the spirv has the alignment.. let's see...
<karolherbst>
ohh.. it applies Alignment to the variable, not the pointer type...
<karolherbst>
guess we could work around that.. *sigh*
fab has joined #dri-devel
fab has quit [Remote host closed the connection]
<gfxstrand>
Oh...
<gfxstrand>
What do you mean?
<karolherbst>
gfxstrand: like the spirv-llvm-translators puts `Alignment` on the `OpVariable` not on its pointer type
<karolherbst>
should have been `OpDecorate %_ptr_Workgroup__arr_uchar_ulong_1024 Alignment 8` instead I think
vliaskov_ has quit [Remote host closed the connection]
vliaskov_ has joined #dri-devel
kugel has joined #dri-devel
<gfxstrand>
Weird
<gfxstrand>
I thought we handled that.
<karolherbst>
well.. doesn't look like it
<karolherbst>
well.. it's also invalid to put it on the variable
<karolherbst>
"Apply only to a pointer. Alignment is an unsigned 32-bit integer declaring a known minimum alignment the pointer has."
<wv>
are there known issues regarding colors on freedreno/dmabuf? It appears that when I use glimagesink or appsink using dmabufs, I have color issues. Or video is green, or there's like a shift of 180 pixels in colors (like a mask is played next to the grey image)
frieder has quit [Remote host closed the connection]
aravind has quit [Ping timeout: 480 seconds]
AnuthaDev has joined #dri-devel
<gfxstrand>
karolherbst: Well, an OpVariable is a pointer...
<gfxstrand>
karolherbst: Or is it a variable that contains a pointer?
<gfxstrand>
I honestly don't know what we're supposed to do with an alignment on a variable.
<karolherbst>
the variable is just an array
<gfxstrand>
ugh...
<karolherbst>
in OpenCL C: __local uchar data[512/4*sizeof(ALIGN_TYPE)] __attribute__((aligned(sizeof(ALIGN_TYPE))));
<gfxstrand>
Right
<gfxstrand>
Yeah, we don't handle that sort of alignment yet.
<karolherbst>
mhh.. we'd also have to place the variable aligned I guess...
f11f12 has quit [Quit: Leaving]
<karolherbst>
yeah.. it kinda makes sense to be able to align the type actually...
<karolherbst>
the variable I mean
<gfxstrand>
And... The spec says nothing. Because of course it does.
<gfxstrand>
Let me type something quick.
Guest9236 is now known as DemiMarie
<karolherbst>
gfxstrand: where does the spec state that a variable is a pointer actually?
<gfxstrand>
OpVariable returns a pointer
<gfxstrand>
Just look at the OpVariable spec
<karolherbst>
ohhh...
<gfxstrand>
But, as with most SPIR-V things, it's legal to write it but no spec actually says what it means so we kinda have to make it up.
<karolherbst>
let's see...
<karolherbst>
"The compiler is responsible for aligning objects allocated by OpVariable to the appropriate alignment as required by the Result Type."
<karolherbst>
I guess that's everything we get here
<gfxstrand>
Yeah, but what does that mean?!?
<karolherbst>
well.. if the result type would be decorated with "alignment"
kts has quit [Quit: Konversation terminated!]
<karolherbst>
but I think this just means we have to align per CL rules (type alignment) or according to the alignment attribute (Alignment decoration)
<gfxstrand>
Yeah, but alignment is a hint, not a command.
<gfxstrand>
At least on all other pointer types
<karolherbst>
I don't think it's a hint in CL
<gfxstrand>
In any other environment it is.
<karolherbst>
like if you align a shared memory block you can hope that it's indeed aligned that way
<karolherbst>
or if you align struct members
<gfxstrand>
Well, it's a hint in the sense that if the client sets an alignment then it's up to the client to make sure it's true.
<karolherbst>
yeah
Duke`` has joined #dri-devel
<gfxstrand>
But I mean that it's data provided to the compiler. The compiler is free to ignore it.
<karolherbst>
but the client also expects this alignment to be honored by thec ompiler
<karolherbst>
mhhh
<karolherbst>
I don't think it is
<karolherbst>
not in CL
<gfxstrand>
Sure you can. You can use unaligned loads for everything. There's nothing stopping you.
<karolherbst>
like again, if you annotate a shared memory block with "this is aligned with 8 bytes" then you kinda want that block to be 8 byte aligned
<karolherbst>
well...
<karolherbst>
right
<karolherbst>
but...
<karolherbst>
the thing is
<karolherbst>
what if you cast that array to an array of longs
<karolherbst>
and then use load/stores on it in functions
<karolherbst>
you can't assume everywhere that things are unaligned if you care about perf
<karolherbst>
but yes.. you can just ignore it and do unaligned load/stores everywhere :)
<DavidHeidelberg>
karolherbst: where is the docs for it? :P
<karolherbst>
there is
<karolherbst>
uhm.. for what anyway?
<karolherbst>
anyway.. done with the MR update
<DavidHeidelberg>
karolherbst: weird, I see it with changes on top of both commits, but didn't show up when browsing one-by-one
<DavidHeidelberg>
so good :)
<karolherbst>
yeah.. gitlab also screwed up for me a few minutes ago
<kisak>
milestone, 20k merge requests merged into mesa since migrating to gitlab.
<karolherbst>
I hope it was mine
<karolherbst>
ohh.. it was already a while ago
<hch12907>
karolherbst: for a moment I thought it was "dump" in the "to throw away" sense
<karolherbst>
not yet
<hch12907>
I was like, are we adding a C parser to mesa now?
<hch12907>
š
<karolherbst>
like
<karolherbst>
if that's the fun you are looking for, sure
<kisak>
oh, I got the milestone wrong ... 20k merge requests merged by Marge Bot just now
<karolherbst>
ahh
tomba has quit [Ping timeout: 480 seconds]
idr_ has quit []
idr has joined #dri-devel
<jenatali>
karolherbst: Yeah zink's callback is very wrong
<karolherbst>
figures
<jenatali>
It's saying that it requires component-aligned accesses, which defeats the whole purpose of the pass
<jenatali>
Which is to change the component size
donaldrobson has quit [Ping timeout: 480 seconds]
alanc has quit [Remote host closed the connection]
<karolherbst>
right
alanc has joined #dri-devel
<karolherbst>
so if it sees a 32 bit load with an alignment of 1, it should return 8x4 instead I guess
<jenatali>
Right
<jenatali>
Unless it can actually do a 32bit load with an alignment of 1, in which case it can return 32x1, it just needs to set the out alignment to 1
<karolherbst>
but will the pass use atomics for that already (if allowed)?
<karolherbst>
I think vulkan spir-v requires type size aligned load/stores for everything, no?
<jenatali>
Probably
<karolherbst>
yeah.. the main intention of adding that pass/code was to deal with vec8/vec16
<karolherbst>
but I'll have to figure out the unaligned bits as well
<jenatali>
The pass will do atomics for stores where the returned alignment is greater than the input alignment, yeah
<karolherbst>
mhhh
<jenatali>
I.e. if the input alignment is 1, but the driver can only do 2- or 4-byte-aligned stores, then the pass will promote the store to atomics with 4-byte alignment
<karolherbst>
okay, so zink should just enable that atomic option and it should be good?
<jenatali>
It already is setting that flag, which just guards an assert. It needs to return correct data from the callback
<karolherbst>
well.. I guess for private memory it shouldn't rely on that option
<karolherbst>
jenatali: but how would that then work.. if you e.g. have a 32 unaligned store, but you don't want to split that up into 8x4 because it would violate API rules
<jenatali>
It has to get split up into 4 8x1 stores
<karolherbst>
okay, so the pass won't do that automatically, but it will use atomics for it?
<jenatali>
The callback has to return that you want 8x1 with 4-byte alignment, and then the pass will turn that into atomics, yeah
<karolherbst>
8x1 or 8x4?
<jenatali>
Well, if you can do 8x4 with 1-byte alignment, then you can just return that. Zink can do that with the 8bit load/store extension
<jenatali>
If that extension isn't present, then zink needs to return a higher alignment, and in that case you'd want to do 1 byte at a time I think
<karolherbst>
I tihnk the problem is that a vulkan compiler would be free to split those stores
<jenatali>
Sure, then the vulkan compiler can do that
<karolherbst>
but wouldn't that be illegal from a CL perspective?
<jenatali>
Why?
<karolherbst>
mhhh, not quite sure actually...
<jenatali>
The only requirement is that the writes don't modify other data, and that they don't race with other writes to nearby data
<karolherbst>
ohh right..
<jenatali>
If the vulkan driver (or zink) splits an unaligned store into 4x byte stores, that's fine as long as each store actually only writes the relevant byte
<karolherbst>
like if you do a 64x4 write but the pointer is aligned with 2
<karolherbst>
wait.. bad example
<karolherbst>
32x2 but aligned with +0x3 and you want to use a 32x3 write for the whole area, but the edges need to update in a way it doesn't interfer with anything
<jenatali>
Then if you can do 2-byte-aligned stores, you'd return that, otherwise 4. If you return 4, you'll need to write one 2-byte chunk at a time via atomics. If you return 2, then you can return 16x4 and the pass will chunk it up appropriately
<karolherbst>
right...
<karolherbst>
yeah, makes sense
<jenatali>
32x2 with +0x3 offset, you'd use a 1-byte store for the first bit, then the pass would call you back again with a 4-byte-aligned store
<jenatali>
It's really quite a clever pass as long as the callback returns good data. Took me a while / a few tries to get the callback right for our limitations though
AnuthaDev has quit []
gouchi has joined #dri-devel
mbrost has joined #dri-devel
illwieckz has quit [Read error: Connection reset by peer]
illwieckz has joined #dri-devel
tursulin has quit [Ping timeout: 480 seconds]
rz_ has joined #dri-devel
rz has quit [Ping timeout: 480 seconds]
sravn has quit []
<DemiMarie>
@_oftc_CADIndie:matrix.org: could you push Qualcomm to do what Intel and AMD do and have launch-day upstreamed drivers?
mbrost_ has joined #dri-devel
<gfxstrand>
That would require Qualcomm to actually contribute to the upstream drivers.
<gfxstrand>
Or any open-source upstream for that matter.
<DemiMarie>
They absolutely should
<DemiMarie>
Google should just ban out of tree drivers from Android
mbrost has quit [Ping timeout: 480 seconds]
<DemiMarie>
Make people upstream their drivers or no Google Play Services
<DemiMarie>
Heck, I suspect that Android is a bigger market for Qualcomm than desktop Linux is for Intel.
hansg has quit [Quit: Leaving]
pac85 has joined #dri-devel
<pac85>
Demi: that would ruin the fun for those that do the reverse engineering
<DavidHeidelberg>
DemiMarie: Google should ban proprietary rootkit (GMS) which runs on 99% of Androids :) but it's not going to happen
<karolherbst>
jenatali: yeah.. I kinda wished this pass would have proper documentation on the callback :)
<jenatali>
karolherbst: Be the change you want to see in the world :P
<karolherbst>
I'm already am, hence me not having time to also document code anymore š
<karolherbst>
oh hey, now I even have a good excuse for real
<robclark>
DemiMarie: qcom did actually have patches on list for sm8650 and sc8380xp basically the day they were announced.. I don't believe either are shipping yet
<robclark>
(also no idea who you were replying to, I guess yet another matrix ghost)
<pac85>
Including GPU driver patches?
<robclark>
I don't think they have posted kernel parts yet, but I expect they will before those devices ship
<robclark>
(which is sometime next year for sc8380 ... not sure about the mobile part, but tbh the laptop part is more interesting)
<pac85>
join #freedreno
<pac85>
whoops
<gfxstrand>
DemiMarie: Oh, if only it were that simple...
mbrost_ has quit [Ping timeout: 480 seconds]
<mattst88>
magical thinking
sravn has joined #dri-devel
docelwilif^ has joined #dri-devel
sima has quit [Ping timeout: 480 seconds]
fab has quit [Quit: fab]
Duke`` has quit [Ping timeout: 480 seconds]
docelwilif^ has quit [Remote host closed the connection]
docelwilif^ has joined #dri-devel
mvlad has quit [Remote host closed the connection]
mbrost has joined #dri-devel
glennk has quit [Ping timeout: 480 seconds]
gouchi has quit [Remote host closed the connection]
anarsoul|2 has joined #dri-devel
YuGiOhJCJ has joined #dri-devel
pac85 has quit [Remote host closed the connection]
angerctl has joined #dri-devel
anarsoul has quit [Ping timeout: 480 seconds]
Namarrgon has quit [Ping timeout: 480 seconds]
crabbedhaloablut has quit []
i509vcb has joined #dri-devel
Haaninjo has quit [Quit: Ex-Chat]
<DemiMarie>
mattst88 gfxstrand robclark DavidHeidelberg: Iām still very much an idealist.
<robclark>
the situation has improved somewhat dramatically from 5 or so years ago, now we are getting SoC support the day it was announced rather than a few years after it shipped.. qcom contributing to the UMD is still a bit of a red line for them, but they do contribute to KMD
<mattst88>
DemiMarie: that's great, but you're kind of doing it again...
apinheiro has quit [Quit: Leaving]
jsa has quit [Read error: Connection reset by peer]