00:36mdnavare: Lyude: Found a patch where this function got added drm_dp_get_adjust_request_post_cursor() and reviewed by you where we call dp_link_status(link_status, offset); with offset = DP_ADJUST_REQUEST_POST_CURSOR2 which results in reading link_status however DP_LINK_STTAUS_SIZE is only 6
00:38mdnavare: Lyude: Any idea how this will work?
00:39imirkin: hm, fun. something changed and now llvm is detected properly.
00:40mdnavare: Lyude: Its added by Thierry Reding in this patch : 078c445733c1e8092e2
00:43mdnavare: Lyude: And this patch 79465e0ffeb9e4866939ea562bc reviewed by you
00:56imirkin: airlied: i flipped it to checking lod_info.file != PROGRAM_UNDEFINED ... still ok?
00:58airlied: imirkin: looks good
01:50kode54: user experiencing this issue on their system
01:52Lyude: mdnavare: might be better off asking Thierry, I remember reviewing the patch but I don't remember much more then that
08:44pq: so, platform or SoC etc. dependent DRM format modifiers are ok? Meaning that the format + modifier alone is not enough to describe the data layout.
08:45danvet: pq, we try really hard to avoid them
08:45danvet: there's some legacy stuff for historical reasons
08:46danvet: e.g. the intel tiling stuff
08:46danvet: but on anything remotely modern those intel tiling modifiers have exactly one layout only
08:46danvet: pq, where was that discussion?
08:55pq: danvet, the new AmLogic modifiers from narmstrong
08:57pq: danvet, I first started typing a reply, then started thinking that maybe they are ok if there are existing examples, found the intel stuff, and discarded my email...
08:58narmstrong: pq: it's not a tiling format, but a compressed stuff, which is way more vendor specific in our case
08:58pq: narmstrong, the same difference.
08:58pq: You're using format modifiers for it.
08:59pq: if it's platform or SoC specific, it means no-one can write a reliably CPU parser for the data layout
09:00narmstrong: pq: I don't see your point, isn't it the sole reason to have vendor specific modifiers ?
09:00pq: narmstrong, the thing that bothers me, is that the modifier is not *enough* to interpret the data.
09:01narmstrong: pq: so, what's the solution ? the ARM afbc has the same issue so far
09:01pq: define one modifies for each unique data layout
09:02narmstrong: pq: it's the case, combined with the fourcc, we have an unique combination for a data layout
09:03pq: narmstrong, then why does your documentation say that the data layout depends on platform and/or the SoC?
09:03danvet: narmstrong, arm afbc is unique for a given modifier/format combo
09:03danvet: or supposed to be at least
09:03narmstrong: pq: because the header encodes data describing the layout
09:04narmstrong: danvet: I assume it's the same as AFBC, but much simpler
09:04pq: narmstrong, ok, so have the doc refer to a specification that defined what the header means
09:04danvet: narmstrong, but hw can parse that header?
09:04emersion: some user-space relies on format/modifier representing a single data layout, e.g. waypipe
09:04narmstrong: danvet: yes
09:04emersion: it copies byte-by-byte DMA-BUFs from one machine to another
09:05narmstrong: still don't understand the issue
09:05emersion: is there hw that half-supports the modifier?
09:05narmstrong: why would ARM afbc be ok ? AFAIK they never documented the header and layout format
09:05pq: narmstrong, the issue is: your modifier doc says the data layout depends on platform or SoC, when instead it needs to refer to a specification that fully explains what the header means and how it is used.
09:06emersion: ie. supports some headers but not all of them?
09:06narmstrong: emersion: yes
09:06emersion: well that breaks stuff
09:06narmstrong: emersion: it's the role of the driver to describe which modifier is supported on eah platform
09:07pq: emersion makes a good point
09:07emersion: sure, but a modifier can't be at the same time supported and not supported depending on the buffer data
09:07narmstrong: emersion: no I meant, one platform doesn't support a modifier option, but all the platforms supports the default modifier
09:08emersion: one guarantee i think is important is: if a platform supports a given modifier, there's a guarantee that *any* buffer using said modifier will be supported
09:08narmstrong: actually it's modeled exactly like ARM afbc, where some producers can support some options, and some consumers can support some options
09:08narmstrong: emersion: it's the case
09:08emersion: ah, okay
09:08narmstrong: the platform supports the data that have been produced on the same platform
09:09emersion: so the header doesn't have a "sub-modifier"?
09:09narmstrong: exactly like ARM afbc for instance
09:09pq: narmstrong, no, it needs to work across platforms
09:09narmstrong: pq: it's not the case for ARM afbc
09:09pq: what? Then ARM afbc is broken too.
09:09pq: danvet, ^
09:09narmstrong: and it;s not the case for QCOM_COMPRESSED either
09:10pq: bloody hell
09:10pq: so now we need to have modifier blacklists in userspace to know which ones cannot be sent to other machine
09:11narmstrong: and DRM_FORMAT_MOD_BROADCOM_UIF is also obviously broken aswell
09:11daniels: AFBC does encode the options into the modifier code youth
09:12narmstrong: yes, but the set of supported options depends on the actual HW present on the platform
09:12narmstrong: and it's very inconsistent, like consumers
09:12emersion: ah, if the options are encoded in the modifier then it's fine
09:15pq: if supported options are encoded in the modifiers, then advertising a list of modifiers unambiguously explains which options are supported: every combination of options is a different modifier to ve advertised
09:15pq: and if some particular combination of options is not supported, then that modifier is simply not advertised
09:17pq: narmstrong, as I mentioned, the problem is saying that your modifier is platform or SoC dependent. If your modifier means that there is a data header that actually specifies the data layout, then the modifier refers to the header and you should add a reference pointing to the specification that explains the header.
09:19narmstrong: pq: the header obviously specified the data layout since it's a compressed format, like ARM afbc, nothing different, and none are publicly specified
09:19pq: narmstrong, but this has also another consequence: since there is one modifier for the header, it means that anyone saying they support that modifier *must* be able to handle *all* valid header contents. I'm not sure if you mean that this is true or not.
09:20narmstrong: pq: I assume it's the case since only amlogic will write decoders for their own format
09:21pq: "not publicly specified" could be a problem, because it essentially forbids interaction with anything else by obscurity, but that's for DRM maintainers to decide
09:23pq: narmstrong, you don't even know? What stops Amlogic from starting use a header instance that won't work on their older product?
09:23narmstrong: pq: so what's the solution ? forbid proprietary in-SoC formats ?
09:24pq: I'm not the one to set the policy. I also have no problem with proprietary formats as long as the format specification is public, and how it is used with modifiers agrees to how modifiers are used.
09:25narmstrong: pq: I would also like to have these formats specified, be sure of that
09:26pq: "how modifiers are used" means that if a system advertises a particular format+modifier supported, then that means the same thing on all systems ever
09:28pq: narmstrong, to get actual answers and suggestions, you should turn to the DRM maintainers with all these caveats. Which you kind of did in your patch. I'm raising the awareness here to avoid having it slip in accidentally.
09:29narmstrong: pq: this is why I asked danvet for that, but we all know how SoC vendors are and we cannot have any guarantees, even from ARM since we don't have the actual spec of AFBC formats either
09:30pq: So far I have thought that format+modifier with the usual width,height,stride uniquely define the data layout. Whether that is a set policy or not, I'm not sure, but danvet sounded like it is the intended policy.
09:31narmstrong: and it what I had in mind when writing the modifiers
09:32pq: The existence of the header is a bit of a problem, if the header acts like a sub-modifier in that some header instances could be unsupported while others are supported. If so, it means that adversiting the modifier does not actually tell what is supported or not.
09:33pq: Whether modifiers need to be portable across machines is a whole another matter, where again I have assumed they should be portable.
09:34narmstrong: The existence of the header wasn't an issue when the ARM AFBC modifier was pushed
09:34narmstrong: So I assumed it was ok
09:34pq: I have no idea what ARM AFBC has or does.
09:36pq: The questions are: Does the header act as a sub-modifier or not (the problem of supported cases)? And is the specification of the data layout, with or without the header, uniquely defined as is, or do one need to know e.g. the platform or SoC to be able to interpret the data?
09:37narmstrong: pq: no, it's not a sub-modifier, all the options describing the layout (bitdepth), width, height, layout are taken from the fourcc and the modifier
09:38pq: answering "yes" to either question makes the modifier ill-specified, IMO, meaning that it needs to be split into multiple modifiers where neither answer is "yes"
09:38emersion: so if any hw A claims to supprot modifier 0x12345, can you guarantee that any other hw B also claiming to support it will be able to import a buffer from A?
09:39pq: So what exactly does the header contain? You say we cannot know? How can we then know that it cannot act like a sub-modifier?
09:39narmstrong: no since I don't have the layout specification and the format producers firmware source and specification
09:40emersion: that's an issue
09:40pq: narmstrong, then it is an ill-specified modifier, and personally I would forbid it as such. (But I'm not making the rules, so.)
09:40narmstrong: so it's the same issue for ARM AFBC, sorry to repeat the same stuff again & again
09:41pq: I would say the same things about ARM AFBC as well.
09:42narmstrong: pq: please revert all ill-specific modifers and nack mine, so we can solve how to handle these, I'd say they is no cleanest solution at this point
09:42pq: I also think that past mistakes do not justify new mistakes.
09:43pq: looks like AFBC stuff is already released, so it cannot be reverted
09:43pq: UAPI, as you know
09:43narmstrong: pq: thanks for your support anyway, nice to have people helping support advanced multimedia features
09:44narmstrong: no we know why we can't support advanced feature sin the kernel
09:44pq: narmstrong, no problem. I really want to see things improving too.
09:44narmstrong: pq: don't think so
09:45narmstrong: I've been constantly hitting the same wall in the last 3y upstream new features, same ol song "past mistakes do not justify new mistakes"
09:46narmstrong: but big vendors can do mistakes, small can't
09:48narmstrong: this modifier won't hit anything except a very small SoC market and will be used only between a v4l2 producer and drm consumer, and never set by an userspace utility
09:48narmstrong: the impact is so negligible
09:49narmstrong: if it was targeting 96% of server and PC market, the discussion would be very different
09:49daniels: narmstrong: the big difference is that Arm _have_ documented which bits of AFBC are optional (split-block, sparse layout, block sizes, etc etc), and each of those are broken out into separate modifiers, so it is unambiguous which AFBC features are supported and which AFBC features are not supported
09:49narmstrong: daniels: I did the same
09:50daniels: except 'default' explicitly says \_o_/
09:50narmstrong: daniels: so what's the default 16x16 AFBC then ?
09:52daniels: it's AFBC without lossless-colourspace, without split blocks, without sparse blocks, without copy-block restriction, without superblock tiling, without solid-colour blocks, without front-buffer safety, without content hints
09:52daniels: Documentation/gpu/afbc.rst documents this pretty clearly?
09:52narmstrong: yes and DRM_FORMAT_MOD_AMLOGIC_FBC_DEFAULT is 64x32 superblocks with 4096 bytes per superblock followed by a 32 bytes per 128x64 header block
09:53narmstrong: as documented
09:55daniels: your 'Implementation details may be platform and SoC specific, and shared between the producer and the decoder on the same platform.' sentence makes me think that it's not as well defined as you're making out here
09:55daniels: and it's certainly a lot less well defined than AFBC which is pretty exhaustively documented?
09:56narmstrong: the decoding process is certainly vendoer specific, and only available on the amlogic SoCs, what should I say ?
09:56daniels: also as far as special treatment goes, the initial '#define DRM_FORMAT_MOD_ARM_AFBC fourcc_mod_code(ARM, 1)' submission was obviously rejected in 2016, and not merged until two years later
09:57narmstrong: I'm ok for discussions, but I documented the maximun that I'm aware of, without hidding anything, the modifier matches exactly the different HW options, nothing mode nothing less
09:58daniels: no-one's accusing you of hiding anything
09:59daniels: but the accusation that AmLogic is being victimised and people just wave through anything from Arm without saying anything, is totally unfair
09:59daniels: the amount of detail in your AmLogic tiling submission vs. the amount of detail in what was finally landed for AFBC is _very_ different
09:59narmstrong: oh sorry you interpreted that, it was not my point
09:59daniels: it's hard to read anything else into 'but big vendors can do mistakes, small can't' tbh
09:59narmstrong: it was a more global point of view, not directed against ARM
10:01emersion: i think in this case "the first vendor to use the new modifiers feature can do mistakes" is maybe a little bit more accurate
10:02narmstrong: I'd love to see the other vendore using a same scheme, mtk for example, but they don't care about upstreaming these features
10:03narmstrong: so my only similar examples are ARM AFBC and QCOM_COMPRESSED
10:03narmstrong: so now I'm said they were mistakes
10:04emersion: it's not clear
10:04narmstrong: so, as I sent all this publicly, I asked for a review to go forward and find a proper solution to solve this technical issue
10:04emersion: if there's anything like "Implementation details may be platform and SoC specific", it's not fine
10:05narmstrong: a set of modifier seemd to be a good idea and matches really well
10:05emersion: maybe you can consider adding a field in the modifier for the particular platform/SoC used?
10:05daniels: i don't see that AFBC is a mistake - it's pretty exhaustively documented and it seems to give no scope for invention or variation
10:05narmstrong: emersion: so ? I should limit the modifier to exact platforms ? add a version ?
10:06narmstrong: daniels: except the underlying layout
10:06emersion: note, i'm not a DRM maintainer, i'm just trying to give some ideas
10:07daniels: (i agree that QCOM is a bit of a mockery, given that Intel, Samsung, Vivante, NVIDIA, Broadcom (non-UIF), and Arm are all very clearly documented
10:07daniels: Allwinner probably seems fine as well
10:11pq: I replied in email, FWIW
10:14pq: narmstrong, if the data layout depends on platform and SoC, then encoding specific platform and SoC in the modifier could be a solution.
10:16narmstrong: pq: ok, seems logical
10:18danvet: seanpaul, robclark ^^ is the qcom modifier solid or some per-platform wobliness?
10:18danvet: also complaints about the thing being underspecced ... not r/e'd or what are you doing?
10:18danvet: more details to make sure there's no platform dependency in the modifier would be really good
10:19pq: A very important property is that the meaning of a modifier cannot change over time. If the meaning depends on the hardware, then it's far too easy to miss that the meaning changed between different hardware. Having an explicit documented data layout helps immensely in keeping the meaning stable.
10:19narmstrong: I haven't complaint about these modifiers
10:20danvet: daniels has at least somewhere upthread
10:20narmstrong: If they need to be inter-platform yeah, there is an issue
10:32danvet: they're supposed to be
14:01imirkin_: wasn't varying packing *required* for xfb for some reason?
14:01imirkin_: i can't remember *why* that would have been the case, of course... just the fact
14:05imirkin_: why can't panfrost deal with packing for xfb btw? (how would it even know it was packed?)
14:05daniels: imirkin_: there's a bunch of discussion on the MR
14:06imirkin_: ah, so it's slot <-> buffer relationship which causes problems
14:06imirkin_: not packing
14:06imirkin_: that makes sense.
15:17robclark: danvet: not sure what you mean about per-platform wobbliness.. there does seem to be some per-SoC configuration in the different blocks but not sure if that effects the format (we don't seem to have description of those registers in the display controller block).. there isn't ever any case where, for ex, the video/display/gpu support *different* UBWC.. and it doesn't effect anything userspace cares about (the
15:17robclark: layout/size/alignment of the ubwc meta buffer).. other than that userspace just treats the format as opaque.. ie. if some random thing wants access to the framebuffer, it should do what CrOS cmdline screenshotter tool does, and GETFB2 to get handle/fourcc/modifier, and then use egl dmabuf extension to import into gl driver. (Or I suppose display writeback pipe could also work)
15:22pq: robclark, what about userspace that scrapes all bytes from a buffer and re-creates the buffer with the same bytes and modifier in another machine, that too claims to support the same modifier?
15:25emersion: it would be handy to have a list of historical modifiers where this ^ doesn't work
15:26pq: emersion, btw. do you know why this is a real use case? Wouldn't stuff going over network be encoded lossy or lossless, why send (compressed) dmabuf contents as is?
15:27emersion: yes, this is a real-world use-case https://gitlab.freedesktop.org/mstoeckl/waypipe/
15:27emersion: sometimes you don't want to use losy compression: on a LAN for instance
15:27emersion: you want to reduce encoding overhead, so you don't compress anything, and it doesn't matter too much because the network is very fast
15:28pq: oh, Wayland pass-through via network
15:29pq: "don't compress anything" is something I have a hard time believeing that it could ever be a win :-)
15:30emersion: compress everything with lz4, but don't use any video encoding
15:30pq: sure, that's cool
15:38robclark: pq: if you are going to serialize, you are better off converting to linear.. it will be smaller, and probably something that will compress better
19:10mdnavare: Lyude: What is thierry's irc nick?
19:12imirkin_: mdnavare: tagr
19:13mdnavare: imirkin_: Great thanks will ping him
19:15mdnavare: tagr: I was looking at this patch from you 79465e0ffeb9e4866939ea562bc to add drm_dp_get_adjust_request_post_cursor() and where we call dp_link_status(link_status, offset); with offset = DP_ADJUST_REQUEST_POST_CURSOR2 which results in reading link_status however DP_LINK_STTAUS_SIZE is only 6 and thats resulting into static check analysis errors
19:39sravn: pinchartl: Thanks for the reviews of panel-timing patches. As they are already pushed to drm-misc-next I need to push a few fixes patches. Will try to find time tomorrow.
19:40pinchartl: sravn: take your time. sorry for being slow :-(
19:40sravn: pinchartl: Will ofc post patches and await review before applying them
19:40sravn: pinchartl: np
21:48airlied:thought I'd understood rasterizer multisample maths, but then I fixed a bug in my code, and now I don't understand it again :-(
21:51imirkin_: and of course there are no comments
21:51imirkin_: coz if it was hard to write, it should be hard to read
21:57Lyude: hey mripard - any preference for how you want me to push this patch series? was going to push it to drm-intel, but wanted to double check since it definitely touches drivers outside of i915 https://patchwork.freedesktop.org/series/72991/#rev4
22:14airlied: imre: decoding the scrolls :-P
22:24airlied: yay passed accuracy test hacked to only draw 1 triangle
22:25airlied: oh passed accuracy test properly, looks like I have the basics for llvmpipe msaa then
22:30imirkin_: airlied: the ext_framebuffer_multisample piglits?
22:30imirkin_: those are super-picky
22:31airlied: imirkin_: yeah the accuracy one is my have I got the maths right test :-P
22:32airlied: need to cleanup a bit and run the full set
22:36danvet: Lyude, ack for merging through drm-intel
22:37danvet: Lyude, also not seeing where this touches other drivers?
22:37danvet: seems to be just dp helpers + i915
22:37danvet: mripard, nag about that backmerge again
22:41jekstrand: Where does gallium create depth renderbuffers?
22:41jekstrand: imirkin_: ^^
22:42imirkin_: jekstrand: you mean st/mesa, presumably?
22:42jekstrand: Yeah, but where?
22:42imirkin_: right, just trying to understand the question. i don't remember offhand, give me a minute to find it
22:42jekstrand: Or maybe you can just answer the question: Does it set PIPE_BIND_SAMPLER_VIEW for render buffers?
22:42imirkin_: it shouldn't
22:42imirkin_: this is a slightly tricky question though
22:43jekstrand: yeah :)
22:43imirkin_: let me check the code ... give me a few mins
22:43imirkin_: i can guarantee that if nothing else, the code around this is confusing.
22:43jekstrand: Naturally. :)
22:44imirkin_: st_renderbuffer_alloc_storage is hooked up to gl_renderbuffer::AllocStorage (st_cb_fbo.c)
22:44imirkin_: this in turn will set bind=PIPE_BIND_DEPTH_STENCIL
22:44imirkin_: no path will set PIPE_BIND_SAMPLER_VIEW for a renderbuffer backing
22:45jekstrand: So nchery's optimization is actually doing something
22:45jekstrand: And maybe even the right something
22:45imirkin_: however for textures that back fbo's, no such exception exists
22:45imirkin_: those should be getting created with PIPE_BIND_SAMPLER_VIEW
22:45imirkin_: even if backing storage isn't set at draw time
22:46jekstrand: And, of course, the moment you do BlitFramebuffers you're effectively texturing from the renderbuffer.
22:46jekstrand: i.e., it's a nice hint but that's about it.
22:46imirkin_: i'd have to double-check how the gallium blit works
22:46imirkin_: but it might be from surface to surface
22:47imirkin_: not all blits are done by sampling :)
22:47jekstrand: All our blits are :)
22:47imirkin_: sure, but that's your problem :p
22:47jekstrand: And, to be clear, it *is* a problem....
22:49imirkin_: just checked -- blit takes a naked pipe_resource as the src/dst (+ level/layer/format)
22:50imirkin_: which makes sense ... but yeah, there's a guarantee that the dst is a PIPE_BIND_RENDER_TARGET, but no guarantee of PIPE_BIND_SAMPLER_VIEW for the src
22:50imirkin_: (or PIPE_BIND_DEPTH_STENCIL instead of RT, obviously)
22:50Lyude: danvet: oh
22:50Lyude: i guess that's the only driver using those helpers, huh :S
22:50Lyude: (this will not be the case soon since i'm planning on making nouveau use them)
22:52danvet: hm yeah -rc6 didn't happen yet, so if you merge them this week nouveau can get them before the merge window
22:55Lyude: danvet: haven't even written the patches yet :P, focusing on fixing the mst regressions first
22:57imirkin_: Lyude: there was someone on the nouveau@ list with a weird MST issue, dunno if you saw
22:58nchery: jekstrand: looking at HIZ_CCS's write-through mode?
22:59jekstrand: nchery: Yup
23:00Lyude: imirkin_: yeah it's probably that one