Re: [Freedreno] [PATCH RFC v6 07/10] drm/atomic: Loosen FB atomic checks
Hi Jessica,
On Tue, 17 Oct 2023 at 03:41, Jessica Zhang wrote:
> On 9/24/2023 3:23 AM, Dmitry Baryshkov wrote:
> > On 22/09/2023 20:49, Jessica Zhang wrote:
> >>
> >>
> >> On 8/29/2023 1:22 AM, Pekka Paalanen wrote:
> >>> On Mon, 28 Aug 2023 17:05:13 -0700
> >>> Jessica Zhang wrote:
> >>>
> Loosen the requirements for atomic and legacy commit so that, in cases
> where pixel_source != FB, the commit can still go through.
>
> This includes adding framebuffer NULL checks in other areas to
> account for
> FB being NULL when non-FB pixel sources are enabled.
>
> To disable a plane, the pixel_source must be NONE or the FB must be
> NULL
> if pixel_source == FB.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic.c| 20 +++-
> drivers/gpu/drm/drm_atomic_helper.c | 36
>
> include/drm/drm_atomic_helper.h | 4 ++--
> include/drm/drm_plane.h | 29
> +
> 4 files changed, 62 insertions(+), 27 deletions(-)
> >>>
> >>> ...
> >>>
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index a58f84b6bd5e..4c5b7bcdb25c 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -992,6 +992,35 @@ static inline struct drm_plane
> *drm_plane_find(struct drm_device *dev,
> #define drm_for_each_plane(plane, dev) \
> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
> +/**
> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on
> plane
> + * @state: plane state
> + *
> + * Returns:
> + * Whether the plane has been assigned a solid_fill_blob
> + */
> +static inline bool drm_plane_solid_fill_enabled(struct
> drm_plane_state *state)
> +{
> +if (!state)
> +return false;
> +return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL
> && state->solid_fill_blob;
> +}
> +
> +static inline bool drm_plane_has_visible_data(const struct
> drm_plane_state *state)
> +{
> +switch (state->pixel_source) {
> +case DRM_PLANE_PIXEL_SOURCE_NONE:
> +return false;
> +case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL:
> +return state->solid_fill_blob != NULL;
> >>>
> >>> This reminds me, new UAPI docs did not say what the requirements are for
> >>> choosing solid fill pixel source. Is the atomic commit rejected if
> >>> pixel source is solid fill, but solid_fill property has no blob?
> >>
> >> Hi Pekka,
> >>
> >> Yes, if pixel_source is solid_fill and the solid_fill property blob
> >> isn't set, the atomic commit should throw an error.
> >>
> >> Will document this in the UAPI.
> >
> > I don't see a corresponding error check in atomic_check() functions.
> > Could you please check that there is one, as you are updating the uAPI.
>
> Hi Dmitry,
>
> Sorry for the late response.
No worries.
>
> drm_plane_has_visible_data() is being called from
> drm_atomic_plane_check() which is called from drm_atomic_commit() (via
> drm_atomic_check_only()).
>
> It's also called within the atomic_check() callstack in
> drm_atomic_helper_check_plane_state(), though that check will set
> plane.visible to false and return 0.
>
> Would it be better to have a more explicit `if (source == solid_fill &&
> !plane->solid_fill_blob) then return -EINVAL` check in atomic_check()?
No. Your current code is good already. It was me who missed the
visible data check.
If you are going to send the next version for some reason, it might be
good to add a small comment to drm_atomic_helper_check_plane_state().
Something like 'check that the selected pixel source (fb, solid fill,
etc.) is valid'.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >>
> >> Thanks,
> >>
> >> Jessica Zhang
> >>
> >>>
> >>> This should be doc'd.
> >>>
> >>>
> >>> Thanks,
> >>> pq
> >>>
> +case DRM_PLANE_PIXEL_SOURCE_FB:
> +default:
> +WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB);
> +}
> +
> +return state->fb != NULL;
> +}
> +
> bool drm_any_plane_has_format(struct drm_device *dev,
> u32 format, u64 modifier);
>
> >>>
> >
> > --
> > With best wishes
> > Dmitry
> >
--
With best wishes
Dmitry
Re: [PATCH v6 6/6] drm/doc: Define KMS atomic state set
On Mon, Oct 16, 2023 at 10:00:51PM +, Simon Ser wrote: > On Monday, October 16th, 2023 at 17:10, Ville Syrjälä > wrote: > > > On Mon, Oct 16, 2023 at 05:52:22PM +0300, Pekka Paalanen wrote: > > > > > On Mon, 16 Oct 2023 15:42:16 +0200 > > > André Almeida andrealm...@igalia.com wrote: > > > > > > > Hi Pekka, > > > > > > > > On 10/16/23 14:18, Pekka Paalanen wrote: > > > > > > > > > On Mon, 16 Oct 2023 12:52:32 +0200 > > > > > André Almeida andrealm...@igalia.com wrote: > > > > > > > > > > > Hi Michel, > > > > > > > > > > > > On 8/17/23 12:37, Michel Dänzer wrote: > > > > > > > > > > > > > On 8/15/23 20:57, André Almeida wrote: > > > > > > > > > > > > > > > From: Pekka Paalanen pekka.paala...@collabora.com > > > > > > > > > > > > > > > > Specify how the atomic state is maintained between userspace and > > > > > > > > kernel, plus the special case for async flips. > > > > > > > > > > > > > > > > Signed-off-by: Pekka Paalanen pekka.paala...@collabora.com > > > > > > > > Signed-off-by: André Almeida andrealm...@igalia.com > > > > > > > > [...] > > > > > > > > > > > > > > > +An atomic commit with the flag DRM_MODE_PAGE_FLIP_ASYNC is > > > > > > > > allowed to > > > > > > > > +effectively change only the FB_ID property on any planes. > > > > > > > > No-operation changes > > > > > > > > +are ignored as always. [...] > > > > > > > > During the hackfest in Brno, it was mentioned that a commit > > > > > > > > which re-sets the same FB_ID could actually have an effect with > > > > > > > > VRR: It could trigger scanout of the next frame before vertical > > > > > > > > blank has reached its maximum duration. Some kind of mechanism > > > > > > > > is required for this in order to allow user space to perform > > > > > > > > low frame rate compensation. > > > > > > > > > > > > Xaver tested this hypothesis in a flipping the same fb in a VRR > > > > > > monitor > > > > > > and it worked as expected, so this shouldn't be a concern. > > > > > > Right, so it must have some effect. It cannot be simply ignored > > > > > > like in > > > > > > the proposed doc wording. Do we special-case re-setting the same > > > > > > FB_ID > > > > > > as "not a no-op" or "not ignored" or some other way? > > > > > > There's an effect in the refresh rate, the image won't change but it > > > > > > will report that a flip had happened asynchronously so the reported > > > > > > framerate will be increased. Maybe an additional wording could be > > > > > > like: > > > > > > > > Flipping to the same FB_ID will result in a immediate flip as if it was > > > > changing to a different one, with no effect on the image but effecting > > > > the reported frame rate. > > > > > > Re-setting FB_ID to its current value is a special case regardless of > > > PAGE_FLIP_ASYNC, is it not? > > > > No. The rule has so far been that all side effects are observed > > even if you flip to the same fb. And that is one of my annoyances > > with this proposal. The rules will now be different for async flips > > vs. everything else. > > Well with the patches the async page-flip case is exactly the same as > the non-async page-flip case. In both cases, if a FB_ID is included in > an atomic commit then the side effects are triggered even if the property > value didn't change. The rules are the same for everything. I see it only checking if FB_ID changes or not. If it doesn't change then the implication is that the side effects will in fact be skipped as not all planes may even support async flips. -- Ville Syrjälä Intel
Sub 16ms render but missing swap
In a GLES app wired up to Weston/Wayland using EGL, we're calling `eglSwapBuffers` in a render loop that takes about 12-13ms to draw our content, based on CPU timing the run loop. However, the call to swap incurs a duration of around 20ms, meaning we're only hitting 30fps. I've investigated a little bit into using the presentation callback protocol, but my understanding is that the EGL display (part of drm-wayland in the mesa source IIRC) is itself already plugged in to the necessary Wayland callbacks such that using `eglSwapBuffers` is sufficient to maximize available draw time. I also tried enabling the profile flag in weston.ini that allows you to collect a trace, viewable in *wesgr*. And I read Pekka's blog posts about these diagrams, but I still can't understand if our app is doing something suboptimally. I can't decipher what the wesgr diagram is saying. The other weston.ini knob I've tried is the `repaint` window, setting it to 12 or 13 to match our drawing loop. This seems to help sometimes but not in this case. A few questions: 1. What other avenues of investigation should I pursue for the swap delay? As in, why when I take 12 ms to render do I not see about 4ms for the swap call to return? My display is running in at 60hz. 2. Has EGL been optimized to use the available wayland callbacks and maximize available client drawing time? 3. Does EGL leverage "weston_direct_display_v1" when available? What's required to take advantage of it in the app code? (ie. run fullscreen?) Thanks!
Re: [PATCH v1] dynamic_debug: add support for logs destination
adding in Jason, not sure how he got missed. On Mon, Oct 16, 2023 at 9:13 AM Łukasz Bartosik wrote: > > czw., 12 paź 2023 o 20:48 napisał(a): > > > > > If you want the kernel to keep separate flight recorders I guess we could > > > add that, but I don't think it currently exists for the dyndbg stuff at > > > least. Maybe a flight recorder v2 feature, once the basics are in. > > > > > > > dyndbg has +pwrites to syslog > > +T would separately independently write the same to global trace > > > > This would allow graceful switchover to tracefs, > > without removing logging from dmesg, where most folks > > (and any monitor tools) would expect it. > > > > Lukas (iiuc) wants to steer each site to just 1 destination. > > Or maybe (in addition to +p > syslog) one trace destination, > > either global via events, or a separate tracebuf > > > > Im ambivalent, but thinking the smooth rollover from syslog to trace > > might be worth having to ease migration / weaning off syslog. > > > > And we have a 4 byte hole in struct _ddebug we could just use. > > I'm glad you brought that up. This means we can leave class_id field > untouched, have separate +T in flags (for consistency) > and dst_id can be easily 8 bits wide. > > Also can you point me to the latest version of writing debug logs to > trace events (+T option). > I would like to base trace instances work on that because both are > closely related. > Ive got 3 branches, all stacked up on rc6 (last I checked, they were clean on drm-tip) https://github.com/jimc/linux/tree/dd-fix-7c the regression fix, reposting this version next. it also grabs another flag bit: _DPRINTK_FLAGS_INCL_LOOKUP https://github.com/jimc/linux/tree/dd-shrink-3b split modname,filename,function to new __dyndbg_sites section loads the 3 column values and their intervals into 3 maple trees and implements 3 accessor functions to retrieve the vals from the descriptor addresses. it "works" but doesnt erase intervals properly on rmmod it also claims another flag - _DPRINTK_FLAGS_PREFIX_CACHED and uses it to mark cached prefix fragment that get created for enabled calls. https://github.com/jimc/linux/tree/dd-kitchen-sink this adds the +T flag stuff. its still a little fuzzy, esp around the descriptor arg - using it to render the prefix str at buffer-render time got UNSAFE warnings - likely due to loadable module descriptors which could or did go away between capture and render (after rmmod) > > Unless the align 8 is optional on 32-bits, > > I verified with "gcc -g -m32 ..." that the align(8) is honored on 32 bits. > this is sorta the opposite of what I was probing, but I think result is the same. istm align(8) is only for JUMP_LABEL, nothing else in the struct appears to need it so moving it to the top trades the hole for padding. > > I think we're never gonna close the hole anywhere. > > > > :) > > > is align 8 a generic expression of an architectural simplifying constraint ? > > or a need for 1-7 ptr offsets ? > > > > > > > > > > > > That's my idea of it. It is interesting to see how far the requirements > > > > can be reasonably realised. > > > > > > I think aside from the "make it available directly to unpriviledged > > > userspace" everything sounds reasonable and doable. > > > > > > More on the process side of things, I think Jim is very much looking for > > > acks and tested-by by people who are interested in better drm logging > > > infra. That should help that things are moving in a direction that's > > > actually useful, even when it's not yet entirely complete. > > > > > > > yes, please. Now posted at > > > > https://lore.kernel.org/lkml/20231012172137.3286566-1-jim.cro...@gmail.com/T/#t > > > > Lukas, I managed to miss your email in the send phase. > > please consider yourself a direct recipient :-) > > > > thanks everyone > > > > > Cheers, Sima > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch
