00:01 Vanfanel: pcercuei: in current MERCURIAL code?
00:02 pcercuei: Vanfanel: kernel DRM
00:03 pcercuei: emersion: yes, sorry. Have a look at https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/pl111/pl111_drv.c#L73
00:03 pcercuei: It calls drmm_mode_config_init() at the beginning, just like it should
00:04 pcercuei: Line 145 it calls drm_panel_bridge_add_typed(), which will allocate a bridge using devm_kzalloc()
00:04 pcercuei: As a result the bridge is freed before drm_mode_config_cleanup() is called
00:04 pcercuei: (when the driver requests a probe deferral for instance)
00:05 pcercuei: I see the same thing in the ingenic-drm driver, probably elsewhere too
00:07 pcercuei: The devm_kzalloc() in drm_panel_bridge_add_typed() should be changed to a drmm_kzalloc(), but then there's no drm_device pointer in there, so the prototypes would need to be changed
02:15 dcbaker: imirkin: nope. But the cmake is pretty fragile
07:04 ascent12: If a GPU reset happens, are dumb buffers or other modesetting state affected?
07:05 imirkin: if it's in GPU VRAM, then yes
07:05 ascent12: I assume most dumb buffers would be.
07:06 imirkin: well, if you have a UMA setup, then perhaps not
07:06 imirkin: e.g. intel, SoC drivers, etc
07:06 ascent12: Is there a standard way to detect if a GPU reset happens and your buffers are bad?
07:06 ascent12: I know opengl robustness and vulkan can tell you, but not when you're not using them
07:09 ascent12: grepping around, I can see i915 sends an ERROR=1 and RESET=1 uevent, but I guess only they do that
07:10 imirkin: with opengl (and i assume vulkan) there is. i don't think that there's anything standardized with DRM though
07:39 Plagman: https://gitlab.freedesktop.org/mesa/mesa/-/commit/4292fb2139282e6906d4ad2a8be2fd81ed7ca8af
07:40 Plagman: MrCooper: this is a great change - my opinion is also that normal FIFO should also always be async and make its way asap to the compositor
07:41 Plagman: the compositor just needs to know what present mode the client is using - it's the only thing that has the context and capabilities needed to implement them
07:42 Plagman: been discussed there as well https://github.com/Plagman/gamescope/issues/25
07:43 Plagman: if you think it's a fine idea, we'd be interested to try to plumb that
07:57 Plagman: same with gl swapinteral==1
07:58 Plagman: oh, i see bnieuwenhuizen asked the same in the MR
08:00 Plagman: even if it's not strictly with XCB_PRESENT_OPTION_ASYNC semantics, vsync should be implementable by the compositor and it should get the buffers asap
08:04 imirkin: gah! i thought the DRM_FORMAT_BIG_ENDIAN flag had been worked out. but it hasn't ... just the logic for flipping between rgba8/argb8. boo!
08:05 imirkin: airlied: --^ do you know if there were follow-up patches on that topic that were never merged?
08:05 imirkin: airlied: basically nothing checks for the DRM_FORMAT_BIG_ENDIAN flag, so you'll get failures pretty quickly when it can't locate the drm_format_info
08:28 imirkin: airlied: i guess i could just add the 2 relevant extra entries into the drm_fourcc table...
08:29 imirkin: airlied: i have a user who has a ppc, so hoping to make addfb2 work by implementing the quirk properly
08:30 imirkin: but i don't want to lose rgb565/rgb555 while i'm at it
11:28 MrCooper: imirkin: I don't remember ever seeing anything beyond adding the DRM_FORMAT_BIG_ENDIAN flag
11:39 MrCooper: Plagman: for now, Xwayland would still have to wait for frame callbacks from the Wayland compositor normally, because there's no mechanism yet to specify the target for the frame to the Wayland compositor
11:40 MrCooper: XCB_PRESENT_OPTION_ASYNC would only affect late frames
11:43 MrCooper: ("late" as in Xwayland has already received frame callback for target MSC)
11:43 mareko: imirkin: see _mesa_DrawArrays, leave the fields uninitialized
11:45 MrCooper: Plagman: and late frames are already sent to the compositor immediately anyway... therefore, per my MR comment, I think it'd only make an actual difference for how completion is reported
11:46 MrCooper: (for "late" frames only)
11:53 MrCooper: Plagman: BTW, in contrast to romangg's comment on your gamescope issue, I expect that Xwayland will keep using frame callbacks for this even once it uses the presentation-time protocol (that will merely result in more accurate completion reporting), otherwise Xwayland won't benefit from Wayland compositors minimizing latency via frame callback timing
16:17 marex: Hi, if I run $ LIBGL_ALWAYS_SOFTWARE=1 SOFTPIPE_DEBUG=use_tgsi eglretrace pointsprite.trace # pointsprite.trace coming from https://github.com/ptitSeb/gl4es/blob/master/traces/pointsprite.tgz , the TGSI generates TXP instruction, is that expected ?
16:17 marex: shouldn't the pointsprite use only XY coordinates and thus result in TEX instruction ?
16:19 marex: if I comment out https://gitlab.freedesktop.org/mesa/mesa/-/blob/master/src/mesa/main/ff_fragment_shader.cpp#L848 , then it generates TEX, as expected
16:21 imirkin: marex: pointsprite is drawing points with a GL_QUAD essentially
16:21 imirkin: the shader is generated irrespective of what the drawing mode might be
16:22 imirkin: so if TXP is the right thing, then it's the right thing
16:22 imirkin: note that when pointsprite coord replacement is enabled, you're supposed to set z = 0, w = 1
16:22 marex: imirkin: can you elaborate on that last "note" ?
16:23 marex: imirkin: the reason I'm asking is because with etnaviv, the point sprites are broken and I'm trying to find out the right way to fix them
16:23 imirkin: gl_TexCoord is a varying
16:23 imirkin: which contains ... the tex coord (surprising, right?)
16:23 imirkin: it's a vec4
16:23 marex: right
16:23 imirkin: when point sprite coord replacement is enabled
16:24 imirkin: gl_TexCoord[n] is replaced with (s,t,0,1)
16:24 imirkin: (or (s, 1-t, 0, 1) depending on settings)
16:25 imirkin: if you don't replace z and w, then iirc games like neversomething (neverlight?) don't work
16:25 imirkin: i debugged this very issue on freedreno :)
16:25 marex: imirkin: I hit this very issue with neverball on etnaviv, and then found an easier test ^ above
16:25 imirkin: yes. neverball, that's the one
16:26 imirkin: see e.g. 6cdb29d52fc51e3d904b50bb7003c9fa38bb7896
16:27 marex: imirkin: so the shader compiler in etnaviv has to effectively set Z,W=0,1 ?
16:27 marex: ah
16:27 marex: imirkin: thank you, I will try to digest this, might take a bit
16:28 imirkin: marex: well - ideally the hardware will do that for you
16:28 imirkin: but if not, then yes.
16:28 imirkin: like on freedreno, i just had to set some additional bits in the varying replacement config
16:31 marex: imirkin: I dont think the vivante GPU can do it, at least the blob traces dont indicate so
16:31 imirkin: well, blob traces on a3xx didn't indicate it either
16:31 imirkin: the functionality is only necessary for desktop GL point sprite replacement
16:31 imirkin: if there are spare bits, just try them
16:46 marex: imirkin: hmmm, seems like if the FS input X,Y is marked as pointcoord, the ZW coordinates are ignored, so the shader has to set those manually
16:47 marex: imirkin: cant the TXP be somehow lowered to TEX instead ?
16:47 imirkin: marex: then the shader contents depend on point sprite settings
16:47 imirkin: don't really want that...
17:12 marex: imirkin: I dont see any bits which would tie Z/W to 0/1, so I guess the driver has to fix it up
17:18 imirkin: marex: is there a register description or something which shows how point sprite is enabled in the first place?
17:18 imirkin: or a link to the code?
17:19 marex: there is src/gallium/drivers/etnaviv/hw/state_3d.xml.h , you probably want to look at VIVS_PA_CONFIG_ register
17:19 marex: the bottom two bits are undocumented, but they dont seem to have impact
17:20 imirkin: marex: mmmm ... this can't work
17:20 imirkin: there's only one POINT_SPRITE_ENABLE?
17:21 marex: imirkin: so it seems
17:21 imirkin: ok, so you need to do it in shader anyways
17:21 imirkin: point sprite coord replacement is supposed to replace only the tex coords that it says to
17:21 imirkin: so it has to be a bitmask of 8 tex coords
17:21 imirkin: (although frequently hw will let you replace any varying)
17:22 imirkin: marex: i'm confused - what are the _MASK things? is that a "care about the setting" setting?
17:23 marex: there is also src/gallium/drivers/etnaviv/hw/state.xml.h VARYING_COMPONENT_USE_
17:23 marex: but that also doesn't seem to apply here
17:24 imirkin: that's exactly what you want
17:24 imirkin: where are those values used?
17:24 imirkin: oh i see - VIVS_GL_VARYING_COMPONENT_USE presumably?
17:25 marex: right, but even if I set XY to POINTCOORD_X,Y and ZW to _USED, ZW isn't 0,1
17:25 marex: also, the _MASK, I have no idea
17:25 imirkin: right
17:26 imirkin: so basically you'd want a VARYING_COMPO
17:26 imirkin: VARYING_COMPONENT_USE_0 / USE_1
17:26 imirkin: or ... something along those lines.
17:28 marex: that's what I do now
17:28 marex: imirkin: with this awful patch https://gitlab.freedesktop.org/marex/mesa/-/commit/daafe97bbfe9fc5e33d0d986b668c9006846b1ac neverball seems to work
17:29 marex: imirkin: but that avoids emiting the TXP, but rather emits TEX
17:30 imirkin: marex: oooh
17:30 imirkin: yeah
17:30 imirkin: that won't work
17:30 imirkin: but ... heh
17:30 imirkin: so (a) you can't change anything in mesa/main
17:31 marex: imirkin: so I guess I need to figure out how to get ZW=0,1 into the FS using those VARYING_COMPONENT_ bits
17:31 imirkin: (b) you should only do the coord replacement when point_sprite_coord or whatever that bitfield is called indicates it
17:31 marex: sprite_coord_enable ?
17:32 imirkin: (c) i'd try forcing the use[] to be UNUSED or maybe USED for use[2] and [3]
17:32 imirkin: yes, that's the one
17:34 marex: imirkin: I already tried both UNUSED and USED, that didn't help
17:34 marex: but the etnaviv implementation of TXP might have something to do with that
17:35 marex: since the TXP implementation there does the projection adjustment in the shader program, so if W is unused and thus likely 0, we end up with division by zero in the shader
17:35 imirkin: which is why W has to = 1 ;)
17:36 marex: well, yes
17:37 imirkin: point sprite coord replacement is about just that -- making certain varyings appear to have certain values
17:37 imirkin: anyways, you can workaround it in several ways
17:38 marex: I guess I can add shader instructions to set W=1 if W=0
17:38 imirkin: emersion: the problem? glClear with some sort of setting which caused it to be presented as a draw
17:40 imirkin: mareko: ok, so it looks like while _mesa_draw_arrays & co set info.index_bounds_valid = true, cso_draw_arrays and cso_draw_arrays_instanced don't
17:40 imirkin: mareko: should i fix this once in the cso, or fix up nouveau and hope for the best in other drivers?
17:40 marex: imirkin: I will keep digging, thanks
17:41 imirkin: marex: the idea would be to always set W=1 if sprite coord replacement is on
17:41 imirkin: for that varying
17:42 imirkin: marex: and then DCE should remove it completely if it doesn't end up getting used
17:42 imirkin: it does mean you have to add the sprite coord replacement mask to your shader key
17:47 emersion: imirkin: interesting
18:05 mareko: imirkin: I replied on the ticket
18:06 mareko: imirkin: I would like _mesa_draw_arrays not to set the fields
18:06 mareko: imirkin: but it's a long uphill battle
18:07 mareko: by ticket, I meant the MR
18:08 mareko: imirkin: for non-indexed draws, pipe_draw_info::drawid should be the last initialized fields
18:08 mareko: *field
18:15 imirkin: mareko: so ... this is broken for 21.0 -- either we go with the safe approach, and then later enforce that, or we leave other drivers potentially broken
18:15 imirkin: in either case, it should be getting set or not set consistently
18:15 imirkin: not set in some paths, not set in others
18:15 emersion: that last sentence was confusing
18:16 imirkin: lol
18:16 imirkin: it wasn't confusing - it was wrong
18:16 imirkin: "set in some paths, not set in others" is what i meant
18:17 emersion: well, what we want is not that :P
18:18 emersion:always here to help disambiguate
18:29 mareko: imirkin: when you get to 40 million draws per second, every little thing that you do will slow you down, thus for non-indexed draws it's better to keep as many fields uninitialized as possible
18:29 imirkin: mareko: i fully support your goal of as little overhead as possible
18:30 imirkin: the issue i have right now is the inconsistency - sometimes it's set, other times it's not
18:30 imirkin: we should either always set it, or never set it (for non-indexed draws)
18:30 imirkin: so here's what i propose:
18:30 imirkin: 1. fix up cso_draw_arrays as i have done
18:31 imirkin: 2. fix up nouveau, and any other drivers we can find to deal with the "new" situation
18:31 imirkin: 3. set index_bounds_valid = false from all non-indexed draw paths that hit gallium drivers
18:32 imirkin: (which will effectively revert the fix from #1, as well as make some changes in st/mesa)
18:32 mareko: sounds good, you can push your MR with ym Rb
18:33 imirkin: thanks
18:33 imirkin: i have to run right now, but will do a bit later on