<tzimmermann>
javierm, hi! one thing i don't understand is why the encoder allocates that buffer
<sima>
javierm, I'm kinda wondering whether we shouldn't make the _rpm one the default
<sima>
it results in constant surprises like this, and the only reason the default is the default is because the legacy crtc helpers where backwards like this too
<javierm>
tzimmermann: well, there's no encoder really on this device. So is hard to map the DRM/KMS abstraction. At some point we decided that due the plane not having .enable/disable we could put the init seq in the encoder
<javierm>
so that's where I moved the allocate of the buffer to be close to the panel init code
<tzimmermann>
javierm, i see
<tzimmermann>
let me think about this for a bit
<javierm>
sima: I wondered exactly the same. It seems to me as if the default should be to not update planes for CRTCs that are disabled
<sima>
javierm, yeah
<sima>
also the ordering of first enabling the crtc and then the planes seems to be a lot more what driver authors expect
<tzimmermann>
javierm, sima, AFAIU the crtc is enabled in the atomic state, just not yet commits
<sima>
but it's a bit a big patch set to make it happen
<tzimmermann>
'commited'
<sima>
tzimmermann, yeah that's the backwards ordering inherited from the legacy crtc helpers
<javierm>
tzimmermann: is it? At least in this case the encoder enable callback was never called
<javierm>
so the CRTC is defined disabled when the plane atomic state is commited
<tzimmermann>
that sounds like a problem in the the commit_tail function.
jagan_ has quit [Remote host closed the connection]
<tzimmermann>
if the crtc is disabled, the plane should not be enabled IMHO
<tzimmermann>
javierm, can't you allocate the buffer initally and leave it at that?
<tzimmermann>
why is it done in encoder->enable ?
<sima>
tzimmermann, yeah hence why I think we should make the _rpm the default, because that's all around more reasonable
<javierm>
tzimmermann: yes, but that's an orthogonal issue IMO. Just exposed this one about trying to update disable planes
<tzimmermann>
sima, but i'm rather surprised that atomic_check doesn't enforce this?
<javierm>
that's the only difference between _rpm and the default
<javierm>
tzimmermann: about the buffer allocation, yes I plan to use devres helper for that. I didn't because I know that there are some impedance mismatch between device and drm_device lifecycles
<javierm>
but Geert is correct that in this case we could use devm_* for this buffer so I'll do that
<tzimmermann>
javierm, i'd rather change that in the driver
<javierm>
tzimmermann: and probably will remove that encoder funcs and just use the plane enable/disable now that you added the enable too
<javierm>
tzimmermann: just make it safe for the atomic state to be commited to disabled planes ?
<javierm>
Ok, that would work but I think that is masking another issue
<tzimmermann>
sima, IMHO the non-_rpm code is more logical
<javierm>
tzimmermann: I disagree. For instance, I assumed that was the default
<javierm>
_rpm I meant. I wonder how many people thought the same
<tzimmermann>
javierm, plane->atomic_enable is being called *after* update_plane. so that would not fix the allocation issue
<javierm>
tzimmermann: hmm, right. Hence why I prefer to use the _rpm version too :)
<tzimmermann>
IMHO it makes sense to first do an update of the plane and its content. then enable the display pipeline frm plane to encoder. that's what the non-_rpm commit tail does, as this only enables a stage if the input has been set up already
<tzimmermann>
but it also means that the driver has to allocate that buffer before calling plane_update
<javierm>
tzimmermann: hmm, I see. So that when is enabled you already have content in the framebuffer and prevent an initia visual glitch
<tzimmermann>
that's the idea, at least
<tzimmermann>
javierm, the size of that buffer is static?
Company has joined #dri-devel
<javierm>
tzimmermann: for now, but it won't once I add support for 4-bit grayscale and 16-bit RGB
<javierm>
tzimmermann: I've started pushing the preparatory patches for these new panel families (ssd132x and ssd133x)
<javierm>
tzimmermann: but I'll just allocate at probe time then using devm_kzalloc() and that should fix it too
<javierm>
Geert also wanted that so it shows that's better than my approach. Could you please comment on the list ?
<tzimmermann>
javier, IMHO a real fix would be to allocate this buffer within the plane state in atomic_check. that might need some extra thoughts on how to share the memory across plane updates.
frankbinns2 has quit [Quit: Leaving]
frankbinns has joined #dri-devel
<tzimmermann>
javierm, can't you simply allocate enough memory for the maximum buffer size? the panels are small and 16-bit color isn't so much
donaldrobson has joined #dri-devel
<javierm>
tzimmermann: but also depend on the panel resolution. I think that will just move ssd130x_buf_alloc() to ssd130x_init_modeset()
<javierm>
and change the kcalloc() to devm_kcalloc()
<tzimmermann>
ok
<tzimmermann>
sima, why don't we make DRM_PLANE_COMMIT_ACTIVE_ONLY unconditional?
<tzimmermann>
or why don't we outright reject enabled planes on disabled crtcs? if find the current semantics fairly unintuitive
<tzimmermann>
s/if/i/
<javierm>
tzimmermann: agreed
<sima>
tzimmermann, because it could break drivers
<sima>
on really dumb hw it doesn't matter much when you program the plane registers
<sima>
and if your crtc->enable doesn't restore planes, then ACTIVE_ONLY drops updates
<sima>
hm the kerneldoc could maybe be improved a bit and mention drm_atomic_helper_disable/commit_planes_on_crtc
<sima>
since you might want to use those too if you set ACTIVE_ONLY in your crtc hooks
<tzimmermann>
ok, no easy fix here
apinheiro has joined #dri-devel
cmichael has joined #dri-devel
<sima>
tzimmermann, actually I think since we're adding all planes on modeset it could work for everyone
<sima>
if you use the _rpm version
<sima>
we definitely had a few confusing iterations on the helper design in this area
<sima>
overall I think making the flow in _rpm the default would be best
<emersion>
what is the difference between DRM_EVENT_VBLANK and FLIP_COMPLETE?
<sima>
so maybe also inverting the ACTIVE_ONLY flag and requiring those who need them all to set it
<sima>
emersion, uh ...
<sima>
emersion, in atomic?
<emersion>
oh, is it that VBLANK is for DRM_IOCTL_WAIT_VBLANK, and FLIP_COMPLETE is for page-flips? but otherwise sent at the same time?
<emersion>
hm, seems like it
<sima>
emersion, yeah
<sima>
the timestamp and frame # are supposed to match between vblank events and flip complete events
<sima>
(you'd need a really funny driver for them not to)
<javierm>
sima: but wouldn't make _rpm the default would prevent what tzimmermann mentioned? That is, updating a plane state prior to an enable ?
<sima>
javierm, well you can still open-code
<sima>
it's just about flipping the default around to the less surprising one
<sima>
not even open-code but just use the one we have as default now
<javierm>
sima, tzimmermann: actually, you could even with _rpm if you use a shadow buffer right? Because user-space can update that shadow bufer, is only the commit that will not happen
<javierm>
which makes sense because the HW plane is disabled
<sima>
javierm, tzimmermann oh I got confused, we do not reject planes on disabled crtc
<sima>
it's just about what we push to hw and what we don't
<javierm>
sima: yeah
<javierm>
sima: also, _rpm (I understand is for runtime power management) is a terrible name
<sima>
javierm, mripard, mlankhorst I thought we've done this already, but apparently not: ack for adding ogabbay to drm-misc for pushing the occasional accel patch?
<javierm>
because is not only about rpm but also about DRM_PLANE_COMMIT_ACTIVE_ONLY
<sima>
javierm, well it's just the flow you want for a driver doing rpm
<javierm>
sima: yes but even if you don't have rpm support in the driver, it may not want planes updates when disabled
<sima>
where you can only update planes when the crtc is on, which both needs a) plane commit after crtc changes and b) active_only
<javierm>
sima: so _active_only would be a better name and keep the kernel-doc that explains that is what drivers that support rpm want
<sima>
javierm, yeah that's why I think we should just make it the default, because it's the less surprising one of the two we have
<javierm>
sima: 100%
<sima>
javierm, hm yeah, but gets a bit a long name
<sima>
so maybe a patch set that 1. sets the current default for all the drivers that use it 2. flips the default to _rpm 3. removes _rpm explicitly set from all drivers currently doing that 4. rename to a more descriptive but longer name?
<sima>
maybe in 4. also mention the 2 per-crtc plane commit functions, since they might be needed
Haaninjo has joined #dri-devel
<sima>
and perhaps a few notes that helpers by default add all planes when a crtc has a modeset
<javierm>
sima: 5. everyone yells at you for breaking a few CIs :P
<sima>
since o.g. atomic helpers didn't have that, and I think some of the confusion is from those times 8+ years ago :-/
<sima>
javierm, well more for breaking backports, but yes
<javierm>
but agree that the current behaviour is counter intuitive. That's why I broke Geert workflow since he was using the panel for fbcon and displaying the linux logo
donaldrobson has quit [Remote host closed the connection]
andrey-k- has joined #dri-devel
rsalvaterra has quit []
rsalvaterra has joined #dri-devel
SuDo has joined #dri-devel
sgruszka has joined #dri-devel
pa has quit [Ping timeout: 480 seconds]
pa has joined #dri-devel
<sima>
PSA I just rolled drm-next to -rc2, in case anyone needs that
pa- has joined #dri-devel
<sima>
agd5f, tzimmermann mlankhorst mripard robclark jani dolphin tursulin ogabbay ^^ hope I got all the usual suspects
penguin42 has joined #dri-devel
simon-perretta-img has joined #dri-devel
pa has quit [Ping timeout: 480 seconds]
lemonzest has quit [Quit: WeeChat 3.6]
smiles_1111 has joined #dri-devel
SuDo has quit []
lemonzest has joined #dri-devel
<tzimmermann>
sima, thanks! i want to forward -misc-next after the PR has been merged
<sima>
tzimmermann, ah I can look at that too
<javierm>
tzimmermann: as mentioned in the thread and v2 shared, I believe that allocating in .fb_create callback and taking into account the format as Geert mentioned is the correct approach
<javierm>
that way we only allocate when and what is needed and let the resources to be freed on device unbound
mvchtz is now known as Guest6228
mvchtz has joined #dri-devel
<javierm>
tzimmermann: that should work from an atomic state management POV right ?
Guest6228 has quit [Ping timeout: 480 seconds]
kts has joined #dri-devel
<sima>
javierm, hm allocating in fb_create sounds a bit strange ... I'd expect drivers to either allocate in gem_create or in prepare_fb
<sima>
the drm_framebuffer really should be a metadata container only, plus fb_create should try to validate as many constraints as possible
<sima>
or maybe I'm just completely lost :-)
<pq>
Is there any KMS UAPI doc saying KMS object IDs are totally volatile and not persistent in any intended way?
<pq>
I have a "why don't we just make persistent connector names from CRTC IDs?"
<pq>
...not to mention that CRTC-connector association is not necessarily 1:1
<pq>
or fixed
alyssa has joined #dri-devel
frieder has quit [Ping timeout: 480 seconds]
Duke`` has quit [Ping timeout: 480 seconds]
frieder has joined #dri-devel
<javierm>
sima: I think you are correct
jagan_ has joined #dri-devel
<sima>
pq, hm we don't have docs for that I think :-(
<sima>
pq, I think the rule we could document is that for a connector either the PATH property, or if that doesn't exist, the connector's name can be assumed to be stable by userspace for configuration management purposes
<sima>
anything else is indeed not stable
<sima>
or at least it would be great if userspace treats it as unstable, because I don't really want to handle those kind of regressions
Duke`` has joined #dri-devel
<sima>
javierm, might be good to augment the kerneldoc for drm_mode_config_funcs.fb_create and link to the other two hooks as guideline?
fab has quit [Quit: fab]
<sima>
pq, I guess for plane/crtc/encoder id uapi doc it would be good to add a disclaimer that userspace should treat them as unstable, and select suitable objects by looking at their properties/features
Duke`` has quit [Remote host closed the connection]
<sima>
and for connector's id spell the PATH/name rule out?
<javierm>
sima, tzimmermann: by reading the kernel docs it seems that struct drm_plane_helper_funcs .begin_fb_access and .end_fb_access are the correct hooks to allocate/free these buffers ?
<javierm>
sima: or .prepare_fb and .cleanup_fb as you suggested but it looks to me that the former are more suitable?
<alyssa>
mareko: --> gfxstrand
<alyssa>
gfxstrand: punting that to you. if you're ok with it, i'll retract my objection. if you're not, hopefully you'll have input on a better approach
itoral has quit [Remote host closed the connection]
YuGiOhJCJ has joined #dri-devel
Dr_Who has joined #dri-devel
<pq>
sima, there is no connector's name in UAPI, and it's not stable either, right?
<pq>
or did connector_type_id counter get converted into something more stable than a global counter shared by all driver instances probing at the same time?
<emersion>
i think it's per-device
<pq>
that would be new
<emersion>
yes, the counter is drm_mode_config.connector_ida
<emersion>
that thing is per device
<emersion>
afaik
<emersion>
hmmm
<emersion>
wait, nevermind
<pq>
I think there was IRC talk about maybe making it more reliable, but I presume it was left at just talk.
<emersion>
there are two ida_alloc, one for connector index, one for connector_type_id
<emersion>
and yeah, type_id is global
<emersion>
the connector index can still be used
<emersion>
that should be stable
<pq>
yes, using the index in a connector (resource?) array was a suggestion
<emersion>
and it's what my wlr patch is doing
<pq>
but connector names are traditionally manufactured from connector_type_id
<emersion>
the connector name is not stable
<pq>
yes
<emersion>
connector path, then fallback to index, should be
<pq>
combined with device path, yes
<emersion>
yup
<pq>
good, we still agree, and I didn't miss something over the years :-)
elongbug has joined #dri-devel
<emersion>
yup, sorry, i misremembered the details
frieder has joined #dri-devel
<pq>
np
<pq>
soo... did we have connector names in sysfs? :-p
<pq>
yes, we do
<pq>
I suspect it's the name unstable connector name there
<sima>
pq, oops I assumed something exists that doesn't ...
kts has quit [Ping timeout: 480 seconds]
<pq>
sima, welll... does sysfs count?
<sima>
pq, emersion so first thing is that I think we indeed once had a global id assigner for connector ids or something like that
<emersion>
pq, the DRM name == sysfs name is unique but not stable, indeed
<pq>
there is no way to associate a /sys/class/drm/card1/card1-DP-2 with a KMS connector ID, is there?
<emersion>
DRM name as in drm_connector.base.name, used in logs, not exposed to user-space
<emersion>
apart from sysfs
<sima>
hm we still do actually
<pq>
:-D
<emersion>
pq, type + type_id no?
<pq>
emersion, d'oh, right. Manufacturing the kernel's name and looking for that.
<emersion>
you need to have the function which converts a type to a string, we have a copy in libdrm but might lag behind
<pq>
yeah
tyalie has joined #dri-devel
<pq>
sima, someone even sent patches to add PATH property for every connector to fix this, but I don't think it went anywhere.
<sima>
yeah I think connector->name (kernel-internal) respectively sysfs name file is fairly unstable too
<emersion>
i think there were some concerns wrt. userspace assuming PATH means DP MST :/
<emersion>
but yeah PATH for all would be nice
<pq>
4 years later...
<pq>
damn, time flies
fab has joined #dri-devel
<sima>
uh the patch to improve mst PATH docs would be good at least ...
<sima>
well maybe with some link to point at which exact connector id is used, since we have the kms obj id and the connector type id
frieder has quit [Ping timeout: 480 seconds]
<Company>
is there something like godbolt for GLSL => SPIRV or GLSL => nir?
<Company>
godbolt has HLSL => llvm il but I'm wondering if there's something better
heat has joined #dri-devel
kzd has joined #dri-devel
mbrost has joined #dri-devel
frieder has joined #dri-devel
heat has quit [Remote host closed the connection]
<gfxstrand>
alyssa, mareko: I think alyssa's argument about how this is the most conservative possible thing has me about won over.
<gfxstrand>
Give me a few more hours to let it all settle in my brain (it's still early on a monday) and I'll give a full RB.
heat has joined #dri-devel
mbrost_ has joined #dri-devel
frieder has quit [Ping timeout: 480 seconds]
mbrost has quit [Ping timeout: 480 seconds]
heat_ has joined #dri-devel
heat has quit [Read error: Connection reset by peer]
frieder has joined #dri-devel
kts has joined #dri-devel
kugel is now known as Guest6245
kugel has joined #dri-devel
Guest6245 has quit [Ping timeout: 480 seconds]
sgruszka has quit [Ping timeout: 480 seconds]
heat_ has quit [Remote host closed the connection]
heat has joined #dri-devel
frieder has quit [Remote host closed the connection]
<alyssa>
enunes: did you get a chance to look at nir_reg stuff?
<alyssa>
do you need me to do the lima patches?
jkrzyszt has quit [Ping timeout: 480 seconds]
<sima>
tzimmermann, drm-misc-next pr pulled and pushed out
<sima>
airlied, ^^ fyi
<tzimmermann>
thanks
<sima>
airlied, the backlight fbdev->dev refactor also touches some drivers in arch and some platform_data, and the screen_info.h refactor also touches some arch stuff
<sima>
airlied, should we have some shared gdoc stuff for these kind of things that need to go into the pr to linus?
<grillo_0>
tzimmermann, I started making a patch addressing the TODO about the multi-plane support on `drm_format_helper.c`. Did you have any drivers in mind that would benefit from this when you wrote the TODO?
<tzimmermann>
grillo_0, there are none AFAIK
tzimmermann has quit [Quit: Leaving]
<enunes>
alyssa: I started looking into it, so far broke everything with the new intrinsics and am going through them with deqp
<enunes>
this week I should have more time to finish that and send the patches
<alyssa>
enunes: awesome, thanks!
<alyssa>
if you post the patches I can eyeball them now to see if anything looks obviously wrong
<alyssa>
maybe save some debug time
<grillo_0>
Oh ok, so maybe is not worth solving this TODO if no driver will use it rn.
<tyalie>
hi. I would like to add a 32bit floating point single channel video format to the fourcc codes. I'm here to test the waters a bit before posting on the ml
<eric_engestrom>
Company: for dumping NIR, Mesa has NIR_DEBUG=print
<eric_engestrom>
not as interactive, but it's better than nothing
<Company>
both of those are pretty much exactly what I was looking for
<Company>
awesome
<sima>
javierm, yeah that's defo a lot better, allocating anything in the commit paths past the point of no return is no-no (and I thought it's documented in the right places, and kinda should be implied by the void return values everywhere ...)
cmichael has quit [Quit: Leaving]
<emersion>
ekurzinger: i believe this new IOCTL would be mostly useful for compositors, and for implementing a Vulkan/GL ext to import/export syncobjs
<gfxstrand>
Oh, someone should make that an MR so we can stick it in our tracker.
mbrost_ has quit [Read error: Connection reset by peer]
djbw has joined #dri-devel
kts has quit [Quit: Konversation terminated!]
benjamin1 has joined #dri-devel
isinyaaa[m] has joined #dri-devel
benjaminl has quit [Ping timeout: 480 seconds]
DPA2 has joined #dri-devel
DPA has quit [Ping timeout: 480 seconds]
heat has quit [Read error: Connection reset by peer]
heat has joined #dri-devel
junaid has quit [Quit: leaving]
junaid has joined #dri-devel
junaid has quit []
junaid has joined #dri-devel
DPA has joined #dri-devel
DPA2 has quit [Ping timeout: 480 seconds]
tursulin has quit [Ping timeout: 480 seconds]
junaid has quit [Ping timeout: 480 seconds]
rasterman has quit [Quit: Gettin' stinky!]
DPA has quit [Ping timeout: 480 seconds]
<mdnavare>
vsyrjala: emersion : Looking at how DRM adds EDID modes, looking at how hactive and vactive are populated, it only takes 12 bits so the max hdisplay would be 4095, how does DRM handle 5k and 8k modes?
<mdnavare>
+gildekel
<gildekel>
Thanks mdnavare
<emersion>
sorry, i am not familiar with this code
<emersion>
the uAPI uses more than 12 bits i believe
<alyssa>
instr->dest.write_mask is unused in the new approach
<alyssa>
this should be legacy_dest.write_mask
<alyssa>
enunes: also, lima/pp uses func->reg_alloc, but that's no longer valid in the new approach
<alyssa>
change to func->ssa_alloc
<alyssa>
(since you're using the handle->index as the reg indices)
<alyssa>
with those 2 fixes, things /should/ work for lima/pp. I hope :)
<cambrian_invader>
well, disabling the check causes problems
<cambrian_invader>
so I guess whatever's allocating the buffer needs to add some lines at the end?
<DavidHeidelberg[m]>
mupuf: do you plan to do the CI workshop on XDC 2023, as on 2022 again?
<DavidHeidelberg[m]>
gallo: ^
agd5f has quit [Read error: Connection reset by peer]
fab has quit [Quit: fab]
Mamta__ has quit [Quit: Connection closed for inactivity]
smiles_1111 has joined #dri-devel
<anarsoul|2>
cambrian_invader: kernel isn't really aware of BO width or height, it cares about size. However if you want to render into BO it needs to be aligned to tile boundaries
<anarsoul|2>
tile on Utgard is 16x16
<anarsoul|2>
lima ensures that BOs that it allocates for rendering or for sampling are aligned properly. If you need to import an BO, it's a job of the driver that allocates the BO to ensure that it's compatible with lima (or rather with HW)
<anarsoul|2>
IIRC you'll get ppmmu fault if you attempt to render into a buffer that is not aligned to tile boundaries