Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
On Tue, Oct 29, 2024 at 04:38:34PM -0400, Johannes Weiner wrote: > On Mon, Oct 28, 2024 at 11:05:48AM +0100, Maxime Ripard wrote: > > On Thu, Oct 24, 2024 at 07:06:36AM -1000, Tejun Heo wrote: > > > Hello, > > > > > > On Thu, Oct 24, 2024 at 09:20:43AM +0200, Maxime Ripard wrote: > > > ... > > > > > Yeah, let's not use "dev" name for this. As Waiman pointed out, it > > > > > conflicts > > > > > with the devices controller from cgroup1. While cgroup1 is mostly > > > > > deprecated, the same features are provided through BPF in systemd > > > > > using the > > > > > same terminologies, so this is going to be really confusing. > > > > > > > > Yeah, I agree. We switched to dev because we want to support more than > > > > just DRM, but all DMA-able memory. We have patches to support for v4l2 > > > > and dma-buf heaps, so using the name DRM didn't feel great either. > > > > > > > > Do you have a better name in mind? "device memory"? "dma memory"? > > > > > > Maybe just dma (I think the term isn't used heavily anymore, so the word > > > is > > > kinda open)? But, hopefully, others have better ideas. > > > > > > > > What happened with Tvrtko's weighted implementation? I've seen many > > > > > proposed > > > > > patchsets in this area but as far as I could see none could establish > > > > > consensus among GPU crowd and that's one of the reasons why nothing > > > > > ever > > > > > landed. Is the aim of this patchset establishing such consensus? > > > > > > > > Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red > > > > Hat have been involved in that series and we all agree on the > > > > implementation. > > > > > > That's great to hear. > > > > > > > Tvrtko aims at a different feature set though: this one is about memory > > > > allocation limits, Tvrtko's about scheduling. > > > > > > > > Scheduling doesn't make much sense for things outside of DRM (and even > > > > for a fraction of all DRM devices), and it's pretty much orthogonal. So > > > > i guess you can expect another series from Tvrtko, but I don't think > > > > they should be considered equivalent or dependent on each other. > > > > > > Yeah, I get that this is about memory and that is about processing > > > capacity, > > > so the plan is going for separate controllers for each? Or would it be > > > better to present both under the same controller interface? Even if > > > they're > > > going to be separate controllers, we at least want to be aligned on how > > > devices and their configurations are presented in the two controllers. > > > > It's still up in the air, I think. > > > > My personal opinion is that there's only DRM (and accel) devices that > > really care about scheduling constraints anyway, so it wouldn't (have > > to) be as generic as this one. > > If they represent different resources that aren't always controlled in > conjunction, it makes sense to me to have separate controllers as well. > > Especially if a merged version would have separate control files for > each resource anyway (dev.region.*, dev.weight etc.) > > > And if we would call it dma, then the naming becomes a bit weird since > > DMA doesn't have much to do with scheduling. > > > > But I guess it's just another instance of the "naming is hard" problem :) > > Yes it would be good to have something catchy, easy on the eyes, and > vaguely familiar. devcomp(ute), devproc, devcpu, devcycles all kind of > suck. drm and gpu seem too specific for a set that includes npus and > potentially other accelerators in the future. > > I don't think we want to go full devspace & devtime, either, though. > > How about dmem for this one, and dpu for the other one. For device > memory and device processing unit, respectively. dmem sounds great to me, does everyone agree? Maxime signature.asc Description: PGP signature
Re: (subset) [PATCH v5 00/13] Add ITE IT6263 LVDS to HDMI converter support
On Tue, Nov 05, 2024 at 05:33:21PM +, Dmitry Baryshkov wrote:
> On 5 November 2024 16:13:26 GMT, Maxime Ripard wrote:
> >On Tue, Nov 05, 2024 at 01:28:48PM +0200, Dmitry Baryshkov wrote:
> >> On Mon, 04 Nov 2024 11:27:53 +0800, Liu Ying wrote:
> >> > This patch series aims to add ITE IT6263 LVDS to HDMI converter on
> >> > i.MX8MP EVK. Combined with LVDS receiver and HDMI 1.4a transmitter,
> >> > the IT6263 supports LVDS input and HDMI 1.4 output by conversion
> >> > function. IT6263 product link can be found at [1].
> >> >
> >> > Patch 1 is a preparation patch to allow display mode of an existing
> >> > panel to pass the added mode validation logic in patch 3.
> >> >
> >> > [...]
> >>
> >> Applied to drm-misc-next, thanks!
> >>
> >> [04/13] media: uapi: Add MEDIA_BUS_FMT_RGB101010_1X7X5_{SPWG, JEIDA}
> >> commit: 5205b63099507a84458075c3ca7e648407e6c8cc
> >
> >Where's the immutable branch Laurent asked for?
>
> The patch set has been picked up after getting an Ack from Sakari,
> before Laurent's email. I am sorry if I rushed it in.
I mean, this was less than a day after you've asked that question
yourself. Waiting less than a day for a mail to be answered seems a bit
short, especially when there's no rush to merge these patches in the
first place.
Maxime
signature.asc
Description: PGP signature
Re: (subset) [PATCH v5 00/13] Add ITE IT6263 LVDS to HDMI converter support
On Tue, Nov 05, 2024 at 01:28:48PM +0200, Dmitry Baryshkov wrote:
> On Mon, 04 Nov 2024 11:27:53 +0800, Liu Ying wrote:
> > This patch series aims to add ITE IT6263 LVDS to HDMI converter on
> > i.MX8MP EVK. Combined with LVDS receiver and HDMI 1.4a transmitter,
> > the IT6263 supports LVDS input and HDMI 1.4 output by conversion
> > function. IT6263 product link can be found at [1].
> >
> > Patch 1 is a preparation patch to allow display mode of an existing
> > panel to pass the added mode validation logic in patch 3.
> >
> > [...]
>
> Applied to drm-misc-next, thanks!
>
> [04/13] media: uapi: Add MEDIA_BUS_FMT_RGB101010_1X7X5_{SPWG, JEIDA}
> commit: 5205b63099507a84458075c3ca7e648407e6c8cc
Where's the immutable branch Laurent asked for?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism
On Tue, Nov 05, 2024 at 09:15:03AM +0100, Herve Codina wrote:
> Hi Maxime, Laurent,
>
> On Mon, 28 Oct 2024 16:09:13 +0200
> Laurent Pinchart wrote:
>
> > On Mon, Oct 28, 2024 at 02:55:47PM +0100, Maxime Ripard wrote:
> > > On Mon, Oct 28, 2024 at 03:28:58PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Oct 28, 2024 at 01:21:45PM +0100, Maxime Ripard wrote:
> > > > > On Mon, Oct 28, 2024 at 01:28:57PM +0200, Laurent Pinchart wrote:
> > > > > > On Mon, Oct 28, 2024 at 09:13:31AM +0100, Herve Codina wrote:
> > > > > > > On Sun, 27 Oct 2024 18:23:50 +0200 Laurent Pinchart wrote:
> > > > > > >
> > > > > > > [...]
> > > > > > > > > +static int sn65dsi83_reset_pipeline(struct sn65dsi83
> > > > > > > > > *sn65dsi83)
> > > > > > > > > +{
> > > > > > > > > + struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > > > > + struct drm_modeset_acquire_ctx ctx;
> > > > > > > > > + struct drm_atomic_state *state;
> > > > > > > > > + int err;
> > > > > > > > > +
> > > > > > > > > + /* Use operation done in drm_atomic_helper_suspend()
> > > > > > > > > followed by
> > > > > > > > > + * operation done in drm_atomic_helper_resume() but
> > > > > > > > > without releasing
> > > > > > > > > + * the lock between suspend()/resume()
> > > > > > > > > + */
> > > > > > > > > +
> > > > > > > > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > > > > > +
> > > > > > > > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > > > > + if (IS_ERR(state)) {
> > > > > > > > > + err = PTR_ERR(state);
> > > > > > > > > + goto unlock;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + err = drm_atomic_helper_disable_all(dev, &ctx);
> > > > > > > > > + if (err < 0)
> > > > > > > > > + goto unlock;
> > > > > > > > > +
> > > > > > > > > + drm_mode_config_reset(dev);
> > > > > > > > > +
> > > > > > > > > + err = drm_atomic_helper_commit_duplicated_state(state,
> > > > > > > > > &ctx);
> > > > > > > >
> > > > > > > > Committing a full atomic state from a bridge driver in an
> > > > > > > > asynchronous
> > > > > > > > way seems quite uncharted territory, and it worries me. It's
> > > > > > > > also a very
> > > > > > > > heavyweight, you disable all outputs here, instead of focussing
> > > > > > > > on the
> > > > > > > > output connected to the bridge. Can you either implement
> > > > > > > > something more
> > > > > > > > local, resetting the bridge only, or create a core helper to
> > > > > > > > handle this
> > > > > > > > kind of situation, on a per-output basis ?
> > > > > > >
> > > > > > > A full restart of the bridge (power off/on) is needed and so we
> > > > > > > need to
> > > > > > > redo the initialization sequence. This initialization sequence
> > > > > > > has to be
> > > > > > > done with the DSI data lanes (bridge inputs) driven in LP11 state
> > > > > > > and so
> > > > > > > without any video stream. Only focussing on bridge outputs will
> > > > > > > not be
> > > > > > > sufficient. That's why I brought the pipeline down and restarted
> > > > > > > it.
> > > > > >
> > > > > > Fair point.
> > > > > >
> > > > > > > Of course, I can copy/paste sn65dsi83_reset_pipeline() to a core
> > > > > > > helper
> > > > > > > function. Is drm_atomi
[PATCH] drm/tests: hdmi: Fix WW_MUTEX_SLOWPATH failures
The light_up_connector helper function in the HDMI infrastructure unit
tests uses drm_atomic_set_crtc_for_connector(), but fails when it
returns an error.
This function can return EDEADLK though if the sequence needs to be
restarted, and WW_MUTEX_SLOWPATH is meant to test that we handle it
properly.
Let's handle EDEADLK and restart the sequence in our tests as well.
Fixes: eb66d34d793e ("drm/tests: Add output bpc tests")
Closes:
https://lore.kernel.org/r/CAPM=9tzJ4-ERDxvuwrCyUPY0=+p44orhp1klwvgl7mcfpqj...@mail.gmail.com/
Reported-by: Dave Airlie
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 34ee95d41f29..e2441092a8e9 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -68,14 +68,21 @@ static int light_up_connector(struct kunit *test,
int ret;
state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+retry:
conn_state = drm_atomic_get_connector_state(state, connector);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
ret = drm_atomic_set_crtc_for_connector(conn_state, crtc);
+ if (ret == -EDEADLK) {
+ drm_atomic_state_clear(state);
+ ret = drm_modeset_backoff(ctx);
+ if (!ret)
+ goto retry;
+ }
KUNIT_EXPECT_EQ(test, ret, 0);
crtc_state = drm_atomic_get_crtc_state(state, crtc);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
--
2.47.0
Re: lockdep and ww mutex debug interactions in hdmi tests
Hi, On Wed, Oct 30, 2024 at 05:03:50AM +1000, Dave Airlie wrote: > Hi, > > I mentioned this internally, but wanted to get it on the list, > > I ran the hdmi kunit tests with LOCKDEP and WW_MUTEX_SLOWPATH enabled > and hit some issues. > > With the slowpath we get the occasional EDEADLK to test the paths are > doing things right, I think you should handle EDEADLK in the tests > with a retry loop. Thanks for the report, I've just sent a patch fixing this. The vc4 have the same issue though, and I haven't been able to fix all of them yet. Maxime signature.asc Description: PGP signature
Re: Requirements to merge new heaps in the kernel
On Wed, Oct 30, 2024 at 12:16:22PM +0100, metux wrote: > On 22.10.24 10:38, Maxime Ripard wrote: > > I'm still interested in merging a carve-out driver[1], since it seems to be > > in every vendor BSP and got asked again last week. > > > > I remember from our discussion that for new heap types to be merged, we > > needed a kernel use-case. Looking back, I'm not entirely sure how one > > can provide that given that heaps are essentially facilities for > > user-space. > > For those who didn't follow your work, could you please give a short > intro what's that all about ? > > If I understand you correctly, you'd like the infrastructure of > kmalloc() et al for things / memory regions that aren't the usual heap, > right ? No, not really. The discussion is about dma-buf heaps. They allow to allocate buffers suitable for DMA from userspace. It might or might not from the system memory, at the heap driver discretion. > What's the practical use case ? GPU memory ? Shared memory between > nodes in a multi-CPU / cluster machine ? > > Is it related to NUMA ? And since it's about DMA, it doesn't have much to do with CPUs either. Maxime signature.asc Description: PGP signature
Re: [PATCH v4 0/3] drm/tests: Fix some memory leaks
On Wed, 30 Oct 2024 10:35:01 +0800, Jinjie Ruan wrote: > Fix some memory leaks in drm tests. > > Changes in v4: > - Return NULL early if drm_display_mode_from_cea_vic() return NULL > for drm_kunit_display_mode_from_cea_vic() helper as Maxime suggested. > - Split out the separate ttm test patch. > > [...] Applied to misc/kernel.git (drm-misc-fixes). Thanks! Maxime
Re: [PATCH v4 03/13] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate
On Mon, Oct 28, 2024 at 10:37:30AM +0800, Liu Ying wrote: > Multiple display modes could be read from a display device's EDID. > Use clk_round_rate() to validate the "ldb" clock rate for each mode > in drm_bridge_funcs::mode_valid() to filter unsupported modes out. > > Also, since this driver doesn't directly reference pixel clock, use > clk_round_rate() to validate the pixel clock rate against the "ldb" > clock if the "ldb" clock and the pixel clock are sibling in clock > tree. This is not done in display controller driver because > drm_crtc_helper_funcs::mode_valid() may not decide to do the > validation or not if multiple encoders are connected to the CRTC, > e.g., i.MX93 LCDIF may connect with MIPI DSI controller, LDB and > parallel display output simultaneously. > > Signed-off-by: Liu Ying > --- > Note that this patch depends on a patch in shawnguo/imx/fixes: > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20241017031146.157996-1-ma...@denx.de/ I still believe that the root cause of this issue is your clock tree and driver setup, and since I've asked for explanations and didn't get any, I don't really see how we can move forward here. Maxime signature.asc Description: PGP signature
Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism
On Tue, Oct 29, 2024 at 04:02:33PM +0800, Andy Yan wrote:
> At 2024-10-28 00:23:50, "Laurent Pinchart"
> wrote:
> >On Thu, Oct 24, 2024 at 11:55:38AM +0200, Herve Codina wrote:
> >> In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
> >> from errors by itself. A full restart of the bridge is needed in those
> >> cases to have the bridge output LVDS signals again.
> >>
> >> The TI SN65DSI83 has some error detection capabilities. Introduce an
> >> error recovery mechanism based on this detection.
> >>
> >> The errors detected are signaled through an interrupt. On system where
> >> this interrupt is not available, the driver uses a polling monitoring
> >> fallback to check for errors. When an error is present, the recovery
> >> process is launched.
> >>
> >> Restarting the bridge needs to redo the initialization sequence. This
> >> initialization sequence has to be done with the DSI data lanes driven in
> >> LP11 state. In order to do that, the recovery process resets the entire
> >> pipeline.
> >>
> >> Signed-off-by: Herve Codina
> >> ---
> >> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 128 ++
> >> 1 file changed, 128 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >> index 96e829163d87..22975b60e80f 100644
> >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> >> @@ -35,9 +35,12 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >> +#include
> >>
> >> #include
> >> #include
> >> +#include /* DRM_MODESET_LOCK_ALL_BEGIN() need
> >> drm_drv_uses_atomic_modeset() */
> >> #include
> >> #include
> >> #include
> >> @@ -147,6 +150,9 @@ struct sn65dsi83 {
> >>struct regulator*vcc;
> >>boollvds_dual_link;
> >>boollvds_dual_link_even_odd_swap;
> >> + booluse_irq;
> >> + struct delayed_work monitor_work;
> >> + struct work_struct reset_work;
> >> };
> >>
> >> static const struct regmap_range sn65dsi83_readable_ranges[] = {
> >> @@ -321,6 +327,92 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> >>return dsi_div - 1;
> >> }
> >>
> >> +static int sn65dsi83_reset_pipeline(struct sn65dsi83 *sn65dsi83)
> >> +{
> >> + struct drm_device *dev = sn65dsi83->bridge.dev;
> >> + struct drm_modeset_acquire_ctx ctx;
> >> + struct drm_atomic_state *state;
> >> + int err;
> >> +
> >> + /* Use operation done in drm_atomic_helper_suspend() followed by
> >> + * operation done in drm_atomic_helper_resume() but without releasing
> >> + * the lock between suspend()/resume()
> >> + */
> >> +
> >> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> >> +
> >> + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> >> + if (IS_ERR(state)) {
> >> + err = PTR_ERR(state);
> >> + goto unlock;
> >> + }
> >> +
> >> + err = drm_atomic_helper_disable_all(dev, &ctx);
> >> + if (err < 0)
> >> + goto unlock;
> >> +
> >> + drm_mode_config_reset(dev);
> >> +
> >> + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> >
> >Committing a full atomic state from a bridge driver in an asynchronous
> >way seems quite uncharted territory, and it worries me. It's also a very
> >heavyweight, you disable all outputs here, instead of focussing on the
> >output connected to the bridge. Can you either implement something more
> >local, resetting the bridge only, or create a core helper to handle this
> >kind of situation, on a per-output basis ?
>
> If we could simulate a hotplug（disconnected then connected） event to
> user space and let userspace do the disable/enable of the output
> pipeline, would things be simpler?
No, because you can't expect the userspace to handle that event in the
first place.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3 1/4] drm/tests: helpers: Add helper for drm_display_mode_from_cea_vic()
On Thu, Oct 17, 2024 at 02:31:22PM +0800, Jinjie Ruan wrote:
> As Maxime suggested, add a new helper
> drm_kunit_display_mode_from_cea_vic(), it can replace the direct call
> of drm_display_mode_from_cea_vic(), and it will help solving
> the `mode` memory leaks.
>
> Acked-by: Maxime Ripard
> Suggested-by: Maxime Ripard
> Signed-off-by: Jinjie Ruan
> ---
> v3:
> - Adjust drm/drm_edid.h header to drm_kunit_helpers.c.
> - Drop the "helper" in the helper name.
> - Add Acked-by.
> ---
> drivers/gpu/drm/tests/drm_kunit_helpers.c | 40 +++
> include/drm/drm_kunit_helpers.h | 4 +++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index aa62719dab0e..565172990044 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -3,6 +3,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -311,6 +312,45 @@ drm_kunit_helper_create_crtc(struct kunit *test,
> }
> EXPORT_SYMBOL_GPL(drm_kunit_helper_create_crtc);
>
> +static void kunit_action_drm_mode_destroy(void *ptr)
> +{
> + struct drm_display_mode *mode = ptr;
> +
> + drm_mode_destroy(NULL, mode);
> +}
> +
> +/**
> + * drm_kunit_display_mode_from_cea_vic() - return a mode for CEA VIC
> +for a KUnit test
> + * @test: The test context object
> + * @dev: DRM device
> + * @video_code: CEA VIC of the mode
> + *
> + * Creates a new mode matching the specified CEA VIC for a KUnit test.
> + *
> + * Resources will be cleaned up automatically.
> + *
> + * Returns: A new drm_display_mode on success or NULL on failure
> + */
> +struct drm_display_mode *
> +drm_kunit_display_mode_from_cea_vic(struct kunit *test, struct drm_device
> *dev,
> + u8 video_code)
> +{
> + struct drm_display_mode *mode;
> + int ret;
> +
> + mode = drm_display_mode_from_cea_vic(dev, video_code);
I'd rather return directly if mode is NULL here...
> + ret = kunit_add_action_or_reset(test,
> + kunit_action_drm_mode_destroy,
> + mode);
> + if (ret)
> + return NULL;
Because it doesn't really make much sense to register a cleanup action
if we know that it's going to be useless, and possibly be executed right
away.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3 0/4] drm/tests: Fix some memory leaks
On Sat, Oct 26, 2024 at 10:02:51AM +0800, Jinjie Ruan wrote: > > > On 2024/10/25 22:33, Maxime Ripard wrote: > > On Wed, Oct 23, 2024 at 09:35:59AM +0800, Jinjie Ruan wrote: > >> > >> > >> On 2024/10/18 16:12, Jinjie Ruan wrote: > >>> > >>> > >>> On 2024/10/18 15:55, Maxime Ripard wrote: > >>>> Hi, > >>>> > >>>> On Thu, Oct 17, 2024 at 02:31:21PM GMT, Jinjie Ruan wrote: > >>>>> Fix some memory leaks in drm tests. > >>>>> > >>>>> Changes in v3: > >>>>> - Adjust drm/drm_edid.h header to drm_kunit_helpers.c. > >>>>> - Drop the "helper" in the helper name. > >>>>> - s/fllowing/following/ > >>>>> - Add Acked-by. > >>>> > >>>> This creates build failures since drm_display_mode were const before, > >>>> and can't anymore. > >>> > >>> It seems it came from bellowing v1, and this v3 has not reported the > >>> issue yet. > >>> > >>> https://lore.kernel.org/all/202410180830.oitxtsov-...@intel.com/ > >> > >> Hi, Maxime, > >> > >> Should this series send again? The issue seems not related to this version. > > > > As far as I know, the issues reported still apply there, so yes > > I make this version code with "C=2", there is no these build failures. Sorry, you're right. I still have a comment on the first patch Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/bridge: tc358767: Fix odd pixel alignment
On Mon, Oct 28, 2024 at 01:36:58PM +0100, Marek Vasut wrote:
> On 10/28/24 10:25 AM, Maxime Ripard wrote:
> > On Sat, Oct 26, 2024 at 06:10:01AM +0200, Marek Vasut wrote:
> > > Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
> > > bitfields description state "These bits must be multiple of even
> > > pixel". It is not possible to simply align every bitfield to the
> > > nearest even pixel, because that would unalign the line width and
> > > cause visible distortion. Instead, attempt to re-align the timings
> > > such that the hardware requirement is fulfilled without changing
> > > the line width if at all possible.
> > >
> > > Warn the user in case a panel with odd active pixel width or full
> > > line width is used, this is not possible to support with this one
> > > bridge.
> > >
> > > Signed-off-by: Marek Vasut
> > > ---
> > > Cc: Andrzej Hajda
> > > Cc: David Airlie
> > > Cc: Jernej Skrabec
> > > Cc: Jonas Karlman
> > > Cc: Laurent Pinchart
> > > Cc: Maarten Lankhorst
> > > Cc: Maxime Ripard
> > > Cc: Neil Armstrong
> > > Cc: Robert Foss
> > > Cc: Simona Vetter
> > > Cc: Thomas Zimmermann
> > > Cc: dri-devel@lists.freedesktop.org
> > > ---
> > > drivers/gpu/drm/bridge/tc358767.c | 63 +--
> > > 1 file changed, 60 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > > b/drivers/gpu/drm/bridge/tc358767.c
> > > index 0a6894498267e..7968183510e63 100644
> > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > @@ -901,6 +901,63 @@ static int tc_set_common_video_mode(struct tc_data
> > > *tc,
> > > int vsync_len = mode->vsync_end - mode->vsync_start;
> > > int ret;
> > > + /*
> > > + * Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
> > > + * bitfields description state "These bits must be multiple of even
> > > + * pixel". It is not possible to simply align every bitfield to the
> > > + * nearest even pixel, because that would unalign the line width.
> > > + * Instead, attempt to re-align the timings.
> > > + */
> > > +
> > > + /* Panels with odd active pixel count are not supported by the bridge */
> > > + if (mode->hdisplay & 1)
> > > + dev_warn(tc->dev, "Panels with odd pixel count per active line
> > > are not supported.\n");
> > > +
> > > + /* HPW is odd */
> > > + if (hsync_len & 1) {
> > > + /* Make sure there is some margin left */
> > > + if (left_margin >= 2) {
> > > + /* Align HPW up */
> > > + hsync_len++;
> > > + left_margin--;
> > > + } else if (right_margin >= 2) {
> > > + /* Align HPW up */
> > > + hsync_len++;
> > > + right_margin--;
> > > + } else if (hsync_len > 2) {
> > > + /* Align HPW down as last-resort option */
> > > + hsync_len--;
> > > + left_margin++;
> > > + } else {
> > > + dev_warn(tc->dev, "HPW is odd, not enough margins to
> > > compensate.\n");
> > > + }
> > > + }
> > > +
> > > + /* HBP is odd (HPW is surely even now) */
> > > + if (left_margin & 1) {
> > > + /* Make sure there is some margin left */
> > > + if (right_margin >= 2) {
> > > + /* Align HBP up */
> > > + left_margin++;
> > > + right_margin--;
> > > + } else if (hsync_len > 2) {
> > > + /* HPW is surely even and > 2, which means at least 4 */
> > > + hsync_len -= 2;
> > > + /*
> > > + * Subtract 2 from sync pulse and distribute it between
> > > + * margins. This aligns HBP and keeps HPW aligned.
> > > + */
> > > + left_margin++;
> > > + right_margin++;
> > > + } else {
> > > + dev_warn(tc->dev, "HBP is odd, not enough pixels to
> > > compensate.\n");
> > > + }
> > > + }
> > > +
> > > + /* HFP is odd (HBP and HPW is surely even now) */
> > > + if (right_margin & 1)
> > > + dev_warn(tc->dev, "HFP is odd, panels with odd pixel count per
> > > full line are not supported.\n");
> > > +
> >
> > This should all happen in atomic_check, and reject modes that can't
> > be supported.
> No, that would reject panels I need to support and which can be
> supported by this bridge.
Then drop the warnings, either you support it or you don't.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism
On Mon, Oct 28, 2024 at 03:28:58PM +0200, Laurent Pinchart wrote:
> On Mon, Oct 28, 2024 at 01:21:45PM +0100, Maxime Ripard wrote:
> > On Mon, Oct 28, 2024 at 01:28:57PM +0200, Laurent Pinchart wrote:
> > > On Mon, Oct 28, 2024 at 09:13:31AM +0100, Herve Codina wrote:
> > > > On Sun, 27 Oct 2024 18:23:50 +0200 Laurent Pinchart wrote:
> > > >
> > > > [...]
> > > > > > +static int sn65dsi83_reset_pipeline(struct sn65dsi83 *sn65dsi83)
> > > > > > +{
> > > > > > + struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > + struct drm_modeset_acquire_ctx ctx;
> > > > > > + struct drm_atomic_state *state;
> > > > > > + int err;
> > > > > > +
> > > > > > + /* Use operation done in drm_atomic_helper_suspend() followed by
> > > > > > +* operation done in drm_atomic_helper_resume() but without
> > > > > > releasing
> > > > > > +* the lock between suspend()/resume()
> > > > > > +*/
> > > > > > +
> > > > > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > > > +
> > > > > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > + if (IS_ERR(state)) {
> > > > > > + err = PTR_ERR(state);
> > > > > > + goto unlock;
> > > > > > + }
> > > > > > +
> > > > > > + err = drm_atomic_helper_disable_all(dev, &ctx);
> > > > > > + if (err < 0)
> > > > > > + goto unlock;
> > > > > > +
> > > > > > + drm_mode_config_reset(dev);
> > > > > > +
> > > > > > + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > > > >
> > > > > Committing a full atomic state from a bridge driver in an asynchronous
> > > > > way seems quite uncharted territory, and it worries me. It's also a
> > > > > very
> > > > > heavyweight, you disable all outputs here, instead of focussing on the
> > > > > output connected to the bridge. Can you either implement something
> > > > > more
> > > > > local, resetting the bridge only, or create a core helper to handle
> > > > > this
> > > > > kind of situation, on a per-output basis ?
> > > >
> > > > A full restart of the bridge (power off/on) is needed and so we need to
> > > > redo the initialization sequence. This initialization sequence has to be
> > > > done with the DSI data lanes (bridge inputs) driven in LP11 state and so
> > > > without any video stream. Only focussing on bridge outputs will not be
> > > > sufficient. That's why I brought the pipeline down and restarted it.
> > >
> > > Fair point.
> > >
> > > > Of course, I can copy/paste sn65dsi83_reset_pipeline() to a core helper
> > > > function. Is drm_atomic_helper_reset_all() could be a good candidate?
> > >
> > > The helper should operate on a single output, unrelated outputs should
> > > not be affected.
> >
> > Also, you don't want to reset anything, you just want the last commit to
> > be replayed.
>
> I'm not sure about that. If the last commit is just a page flip, that
> won't help, will it ?
The alternative would be that you start anew with a blank state, which
effectively drops every configuration that has been done by userspace.
This is terrible.
And a page flip wouldn't have affected the connector and
connector->state would still be to the last state that affected it, so
it would work.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] drm/bridge: tc358767: Fix odd pixel alignment
On Mon, Oct 28, 2024 at 02:52:09PM +0100, Maxime Ripard wrote:
> On Mon, Oct 28, 2024 at 01:36:58PM +0100, Marek Vasut wrote:
> > On 10/28/24 10:25 AM, Maxime Ripard wrote:
> > > On Sat, Oct 26, 2024 at 06:10:01AM +0200, Marek Vasut wrote:
> > > > Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
> > > > bitfields description state "These bits must be multiple of even
> > > > pixel". It is not possible to simply align every bitfield to the
> > > > nearest even pixel, because that would unalign the line width and
> > > > cause visible distortion. Instead, attempt to re-align the timings
> > > > such that the hardware requirement is fulfilled without changing
> > > > the line width if at all possible.
> > > >
> > > > Warn the user in case a panel with odd active pixel width or full
> > > > line width is used, this is not possible to support with this one
> > > > bridge.
> > > >
> > > > Signed-off-by: Marek Vasut
> > > > ---
> > > > Cc: Andrzej Hajda
> > > > Cc: David Airlie
> > > > Cc: Jernej Skrabec
> > > > Cc: Jonas Karlman
> > > > Cc: Laurent Pinchart
> > > > Cc: Maarten Lankhorst
> > > > Cc: Maxime Ripard
> > > > Cc: Neil Armstrong
> > > > Cc: Robert Foss
> > > > Cc: Simona Vetter
> > > > Cc: Thomas Zimmermann
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > ---
> > > > drivers/gpu/drm/bridge/tc358767.c | 63 +--
> > > > 1 file changed, 60 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > > > b/drivers/gpu/drm/bridge/tc358767.c
> > > > index 0a6894498267e..7968183510e63 100644
> > > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > > @@ -901,6 +901,63 @@ static int tc_set_common_video_mode(struct tc_data
> > > > *tc,
> > > > int vsync_len = mode->vsync_end - mode->vsync_start;
> > > > int ret;
> > > > + /*
> > > > +* Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02)
> > > > Register
> > > > +* bitfields description state "These bits must be multiple of
> > > > even
> > > > +* pixel". It is not possible to simply align every bitfield to
> > > > the
> > > > +* nearest even pixel, because that would unalign the line
> > > > width.
> > > > +* Instead, attempt to re-align the timings.
> > > > +*/
> > > > +
> > > > + /* Panels with odd active pixel count are not supported by the
> > > > bridge */
> > > > + if (mode->hdisplay & 1)
> > > > + dev_warn(tc->dev, "Panels with odd pixel count per
> > > > active line are not supported.\n");
> > > > +
> > > > + /* HPW is odd */
> > > > + if (hsync_len & 1) {
> > > > + /* Make sure there is some margin left */
> > > > + if (left_margin >= 2) {
> > > > + /* Align HPW up */
> > > > + hsync_len++;
> > > > + left_margin--;
> > > > + } else if (right_margin >= 2) {
> > > > + /* Align HPW up */
> > > > + hsync_len++;
> > > > + right_margin--;
> > > > + } else if (hsync_len > 2) {
> > > > + /* Align HPW down as last-resort option */
> > > > + hsync_len--;
> > > > + left_margin++;
> > > > + } else {
> > > > + dev_warn(tc->dev, "HPW is odd, not enough
> > > > margins to compensate.\n");
> > > > + }
> > > > + }
> > > > +
> > > > + /* HBP is odd (HPW is surely even now) */
> > > > + if (left_margin & 1) {
> > > > + /* Make sure there is some margin left */
> > > > + if (right_margin >= 2) {
> > > > + /* Align HBP up */
> > > > + lef
Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism
On Mon, Oct 28, 2024 at 01:28:57PM +0200, Laurent Pinchart wrote:
> On Mon, Oct 28, 2024 at 09:13:31AM +0100, Herve Codina wrote:
> > Hi Laurent,
> >
> > On Sun, 27 Oct 2024 18:23:50 +0200
> > Laurent Pinchart wrote:
> >
> > [...]
> > > > +static int sn65dsi83_reset_pipeline(struct sn65dsi83 *sn65dsi83)
> > > > +{
> > > > + struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > + struct drm_modeset_acquire_ctx ctx;
> > > > + struct drm_atomic_state *state;
> > > > + int err;
> > > > +
> > > > + /* Use operation done in drm_atomic_helper_suspend() followed by
> > > > +* operation done in drm_atomic_helper_resume() but without
> > > > releasing
> > > > +* the lock between suspend()/resume()
> > > > +*/
> > > > +
> > > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > > > +
> > > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > + if (IS_ERR(state)) {
> > > > + err = PTR_ERR(state);
> > > > + goto unlock;
> > > > + }
> > > > +
> > > > + err = drm_atomic_helper_disable_all(dev, &ctx);
> > > > + if (err < 0)
> > > > + goto unlock;
> > > > +
> > > > + drm_mode_config_reset(dev);
> > > > +
> > > > + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > >
> > > Committing a full atomic state from a bridge driver in an asynchronous
> > > way seems quite uncharted territory, and it worries me. It's also a very
> > > heavyweight, you disable all outputs here, instead of focussing on the
> > > output connected to the bridge. Can you either implement something more
> > > local, resetting the bridge only, or create a core helper to handle this
> > > kind of situation, on a per-output basis ?
> >
> > A full restart of the bridge (power off/on) is needed and so we need to
> > redo the initialization sequence. This initialization sequence has to be
> > done with the DSI data lanes (bridge inputs) driven in LP11 state and so
> > without any video stream. Only focussing on bridge outputs will not be
> > sufficient. That's why I brought the pipeline down and restarted it.
>
> Fair point.
>
> > Of course, I can copy/paste sn65dsi83_reset_pipeline() to a core helper
> > function. Is drm_atomic_helper_reset_all() could be a good candidate?
>
> The helper should operate on a single output, unrelated outputs should
> not be affected.
Also, you don't want to reset anything, you just want the last commit to
be replayed.
Maxime
signature.asc
Description: PGP signature
Re: [RFC PATCH 00/10] Support simple-framebuffer on imx8m
Hi, On Mon, Oct 28, 2024 at 11:25:23AM +0100, Dario Binacchi wrote: > This series is the Linux counterpart of what was sent to U-Boot [1] > for the support of the simple-framebuffer for the BSH SMM S2Pro board. I'm confused. simple-framebuffer is a mechanism for which the entire point is that the kernel doesn't need the driver for. Why do you need to have patches for bridges and panels for simple-framebuffer? They won't be used. > The need to avoid re-initializing the hardware (power domains, > controllers, bridges, display panels) that has already been initialized > and kept powered on by the bootloader has required updating more than > one YAML file, with the addition of boolean properties to inform the > driver that the corresponding hardware has been initialized and left > on by the bootloader. All these properties are added on the fly by the > bootloader to the various relevant nodes. So it's not about simple-framebuffer at all, but rather that you don't want to re-initialize the hardware that has already been setup? If so, this isn't how you should do it, but rather: https://lore.kernel.org/all/CAKMK7uHtqHy_oz4W7F+hmp9iqp7W5Ra8CxPvJ=9bwmvfu-o...@mail.gmail.com/ Maxime signature.asc Description: PGP signature
Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
On Thu, Oct 24, 2024 at 07:06:36AM -1000, Tejun Heo wrote: > Hello, > > On Thu, Oct 24, 2024 at 09:20:43AM +0200, Maxime Ripard wrote: > ... > > > Yeah, let's not use "dev" name for this. As Waiman pointed out, it > > > conflicts > > > with the devices controller from cgroup1. While cgroup1 is mostly > > > deprecated, the same features are provided through BPF in systemd using > > > the > > > same terminologies, so this is going to be really confusing. > > > > Yeah, I agree. We switched to dev because we want to support more than > > just DRM, but all DMA-able memory. We have patches to support for v4l2 > > and dma-buf heaps, so using the name DRM didn't feel great either. > > > > Do you have a better name in mind? "device memory"? "dma memory"? > > Maybe just dma (I think the term isn't used heavily anymore, so the word is > kinda open)? But, hopefully, others have better ideas. > > > > What happened with Tvrtko's weighted implementation? I've seen many > > > proposed > > > patchsets in this area but as far as I could see none could establish > > > consensus among GPU crowd and that's one of the reasons why nothing ever > > > landed. Is the aim of this patchset establishing such consensus? > > > > Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red > > Hat have been involved in that series and we all agree on the > > implementation. > > That's great to hear. > > > Tvrtko aims at a different feature set though: this one is about memory > > allocation limits, Tvrtko's about scheduling. > > > > Scheduling doesn't make much sense for things outside of DRM (and even > > for a fraction of all DRM devices), and it's pretty much orthogonal. So > > i guess you can expect another series from Tvrtko, but I don't think > > they should be considered equivalent or dependent on each other. > > Yeah, I get that this is about memory and that is about processing capacity, > so the plan is going for separate controllers for each? Or would it be > better to present both under the same controller interface? Even if they're > going to be separate controllers, we at least want to be aligned on how > devices and their configurations are presented in the two controllers. It's still up in the air, I think. My personal opinion is that there's only DRM (and accel) devices that really care about scheduling constraints anyway, so it wouldn't (have to) be as generic as this one. And if we would call it dma, then the naming becomes a bit weird since DMA doesn't have much to do with scheduling. But I guess it's just another instance of the "naming is hard" problem :) Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/bridge: tc358767: Fix odd pixel alignment
On Sat, Oct 26, 2024 at 06:10:01AM +0200, Marek Vasut wrote:
> Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
> bitfields description state "These bits must be multiple of even
> pixel". It is not possible to simply align every bitfield to the
> nearest even pixel, because that would unalign the line width and
> cause visible distortion. Instead, attempt to re-align the timings
> such that the hardware requirement is fulfilled without changing
> the line width if at all possible.
>
> Warn the user in case a panel with odd active pixel width or full
> line width is used, this is not possible to support with this one
> bridge.
>
> Signed-off-by: Marek Vasut
> ---
> Cc: Andrzej Hajda
> Cc: David Airlie
> Cc: Jernej Skrabec
> Cc: Jonas Karlman
> Cc: Laurent Pinchart
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Neil Armstrong
> Cc: Robert Foss
> Cc: Simona Vetter
> Cc: Thomas Zimmermann
> Cc: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/bridge/tc358767.c | 63 +--
> 1 file changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index 0a6894498267e..7968183510e63 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -901,6 +901,63 @@ static int tc_set_common_video_mode(struct tc_data *tc,
> int vsync_len = mode->vsync_end - mode->vsync_start;
> int ret;
>
> + /*
> + * Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
> + * bitfields description state "These bits must be multiple of even
> + * pixel". It is not possible to simply align every bitfield to the
> + * nearest even pixel, because that would unalign the line width.
> + * Instead, attempt to re-align the timings.
> + */
> +
> + /* Panels with odd active pixel count are not supported by the bridge */
> + if (mode->hdisplay & 1)
> + dev_warn(tc->dev, "Panels with odd pixel count per active line
> are not supported.\n");
> +
> + /* HPW is odd */
> + if (hsync_len & 1) {
> + /* Make sure there is some margin left */
> + if (left_margin >= 2) {
> + /* Align HPW up */
> + hsync_len++;
> + left_margin--;
> + } else if (right_margin >= 2) {
> + /* Align HPW up */
> + hsync_len++;
> + right_margin--;
> + } else if (hsync_len > 2) {
> + /* Align HPW down as last-resort option */
> + hsync_len--;
> + left_margin++;
> + } else {
> + dev_warn(tc->dev, "HPW is odd, not enough margins to
> compensate.\n");
> + }
> + }
> +
> + /* HBP is odd (HPW is surely even now) */
> + if (left_margin & 1) {
> + /* Make sure there is some margin left */
> + if (right_margin >= 2) {
> + /* Align HBP up */
> + left_margin++;
> + right_margin--;
> + } else if (hsync_len > 2) {
> + /* HPW is surely even and > 2, which means at least 4 */
> + hsync_len -= 2;
> + /*
> + * Subtract 2 from sync pulse and distribute it between
> + * margins. This aligns HBP and keeps HPW aligned.
> + */
> + left_margin++;
> + right_margin++;
> + } else {
> + dev_warn(tc->dev, "HBP is odd, not enough pixels to
> compensate.\n");
> + }
> + }
> +
> + /* HFP is odd (HBP and HPW is surely even now) */
> + if (right_margin & 1)
> + dev_warn(tc->dev, "HFP is odd, panels with odd pixel count per
> full line are not supported.\n");
> +
This should all happen in atomic_check, and reject modes that can't be
supported.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] drm/bridge: tc358767: Improve DPI output pixel clock accuracy
On Sat, Oct 26, 2024 at 06:11:12AM +0200, Marek Vasut wrote: > The Pixel PLL is not very capable and may come up with wildly inaccurate > clock. Since DPI panels are often tolerant to slightly higher pixel clock > without being operated outside of specification, calculate two Pixel PLL > settings for DPI output, one for desired output pixel clock and one for > output pixel clock with 1% increase, and then pick the result which is > closer to the desired pixel clock and use it as the Pixel PLL settings. The typical tolerance we've used is .5%, which is recommended by VESA in several specs. Differing from it for a good reason is ok I guess, but you need to document why. Maxime signature.asc Description: PGP signature
Re: [PATCH 2/2] drm: bridge: ti-sn65dsi83: Add error recovery mechanism
On Sun, Oct 27, 2024 at 06:23:50PM +0200, Laurent Pinchart wrote:
> On Thu, Oct 24, 2024 at 11:55:38AM +0200, Herve Codina wrote:
> > In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
> > from errors by itself. A full restart of the bridge is needed in those
> > cases to have the bridge output LVDS signals again.
> >
> > The TI SN65DSI83 has some error detection capabilities. Introduce an
> > error recovery mechanism based on this detection.
> >
> > The errors detected are signaled through an interrupt. On system where
> > this interrupt is not available, the driver uses a polling monitoring
> > fallback to check for errors. When an error is present, the recovery
> > process is launched.
> >
> > Restarting the bridge needs to redo the initialization sequence. This
> > initialization sequence has to be done with the DSI data lanes driven in
> > LP11 state. In order to do that, the recovery process resets the entire
> > pipeline.
> >
> > Signed-off-by: Herve Codina
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 128 ++
> > 1 file changed, 128 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index 96e829163d87..22975b60e80f 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -35,9 +35,12 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> >
> > #include
> > #include
> > +#include /* DRM_MODESET_LOCK_ALL_BEGIN() need
> > drm_drv_uses_atomic_modeset() */
> > #include
> > #include
> > #include
> > @@ -147,6 +150,9 @@ struct sn65dsi83 {
> > struct regulator*vcc;
> > boollvds_dual_link;
> > boollvds_dual_link_even_odd_swap;
> > + booluse_irq;
> > + struct delayed_work monitor_work;
> > + struct work_struct reset_work;
> > };
> >
> > static const struct regmap_range sn65dsi83_readable_ranges[] = {
> > @@ -321,6 +327,92 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> > return dsi_div - 1;
> > }
> >
> > +static int sn65dsi83_reset_pipeline(struct sn65dsi83 *sn65dsi83)
> > +{
> > + struct drm_device *dev = sn65dsi83->bridge.dev;
> > + struct drm_modeset_acquire_ctx ctx;
> > + struct drm_atomic_state *state;
> > + int err;
> > +
> > + /* Use operation done in drm_atomic_helper_suspend() followed by
> > +* operation done in drm_atomic_helper_resume() but without releasing
> > +* the lock between suspend()/resume()
> > +*/
> > +
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > +
> > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > + if (IS_ERR(state)) {
> > + err = PTR_ERR(state);
> > + goto unlock;
> > + }
> > +
> > + err = drm_atomic_helper_disable_all(dev, &ctx);
> > + if (err < 0)
> > + goto unlock;
> > +
> > + drm_mode_config_reset(dev);
> > +
> > + err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
>
> Committing a full atomic state from a bridge driver in an asynchronous
> way seems quite uncharted territory, and it worries me. It's also a very
> heavyweight, you disable all outputs here, instead of focussing on the
> output connected to the bridge. Can you either implement something more
> local, resetting the bridge only, or create a core helper to handle this
> kind of situation, on a per-output basis ?
I think you can't just shut down the bridge and restart it, since some
require particular power sequences that will only occur if you also shut
down the upstream controller.
So I think we'd need to tear down the CRTC, connector and everything
in between.
I do agree that it needs to be a generic helper though. In fact, it
looks awfully similar to what vc4 and i915 are doing in reset_pipe and
and intel_modeset_commit_pipes, respectively. It looks like a good
opportunity to make it a helper.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3 0/4] drm/tests: Fix some memory leaks
On Wed, Oct 23, 2024 at 09:35:59AM +0800, Jinjie Ruan wrote: > > > On 2024/10/18 16:12, Jinjie Ruan wrote: > > > > > > On 2024/10/18 15:55, Maxime Ripard wrote: > >> Hi, > >> > >> On Thu, Oct 17, 2024 at 02:31:21PM GMT, Jinjie Ruan wrote: > >>> Fix some memory leaks in drm tests. > >>> > >>> Changes in v3: > >>> - Adjust drm/drm_edid.h header to drm_kunit_helpers.c. > >>> - Drop the "helper" in the helper name. > >>> - s/fllowing/following/ > >>> - Add Acked-by. > >> > >> This creates build failures since drm_display_mode were const before, > >> and can't anymore. > > > > It seems it came from bellowing v1, and this v3 has not reported the > > issue yet. > > > > https://lore.kernel.org/all/202410180830.oitxtsov-...@intel.com/ > > Hi, Maxime, > > Should this series send again? The issue seems not related to this version. As far as I know, the issues reported still apply there, so yes Maxime signature.asc Description: PGP signature
Re: [PATCH 28/37] drm/vc4: Enable bg_fill if there are no planes enabled
On Wed, 23 Oct 2024 17:50:25 +0100, Dave Stevenson wrote: > The default was to have enable_bg_fill disabled and the first > plane set it if it wasn't opaque and covering the whole screen. > However that meant that if no planes were enabled, then the > background fill wasn't enabled, and would give a striped > output from the uninitialised output buffer. > > [ ... ] Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 23/37] drm/vc4: drv: Add support for 2712 D-step
On Wed, 23 Oct 2024 17:50:20 +0100, Dave Stevenson wrote: > Add in the compatible string and VC4_GEN_ enum for the D-step > > Signed-off-by: Dave Stevenson Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 01/37] drm/vc4: Limit max_bpc to 8 on Pi0-3
On Wed, Oct 23, 2024 at 05:49:58PM +0100, Dave Stevenson wrote:
> Pi 0-3 have no deep colour support and only 24bpp output,
> so max_bpc should remain as 8, and no HDR metadata property
> should be registered.
>
> Fixes: ba8c0faebbb0 ("drm/vc4: hdmi: Enable 10/12 bpc output")
> Signed-off-by: Dave Stevenson
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 62b82b1eeb36..6ebcc38be291 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -640,6 +640,11 @@ static int vc4_hdmi_connector_init(struct drm_device
> *dev,
> if (ret)
> return ret;
>
> + if (vc4_hdmi->variant->supports_hdr)
> + drm_connector_attach_max_bpc_property(connector, 8, 12);
> + else
> + drm_connector_attach_max_bpc_property(connector, 8, 8);
> +
Are you sure that one is needed?
https://elixir.bootlin.com/linux/v6.11.5/source/drivers/gpu/drm/vc4/vc4_hdmi.c#L594
should take care about all that already.
Maxime
signature.asc
Description: PGP signature
Re: Requirements to merge new heaps in the kernel
On Tue, Oct 22, 2024 at 09:19:05AM -0700, John Stultz wrote: > On Tue, Oct 22, 2024 at 1:38 AM Maxime Ripard wrote: > > > > I wanted to follow-up on the discussion we had at Plumbers with John and > > T.J. about (among other things) adding new heaps to the kernel. > > > > I'm still interested in merging a carve-out driver[1], since it seems to be > > in every vendor BSP and got asked again last week. > > > > I remember from our discussion that for new heap types to be merged, we > > needed a kernel use-case. Looking back, I'm not entirely sure how one > > can provide that given that heaps are essentially facilities for > > user-space. > > > > Am I misremembering or missing something? What are the requirements for > > you to consider adding a new heap driver? > > It's basically the same as the DRM subsystem rules. > https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements > ie: There has to be opensource user for it, and the user has to be > more significant than a "toy" implementation (which can be a bit > subjective and contentious when trying to get out of a chicken and egg > loop). Ok, so I'm definitely misremembering things then, I thought there was another requirement in addition to that one. Thanks :) Maxime signature.asc Description: PGP signature
Re: [RFC PATCH] drm/bridge: panel: Use devm_drm_bridge_add()
On Wed, Oct 09, 2024 at 01:23:31PM +0800, Fei Shao wrote: > In the mtk_dsi driver, its DSI host attach callback calls > devm_drm_of_get_bridge() to get the next bridge. If that next bridge is > a panel bridge, a panel_bridge object is allocated and managed by the > panel device. > > Later, if the attach callback fails with -EPROBE_DEFER from subsequent > component_add(), the panel device invoking the callback at probe time > also fails, and all device-managed resources are freed accordingly. > > This exposes a drm_bridge bridge_list corruption due to the unbalanced > lifecycle between the DSI host and the panel devices: the panel_bridge > object managed by panel device is freed, while drm_bridge_remove() is > bound to DSI host device and never gets called. > The next drm_bridge_add() will trigger UAF against the freed bridge list > object and result in kernel panic. > > This bug is observed on a MediaTek MT8188-based Chromebook with MIPI DSI > outputting to a DSI panel (DT is WIP for upstream). > > As a fix, using devm_drm_bridge_add() with the panel device in the panel > path seems reasonable. This also implies a chain of potential cleanup > actions: > > 1. Removing drm_bridge_remove() means devm_drm_panel_bridge_release() >becomes hollow and can be removed. > > 2. devm_drm_panel_bridge_add_typed() is almost emptied except for the >`bridge->pre_enable_prev_first` line. Itself can be also removed if >we move the line into drm_panel_bridge_add_typed(). (maybe?) > > 3. drm_panel_bridge_add_typed() now calls all the needed devm_* calls, >so it's essentially the new devm_drm_panel_bridge_add_typed(). > > 4. drmm_panel_bridge_add() needs to be updated accordingly since it >calls drm_panel_bridge_add_typed(). But now there's only one bridge >object to be freed, and it's already being managed by panel device. >I wonder if we still need both drmm_ and devm_ version in this case. >(maybe yes from DRM PoV, I don't know much about the context) > > This is a RFC patch since I'm not sure if my understanding is correct > (for both the fix and the cleanup). It fixes the issue I encountered, > but I don't expect it to be picked up directly due to the redundant > commit message and the dangling devm_drm_panel_bridge_release(). > I plan to resend the official patch(es) once I know what I supposed to > do next. > > For reference, here's the KASAN report from the device: > == > BUG: KASAN: slab-use-after-free in drm_bridge_add+0x98/0x230 > Read of size 8 at addr ff80c4e9e100 by task kworker/u32:1/69 > > CPU: 1 UID: 0 PID: 69 Comm: kworker/u32:1 Not tainted > 6.12.0-rc1-next-20241004-kasan-00030-g062135fa4046 #1 > Hardware name: Google Ciri sku0/unprovisioned board (DT) > Workqueue: events_unbound deferred_probe_work_func > Call trace: > dump_backtrace+0xfc/0x140 > show_stack+0x24/0x38 > dump_stack_lvl+0x40/0xc8 > print_report+0x140/0x700 > kasan_report+0xcc/0x130 > __asan_report_load8_noabort+0x20/0x30 > drm_bridge_add+0x98/0x230 > devm_drm_panel_bridge_add_typed+0x174/0x298 > devm_drm_of_get_bridge+0xe8/0x190 > mtk_dsi_host_attach+0x130/0x2b0 > mipi_dsi_attach+0x8c/0xe8 > hx83102_probe+0x1a8/0x368 > mipi_dsi_drv_probe+0x6c/0x88 > really_probe+0x1c4/0x698 > __driver_probe_device+0x160/0x298 > driver_probe_device+0x7c/0x2a8 > __device_attach_driver+0x2a0/0x398 > bus_for_each_drv+0x198/0x200 > __device_attach+0x1c0/0x308 > device_initial_probe+0x20/0x38 > bus_probe_device+0x11c/0x1f8 > deferred_probe_work_func+0x80/0x250 > worker_thread+0x9b4/0x2780 > kthread+0x274/0x350 > ret_from_fork+0x10/0x20 > > Allocated by task 69: > kasan_save_track+0x40/0x78 > kasan_save_alloc_info+0x44/0x58 > __kasan_kmalloc+0x84/0xa0 > __kmalloc_node_track_caller_noprof+0x228/0x450 > devm_kmalloc+0x6c/0x288 > devm_drm_panel_bridge_add_typed+0xa0/0x298 > devm_drm_of_get_bridge+0xe8/0x190 > mtk_dsi_host_attach+0x130/0x2b0 > mipi_dsi_attach+0x8c/0xe8 > hx83102_probe+0x1a8/0x368 > mipi_dsi_drv_probe+0x6c/0x88 > really_probe+0x1c4/0x698 > __driver_probe_device+0x160/0x298 > driver_probe_device+0x7c/0x2a8 > __device_attach_driver+0x2a0/0x398 > bus_for_each_drv+0x198/0x200 > __device_attach+0x1c0/0x308 > device_initial_probe+0x20/0x38 > bus_probe_device+0x11c/0x1f8 > deferred_probe_work_func+0x80/0x250 > worker_thread+0x9b4/0x2780 > kthread+0x274/0x350 > ret_from_fork+0x10/0x20 > > Freed by task 69: > kasan_save_track+0x40/0x78 > kasan_save_free_info+0x58/0x78 > __kasan_slab_free+0x48/0x68 > kfree+0xd4/0x750 > devres_release_all+0x144/0x1e8 > really_probe+0x48c/0x698 > __driver_probe_device+0x160/0x298 > driver_probe_device+0x7c/0x2a8 > __device_attach_driver+0x2a0/0x398 > bus_for_each_drv+0x198/0x200 > __device_attach+0x1c0/0x308 > device_initial_probe+0x20/0x38 > bus_probe_device+0x11c/0x1f8 > deferred_probe_work_
Re: [PATCH 31/37] clk: bcm: rpi: Allow cpufreq driver to also adjust gpu clocks
On Wed, Oct 23, 2024 at 05:50:28PM +0100, Dave Stevenson wrote:
> From: Dom Cobley
>
> For performance/power it is beneficial to adjust gpu clocks with arm clock.
> This is how the downstream cpufreq driver works
>
> Signed-off-by: Dom Cobley
> Signed-off-by: Dave Stevenson
> ---
> drivers/clk/bcm/clk-raspberrypi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c
> b/drivers/clk/bcm/clk-raspberrypi.c
> index 6d5ee1cddded..274176a938c6 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -156,7 +156,7 @@ static int raspberrypi_clock_property(struct rpi_firmware
> *firmware,
> struct raspberrypi_firmware_prop msg = {
> .id = cpu_to_le32(data->id),
> .val = cpu_to_le32(*val),
> - .disable_turbo = cpu_to_le32(1),
> + .disable_turbo = cpu_to_le32(0),
I guess we can simply remove that line?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 25/37] drm/vc4: plane: Add support for 2712 D-step.
On Wed, 23 Oct 2024 17:50:22 +0100, Dave Stevenson wrote: > There are a few minor changes in the display list generation > for the D-step of the chip, so add them. > > Signed-off-by: Dave Stevenson Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 03/37] drm/vc4: Fix reading of frame count on GEN5 / Pi4
On Wed, 23 Oct 2024 17:50:00 +0100, Dave Stevenson wrote:
> The frame count values moved within registers DISPSTAT1 and
> DISPSTAT2 with GEN5, so update the accessor function to
> accommodate that.
>
> Fixes: b51cd7ad143d ("drm/vc4: hvs: Fix frame count register readout")
>
> [ ... ]
Reviewed-by: Maxime Ripard
Thanks!
Maxime
Re: [PATCH 02/37] drm/vc4: Use of_device_get_match_data to set generation
On Wed, 23 Oct 2024 17:49:59 +0100, Dave Stevenson wrote: > Use of_device_get_match_data to retrieve the generation value > as set in the struct of_device_id, rather than manually comparing > compatible strings. > > Signed-off-by: Dave Stevenson > > [ ... ] Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 29/37] drm/vc4: Drop planes that are completely off-screen or 0 crtc size
On Wed, 23 Oct 2024 17:50:26 +0100, Dave Stevenson wrote: > It is permitted for a plane to be configured such that none > of it is on-screen via either negative dest rectangle X,Y > offset, or an offset that is greater than the crtc dimensions. > > These planes were resized via drm_atomic_helper_check_plane_state > > [ ... ] Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 26/37] drm/vc4: hdmi: Support 2712 D-step register map
On Wed, 23 Oct 2024 17:50:23 +0100, Dave Stevenson wrote: > The D-step has increased FIFO sizes of the MAI_THR blocks, > resulting in changes to the register masking. Add support for > it. > > Signed-off-by: Dave Stevenson > > [ ... ] Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 24/37] drm/vc4: hvs: Add in support for 2712 D-step.
On Wed, 23 Oct 2024 17:50:21 +0100, Dave Stevenson wrote: > THe registers have been moved around, and a couple of minor changes > made, so adapt for this. > > Signed-off-by: Dave Stevenson Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH v12 13/15] drm/vkms: Create KUnit tests for YUV conversions
On Fri, Oct 11, 2024 at 04:29:25PM +0200, Louis Chauvet wrote:
> On 11/10/24 - 12:49, Maxime Ripard wrote:
> > On Tue, Oct 08, 2024 at 11:23:22AM GMT, Louis Chauvet wrote:
> > >
> > > Hi,
> > >
> > > > > + * The YUV color representation were acquired via the colour python
> > > > > framework.
> > > > > + * Below are the function calls used for generating each case.
> > > > > + *
> > > > > + * For more information got to the docs:
> > > > > + *
> > > > > https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html
> > > > > + */
> > > > > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > > > > + /*
> > > > > + * colour.RGB_to_YCbCr(,
> > > > > + * K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > > > > + * in_bits = 16,
> > > > > + * in_legal = False,
> > > > > + * in_int = True,
> > > > > + * out_bits = 8,
> > > > > + * out_legal = False,
> > > > > + * out_int = True)
> > > > > + */
> > > >
> > > > We should really detail what the intent and expected outcome is supposed
> > > > to be here. Relying on a third-party python library call for
> > > > documentation isn't great.
> > >
> > > This was requested by Pekka in the [v2] of this series.
> >
> > Ok.
> >
> > > I can add something like this before each tests, but I think having the
> > > exact python code used may help people to understand what should be the
> > > behavior, and refering to the python code to understand the conversion.
> >
> > Help, sure. Be the *only* documentation, absolutely not.
> >
> > Let's turn this around. You run kunit, one of these tests fail:
> >
> > - It adds cognitive load to try to identify and make sense of an
> >unknown lib.
> >
> > - How can we check that the arguments you provided there are the one
> >you actually wanted to provide, and you didn't make a typo?
> >
> > > I can add something like this before each tests to clarify the tested
> > > case:
> > >
> > > Test cases for conversion between YUV BT601 limited range and
> > > RGB using the ITU-R BT.601 weights.
> > >
> > > Or maybe just documenting the structure yuv_u8_to_argb_u16_case:
> > >
> > > @encoding: Encoding used to convert RGB to YUV
> > > @range: Range used to convert RGB to YUV
> > > @n_colors: Count of test colors in this case
> > > @format_pair.name: Name used for this color conversion, used to
> > > clarify the test results
> > > @format_pair.rgb: RGB color tested
> > > @format_pair.yuv: Same color as @format_pair.rgb, but converted to
> > > YUV using @encoding and @range.
> > >
> > > What do you think?
> >
> > That it's welcome, but it still doesn't allow to figure out what your
> > intent was with this test 2 years from now.
>
> I don't really understand what you want to add. Can you explain what you
> expect here? Did you mean you want a description like this above the test
> function?
I want, for each test case, to have a documentation of what case it's
testing and what the test should expect.
So, for the first one, something like:
/*
* Test the conversion of full range, BT601-encoded, YUVXXX pixel to
* ARGB, for various colors. This has been generated using:
*
* colour.RGB_to_YCbCR(...)
*/
And there's other things you need to document. Like, it seems that you
sometimes pass different values for in_legal and out_legal, and that's
not clear to me.
It also that you uses a matrix for NV12 but are converting a different
format. This needs to be documented.
Finally, You should be documented why you are checking that the colors
difference is less than 257, and not exactly equal.
Maxime
signature.asc
Description: PGP signature
Re: Requirements to merge new heaps in the kernel
On Tue, Oct 22, 2024 at 01:58:47PM -0400, Nicolas Dufresne wrote: > Hi, > > Le mardi 22 octobre 2024 à 09:19 -0700, John Stultz a écrit : > > On Tue, Oct 22, 2024 at 1:38 AM Maxime Ripard wrote: > > > > > > I wanted to follow-up on the discussion we had at Plumbers with John and > > > T.J. about (among other things) adding new heaps to the kernel. > > > > > > I'm still interested in merging a carve-out driver[1], since it seems to > > > be > > > in every vendor BSP and got asked again last week. > > > > > > I remember from our discussion that for new heap types to be merged, we > > > needed a kernel use-case. Looking back, I'm not entirely sure how one > > > can provide that given that heaps are essentially facilities for > > > user-space. > > > > > > Am I misremembering or missing something? What are the requirements for > > > you to consider adding a new heap driver? > > > > It's basically the same as the DRM subsystem rules. > > https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements > > ie: There has to be opensource user for it, and the user has to be > > more significant than a "toy" implementation (which can be a bit > > subjective and contentious when trying to get out of a chicken and egg > > loop). > > If there is a generic logic to decide to use a carve-out when using some > specific device on specific platform, it would not be a problem to make > userspace for it. I'm happy to take DMABuf patches in GStreamer notably (which > could greatly help ensuring zero-copy path). Yeah, that's one of the things we discussed at Plumbers too. My point-of-view was that userspace also had no way to tell which kind of buffers it would get. We settled down on the heap name providing those semantics, and it resulted in: https://lore.kernel.org/r/20240930144057.453751-1-mrip...@kernel.org > But so far, all the proposals was just a base allocator, no way to know when > to > use it and for which device. The actual mapping of heaps and device was left > to > userspace, which to be honest would only work with a userspace Linux Allocator > library, with userspace drivers, or inside mesa if the devices are GPUs/NPUs. > This is a project Laurent Pinchard have hosted a workshop about during XDC. Yeah, that's another issue that needs to be tackled at some point indeed. > p.s. libcamera have device specific knowledge, and could of course be a > shorter > term user. Note that major distro are not happy that there is no memory > accounting for dmabuf, bypassing sandboxes and limits. Meh. The same argument could be said for v4l2 or DRM/KMS, and it never bothered anyone. Fortunately, we're tackling that issue as well: https://lore.kernel.org/dri-devel/20241023075302.27194-1-maarten.lankho...@linux.intel.com/ Maxime signature.asc Description: PGP signature
Re: [PATCH v4 4/5] drm: writeback: Introduce drm managed helpers
Hi, On Thu, Oct 10, 2024 at 07:39:06PM +0200, Louis Chauvet wrote: > Currently drm_writeback_connector are created by > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder. > Both of the function uses drm_connector_init and drm_encoder_init, but > there is no way to properly clean those structure from outside. By using > drm managed variants, we can ensure that the writeback connector is > properly cleaned. > > This patch introduce drmm_writeback_connector_init, an helper to initialize > a writeback connector using drm managed helpers. This function allows the > caller to use its own encoder. > > Signed-off-by: Louis Chauvet > --- > drivers/gpu/drm/drm_connector.c | 4 + > drivers/gpu/drm/drm_writeback.c | 224 > ++-- > include/drm/drm_writeback.h | 10 ++ > 3 files changed, 208 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index fc35f47e2849..fe4c1967860a 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -613,6 +613,7 @@ static void drm_mode_remove(struct drm_connector > *connector, > drm_mode_destroy(connector->dev, mode); > } > > +void drm_writeback_connector_cleanup(struct drm_device *dev, void *data); > /** > * drm_connector_cleanup - cleans up an initialised connector > * @connector: connector to cleanup > @@ -631,6 +632,9 @@ void drm_connector_cleanup(struct drm_connector > *connector) > DRM_CONNECTOR_REGISTERED)) > drm_connector_unregister(connector); > > + if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > + drm_writeback_connector_cleanup(dev, connector); > + So I think it should live in its own patch. You're doing multiple things here. There's a) the bug that writeback connectors aren't built properly, b) the discussion about how it's best to clean it up, and c) how to make every driver clean up properly. AFAIU, you're trying to address a and c here. I think putting that call in drm_connector_cleanup is backward compared to the pattern we're using in the rest of DRM. drm_connector_cleanup should just clean what was allocated by drm_connector_init, and that's it. So we should create a drm_writeback_connector_cleanup function to address a). That should be your first patch. Now, it would indeed be best if drm_writeback_connector_cleanup didn't need to be called at all. That's the second part of your patch, and should be in its own patch as well. It would address b). And finally, addressing c will require some driver changes, to either a call to drmm_writeback_connector_init_* or by using drm_writeback_connector_cleanup, but we'll have to make that change in every driver. Maxime signature.asc Description: PGP signature
Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
Hi Tejun, Thanks a lot for your review. On Wed, Oct 23, 2024 at 09:40:28AM -1000, Tejun Heo wrote: > On Wed, Oct 23, 2024 at 09:52:53AM +0200, Maarten Lankhorst wrote: > > New submission! > > I've added documentation for each call, and integrated the renaming from > > drm cgroup to dev cgroup, based on maxime ripard's work. > > > > Maxime has been testing this with dma-buf heaps and v4l2 too, and it seems > > to work. > > In the initial submission, I've decided to only add the smallest enablement > > possible, > > to have less chance of breaking things. > > > > The API has been changed slightly, from "$name region.$regionname=$limit" > > in a file called > > dev.min/low/max to "$subsystem/$name $regionname=$limit" in a file called > > dev.region.min/low/max. > > > > This hopefully allows us to perhaps extend the API later on with the > > possibility to > > set scheduler weights on the device, like in > > > > https://blogs.igalia.com/tursulin/drm-scheduling-cgroup-controller/ > > > > Maarten Lankhorst (5): > > kernel/cgroup: Add "dev" memory accounting cgroup > > Yeah, let's not use "dev" name for this. As Waiman pointed out, it conflicts > with the devices controller from cgroup1. While cgroup1 is mostly > deprecated, the same features are provided through BPF in systemd using the > same terminologies, so this is going to be really confusing. Yeah, I agree. We switched to dev because we want to support more than just DRM, but all DMA-able memory. We have patches to support for v4l2 and dma-buf heaps, so using the name DRM didn't feel great either. Do you have a better name in mind? "device memory"? "dma memory"? > What happened with Tvrtko's weighted implementation? I've seen many proposed > patchsets in this area but as far as I could see none could establish > consensus among GPU crowd and that's one of the reasons why nothing ever > landed. Is the aim of this patchset establishing such consensus? Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red Hat have been involved in that series and we all agree on the implementation. Tvrtko aims at a different feature set though: this one is about memory allocation limits, Tvrtko's about scheduling. Scheduling doesn't make much sense for things outside of DRM (and even for a fraction of all DRM devices), and it's pretty much orthogonal. So i guess you can expect another series from Tvrtko, but I don't think they should be considered equivalent or dependent on each other. > If reaching consensus doesn't seem feasible in a predictable timeframe, my > suggesstion is just extending the misc controller. If the only way forward > here is fragmented vendor(s)-specific implementations, let's throw them into > the misc controller. I don't think we have a fragmented implementation here, at all. The last patch especially implements it for all devices implementing the GEM interface in DRM, which would be around 100 drivers from various vendors. It's marked as a discussion because we don't quite know how to plumb it in for all drivers in the current DRM framework, but it's very much what we want to achieve. Maxime signature.asc Description: PGP signature
Requirements to merge new heaps in the kernel
Hi Sumit, I wanted to follow-up on the discussion we had at Plumbers with John and T.J. about (among other things) adding new heaps to the kernel. I'm still interested in merging a carve-out driver[1], since it seems to be in every vendor BSP and got asked again last week. I remember from our discussion that for new heap types to be merged, we needed a kernel use-case. Looking back, I'm not entirely sure how one can provide that given that heaps are essentially facilities for user-space. Am I misremembering or missing something? What are the requirements for you to consider adding a new heap driver? Thanks! Maxime 1: https://lore.kernel.org/dri-devel/20240515-dma-buf-ecc-heap-v1-1-54cbbd049...@kernel.org/ signature.asc Description: PGP signature
Re: [PATCH v3 12/15] drm/bridge: Add ITE IT6263 LVDS to HDMI converter
On Tue, Oct 22, 2024 at 04:10:51PM +0800, Liu Ying wrote:
> Hi Maxime,
>
> On 10/22/2024, Maxime Ripard wrote:
> > On Tue, Oct 22, 2024 at 03:36:47PM +0800, Liu Ying wrote:
> >> Hi Maxime,
> >>
> >> On 10/21/2024, Maxime Ripard wrote:
> >>> On Mon, Oct 21, 2024 at 02:44:43PM +0800, Liu Ying wrote:
> >>>> +static int it6263_bridge_atomic_check(struct drm_bridge *bridge,
> >>>> + struct drm_bridge_state
> >>>> *bridge_state,
> >>>> + struct drm_crtc_state *crtc_state,
> >>>> + struct drm_connector_state
> >>>> *conn_state)
> >>>> +{
> >>>> +struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> >>>> +int ret;
> >>>> +
> >>>> +ret =
> >>>> drm_atomic_helper_connector_hdmi_check(conn_state->connector,
> >>>> + conn_state->state);
> >>>> +if (ret)
> >>>> +return ret;
> >>>> +
> >>>> +return mode->clock > MAX_PIXEL_CLOCK_KHZ ? -EINVAL : 0;
> >>>
> >>> drm_atomic_helper_connector_hdmi_check will already make that check, so
> >>> it's redundant.
> >>
> >> MAX_PIXEL_CLOCK_KHZ is 150MHz. With 150MHz pixel clock rate, we'll get
> >> 150MHz HDMI character rate for 8bpc and 187.5MHz HDMI character rate
> >> for 10bpc, both are lower than MAX_HDMI_TMDS_CHAR_RATE_HZ = 225MHz.
> >
> > I guess? I have no idea how that's relevant though. Where are those
> > constraints coming from, and why aren't you checking for them in
> > tmds_char_rate_valid?
>
> All constraints come from IT6263 data sheet. They are also mentioned
> in IT6263 product link(commit message contains the link).
>
> https://www.ite.com.tw/en/product/cate1/IT6263
>
> "
> LVDS RX
> Support input clock rate up to 150 MHz
>
> HDMI TX
> Support link speeds of up to 2.25 Gbps (link clock rate of 225 MHz)
> "
>
> If no objection, I'll check mode clock rate against
> MAX_PIXEL_CLOCK_KHZ in tmds_char_rate_valid.
If you don't support bpc other than 8, and no other format than RGB,
then it's a good enough approximation.
It should be documented though
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3 12/15] drm/bridge: Add ITE IT6263 LVDS to HDMI converter
On Tue, Oct 22, 2024 at 03:36:47PM +0800, Liu Ying wrote:
> Hi Maxime,
>
> On 10/21/2024, Maxime Ripard wrote:
> > On Mon, Oct 21, 2024 at 02:44:43PM +0800, Liu Ying wrote:
> >> +static int it6263_bridge_atomic_check(struct drm_bridge *bridge,
> >> +struct drm_bridge_state *bridge_state,
> >> +struct drm_crtc_state *crtc_state,
> >> +struct drm_connector_state *conn_state)
> >> +{
> >> + struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> >> + int ret;
> >> +
> >> + ret = drm_atomic_helper_connector_hdmi_check(conn_state->connector,
> >> + conn_state->state);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + return mode->clock > MAX_PIXEL_CLOCK_KHZ ? -EINVAL : 0;
> >
> > drm_atomic_helper_connector_hdmi_check will already make that check, so
> > it's redundant.
>
> MAX_PIXEL_CLOCK_KHZ is 150MHz. With 150MHz pixel clock rate, we'll get
> 150MHz HDMI character rate for 8bpc and 187.5MHz HDMI character rate
> for 10bpc, both are lower than MAX_HDMI_TMDS_CHAR_RATE_HZ = 225MHz.
I guess? I have no idea how that's relevant though. Where are those
constraints coming from, and why aren't you checking for them in
tmds_char_rate_valid?
> So, it looks like pixel clock rate is the bottleneck.
The bottleneck to what?
> Remove drm_atomic_helper_connector_hdmi_check() or keep this as-is?
No, like I said, remove the final check for mode->clock.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate
On Tue, Oct 22, 2024 at 02:13:57PM +0800, Liu Ying wrote:
> On 10/13/2024, Marek Vasut wrote:
> > On 10/11/24 8:18 AM, Liu Ying wrote:
> >> On 10/11/2024, Marek Vasut wrote:
> >>> On 10/10/24 7:22 AM, Liu Ying wrote:
> >>>> On 10/09/2024, Marek Vasut wrote:
> >>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
> >>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
> >>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
> >>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
> >>>>> results in accurate serializer clock.
> >>>>>
> >>>>> Signed-off-by: Marek Vasut
> >>>>> ---
> >>>>> Cc: Abel Vesa
> >>>>> Cc: Andrzej Hajda
> >>>>> Cc: David Airlie
> >>>>> Cc: Fabio Estevam
> >>>>> Cc: Isaac Scott
> >>>>> Cc: Jernej Skrabec
> >>>>> Cc: Jonas Karlman
> >>>>> Cc: Laurent Pinchart
> >>>>> Cc: Maarten Lankhorst
> >>>>> Cc: Maxime Ripard
> >>>>> Cc: Michael Turquette
> >>>>> Cc: Neil Armstrong
> >>>>> Cc: Peng Fan
> >>>>> Cc: Pengutronix Kernel Team
> >>>>> Cc: Robert Foss
> >>>>> Cc: Sascha Hauer
> >>>>> Cc: Shawn Guo
> >>>>> Cc: Simona Vetter
> >>>>> Cc: Stephen Boyd
> >>>>> Cc: Thomas Zimmermann
> >>>>> Cc: dri-devel@lists.freedesktop.org
> >>>>> Cc: i...@lists.linux.dev
> >>>>> Cc: ker...@dh-electronics.com
> >>>>> Cc: linux-arm-ker...@lists.infradead.org
> >>>>> Cc: linux-...@vger.kernel.org
> >>>>> ---
> >>>>> drivers/clk/imx/clk-imx8mp.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> >>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
> >>>>> --- a/drivers/clk/imx/clk-imx8mp.c
> >>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
> >>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct
> >>>>> platform_device *pdev)
> >>>>> hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] =
> >>>>> imx8m_clk_hw_composite("media_mipi_phy1_ref",
> >>>>> imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
> >>>>> hws[IMX8MP_CLK_MEDIA_DISP1_PIX] =
> >>>>> imx8m_clk_hw_composite_bus_flags("media_disp1_pix",
> >>>>> imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
> >>>>> hws[IMX8MP_CLK_MEDIA_CAM2_PIX] =
> >>>>> imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels,
> >>>>> ccm_base + 0xbe80);
> >>>>> - hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb",
> >>>>> imx8mp_media_ldb_sels, ccm_base + 0xbf00);
> >>>>> + hws[IMX8MP_CLK_MEDIA_LDB] =
> >>>>> imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels,
> >>>>> ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
> >>>>
> >>>> This patch would cause the below in-flight LDB bridge driver
> >>>> patch[1] fail to do display mode validation upon display modes
> >>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
> >>>
> >>> Why ?
> >>
> >> Mode validation is affected only for dual LVDS link mode.
> >> For single LVDS link mode, this patch does open more display
> >> modes read from the DDC I2C bus. The reason behind is that
> >> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
> >> link mode, while it's 7 for single LVDS link mode.
> >>
> >> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
> >> in imx8mp.dtsi. For 1920x1080-60.00Hz with 148.5MHz pixel
> >> clock rate, "media_ldb" clock rate is 519.75MHz and
> >> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
> >> dual LVDS link mode(x3.5). For newly opened up 1920x1080-59.94Hz
> >> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
> &g
Re: [PATCH] Documentation: dma-buf: heaps: Add heap name definitions
Hi TJ, Thanks for your review On Tue, Oct 01, 2024 at 11:03:41PM +0200, T.J. Mercier wrote: > On Mon, Sep 30, 2024 at 4:41 PM Maxime Ripard wrote: > > > > Following a recent discussion at last Plumbers, John Stultz, Sumit > > Sewal, TJ Mercier and I came to an agreement that we should document > > what the dma-buf heaps names are expected to be, and what the buffers > > attributes you'll get should be documented. > > > > Let's create that doc to make sure those attributes and names are > > guaranteed going forward. > > Hey, thanks for sending this! > > > Signed-off-by: Maxime Ripard > > > > --- > > > > To: Jonathan Corbet > > To: Sumit Semwal > > Cc: Benjamin Gaignard > > Cc: Brian Starkey > > Cc: John Stultz > > Cc: "T.J. Mercier" > > Cc: "Christian König" > > Cc: dri-devel@lists.freedesktop.org > > Cc: linaro-mm-...@lists.linaro.org > > Cc: linux-me...@vger.kernel.org > > Cc: linux-...@vger.kernel.org > > --- > > Documentation/userspace-api/dma-buf-heaps.rst | 71 +++ > > Documentation/userspace-api/index.rst | 1 + > > 2 files changed, 72 insertions(+) > > create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst > > > > diff --git a/Documentation/userspace-api/dma-buf-heaps.rst > > b/Documentation/userspace-api/dma-buf-heaps.rst > > new file mode 100644 > > index ..00436227b542 > > --- /dev/null > > +++ b/Documentation/userspace-api/dma-buf-heaps.rst > > @@ -0,0 +1,71 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +== > > +Allocating dma-buf using heaps > > +== > > + > > +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are > > +typically used to allocate buffers from a specific allocation pool, or to > > share > > +buffers across frameworks. > > + > > +Heaps > > += > > + > > +A heap represent a specific allocator. The Linux kernel currently supports > > the > > "represents" > > > +following heaps: > > + > > + - The ``system`` heap allocates virtually contiguous, cacheable, buffers > > Virtually contiguous sounds a little weird to me here. Sure, that's > what userspace will get when it maps the buffer (and I guess this *is* > UAPI documentation after all), but I'm not sure it's correct to say > that's a property of the buffer itself? What if we invert this and > instead say that there is NO guarantee that the memory for the buffer: > - is physically contiguous > - has any particular alignment (greater than page aligned) > - has any particular page size (large order allocations are attempted > first, but not guaranteed or even likely on some systems) > - has bounds on physical addresses > > Maybe that is too much detail here... Yeah, I don't know. It's getting philosophical, but I guess there's an infinite number of guarantees we wouldn't provide. It seems easier for me to maintain a list of the things a buffer is/has rather than the opposite. But maybe we can rephrase virtually contiguous if it's weird to you? > > + > > + - The ``reserved`` heap allocates physically contiguous, cacheable, > > buffers. > > + Depending on the platform, it might be called differently: > > + > > +- Acer Iconia Tab A500: ``linux,cma`` > > +- Allwinner sun4i, sun5i and sun7i families: ``default-pool`` > > +- Amlogic A1: ``linux,cma`` > > +- Amlogic G12A/G12B/SM1: ``linux,cma`` > > +- Amlogic GXBB/GXL: ``linux,cma`` > > +- ASUS EeePad Transformer TF101: ``linux,cma`` > > +- ASUS Google Nexus 7 (Project Bach / ME370TG) E1565: ``linux,cma`` > > +- ASUS Google Nexus 7 (Project Nakasi / ME370T) E1565: ``linux,cma`` > > +- ASUS Google Nexus 7 (Project Nakasi / ME370T) PM269: ``linux,cma`` > > +- Asus Transformer Infinity TF700T: ``linux,cma`` > > +- Asus Transformer Pad 3G TF300TG: ``linux,cma`` > > +- Asus Transformer Pad TF300T: ``linux,cma`` > > +- Asus Transformer Pad TF701T: ``linux,cma`` > > +- Asus Transformer Prime TF201: ``linux,cma`` > > +- ASUS Vivobook S 15: ``linux,cma`` > > +- Cadence KC705: ``linux,cma`` > > +- Digi International ConnectCore 6UL: ``linux,cma`` > > +- Freescale i.MX8DXL EVK: ``linux,cma`` > > +- Freescale TQMa8Xx: ``linux,cma`` > > +- Hisilicon Hikey: ``linux,cma`` > > +- Lenovo ThinkPad T14s Gen 6: ``linux,cma`` > > +- Len
Re: [PATCH] drm/atomic_helper: Add missing NULL check for drm_plane_helper_funcs.atomic_update
Hi,
On Mon, Sep 30, 2024 at 03:45:13PM -0400, Lyude Paul wrote:
> On Mon, 2024-09-30 at 09:06 +0200, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 30.09.24 um 09:01 schrieb Maxime Ripard:
> > > Hi,
> > >
> > > On Fri, Sep 27, 2024 at 04:46:16PM GMT, Lyude Paul wrote:
> > > > Something I discovered while writing rvkms since some versions of the
> > > > driver didn't have a filled out atomic_update function - we mention that
> > > > this callback is "optional", but we don't actually check whether it's
> > > > NULL
> > > > or not before calling it. As a result, we'll segfault if it's not filled
> > > > in.
> > > >
> > > >rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables]
> > > > modeset on [ENCODER:36:Virtual-36]
> > > >BUG: kernel NULL pointer dereference, address:
> > > >PGD 0 P4D 0
> > > >Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
> > > >Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> > > > edk2-20240813-1.fc40 08/13/2024
> > > >RIP: 0010:0x0
> > > >
> > > > So, let's fix that.
> > > >
> > > > Signed-off-by: Lyude Paul
> > > > Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Cc: # v3.19+
> > > So we had kind of a similar argument with drm_connector_init early this
> > > year, but I do agree we shouldn't fault if we're missing a callback.
> > >
> > > I do wonder how we can implement a plane without atomic_update though?
> > > Do we have drivers in such a case?
> >
> > That would likely be an output with an entirely static display. Hard to
> > imaging, I think.
> >
> > >
> > > If not, a better solution would be to make it mandatory and check it
> > > when registering.
> >
> > Although I r-b'ed the patch already, I'd also prefer this solution.
>
> Gotcha, FWIW the reason I went with this patch:
> * atomic_update is actually documented as being optional in the kernel docs,
>so we'd want to remove that if we make it mandatory
Sure, that makes total sense :)
> * rvkms currently doesn't have an atomic_update. We will likely have one
>whenever I get a chance to actually add CRC and/or writeback connector
>supports - but for the time being all we do is register a KMS device with
>vblank support.
WIP drivers can provide an empty implementation. And even if actually
didn't need it for $REASONS, I'd argue that an empty implementation (and
a comment) makes that explicit instead of making the reader guess why
it's not needed.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3 12/15] drm/bridge: Add ITE IT6263 LVDS to HDMI converter
On Mon, Oct 21, 2024 at 02:44:43PM +0800, Liu Ying wrote:
> +static int it6263_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> + int ret;
> +
> + ret = drm_atomic_helper_connector_hdmi_check(conn_state->connector,
> + conn_state->state);
> + if (ret)
> + return ret;
> +
> + return mode->clock > MAX_PIXEL_CLOCK_KHZ ? -EINVAL : 0;
drm_atomic_helper_connector_hdmi_check will already make that check, so
it's redundant.
Once fixed
Acked-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
On Sun, Oct 20, 2024 at 04:49:29AM +0200, Marek Vasut wrote:
> On 10/19/24 11:49 PM, Kieran Bingham wrote:
> > Quoting Marek Vasut (2024-10-12 21:37:59)
> > > On 10/11/24 5:10 AM, Liu Ying wrote:
> > >
> > > Hi,
> > >
> > > > > > > This Video PLL1 configuration since moved to &media_blk_ctrl {} ,
> > > > > > > but it is still in the imx8mp.dtsi . Therefore, to make your
> > > > > > > panel work at the correct desired pixel clock frequency instead
> > > > > > > of some random one inherited from imx8mp.dtsi, add the following
> > > > > > > to the pollux DT, I believe that will fix the problem and is the
> > > > > > > correct fix:
> > > > > > >
> > > > > > > &media_blk_ctrl {
> > > > > > > // 50680 = 7240 * 7 (for single-link LVDS, this is
> > > > > > > enough)
> > > > > > > // there is no need to multiply the clock by * 2
> > > > > > > assigned-clock-rates = <5>, <2>, <0>, <0>,
> > > > > > > <5>, <50680>;
> > > > > >
> > > > > > This assigns "video_pll1" clock rate to 506.8MHz which is currently
> > > > > > not
> > > > > > listed in imx_pll1443x_tbl[].
> > > > >
> > > > > Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic
> > > > > rates") the 1443x PLLs can be configured to arbitrary rates which for
> > > > > video PLL is desirable as those should produce accurate clock.
> > > >
> > > > Ack.
> > > >
> > > > >
> > > > > > Does the below patch[1] fix the regression issue? It explicitly sets
> > > > > > the clock frequency of the panel timing to 74.25MHz.
> > > > > >
> > > > > > [1]
> > > > > > https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1
> > > > > That patch is wrong, there is an existing entry for this panel in
> > > > > panel-simple.c which is correct and precise, please do not add that
> > > > > kind of imprecise duplicate timings into DT.
> > > >
> > > > At least the patch[1] is legitimate now to override the display
> > > > timing of the panel because the override mode is something
> > > > panel-simple.c supports.
> > >
> > > It may be possible to override the mode, but why would this be the
> > > desired if the panel-simple.c already contains valid accurate timings
> > > for this particular panel ?
> >
> > I'm confused a little here. Why is it that setting the panel timings in
> > the DT as per [1] make the display work, while the panel timeings in
> > panel-simple alone are not enough?
> >
> > Is there some difference in code path for 'how' the panel timings are
> > set as to whether they will apply fully or not ?
> Because [1] sets inaccurate pixel clock of 74.25 MHz, which can be divided
> down from random default Video PLL setting of 1039.5 MHz set in imx8mp.dtsi
> media_blk_ctrl , 1039.5 / 74.25 = 14 .
>
> The panel-simple pixel clock are 72.4 MHz, to achieve that accurately, it is
> necessary to reconfigure the Video PLL frequency to 506.8 MHz , which
> LCDIFv3 can do, but LDB can not, hence it has to be done in DT for now.
That the clock driver is broken and thus should be fixed through the DT is a
bit backward, don't you think?
AFAIU, the clock can't reach the ideal pixel clock panel-simple has. Do
you have the datasheet for that panel?
If so, using display_timings and shortening/extending the blanking
timings to match the clock that can be provided seems like a more robust
solution.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 1/6] drm/display: hdmi: add generic mode_valid helper
On Fri, Oct 18, 2024 at 11:34:19PM +0300, Dmitry Baryshkov wrote:
> Add drm_hdmi_connector_mode_valid(), generic helper for HDMI connectors.
> It can be either used directly or as a part of the .mode_valid callback.
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/display/drm_hdmi_helper.c | 25 +
> include/drm/display/drm_hdmi_helper.h | 4
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
> b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 74dd4d01dd9b..0ac5cb000ee2 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -256,3 +256,28 @@ drm_hdmi_compute_mode_clock(const struct
> drm_display_mode *mode,
> return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
> }
> EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> +
> +/**
> + * drm_hdmi_connector_mode_valid() - Check if mode is valid for HDMI
> connector
> + * @connector: DRM connector to validate the mode
> + * @mode: Display mode to validate
> + *
> + * Generic .mode_valid implementation for HDMI connectors.
> + */
> +enum drm_mode_status
> +drm_hdmi_connector_mode_valid(const struct drm_connector *connector,
> + const struct drm_display_mode *mode)
> +{
> + const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
> + unsigned long long rate;
> +
> + rate = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
> + if (!rate)
> + return MODE_ERROR;
> +
> + if (!funcs || !funcs->tmds_char_rate_valid)
> + return MODE_OK;
> +
> + return funcs->tmds_char_rate_valid(connector, mode, rate);
> +}
> +EXPORT_SYMBOL(drm_hdmi_connector_mode_valid);
As discussed in the discussion that sparked that change, I believe that
we should use hdmi_clock_valid.
AFAIU, your concern was that max_tmds_clock might get stale, but then it
would not only prevent mode_valid from running but also the commit
entirely.
We don't have any evidence from that, so I'd rather try to keep
consistency between the two. And we can always try to address whatever
issue we might have if it turned out to be a bad idea :)
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 0/6] drm/bridge: add ycbcr_420_allowed support
On Sat, 19 Oct 2024 00:49:11 +0300, Dmitry Baryshkov wrote: > One of the features that drm_bridge_connector can't handle currently is > setting of the ycbcr_420_allowed flag on the connector. Add the flag to > the drm_bridge struct and propagate it to the drm_connector as AND of > all flags in the bridge chain. > > > [ ... ] Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH v3 0/4] drm/tests: Fix some memory leaks
Hi, On Thu, Oct 17, 2024 at 02:31:21PM GMT, Jinjie Ruan wrote: > Fix some memory leaks in drm tests. > > Changes in v3: > - Adjust drm/drm_edid.h header to drm_kunit_helpers.c. > - Drop the "helper" in the helper name. > - s/fllowing/following/ > - Add Acked-by. This creates build failures since drm_display_mode were const before, and can't anymore. Maxime signature.asc Description: PGP signature
Re: vc4: HDMI Sink doesn't support RGB, something's wrong.
On Thu, Oct 17, 2024 at 05:26:46PM GMT, Stefan Wahren wrote: > Am 17.10.24 um 16:27 schrieb Maxime Ripard: > > On Wed, Oct 16, 2024 at 07:16:43PM GMT, Dave Stevenson wrote: > > > Hi Stefan > > > > > > On Tue, 15 Oct 2024 at 22:13, Stefan Wahren wrote: > > > > Hi Dave, > ... > > > > i prepared a branch for you, which contains the latest suspend2idle > > > > patches: > > > > > > > > https://github.com/lategoodbye/linux-dev/commits/v6.12-pm/ > > > > > > > > Steps: > > > > 1. Flash latest Raspberry Pi OS (32 bit) on SD card > > > > 2. Build Kernel from repo above with arm/multi_v7_defconfig > > > > 3. Replace Kernel, modules + DTB on SD card with build ones > > > > 4. add the following to confix.txt > > > > device_tree=bcm2837-rpi-3-b-plus.dtb > > > > enable_uart=1 > > > > 5. change/add the following to cmdline.txt > > > > console=ttyS1,115200 > > > > no_console_suspend=1 > > > > 6. connect the following devices to Raspberry Pi 3 B+ : > > > > USB mouse > > > > USB keyboard > > > > HDMI monitor > > > > Debug UART adapter (USB side to PC) > > > > 7. Power on board and boot into X11 > > > > 8. Change to root > > > > 9. Enable wakeup for ttyS1 > > > So I remember for next time > > > echo enabled > /sys/class/tty/ttyS1/power/wakeup > > > > > > > 10. Trigger suspend to idle via X11 (echo freeze > /sys/power/state) > > > > 11. Wakeup Raspberry Pi via Debug UART > > > I don't get the error you are seeing, but I also don't get the display > > > resuming. > > > pm has obviously killed the power to the HDMI block, and it has the > > > reset values in as can be seen via /sys/kernel/debug/dri/0/hdmi_regs. > > > Nothing in the driver restores these registers, and I'm not sure if it > > > is meant to do so. > > > Run kmstest or similar from this state and the change of mode > > > reprogrammes the blocks so we get the display back again. > > > > > > I've also enabled CONFIG_DRM_LOAD_EDID_FIRMWARE so that I can use your > > > EDID, and get the same results. > > > > > > Knee-capping the HDMI block on suspend seems an unlikely mechanism to > > > work reliably. On the more recent Pis there is a need to be quite > > > careful in disabling the pipeline to avoid getting data stuck in > > > FIFOs. > > > I feel I must be missing something here. > > > > I think we're probably missing calls to drm_mode_config_helper_suspend and > > drm_mode_config_helper_resume. > > Okay, i tried this and it works better (HDMI resumes fast), but it also > triggers a lot of WARN vc4_plane_reset deviates from the helper there: https://elixir.bootlin.com/linux/v6.11.3/source/drivers/gpu/drm/drm_atomic_state_helper.c#L326 We should adjust it. > and the "doesn't support RGB ..." warnings are still there. > > I pushed my changes to the branch and attached the dmesg output. I can't help you there, it doesn't make sense to me. The EDID should be correct. Maxime signature.asc Description: PGP signature
Re: [PATCH v3 00/23] drm/msm/dpu: Add Concurrent Writeback Support for DPU 10.x+
On Wed, Oct 16, 2024 at 06:21:06PM GMT, Jessica Zhang wrote: > Changes in v3: > - Dropped support for CWB on DP connectors for now > - Dropped unnecessary PINGPONG array in *_setup_cwb() > - Add a check to make sure CWB and CDM aren't supported simultaneously > (Dmitry) > - Document cwb_enabled checks in dpu_crtc_get_topology() (Dmitry) > - Moved implementation of drm_crtc_in_clone_mode() to drm_crtc.c (Jani) > - Dropped duplicate error message for reserving CWB resources (Dmitry) > - Added notes in framework changes about posting a separate series to > add proper KUnit tests (Maxime) I mean, I asked for kunit tests, not for a note that is going to be dropped when applying. Maxime signature.asc Description: PGP signature
Re: vc4: HDMI Sink doesn't support RGB, something's wrong.
On Wed, Oct 16, 2024 at 07:16:43PM GMT, Dave Stevenson wrote: > Hi Stefan > > On Tue, 15 Oct 2024 at 22:13, Stefan Wahren wrote: > > > > Hi Dave, > > > > Am 15.10.24 um 11:32 schrieb Dave Stevenson: > > > On Mon, 14 Oct 2024 at 22:16, Stefan Wahren wrote: > > >> > > >> Am 14.10.24 um 12:54 schrieb Dave Stevenson: > > >>> On Mon, 14 Oct 2024 at 10:04, Maxime Ripard wrote: > > >>>> Hi, > > >>>> > > >>>> On Sun, Oct 13, 2024 at 09:57:58PM GMT, Stefan Wahren wrote: > > >>>>> Am 13.10.24 um 21:11 schrieb Dave Stevenson: > > >>>>>> Hi Stefan. > > >>>>>> > > >>>>>> On Sun, 13 Oct 2024, 18:19 Stefan Wahren, wrote: > > >>>>>> > > >>>>>> Hi, > > >>>>>> > > >>>>>> i recently switch for my suspend2idle tests from Raspberry Pi > > >>>>>> Bullseye > > >>>>>> to Bookworm. After that testing suspend2idle shows a new > > >>>>>> warning > > >>>>>> which i > > >>>>>> never saw before: > > >>>>>> > > >>>>>> HDMI Sink doesn't support RGB, something's wrong. > > >>>>>> > > >>>>>> > > >>>>>> Can you provide the edid of your display please? > > >> ... > > >>>>> > > >>>>> The failure is coming from sink_supports_format_bpc()[1], but the flag > > >>>>> for DRM_COLOR_FORMAT_RGB444 should have been set from > > >>>>> update_display_info()[2] parsing the EDID. > > >>>>> > > >>>>> Loading that EDID in via drm.edid_firmware has given me a console at > > >>>>> 1920x1200@60 without any issues, so I'm a little confused as to what > > >>>>> is going on. > > >> Since this warning only occurs on resume and not during normal boot, i > > >> would assume there is no issue with EDID. Maybe the flag get lost. I > > >> should have mention that X11 doesn't recover in this case and the > > >> display stays black. > > > Ah, I hadn't realised you meant it was only on resume that it didn't > > > come back up. > > > > > > I suspect you're right that the state gets lost somehow. It may be > > > triggered by the returning of connector_status_unknown on the > > > connector, but haven't traced it back. > > > > > > If I pick up your patches, what do I need to add to replicate this? > > i prepared a branch for you, which contains the latest suspend2idle patches: > > > > https://github.com/lategoodbye/linux-dev/commits/v6.12-pm/ > > > > Steps: > > 1. Flash latest Raspberry Pi OS (32 bit) on SD card > > 2. Build Kernel from repo above with arm/multi_v7_defconfig > > 3. Replace Kernel, modules + DTB on SD card with build ones > > 4. add the following to confix.txt > > device_tree=bcm2837-rpi-3-b-plus.dtb > > enable_uart=1 > > 5. change/add the following to cmdline.txt > > console=ttyS1,115200 > > no_console_suspend=1 > > 6. connect the following devices to Raspberry Pi 3 B+ : > > USB mouse > > USB keyboard > > HDMI monitor > > Debug UART adapter (USB side to PC) > > 7. Power on board and boot into X11 > > 8. Change to root > > 9. Enable wakeup for ttyS1 > > So I remember for next time > echo enabled > /sys/class/tty/ttyS1/power/wakeup > > > 10. Trigger suspend to idle via X11 (echo freeze > /sys/power/state) > > 11. Wakeup Raspberry Pi via Debug UART > > I don't get the error you are seeing, but I also don't get the display > resuming. > pm has obviously killed the power to the HDMI block, and it has the > reset values in as can be seen via /sys/kernel/debug/dri/0/hdmi_regs. > Nothing in the driver restores these registers, and I'm not sure if it > is meant to do so. > Run kmstest or similar from this state and the change of mode > reprogrammes the blocks so we get the display back again. > > I've also enabled CONFIG_DRM_LOAD_EDID_FIRMWARE so that I can use your > EDID, and get the same results. > > Knee-capping the HDMI block on suspend seems an unlikely mechanism to > work reliably. On the more recent Pis there is a need to be quite > careful in disabling the pipeline to avoid getting data stuck in > FIFOs. > I feel I must be missing something here. I think we're probably missing calls to drm_mode_config_helper_suspend and drm_mode_config_helper_resume. Maxime signature.asc Description: PGP signature
Re: [PATCH v10 0/3] Add initial support for the Rockchip RK3588 HDMI TX Controller
On Wed, 16 Oct 2024 23:06:50 +0300, Cristian Ciocaltea wrote: > The Rockchip RK3588 SoC family integrates the Synopsys DesignWare HDMI > 2.1 Quad-Pixel (QP) TX controller, which is a new IP block, quite > different from those used in the previous generations of Rockchip SoCs. > > The controller supports the following features, among others: > > * Fixed Rate Link (FRL) > * Display Stream Compression (DSC) > * 4K@120Hz and 8K@60Hz video modes > * Variable Refresh Rate (VRR) including Quick Media Switching (QMS) > * Fast Vactive (FVA) > * SCDC I2C DDC access > * Multi-stream audio > * Enhanced Audio Return Channel (EARC) > > [...] Applied to misc/kernel.git (drm-misc-next). Thanks! Maxime
Re: [PATCH v2 1/4] drm/tests: helpers: Add helper for drm_display_mode_from_cea_vic()
On Thu, Oct 17, 2024 at 09:33:07AM GMT, Jinjie Ruan wrote: > >> diff --git a/include/drm/drm_kunit_helpers.h > >> b/include/drm/drm_kunit_helpers.h > >> index e7cc17ee4934..1e7fd4be550c 100644 > >> --- a/include/drm/drm_kunit_helpers.h > >> +++ b/include/drm/drm_kunit_helpers.h > >> @@ -4,6 +4,7 @@ > >> #define DRM_KUNIT_HELPERS_H_ > >> > >> #include > >> +#include > >> > >> #include > >> > >> @@ -120,4 +121,9 @@ drm_kunit_helper_create_crtc(struct kunit *test, > >> const struct drm_crtc_funcs *funcs, > >> const struct drm_crtc_helper_funcs *helper_funcs); > >> > >> +struct drm_display_mode * > >> +drm_kunit_helper_display_mode_from_cea_vic(struct kunit *test, > >> + struct drm_device *dev, > >> + u8 video_code); > > > > It's not clear to me what you need the drm_edid header, you just return > > a drm_display_mode pointer so you can just forward declare the structure > > > There is a compile error without the header，because there is no > "drm_display_mode_from_cea_vic()" declare. > > drivers/gpu/drm/tests/drm_kunit_helpers.c:341:16: error: implicit > declaration of function ‘drm_display_mode_from_cea_vic’; did you mean > ‘drm_kunit_display_mode_from_cea_vic’? > [-Werror=implicit-function-declaration] > 341 | mode = drm_display_mode_from_cea_vic(dev, video_code); > |^ > |drm_kunit_display_mode_from_cea_vic > drivers/gpu/drm/tests/drm_kunit_helpers.c:341:14: warning: assignment to > ‘struct drm_display_mode *’ from ‘int’ makes pointer from integer > without a cast [-Wint-conversion] > 341 | mode = drm_display_mode_from_cea_vic(dev, video_code); > | ^ Right, but the error is in the C file, not the header. Maxime signature.asc Description: PGP signature
Re: [PATCH v2 4/4] drm/tests: hdmi: Fix memory leaks in drm_display_mode_from_cea_vic()
On Mon, Oct 14, 2024 at 08:52:04PM GMT, Jinjie Ruan wrote:
> modprobe drm_hdmi_state_helper_test and then rmmod it, the following
> memory leak occurs.
>
> The `mode` allocated in drm_mode_duplicate() called by
> drm_display_mode_from_cea_vic() is not freed, which cause the memory leak:
>
> unreferenced object 0xff80ccd18100 (size 128):
> comm "kunit_try_catch", pid 1851, jiffies 4295059695
> hex dump (first 32 bytes):
> 57 62 00 00 80 02 90 02 f0 02 20 03 00 00 e0 01 Wb .
> ea 01 ec 01 0d 02 00 00 0a 00 00 00 00 00 00 00
> backtrace (crc c2f1aa95):
> [<0f10b11b>] kmemleak_alloc+0x34/0x40
> [<1cd4cf73>] __kmalloc_cache_noprof+0x26c/0x2f4
> [<f1f3cffa>] drm_mode_duplicate+0x44/0x19c
> [<8cbeef13>] drm_display_mode_from_cea_vic+0x88/0x98
> [<19daaacf>] 0xffedc11ae69c
> [<0aad0f85>] kunit_try_run_case+0x13c/0x3ac
> [<a9210bac>] kunit_generic_run_threadfn_adapter+0x80/0xec
> [<0a0b2e9e>] kthread+0x2e8/0x374
> [<bd668858>] ret_from_fork+0x10/0x20
> ..
>
> Free `mode` by using drm_kunit_helper_display_mode_from_cea_vic()
> to fix it.
>
> Cc: sta...@vger.kernel.org
> Fixes: 4af70f19e559 ("drm/tests: Add RGB Quantization tests")
> Signed-off-by: Jinjie Ruan
Acked-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 1/4] drm/tests: helpers: Add helper for drm_display_mode_from_cea_vic()
On Mon, Oct 14, 2024 at 08:52:01PM GMT, Jinjie Ruan wrote:
> As Maxime suggested, add a new helper
> drm_kunit_helper_display_mode_from_cea_vic(), it can replace
> the direct call of drm_display_mode_from_cea_vic(), and it will
> help solving the `mode` memory leaks.
>
> Suggested-by: Maxime Ripard
> Signed-off-by: Jinjie Ruan
> ---
> drivers/gpu/drm/tests/drm_kunit_helpers.c | 40 +++
> include/drm/drm_kunit_helpers.h | 6
> 2 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index aa62719dab0e..dc70bafcd394 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -311,6 +311,46 @@ drm_kunit_helper_create_crtc(struct kunit *test,
> }
> EXPORT_SYMBOL_GPL(drm_kunit_helper_create_crtc);
>
> +static void kunit_action_drm_mode_destroy(void *ptr)
> +{
> + struct drm_display_mode *mode = ptr;
> +
> + drm_mode_destroy(NULL, mode);
> +}
> +
> +/**
> + * drm_kunit_helper_display_mode_from_cea_vic() - return a mode for CEA VIC
> + for a KUnit test
> + * @test: The test context object
> + * @dev: DRM device
> + * @video_code: CEA VIC of the mode
> + *
> + * Creates a new mode matching the specified CEA VIC for a KUnit test.
> + *
> + * Resources will be cleaned up automatically.
> + *
> + * Returns: A new drm_display_mode on success or NULL on failure
> + */
> +struct drm_display_mode *
> +drm_kunit_helper_display_mode_from_cea_vic(struct kunit *test,
> +struct drm_device *dev,
> +u8 video_code)
> +{
> + struct drm_display_mode *mode;
> + int ret;
> +
> + mode = drm_display_mode_from_cea_vic(dev, video_code);
> +
> + ret = kunit_add_action_or_reset(test,
> + kunit_action_drm_mode_destroy,
> + mode);
> + if (ret)
> + return NULL;
> +
> + return mode;
> +}
> +EXPORT_SYMBOL_GPL(drm_kunit_helper_display_mode_from_cea_vic);
> +
I think you can drop the "helper" name there, it's usually reserved for
blanket implementation of DRM hooks. This one isn't a hook, so just
calling it drm_kunit_display_mode_from_cea_vic makes a bit more sense to
me.
> MODULE_AUTHOR("Maxime Ripard ");
> MODULE_DESCRIPTION("KUnit test suite helper functions");
> MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
> index e7cc17ee4934..1e7fd4be550c 100644
> --- a/include/drm/drm_kunit_helpers.h
> +++ b/include/drm/drm_kunit_helpers.h
> @@ -4,6 +4,7 @@
> #define DRM_KUNIT_HELPERS_H_
>
> #include
> +#include
>
> #include
>
> @@ -120,4 +121,9 @@ drm_kunit_helper_create_crtc(struct kunit *test,
>const struct drm_crtc_funcs *funcs,
>const struct drm_crtc_helper_funcs *helper_funcs);
>
> +struct drm_display_mode *
> +drm_kunit_helper_display_mode_from_cea_vic(struct kunit *test,
> + struct drm_device *dev,
> +u8 video_code);
It's not clear to me what you need the drm_edid header, you just return
a drm_display_mode pointer so you can just forward declare the structure
Once fixed
Acked-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 4/8] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate
On Sat, Oct 12, 2024 at 02:18:16PM GMT, Liu Ying wrote:
> On 10/11/2024, Maxime Ripard wrote:
> > On Mon, Sep 30, 2024 at 03:55:30PM GMT, Liu Ying wrote:
> >> On 09/30/2024, Maxime Ripard wrote:
> >>> On Mon, Sep 30, 2024 at 01:28:59PM GMT, Liu Ying wrote:
> >>>> Multiple display modes could be read from a display device's EDID.
> >>>> Use clk_round_rate() to validate the "ldb" clock rate for each mode
> >>>> in drm_bridge_funcs::mode_valid() to filter unsupported modes out.
> >>>>
> >>>> Also, if the "ldb" clock and the pixel clock are sibling in clock
> >>>> tree, use clk_round_rate() to validate the pixel clock rate against
> >>>> the "ldb" clock. This is not done in display controller driver
> >>>> because drm_crtc_helper_funcs::mode_valid() may not decide to do
> >>>> the validation or not if multiple encoders are connected to the CRTC,
> >>>> e.g., i.MX93 LCDIF may connect with MIPI DSI controller, LDB and
> >>>> parallel display output simultaneously.
> >>>>
> >>>> Signed-off-by: Liu Ying
> >>>> ---
> >>>> drivers/gpu/drm/bridge/fsl-ldb.c | 22 ++
> >>>> 1 file changed, 22 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
> >>>> b/drivers/gpu/drm/bridge/fsl-ldb.c
> >>>> index b559f3e0bef6..ee8471c86617 100644
> >>>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> >>>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> >>>> @@ -11,6 +11,7 @@
> >>>> #include
> >>>> #include
> >>>> #include
> >>>> +#include
> >>>>
> >>>> #include
> >>>> #include
> >>>> @@ -64,6 +65,7 @@ struct fsl_ldb_devdata {
> >>>> u32 lvds_ctrl;
> >>>> bool lvds_en_bit;
> >>>> bool single_ctrl_reg;
> >>>> +bool ldb_clk_pixel_clk_sibling;
> >>>> };
> >>>>
> >>>> static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> >>>> @@ -74,11 +76,13 @@ static const struct fsl_ldb_devdata
> >>>> fsl_ldb_devdata[] = {
> >>>> [IMX8MP_LDB] = {
> >>>> .ldb_ctrl = 0x5c,
> >>>> .lvds_ctrl = 0x128,
> >>>> +.ldb_clk_pixel_clk_sibling = true,
> >>>> },
> >>>> [IMX93_LDB] = {
> >>>> .ldb_ctrl = 0x20,
> >>>> .lvds_ctrl = 0x24,
> >>>> .lvds_en_bit = true,
> >>>> +.ldb_clk_pixel_clk_sibling = true,
> >>>> },
> >>>> };
> >>>>
> >>>> @@ -269,11 +273,29 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
> >>>> const struct drm_display_info *info,
> >>>> const struct drm_display_mode *mode)
> >>>> {
> >>>> +unsigned long link_freq, pclk_rate, rounded_pclk_rate;
> >>>> struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> >>>>
> >>>> if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 16 : 8))
> >>>> return MODE_CLOCK_HIGH;
> >>>>
> >>>> +/* Validate "ldb" clock rate. */
> >>>> +link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> >>>> +if (link_freq != clk_round_rate(fsl_ldb->clk, link_freq))
> >>>> +return MODE_NOCLOCK;
> >>>> +
> >>>> +/*
> >>>> + * Use "ldb" clock to validate pixel clock rate,
> >>>> + * if the two clocks are sibling.
> >>>> + */
> >>>> +if (fsl_ldb->devdata->ldb_clk_pixel_clk_sibling) {
> >>>> +pclk_rate = mode->clock * HZ_PER_KHZ;
> >>>> +
> >>>> +rounded_pclk_rate = clk_round_rate(fsl_ldb->clk,
> >>>> pclk_rate);
> >>>> +if (rounded_pclk_rate != pclk_rate)
> >>>> +return MODE_NOCLOCK;
> >>>> +}
> >>>> +
> >>>
> >>> I guess this i
Re: [PATCH 3/3] drm/tests: hdmi: Fix memory leaks in drm_display_mode_from_cea_vic()
On Mon, Oct 14, 2024 at 03:16:32PM GMT, Jinjie Ruan wrote: > modprobe drm_hdmi_state_helper_test and then rmmod it, the following > memory leak occurs. > > The `mode` allocated in drm_mode_duplicate() called by > drm_display_mode_from_cea_vic() is not freed, which cause the memory leak: > > unreferenced object 0xff80ccd18100 (size 128): > comm "kunit_try_catch", pid 1851, jiffies 4295059695 > hex dump (first 32 bytes): > 57 62 00 00 80 02 90 02 f0 02 20 03 00 00 e0 01 Wb . > ea 01 ec 01 0d 02 00 00 0a 00 00 00 00 00 00 00 > backtrace (crc c2f1aa95): > [<0f10b11b>] kmemleak_alloc+0x34/0x40 > [<1cd4cf73>] __kmalloc_cache_noprof+0x26c/0x2f4 > [] drm_mode_duplicate+0x44/0x19c > [<8cbeef13>] drm_display_mode_from_cea_vic+0x88/0x98 > [<19daaacf>] 0xffedc11ae69c > [<0aad0f85>] kunit_try_run_case+0x13c/0x3ac > [ ] kunit_generic_run_threadfn_adapter+0x80/0xec > [<0a0b2e9e>] kthread+0x2e8/0x374 > [ ] ret_from_fork+0x10/0x20 > .. > > Free `mode` by calling drm_mode_destroy() to fix it. > > Cc: sta...@vger.kernel.org > Fixes: 4af70f19e559 ("drm/tests: Add RGB Quantization tests") > Signed-off-by: Jinjie Ruan > --- > drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c > b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c > index 34ee95d41f29..70b91e8c8219 100644 > --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c > +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c > @@ -466,6 +466,8 @@ static void > drm_test_check_broadcast_rgb_auto_cea_mode_vic_1(struct kunit *test) > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state); > > KUNIT_EXPECT_FALSE(test, conn_state->hdmi.is_limited_range); > + > + drm_mode_destroy(drm, mode); > } Same comment than on patch 1 Maxime signature.asc Description: PGP signature
Re: [PATCH 1/3] drm/connector: hdmi: Fix memory leak in drm_display_mode_from_cea_vic()
On Mon, Oct 14, 2024 at 03:16:30PM GMT, Jinjie Ruan wrote: > modprobe drm_connector_test and then rmmod drm_connector_test, > the following memory leak occurs. > > The `mode` allocated in drm_mode_duplicate() called by > drm_display_mode_from_cea_vic() is not freed, which cause the memory leak: > > unreferenced object 0xff80cb0ee400 (size 128): > comm "kunit_try_catch", pid 1948, jiffies 4294950339 > hex dump (first 32 bytes): > 14 44 02 00 80 07 d8 07 04 08 98 08 00 00 38 04 .D8. > 3c 04 41 04 65 04 00 00 05 00 00 00 00 00 00 00 <.A.e... > backtrace (crc 90e9585c): > [] kmemleak_alloc+0x34/0x40 > [ ] __kmalloc_cache_noprof+0x26c/0x2f4 > [ ] drm_mode_duplicate+0x44/0x19c > [ ] drm_display_mode_from_cea_vic+0x88/0x98 > [ ] 0xffdc982a4868 > [<5d164dbc>] kunit_try_run_case+0x13c/0x3ac > [<6fb23398>] kunit_generic_run_threadfn_adapter+0x80/0xec > [<6ea56ca0>] kthread+0x2e8/0x374 > [<0676063f>] ret_from_fork+0x10/0x20 > .. > > Free `mode` by calling drm_mode_destroy() to fix it. > > Cc: sta...@vger.kernel.org > Fixes: abb6f74973e2 ("drm/tests: Add HDMI TDMS character rate tests") > Signed-off-by: Jinjie Ruan > --- > drivers/gpu/drm/tests/drm_connector_test.c | 24 ++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/tests/drm_connector_test.c > b/drivers/gpu/drm/tests/drm_connector_test.c > index 15e36a8db685..9c94d26b3486 100644 > --- a/drivers/gpu/drm/tests/drm_connector_test.c > +++ b/drivers/gpu/drm/tests/drm_connector_test.c > @@ -1004,6 +1004,8 @@ static void > drm_test_drm_hdmi_compute_mode_clock_rgb(struct kunit *test) > rate = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB); > KUNIT_ASSERT_GT(test, rate, 0); > KUNIT_EXPECT_EQ(test, mode->clock * 1000ULL, rate); > + > + drm_mode_destroy(drm, mode); > } If KUNIT_ASSERT_GT triggers, then we would end up leaking the mode as well. I think we should create a kunit_drm_display_mode_from_cea_vic() function that registers a kunit action to free the mode when the test is done. Maxime signature.asc Description: PGP signature
Re: vc4: HDMI Sink doesn't support RGB, something's wrong.
Hi, On Sun, Oct 13, 2024 at 09:57:58PM GMT, Stefan Wahren wrote: > Am 13.10.24 um 21:11 schrieb Dave Stevenson: > > Hi Stefan. > > > > On Sun, 13 Oct 2024, 18:19 Stefan Wahren, wrote: > > > > Hi, > > > > i recently switch for my suspend2idle tests from Raspberry Pi Bullseye > > to Bookworm. After that testing suspend2idle shows a new warning > > which i > > never saw before: > > > > HDMI Sink doesn't support RGB, something's wrong. > > > > > > Can you provide the edid of your display please? > > Sure > > [ 27.145] (II) modeset(0): Monitor name: HP ZR2440w > [ 27.145] (II) modeset(0): Serial No: CN423402RL > > ... > > [ 27.146] (II) modeset(0): EDID (in hex): > [ 27.146] (II) modeset(0): 000022f0562901010101 > [ 27.146] (II) modeset(0): 22160103803420782afc81a4554d9d25 > [ 27.146] (II) modeset(0): 125054210800d1c081c0814081809500 > [ 27.146] (II) modeset(0): a940b3000101283c80a070b023403020 > [ 27.146] (II) modeset(0): 36000644211a00fd00183c18 > [ 27.146] (II) modeset(0): 5011000a20202020202000fc0048 > [ 27.146] (II) modeset(0): 50205a5232343430770a202000ff > [ 27.146] (II) modeset(0): 00434e343233343032524c0a2020015b > [ 27.146] (II) modeset(0): 02031ff14c901f051404130302070612 > [ 27.147] (II) modeset(0): 0165030c00100023090707830102 > [ 27.147] (II) modeset(0): 3a801871382d40582c4500064421 > [ 27.147] (II) modeset(0): 1e023a80d072382d40102c4580064421 > [ 27.147] (II) modeset(0): 1e011d007251d01e206e28550006 > [ 27.147] (II) modeset(0): 44211e011d00bc52d01e20b82855 > [ 27.147] (II) modeset(0): 400644211e8c0ad08a20e02d1010 > [ 27.147] (II) modeset(0): 3e960006442118c1 It's a bit hard to extract, could you provide the output of cat /sys/class/drm/card/cardX-HDMI-A-X/edid | edid-decode --check Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH v12 09/15] drm/vkms: Remove useless drm_rotation_simplify
On Fri, Oct 11, 2024 at 10:53:52AM GMT, Maira Canal wrote: > Hi Louis, > > On 10/11/24 06:36, Louis Chauvet wrote: > > > > Hi all, > > > > Until this point, this series has not received any major comments since > > v9. I will commit patches 1-9 next week if there are no further comments. > > > > Although we are maintainers of VKMS, it isn't recommended that we push > our own changes without even the Ack of another person. Please, read the > "drm-misc Committer Guidelines" [1]. It's not that it's not recommended, it's that you shouldn't, really. Maxime signature.asc Description: PGP signature
Re: [PATCH 4/8] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate
On Mon, Sep 30, 2024 at 03:55:30PM GMT, Liu Ying wrote:
> On 09/30/2024, Maxime Ripard wrote:
> > On Mon, Sep 30, 2024 at 01:28:59PM GMT, Liu Ying wrote:
> >> Multiple display modes could be read from a display device's EDID.
> >> Use clk_round_rate() to validate the "ldb" clock rate for each mode
> >> in drm_bridge_funcs::mode_valid() to filter unsupported modes out.
> >>
> >> Also, if the "ldb" clock and the pixel clock are sibling in clock
> >> tree, use clk_round_rate() to validate the pixel clock rate against
> >> the "ldb" clock. This is not done in display controller driver
> >> because drm_crtc_helper_funcs::mode_valid() may not decide to do
> >> the validation or not if multiple encoders are connected to the CRTC,
> >> e.g., i.MX93 LCDIF may connect with MIPI DSI controller, LDB and
> >> parallel display output simultaneously.
> >>
> >> Signed-off-by: Liu Ying
> >> ---
> >> drivers/gpu/drm/bridge/fsl-ldb.c | 22 ++
> >> 1 file changed, 22 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
> >> b/drivers/gpu/drm/bridge/fsl-ldb.c
> >> index b559f3e0bef6..ee8471c86617 100644
> >> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> >> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> >> @@ -11,6 +11,7 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >>
> >> #include
> >> #include
> >> @@ -64,6 +65,7 @@ struct fsl_ldb_devdata {
> >>u32 lvds_ctrl;
> >>bool lvds_en_bit;
> >>bool single_ctrl_reg;
> >> + bool ldb_clk_pixel_clk_sibling;
> >> };
> >>
> >> static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> >> @@ -74,11 +76,13 @@ static const struct fsl_ldb_devdata fsl_ldb_devdata[]
> >> = {
> >>[IMX8MP_LDB] = {
> >>.ldb_ctrl = 0x5c,
> >>.lvds_ctrl = 0x128,
> >> + .ldb_clk_pixel_clk_sibling = true,
> >>},
> >>[IMX93_LDB] = {
> >>.ldb_ctrl = 0x20,
> >>.lvds_ctrl = 0x24,
> >>.lvds_en_bit = true,
> >> + .ldb_clk_pixel_clk_sibling = true,
> >>},
> >> };
> >>
> >> @@ -269,11 +273,29 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
> >> const struct drm_display_info *info,
> >> const struct drm_display_mode *mode)
> >> {
> >> + unsigned long link_freq, pclk_rate, rounded_pclk_rate;
> >>struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> >>
> >>if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 16 : 8))
> >>return MODE_CLOCK_HIGH;
> >>
> >> + /* Validate "ldb" clock rate. */
> >> + link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> >> + if (link_freq != clk_round_rate(fsl_ldb->clk, link_freq))
> >> + return MODE_NOCLOCK;
> >> +
> >> + /*
> >> + * Use "ldb" clock to validate pixel clock rate,
> >> + * if the two clocks are sibling.
> >> + */
> >> + if (fsl_ldb->devdata->ldb_clk_pixel_clk_sibling) {
> >> + pclk_rate = mode->clock * HZ_PER_KHZ;
> >> +
> >> + rounded_pclk_rate = clk_round_rate(fsl_ldb->clk, pclk_rate);
> >> + if (rounded_pclk_rate != pclk_rate)
> >> + return MODE_NOCLOCK;
> >> + }
> >> +
> >
> > I guess this is to workaround the fact that the parent rate would be
> > changed, and thus the sibling rate as well? This should be documented in
> > a comment if so.
>
> This is to workaround the fact that the display controller driver
> (lcdif_kms.c) cannot do the mode validation against pixel clock, as
> the commit message mentions.
That part is still not super clear to me, but it's also not super
important to the discussion.
My point is: from a clock API standpoint, there's absolutely no reason
to consider sibling clocks. clk_round_rate() should give you the rate
you want. If it affects other clocks it shouldn't, it's a clock driver
bug.
You might want to workaround it, but this is definitely not something
you should gloss over: it's a hack, it needs to be documented as such.
> The parent clock is IMX8MP_VIDEO_PLL1_OUT and it's clock rate is not
> supposed to be changed any more once IMX8MP_VIDEO_PLL1 clock rate is
> set by using DT assigned-clock-rates property. For i.MX8MP EVK, the
> clock rate is assigned to 103950Hz in imx8mp.dtsi in media_blk_ctrl
> node.
There's two things wrong with what you just described:
- assigned-clock-rates never provided the guarantee that the clock
rate wouldn't change later on. So if you rely on that, here's your
first bug.
- If the parent clock rate must not change, why does that clock has
SET_RATE_PARENT then? Because that's the bug you're trying to work
around.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 0/7] drm: Add physical status and BMC support to conenctor
Hi, On Fri, Oct 11, 2024 at 08:43:05AM GMT, Thomas Zimmermann wrote: > Track a connector's physical status separately from the logical status > and implement BMC support for DRM drivers. Connectors with virtual BMC > stay connected even if no display is physically connected. DRM clients > then continue displaying output to the BMC. > > Ast and mgag200 have been doing this for a while. Moving this into > struct drm_connector and probe helpers simplifies htese divers and > makes the functionality available to others. Hibmc is a candidate here. > > Patch just simplifies code in probe helpers and has been acked as part > of the series at [1]. > > Pathces 2 and 3 add the physical status and a BMC flag to struct > drm_connector. Usually physical connector status and regular, logical > status are in sync, so nothing changes for most drivers. If the the > BMC flag has been set, the logical status is always connected. The > probe helpers also take care of sending hotplug events if the physical > status changes. > > Patches 4 to 7 update ast and mgag200. Both drivers already implement > their own tracking of physical status, which is now handled by DRM > helpers. Ast also receives two simple bug fixes for cleaning up EDID > properties in the BMC case. > > Tested on ast and mgag200 hardware. Another driver that could make use > of this functionality is hibmc. Generally speaking, it looks ok, but given how much of a corner case it is, we should have kunit tests to cover the whole thing. Maxime signature.asc Description: PGP signature
Re: [PATCH v12 13/15] drm/vkms: Create KUnit tests for YUV conversions
On Tue, Oct 08, 2024 at 11:23:22AM GMT, Louis Chauvet wrote:
>
> Hi,
>
> > > + * The YUV color representation were acquired via the colour python
> > > framework.
> > > + * Below are the function calls used for generating each case.
> > > + *
> > > + * For more information got to the docs:
> > > + *
> > > https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html
> > > + */
> > > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > > + /*
> > > + * colour.RGB_to_YCbCr(,
> > > + * K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > > + * in_bits = 16,
> > > + * in_legal = False,
> > > + * in_int = True,
> > > + * out_bits = 8,
> > > + * out_legal = False,
> > > + * out_int = True)
> > > + */
> >
> > We should really detail what the intent and expected outcome is supposed
> > to be here. Relying on a third-party python library call for
> > documentation isn't great.
>
> This was requested by Pekka in the [v2] of this series.
Ok.
> I can add something like this before each tests, but I think having the
> exact python code used may help people to understand what should be the
> behavior, and refering to the python code to understand the conversion.
Help, sure. Be the *only* documentation, absolutely not.
Let's turn this around. You run kunit, one of these tests fail:
- It adds cognitive load to try to identify and make sense of an
unknown lib.
- How can we check that the arguments you provided there are the one
you actually wanted to provide, and you didn't make a typo?
> I can add something like this before each tests to clarify the tested
> case:
>
> Test cases for conversion between YUV BT601 limited range and
> RGB using the ITU-R BT.601 weights.
>
> Or maybe just documenting the structure yuv_u8_to_argb_u16_case:
>
> @encoding: Encoding used to convert RGB to YUV
> @range: Range used to convert RGB to YUV
> @n_colors: Count of test colors in this case
> @format_pair.name: Name used for this color conversion, used to
> clarify the test results
> @format_pair.rgb: RGB color tested
> @format_pair.yuv: Same color as @format_pair.rgb, but converted to
> YUV using @encoding and @range.
>
> What do you think?
That it's welcome, but it still doesn't allow to figure out what your
intent was with this test 2 years from now.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v9 1/3] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library
On Thu, 10 Oct 2024 01:40:49 +0300, Cristian Ciocaltea wrote: > The Synopsys DesignWare HDMI 2.1 Quad-Pixel (QP) TX Controller IP > supports the following features, among others: > > * Fixed Rate Link (FRL) > * Display Stream Compression (DSC) > > [ ... ] Acked-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 2/2] MAINTAINERS: Fix VC4's mailing lists
On Wed, Oct 09, 2024 at 04:13:01PM GMT, Dave Stevenson wrote: > Hi Maíra > > On Wed, 9 Oct 2024 at 15:12, Maíra Canal wrote: > > > > VC4 has two relevant mailing list: kernel-l...@raspberrypi.com and > > dri-devel@lists.freedesktop.org. Therefore, list those two mailing > > lists in the VC4 section. > > dri-devel@lists.freedesktop.org is automatically picked up by > get_maintainer.pl due to vc4 living under /drivers/gpu/drm. Likewise > the DT bindings are covered. AIUI that means we don't need to list it > explicitly. > > > Actually, Raspberry Pi Kernel Maintenance > > was already listed in the VC4 section, but it was listed as a reviewer. > > List it as a mailing list. > > I had this debate with Maxime in v1 when I added this [1]. > It's not an open list as most L: entries are. The top of MAINTAINERS > lists as "L: *Mailing list* that is relevant to this area". That > mailing list is dri-devel. > You also get "Broadcom internal kernel review list > " listed as R: in various > MAINTAINERS entries. > > I don't know the definitive answer here, but it seemed to fit reasonably as > R:. Yeah, I told you last time, it's going to be a recurring discussion because it's super unusual :) Maxime signature.asc Description: PGP signature
Re: Question about possible NULL Pointer Dereference in hx83102_get_modes()
Hi, On Wed, Oct 09, 2024 at 02:38:22PM GMT, Zichen Xie wrote: > Dear Developers for DRM PANEL DRIVERS, > > We are developing a static analyzer for Linux Kernel, and we are > curious about the function drm_mode_duplicate() in > hx83102_get_modes(). > https://elixir.bootlin.com/linux/v6.12-rc2/source/drivers/gpu/drm/panel/panel-himax-hx83102.c#L567 > > ``` > > struct drm_display_mode *mode; > > mode = drm_mode_duplicate(connector->dev, m); > > mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > > ``` > > drm_mode_duplicate() will return NULL if it fails to allocate memory, > so NULL check is necessary for this function. > Directly accessing 'mode->type' may lead to NULL Pointer Dereference. Yes and no. drm_mode_create uses kzalloc to create the new mode structure, and any kmalloc (GFP, really) allocations below 8 pages cannot fail. So, from an API standpoint, you're right. From a practical one, it won't change anything. Maxime signature.asc Description: PGP signature
Re: [PATCH 1/2] MAINTAINERS: Add Maíra to VC4 reviewers
On Wed, 9 Oct 2024 11:12:05 -0300, MaÃra Canal wrote: > Although I don't hold expertise on the display side of VC4, I'd like to > help reviewing patches that are related to the 3D side of the VC4 driver. > As V3D maintainer, I hold some expertise with Broadcom GPUs and I'm > constantly testing kernels on RPi 3-5. > > > [ ... ] Acked-by: Maxime Ripard Thanks! Maxime
Re: [PATCH] drm/vc4: Set `fbdev_probe` in `struct vc5_drm_driver`
On Wed, Oct 09, 2024 at 08:59:29AM GMT, Thomas Zimmermann wrote:
> Hi
>
> Am 09.10.24 um 02:40 schrieb Maíra Canal:
> > Currently, when booting the RPi 4B, we get a NULL pointer dereference:
> >
> > [7.642883] Unable to handle kernel NULL pointer dereference at virtual
> > address 0038
> > [7.642926] Mem abort info:
> > [7.642938] ESR = 0x9644
> > [7.642951] EC = 0x25: DABT (current EL), IL = 32 bits
> > [7.642968] SET = 0, FnV = 0
> > [7.642981] EA = 0, S1PTW = 0
> > [7.642993] FSC = 0x04: level 0 translation fault
> > [7.643007] Data abort info:
> > [7.643017] ISV = 0, ISS = 0x0044, ISS2 = 0x
> > [7.643032] CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> > [7.643046] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> > [7.643063] user pgtable: 4k pages, 48-bit VAs, pgdp=000107487000
> > [7.643081] [0038] pgd=, p4d=
> > [7.643113] Internal error: Oops: 9644 [#1] PREEMPT SMP
> > [7.643131] Modules linked in: snd_bcm2835(C) [...] vc4 v3d [...]
> > drm_shmem_helper drm_dma_helper drm_kms_helper drm [...] ipv6
> > [7.643377] CPU: 1 UID: 0 PID: 48 Comm: kworker/u16:2 Tainted: G
> > C 6.12.0-rc1-00310-g2c34a5464007 #185
> > [7.643407] Tainted: [C]=CRAP
> > [7.643419] Hardware name: Raspberry Pi 4 Model B Rev 1.5 (DT)
> > [7.643438] Workqueue: events_unbound deferred_probe_work_func
> > [7.643477] pstate: 8005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS
> > BTYPE=--)
> > [7.643499] pc : __drm_fb_helper_initial_config_and_unlock+0x40c/0x588
> > [drm_kms_helper]
> > [7.643616] lr : __drm_fb_helper_initial_config_and_unlock+0x374/0x588
> > [drm_kms_helper]
> > [7.643710] sp : 8000806c3900
> > [7.643724] x29: 8000806c3950 x28: 431b01a3ec14 x27:
> > 0010
> > [7.643758] x26: 431b0369f000 x25: 36314752 x24:
> > 431b003b6200
> > [7.643790] x23: 431b0369f000 x22: 02d0 x21:
> > 431b003b6030
> > [7.643821] x20: 431b003b6030 x19: 431b003b6000 x18:
> > 431b002e6e48
> > [7.643852] x17: 0001 x16: b19c2f10614c x15:
> >
> > [7.643882] x14: x13: 431b003b62f0 x12:
> > 0500
> > [7.643913] x11: x10: x9 :
> > 005d6e6f6362665b
> > [7.643945] x8 : x7 : x6 :
> > 003f
> > [7.643975] x5 : b19bcb45c59c x4 : 1e00 x3 :
> > b19bcb420c20
> > [7.644005] x2 : x1 : 0001 x0 :
> > 431b003b6000
> > [7.644036] Call trace:
> > [7.644049] __drm_fb_helper_initial_config_and_unlock+0x40c/0x588
> > [drm_kms_helper]
> > [7.644149] drm_fb_helper_initial_config+0x34/0x4c [drm_kms_helper]
> > [7.644240] drm_fbdev_client_hotplug+0x74/0xc8 [drm_kms_helper]
> > [7.644331] drm_client_register+0x58/0xa0 [drm]
> > [7.644571] drm_fbdev_client_setup+0xc4/0x17c [drm_kms_helper]
> > [7.644664] drm_client_setup_with_fourcc+0x28/0x60 [drm_kms_helper]
> > [7.644755] vc4_drm_bind+0x218/0x264 [vc4]
> > [7.644855] try_to_bring_up_aggregate_device+0x168/0x1b4
> > [7.644884] __component_add+0xb8/0x158
> > [7.644905] component_add+0x14/0x20
> > [7.644925] vc4_hvs_dev_probe+0x1c/0x28 [vc4]
> > [7.645019] platform_probe+0xa8/0xd0
> > [7.645045] really_probe+0x128/0x2c8
> > [7.645065] __driver_probe_device+0xa0/0x128
> > [7.645086] driver_probe_device+0x3c/0x1f8
> > [7.645106] __device_attach_driver+0x118/0x140
> > [7.645127] bus_for_each_drv+0xf4/0x14c
> > [7.645145] __device_attach+0xfc/0x194
> > [7.645164] device_initial_probe+0x14/0x20
> > [7.645184] bus_probe_device+0x94/0x100
> > [7.645202] deferred_probe_work_func+0x88/0xc4
> > [7.645223] process_scheduled_works+0x194/0x2c4
> > [7.645246] worker_thread+0x290/0x39c
> > [7.645265] kthread+0xfc/0x184
> > [7.645289] ret_from_fork+0x10/0x20
> > [7.645317] Code: f2ac6c49 aa1303e0 f2cdcde9 f2e00ba9 (f9001d09)
> > [7.645338] ---[ end trace ]---
> >
> > This happens because commit 45903624e9fc ("drm/vc4: Run DRM default client
> > setup") only added DRM_FBDEV_DMA_DRIVER_OPS to `struct vc4_drm_driver`
> > and didn't add it to `struct vc5_drm_driver`. The `struct vc4_drm_driver`
> > is used in RPi 0-3, as VC4 is also a render node in those RPis. But RPi 4
> > and 5 use V3D as the render node and VC4 as modeset node and therefore,
> > use `struct vc5_drm_driver`.
> >
> > This commit adds DRM_FBDEV_DMA_DRIVER_OPS to `struct vc5_drm_driver`,
> > ensuring that `fbdev_probe` exists for all VC4 generations.
> >
> > Fixes: 45903624e9fc ("drm/vc4: Run DRM default client setup")
> > Signed-off-by: Maíra Canal
>
> Reviewed-by: Thomas Zimmermann
It's the third ti
Re: [PATCH v2 16/22] drm/msm/dpu: Configure CWB in writeback encoder
On Tue, Oct 08, 2024 at 10:00:57AM GMT, Neil Armstrong wrote: > Hi, > > On 01/10/2024 09:37, neil.armstr...@linaro.org wrote: > > Hi, > > > > On 30/09/2024 21:19, Jessica Zhang wrote: > > > > > > > > > On 9/30/2024 7:17 AM, neil.armstr...@linaro.org wrote: > > > > On 25/09/2024 00:59, Jessica Zhang wrote: > > > > > > > > > > > When running igt-test on QRD8650, I get: > > > > # IGT_FRAME_DUMP_PATH=$PWD FRAME_PNG_FILE_NAME=pwet /usr/libexec/igt- > > > > gpu-tools/kms_writeback -d > > > > > > Hi Neil, > > > > > > Thanks for reporting this. Unfortunately, I'm not able to recreate this > > > on the MTP8650. > > > > > > How many/which non-WB outputs are you testing with? > > > > Here's the modetest output: > > ==>< > > Encoders: > > id crtc type possible crtcs possible clones > > 32 103 DSI 0x0007 0x0005 > > 34 0 TMDS 0x0007 0x0006 > > 37 0 Virtual 0x0007 0x0007 > > > > Connectors: > > id encoder status name size (mm) modes encoders > > 33 32 connected DSI-1 71x157 1 32 > > modes: > > index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot > > #0 1080x2400 144.00 1080 1100 1102 1122 2400 2420 2422 2440 394225 > > flags: ; type: preferred, driver > > props: > > 1 EDID: > > flags: immutable blob > > blobs: > > > > value: > > 2 DPMS: > > flags: enum > > enums: On=0 Standby=1 Suspend=2 Off=3 > > value: 0 > > 5 link-status: > > flags: enum > > enums: Good=0 Bad=1 > > value: 0 > > 6 non-desktop: > > flags: immutable range > > values: 0 1 > > value: 0 > > 4 TILE: > > flags: immutable blob > > blobs: > > > > value: > > 35 0 disconnected DP-1 0x0 0 34 > > props: > > 1 EDID: > > flags: immutable blob > > blobs: > > > > value: > > 2 DPMS: > > flags: enum > > enums: On=0 Standby=1 Suspend=2 Off=3 > > value: 0 > > 5 link-status: > > flags: enum > > enums: Good=0 Bad=1 > > value: 0 > > 6 non-desktop: > > flags: immutable range > > values: 0 1 > > value: 0 > > 4 TILE: > > flags: immutable blob > > blobs: > > > > value: > > 36 subconnector: > > flags: immutable enum > > enums: Unknown=0 VGA=1 DVI-D=3 HDMI=11 DP=10 Wireless=18 Native=15 > > value: 0 > > ==>< > > > > and dri state: > > ==>< > > # cat /sys/kernel/debug/dri/0/state > > plane[43]: plane-0 > > crtc=crtc-0 > > fb=106 > > allocated by = [fbcon] > > refcount=2 > > format=XR24 little-endian (0x34325258) > > modifier=0x0 > > size=1080x2400 > > layers: > > size[0]=1080x2400 > > pitch[0]=4352 > > offset[0]=0 > > obj[0]: > > name=0 > > refcount=1 > > start=0010102d > > size=10444800 > > imported=no > > crtc-pos=1080x2400+0+0 > > src-pos=1080.00x2400.00+0.00+0.00 > > rotation=1 > > normalized-zpos=0 > > color-encoding=ITU-R BT.601 YCbCr > > color-range=YCbCr limited range > > color_mgmt_changed=0 > > stage=1 > > sspp[0]=sspp_0 > > multirect_mode[0]=none > > multirect_index[0]=solo > > src[0]=1080x2400+0+0 > > dst[0]=1080x2400+0+0 > > plane[49]: plane-1 > > crtc=(null) > > fb=0 > > crtc-pos=0x0+0+0 > > src-pos=0.00x0.00+0.00+0.00 > > rotation=1 > > normalized-zpos=0 > > color-encoding=ITU-R BT.601 YCbCr > > color-range=YCbCr limited range > > color_mgmt_changed=0 > > stage=0 > > sspp[0]=sspp_1 > > multirect_mode[0]=none > > multirect_index[0]=solo > > src[0]=0x0+0+0 > > dst[0]=0x0+0+0 > > plane[55]: plane-2 > > crtc=(null) > > fb=0 > > crtc-pos=0x0+0+0 > > src-pos=0.00x0.00+0.00+0.00 > > rotation=1 > > normalized-zpos=0 > > color-encoding=ITU-R BT.601 YCbCr > > color-range=YCbCr limited range > > color_mgmt_changed=0 > > stage=0 > > sspp[0]=sspp_2 > > multirect_mode[0]=none > > multirect_index[0]=solo > > src[0]=0x0+0+0 > > dst[0]=0x0+0+0 > > plane[61]: plane-3 > > crtc=(null) > > fb=0 > > crtc-pos=0x0+0+0 > > src-pos=0.00x0.00+0.00+0.00 > > rotation=1 > > normalized-zpos=0 > > color-encoding=ITU-R BT.601 YCbCr > > col
Re: [PATCH v12 13/15] drm/vkms: Create KUnit tests for YUV conversions
Hi,
On Mon, Oct 07, 2024 at 06:10:47PM GMT, Louis Chauvet wrote:
> From: Arthur Grillo
>
> Create KUnit tests to test the conversion between YUV and RGB. Test each
> conversion and range combination with some common colors.
>
> The code used to compute the expected result can be found in comment.
>
> [Louis Chauvet:
> - fix minor formating issues (whitespace, double line)
> - change expected alpha from 0x to 0x
> - adapt to the new get_conversion_matrix usage
> - apply the changes from Arthur
> - move struct pixel_yuv_u8 to the test itself]
>
> Signed-off-by: Arthur Grillo
> Acked-by: Pekka Paalanen
> Signed-off-by: Louis Chauvet
> ---
> drivers/gpu/drm/vkms/Kconfig | 15 ++
> drivers/gpu/drm/vkms/Makefile | 1 +
> drivers/gpu/drm/vkms/tests/.kunitconfig | 4 +
> drivers/gpu/drm/vkms/tests/Makefile | 3 +
> drivers/gpu/drm/vkms/tests/vkms_format_test.c | 232
> ++
> drivers/gpu/drm/vkms/vkms_formats.c | 7 +-
> drivers/gpu/drm/vkms/vkms_formats.h | 5 +
> 7 files changed, 265 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index 9def079f685b..98ecfce929f3 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -14,3 +14,18 @@ config DRM_VKMS
> a VKMS.
>
> If M is selected the module will be called vkms.
> +
> +config DRM_VKMS_KUNIT_TESTS
> + tristate "KUnit tests for VKMS." if !KUNIT_ALL_TESTS
> + depends on DRM_VKMS=y && KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + This builds unit tests for VKMS. This option is not useful for
> + distributions or general kernels, but only for kernel
> + developers working on VKMS.
> +
> + For more information on KUnit and unit tests in general,
> + please refer to the KUnit documentation in
> + Documentation/dev-tools/kunit/.
> +
> + If in doubt, say "N".
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 1b28a6a32948..8d3e46dde635 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -9,3 +9,4 @@ vkms-y := \
> vkms_writeback.o
>
> obj-$(CONFIG_DRM_VKMS) += vkms.o
> +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
> diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig
> b/drivers/gpu/drm/vkms/tests/.kunitconfig
> new file mode 100644
> index ..70e378228cbd
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
> @@ -0,0 +1,4 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_VKMS=y
> +CONFIG_DRM_VKMS_KUNIT_TESTS=y
> diff --git a/drivers/gpu/drm/vkms/tests/Makefile
> b/drivers/gpu/drm/vkms/tests/Makefile
> new file mode 100644
> index ..2d1df668569e
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_format_test.o
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> new file mode 100644
> index ..351409897ca3
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include
> +
> +#include
> +#include
> +
> +#include "../../drm_crtc_internal.h"
> +
> +#include "../vkms_formats.h"
> +
> +#define TEST_BUFF_SIZE 50
> +
> +MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING);
> +
> +struct pixel_yuv_u8 {
> + u8 y, u, v;
> +};
> +
> +struct yuv_u8_to_argb_u16_case {
> + enum drm_color_encoding encoding;
> + enum drm_color_range range;
> + size_t n_colors;
> + struct format_pair {
> + char *name;
> + struct pixel_yuv_u8 yuv;
> + struct pixel_argb_u16 argb;
> + } colors[TEST_BUFF_SIZE];
> +};
> +
> +/*
> + * The YUV color representation were acquired via the colour python
> framework.
> + * Below are the function calls used for generating each case.
> + *
> + * For more information got to the docs:
> + * https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html
> + */
> +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> + /*
> + * colour.RGB_to_YCbCr(,
> + * K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> + * in_bits = 16,
> + * in_legal = False,
> + * in_int = True,
> + * out_bits = 8,
> + * out_legal = False,
> + * out_int = True)
> + */
We should really detail what the intent and expected outcome is supposed
to be here. Relying on a third-party python library call for
documentation isn't great.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 0/2] drm: revert some framebuffer API tests
On Wed, Oct 02, 2024 at 03:10:14PM GMT, Jani Nikula wrote: > On Wed, 25 Sep 2024, Maxime Ripard wrote: > > On Wed, Sep 25, 2024 at 01:52:02PM GMT, Simona Vetter wrote: > >> I think for at least drm the consensus is clear, we won't have kunit tests > >> that splat. > > > > Where was that ever discussed? > > Well, where was it ever agreed that it's okay for drm kunit tests to > emit warnings? :p One policy is upstream, the other isn't. And it does seem like the consensus towards downstream patches and policies is pretty well established. And I wasn't the one claiming that there's a consensus to begin with, so I don't see why I should prove anything? > >> Personally I really don't see the point of unit tests that are > >> somewhere between unecessarily hard or outright too much pain to > >> deploy in a test rig: Either you don't run them (not great), or you > >> filter splats and might filter too much (not great either) or you > >> filter as little as possible and fight false positives (also kinda > >> suboptimal). > > > > Or you don't do any of that, and just rely on the canonical way to run > > kunit test and trust it's going to pass tests that do indeed pass, and > > fail / warn on those that don't. > > That still doesn't address code being tested emitting *unexpected* > warnings. Then make kunit report a warning / failure when there's an unexpected warning splat. At the end of the day, the discussion is that we already require for for each committer to: - Apply patches - Check for checkpatch issues - Solve potential conflicts - Make sure it compiles on three different architectures, with huge defconfigs - Make sure the unit tests still pass I'm not going to add "and grep through the output of those 1000-ish tests for a warning" to that list. Maxime signature.asc Description: PGP signature
Re: [RFC PATCH 1/1] drm/meson: Support drm_panic
+ Jocelyn
On Wed, Oct 02, 2024 at 09:59:57AM GMT, Neil Armstrong wrote:
> > diff --git a/drivers/gpu/drm/meson/meson_plane.c
> > b/drivers/gpu/drm/meson/meson_plane.c
> > index b43ac61201f3..b2def784c00d 100644
> > --- a/drivers/gpu/drm/meson/meson_plane.c
> > +++ b/drivers/gpu/drm/meson/meson_plane.c
> > @@ -20,6 +20,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > #include "meson_plane.h"
> > #include "meson_registers.h"
> > @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct
> > drm_plane *plane,
> > priv->viu.osd1_enabled = false;
> > }
> > +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
> > + struct drm_scanout_buffer *sb)
> > +{
> > + struct meson_plane *meson_plane = to_meson_plane(plane);
> > + struct meson_drm *priv = meson_plane->priv;
> > + struct drm_framebuffer *fb;
> > +
> > + if (!meson_plane->enabled)
> > + return -ENODEV;
> > +
> > + if (priv->viu.osd1_afbcd) {
> > + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
>
> This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)
>
> You should call:
>
> if (priv->afbcd.ops) {
> priv->afbcd.ops->reset(priv);
> priv->afbcd.ops->disable(priv);
> }
I'm not sure it's a good idea. This code is run in the panic path, and
it comes with strict requirements that these functions don't have.
It'll be incredibly easy to add a sleeping call or a lock in there.
On a more fundamental level, I'm not sure we should be disableing AFBC
at all. Do we even have the guarantee that the buffer is large enough to
hold XRGB pixels?
Maxime
signature.asc
Description: PGP signature
[PATCH] Documentation: dma-buf: heaps: Add heap name definitions
Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Signed-off-by: Maxime Ripard --- To: Jonathan Corbet To: Sumit Semwal Cc: Benjamin Gaignard Cc: Brian Starkey Cc: John Stultz Cc: "T.J. Mercier" Cc: "Christian König" Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-...@lists.linaro.org Cc: linux-me...@vger.kernel.org Cc: linux-...@vger.kernel.org --- Documentation/userspace-api/dma-buf-heaps.rst | 71 +++ Documentation/userspace-api/index.rst | 1 + 2 files changed, 72 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index ..00436227b542 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,71 @@ +.. SPDX-License-Identifier: GPL-2.0 + +== +Allocating dma-buf using heaps +== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps += + +A heap represent a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers + + - The ``reserved`` heap allocates physically contiguous, cacheable, buffers. + Depending on the platform, it might be called differently: + +- Acer Iconia Tab A500: ``linux,cma`` +- Allwinner sun4i, sun5i and sun7i families: ``default-pool`` +- Amlogic A1: ``linux,cma`` +- Amlogic G12A/G12B/SM1: ``linux,cma`` +- Amlogic GXBB/GXL: ``linux,cma`` +- ASUS EeePad Transformer TF101: ``linux,cma`` +- ASUS Google Nexus 7 (Project Bach / ME370TG) E1565: ``linux,cma`` +- ASUS Google Nexus 7 (Project Nakasi / ME370T) E1565: ``linux,cma`` +- ASUS Google Nexus 7 (Project Nakasi / ME370T) PM269: ``linux,cma`` +- Asus Transformer Infinity TF700T: ``linux,cma`` +- Asus Transformer Pad 3G TF300TG: ``linux,cma`` +- Asus Transformer Pad TF300T: ``linux,cma`` +- Asus Transformer Pad TF701T: ``linux,cma`` +- Asus Transformer Prime TF201: ``linux,cma`` +- ASUS Vivobook S 15: ``linux,cma`` +- Cadence KC705: ``linux,cma`` +- Digi International ConnectCore 6UL: ``linux,cma`` +- Freescale i.MX8DXL EVK: ``linux,cma`` +- Freescale TQMa8Xx: ``linux,cma`` +- Hisilicon Hikey: ``linux,cma`` +- Lenovo ThinkPad T14s Gen 6: ``linux,cma`` +- Lenovo ThinkPad X13s: ``linux,cma`` +- Lenovo Yoga Slim 7x: ``linux,cma`` +- LG Optimus 4X HD P880: ``linux,cma`` +- LG Optimus Vu P895: ``linux,cma`` +- Loongson 2k0500, 2k1000 and 2k2000: ``linux,cma`` +- Microsoft Romulus: ``linux,cma`` +- NXP i.MX8ULP EVK: ``linux,cma`` +- NXP i.MX93 9x9 QSB: ``linux,cma`` +- NXP i.MX93 11X11 EVK: ``linux,cma`` +- NXP i.MX93 14X14 EVK: ``linux,cma`` +- NXP i.MX95 19X19 EVK: ``linux,cma`` +- Ouya Game Console: ``linux,cma`` +- Pegatron Chagall: ``linux,cma`` +- PHYTEC phyCORE-AM62A SOM: ``linux,cma`` +- PHYTEC phyCORE-i.MX93 SOM: ``linux,cma`` +- Qualcomm SC8280XP CRD: ``linux,cma`` +- Qualcomm X1E80100 CRD: ``linux,cma`` +- Qualcomm X1E80100 QCP: ``linux,cma`` +- RaspberryPi: ``linux,cma`` +- Texas Instruments AM62x SK board family: ``linux,cma`` +- Texas Instruments AM62A7 SK: ``linux,cma`` +- Toradex Apalis iMX8: ``linux,cma`` +- TQ-Systems i.MX8MM TQMa8MxML: ``linux,cma`` +- TQ-Systems i.MX8MN TQMa8MxNL: ``linux,cma`` +- TQ-Systems i.MX8MPlus TQMa8MPxL: ``linux,cma`` +- TQ-Systems i.MX8MQ TQMa8MQ: ``linux,cma`` +- TQ-Systems i.MX93 TQMa93xxLA/TQMa93xxCA SOM: ``linux,cma`` +- TQ-Systems MBA6ULx Baseboard: ``linux,cma`` + diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index 274cc7546efc..4901ce7c6cb7 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -41,10 +41,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas -- 2.46.1
Re: [PATCH v17 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver
On Sun, Sep 29, 2024 at 02:34:36AM GMT, Sandor Yu wrote:
> > > +static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device *mhdp)
> > > +{
> > > + struct drm_display_info *display = &mhdp->curr_conn->display_info;
> > > + struct drm_connector_state *conn_state = mhdp->curr_conn->state;
> >
> > That looks a bit hackish to me. We should probably provide a helper to get
> > the
> > connector state the bridge is attached to.
>
> How about code change to followed, is it more clear?
> 370 struct drm_connector *connector = mhdp->curr_conn;
> 371 struct drm_connector_state *conn_state = connector->state;
> 372 struct drm_display_info *display = &connector->display_info;
> 373 struct drm_scdc *scdc = &display->hdmi.scdc;
What I meant was that I wish bridges had a way to get their connector
pointer. It doesn't look like it's possible with drm_bridge_connector,
and we don't have access to drm_display_info anymore.
I don't really see a good way to do this yet, so maybe that kind of
workaround is ok. Eventually, I guess we'll have the scrambler setup in
the HDMI connector helpers anyway.
Dmitry, any idea?
> > > +static enum drm_mode_status
> > > +cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> > > +const struct drm_display_mode *mode,
> > > +unsigned long long tmds_rate) {
> > > + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > > + union phy_configure_opts phy_cfg;
> > > + int ret;
> > > +
> > > + phy_cfg.hdmi.tmds_char_rate = tmds_rate;
> > > +
> > > + ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
> > > + if (ret < 0)
> > > + return MODE_CLOCK_RANGE;
> > > +
> > > + return MODE_OK;
> > > +}
> > > +
> > > +static enum drm_mode_status
> > > +cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
> > > + const struct drm_display_info *info,
> > > + const struct drm_display_mode *mode) {
> > > + unsigned long long tmds_rate;
> > > +
> > > + /* We don't support double-clocked and Interlaced modes */
> > > + if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
> > > + mode->flags & DRM_MODE_FLAG_INTERLACE)
> > > + return MODE_BAD;
> > > +
> > > + if (mode->hdisplay > 3840)
> > > + return MODE_BAD_HVALUE;
> > > +
> > > + if (mode->vdisplay > 2160)
> > > + return MODE_BAD_VVALUE;
> > > +
> > > + tmds_rate = mode->clock * 1000ULL;
> > > + return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate); }
> >
> > Didn't we agree on creating a mode_valid helper?
>
> In fact, now I'm no idea where should add the mode_valid helper function.
>
> In struct drm_bridge_funcs, it had mode_valid() and
> hdmi_tmds_char_rate_valid().
>
> If create a new mode_valid helper function in struct drm_connector_hdmi_funcs,
> Is it appropriate to call another API function(tmds_char_rate_valid)
> at the same level within this API function?
I'm not quite sure what you mean, but a reasonable approach to me would
be to turn drm_hdmi_state_helper.c hdmi_clock_valid into a public
function, and then call it from drm_bridge_connector mode_valid hook.
It's a similar discussion to the previous one really: in order to
implement it properly, we need access to drm_display_info.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v8 1/3] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library
On Sun, Sep 29, 2024 at 01:36:47AM GMT, Cristian Ciocaltea wrote:
> +static enum drm_mode_status
> +dw_hdmi_qp_bridge_mode_valid(struct drm_bridge *bridge,
> + const struct drm_display_info *info,
> + const struct drm_display_mode *mode)
> +{
> + struct dw_hdmi_qp *hdmi = bridge->driver_private;
> +
> + if (mode->clock > HDMI14_MAX_TMDSCLK / 1000) {
> + dev_dbg(hdmi->dev, "Unsupported mode clock: %d\n", mode->clock);
> + return MODE_CLOCK_HIGH;
> + }
Similarly, you should use drm_hdmi_compute_mode_clock here, with RGB and 8bpc
Once fixed,
Reviewed-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v8 3/3] drm/rockchip: Add basic RK3588 HDMI output support
Hi,
On Sun, Sep 29, 2024 at 01:36:49AM GMT, Cristian Ciocaltea wrote:
> +static void dw_hdmi_qp_rockchip_encoder_enable(struct drm_encoder *encoder)
> +{
> + struct rockchip_hdmi_qp *hdmi = to_rockchip_hdmi_qp(encoder);
> + struct drm_crtc *crtc = encoder->crtc;
> + int rate;
> +
> + /* Unconditionally switch to TMDS as FRL is not yet supported */
> + gpiod_set_value(hdmi->enable_gpio, 1);
> +
> + if (crtc && crtc->state) {
> + clk_set_rate(hdmi->ref_clk,
> + crtc->state->adjusted_mode.crtc_clock * 1000);
Sorry, I should have seen it in your previous version, but the rate here
should be the TMDS character rate, not the pixel clock, right?
Once fixed,
Reviewed-by: Maxime Ripard
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 4/8] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate
On Mon, Sep 30, 2024 at 01:28:59PM GMT, Liu Ying wrote:
> Multiple display modes could be read from a display device's EDID.
> Use clk_round_rate() to validate the "ldb" clock rate for each mode
> in drm_bridge_funcs::mode_valid() to filter unsupported modes out.
>
> Also, if the "ldb" clock and the pixel clock are sibling in clock
> tree, use clk_round_rate() to validate the pixel clock rate against
> the "ldb" clock. This is not done in display controller driver
> because drm_crtc_helper_funcs::mode_valid() may not decide to do
> the validation or not if multiple encoders are connected to the CRTC,
> e.g., i.MX93 LCDIF may connect with MIPI DSI controller, LDB and
> parallel display output simultaneously.
>
> Signed-off-by: Liu Ying
> ---
> drivers/gpu/drm/bridge/fsl-ldb.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
> b/drivers/gpu/drm/bridge/fsl-ldb.c
> index b559f3e0bef6..ee8471c86617 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -64,6 +65,7 @@ struct fsl_ldb_devdata {
> u32 lvds_ctrl;
> bool lvds_en_bit;
> bool single_ctrl_reg;
> + bool ldb_clk_pixel_clk_sibling;
> };
>
> static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> @@ -74,11 +76,13 @@ static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> [IMX8MP_LDB] = {
> .ldb_ctrl = 0x5c,
> .lvds_ctrl = 0x128,
> + .ldb_clk_pixel_clk_sibling = true,
> },
> [IMX93_LDB] = {
> .ldb_ctrl = 0x20,
> .lvds_ctrl = 0x24,
> .lvds_en_bit = true,
> + .ldb_clk_pixel_clk_sibling = true,
> },
> };
>
> @@ -269,11 +273,29 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
> const struct drm_display_info *info,
> const struct drm_display_mode *mode)
> {
> + unsigned long link_freq, pclk_rate, rounded_pclk_rate;
> struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>
> if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 16 : 8))
> return MODE_CLOCK_HIGH;
>
> + /* Validate "ldb" clock rate. */
> + link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> + if (link_freq != clk_round_rate(fsl_ldb->clk, link_freq))
> + return MODE_NOCLOCK;
> +
> + /*
> + * Use "ldb" clock to validate pixel clock rate,
> + * if the two clocks are sibling.
> + */
> + if (fsl_ldb->devdata->ldb_clk_pixel_clk_sibling) {
> + pclk_rate = mode->clock * HZ_PER_KHZ;
> +
> + rounded_pclk_rate = clk_round_rate(fsl_ldb->clk, pclk_rate);
> + if (rounded_pclk_rate != pclk_rate)
> + return MODE_NOCLOCK;
> + }
> +
I guess this is to workaround the fact that the parent rate would be
changed, and thus the sibling rate as well? This should be documented in
a comment if so.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 6/8] drm/bridge: Add ITE IT6263 LVDS to HDMI converter
Hi, On Mon, Sep 30, 2024 at 01:29:01PM GMT, Liu Ying wrote: > Add basic HDMI video output support. Currently, only RGB888 output > pixel format is supported. At the LVDS input side, the driver > supports single LVDS link and dual LVDS links with "jeida-24" LVDS > mapping. > > Product link: > https://www.ite.com.tw/en/product/cate1/IT6263 > > Signed-off-by: Liu Ying Generally speaking, you need to use the new HDMI bridge infrastructure. There's a lot of required things you're not dealing with here (such as infoframes) Also, you should add a MAINTAINERS entry Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/atomic_helper: Add missing NULL check for drm_plane_helper_funcs.atomic_update
Hi,
On Fri, Sep 27, 2024 at 04:46:16PM GMT, Lyude Paul wrote:
> Something I discovered while writing rvkms since some versions of the
> driver didn't have a filled out atomic_update function - we mention that
> this callback is "optional", but we don't actually check whether it's NULL
> or not before calling it. As a result, we'll segfault if it's not filled
> in.
>
> rvkms rvkms.0: [drm:drm_atomic_helper_commit_modeset_disables] modeset on
> [ENCODER:36:Virtual-36]
> BUG: kernel NULL pointer dereference, address:
> PGD 0 P4D 0
> Oops: Oops: 0010 [#1] PREEMPT SMP NOPTI
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> edk2-20240813-1.fc40 08/13/2024
> RIP: 0010:0x0
>
> So, let's fix that.
>
> Signed-off-by: Lyude Paul
> Fixes: c2fcd274bce5 ("drm: Add atomic/plane helpers")
> Cc: dri-devel@lists.freedesktop.org
> Cc: # v3.19+
So we had kind of a similar argument with drm_connector_init early this
year, but I do agree we shouldn't fault if we're missing a callback.
I do wonder how we can implement a plane without atomic_update though?
Do we have drivers in such a case?
If not, a better solution would be to make it mandatory and check it
when registering.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3] docs/gpu: ci: update flake tests requirements
On Fri, 27 Sep 2024 10:54:14 +0530, Vignesh Raman wrote: > Update the documentation to specify linking to a relevant GitLab > issue or email report for each new flake entry. Added specific > GitLab issue urls for i915, msm and amdgpu driver. > > Acked-by: Abhinav Kumar # msm > > [ ... ] Acked-by: Maxime Ripard Thanks! Maxime
Re: [PATCH v2 2/2] drm/panel: simple: Add Microchip AC69T88A LVDS Display panel
On Thu, Sep 26, 2024 at 04:32:59PM GMT, Dmitry Baryshkov wrote: > On Thu, Sep 26, 2024 at 08:17:09AM GMT, manikanda...@microchip.com wrote: > > On 23/09/24 11:37 am, Dmitry Baryshkov wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know > > > the content is safe > > > > > > On Mon, Sep 23, 2024 at 05:50:22AM GMT, manikanda...@microchip.com wrote: > > >> On 20/09/24 9:13 pm, Dmitry Baryshkov wrote: > > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know > > >>> the content is safe > > >>> > > >>> On Thu, Sep 19, 2024 at 02:45:48PM GMT, Manikandan Muralidharan wrote: > > Add support for Microchip AC69T88A 5 inch TFT LCD 800x480 > > Display module with LVDS interface.The panel uses the Sitronix > > ST7262 800x480 Display driver > > >>> > > >>> AC69T88A seems to be a module name, rather than a panel name. What is > > >>> the actual panel name present on this module? > > >> Both names, "Microchip AC69T88A" and "MPU32-LVDS-DISPLAY-WVGA" are > > >> present on the display module > > > > > > Which panel was used for the module? I don't think that Microchip > > > produces LVDS panels. > > Its a new LVDS display from Microchip that uses Sitronix ST7262 TFT LCD > > driver > > > > https://www.crystalfontz.com/controllers/datasheet-viewer.php?id=486 > > Ok. Anyway if somebody ends up looking for the panel, they'll probably > find the module and vice versa. > > Reviewed-by: Dmitry Baryshkov Given that aside from that mail, the module name isn't mentionned anywhere, I'm not sure they would. The way we usually deal with controllers is to have a separate driver for panels based on that controller, even more so since that controller seems to be able to affect the display. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm: Print bad EDID notices only once
Hi, On Thu, Sep 26, 2024 at 06:32:53AM GMT, Andi Kleen wrote: > I have an old monitor that reports a zero EDID block, which results in a > warning message. This happens on every screen save cycle, and maybe in > some other situations, and over time the whole kernel log gets filled > with these redundant messages. Printing it only once should be > sufficient. > > Mark all the bad EDID notices as _once. Is it? I mean, it probably is if you connect and reconnect the same display, but if it's two different then the second definitely has value. Maxime signature.asc Description: PGP signature
Re: Time for drm-ci-next?
On Tue, Sep 24, 2024 at 05:57:07PM GMT, Vignesh Raman wrote: > Hi, > > On 12/09/24 11:18, Dmitry Baryshkov wrote: > > On Mon, Sep 09, 2024 at 07:34:04AM GMT, Rob Clark wrote: > > > On Mon, Sep 9, 2024 at 2:54 AM Dmitry Baryshkov > > > wrote: > > > > > > > > On Mon, 9 Sept 2024 at 10:50, Maxime Ripard wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Tue, Jul 09, 2024 at 01:27:51AM GMT, Dmitry Baryshkov wrote: > > > > > > On Mon, 8 Jul 2024 at 21:38, Rob Clark wrote: > > > > > > > > > > > > > > On Mon, Jul 8, 2024 at 1:52 AM Daniel Vetter > > > > > > > wrote: > > > > > > > > > > > > > > > > On Fri, Jul 05, 2024 at 12:31:36PM -0700, Rob Clark wrote: > > > > > > > > > On Fri, Jul 5, 2024 at 3:36 AM Daniel Vetter > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Thu, Jul 04, 2024 at 08:40:26AM -0700, Rob Clark wrote: > > > > > > > > > > > On Thu, Jul 4, 2024 at 7:08 AM Daniel Vetter > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 02, 2024 at 05:32:39AM -0700, Rob Clark > > > > > > > > > > > > wrote: > > > > > > > > > > > > > On Fri, Jun 28, 2024 at 10:44 AM Daniel Vetter > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jun 27, 2024 at 11:51:37AM -0700, Rob Clark > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > On Wed, Jun 26, 2024 at 10:47 AM Daniel Vetter > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Jun 26, 2024 at 11:38:30AM +0300, > > > > > > > > > > > > > > > > Dmitry Baryshkov wrote: > > > > > > > > > > > > > > > > > On Wed, Jun 26, 2024 at 09:32:44AM GMT, > > > > > > > > > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > On Mon, Jun 24, 2024 at 10:25:25AM -0300, > > > > > > > > > > > > > > > > > > Helen Koike wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 24/06/2024 02:34, Vignesh Raman wrote: > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 15/03/24 22:50, Rob Clark wrote: > > > > > > > > > > > > > > > > > > > > > Basically, I often find myself > > > > > > > > > > > > > > > > > > > > > needing to merge CI patches on top of > > > > > > > > > > > > > > > > > > > > > msm-next in order to run CI, and then > > > > > > > > > > > > > > > > > > > > > after a clean CI run, reset HEAD > > > > > > > > > > > > > > > > > > > > > back before the merge and force-push. > > > > > > > > > > > > > > > > > > > > > Which isn't really how things > > > > > > > > > > > > > > > > > > > > > should work. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This sounds more like you want an > > > > > > > > > > > > > > &
Re: [PATCH v2] docs/gpu: ci: update flake tests requirements
On Thu, Sep 26, 2024 at 12:36:49PM GMT, Vignesh Raman wrote: > Update the documentation to require linking to a relevant GitLab > issue for each new flake entry instead of an email report. Added > specific GitLab issue URLs for i915, xe and other drivers. > > Signed-off-by: Vignesh Raman > --- > > v2: > - Add gitlab issue link for msm driver. > > --- > Documentation/gpu/automated_testing.rst | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/Documentation/gpu/automated_testing.rst > b/Documentation/gpu/automated_testing.rst > index 2d5a28866afe..f918fe56f2b0 100644 > --- a/Documentation/gpu/automated_testing.rst > +++ b/Documentation/gpu/automated_testing.rst > @@ -67,20 +67,26 @@ Lists the tests that for a given driver on a specific > hardware revision are > known to behave unreliably. These tests won't cause a job to fail regardless > of > the result. They will still be run. > > -Each new flake entry must be associated with a link to the email reporting > the > -bug to the author of the affected driver, the board name or Device Tree name > of > -the board, the first kernel version affected, the IGT version used for tests, > -and an approximation of the failure rate. > +Each new flake entry must include a link to the relevant GitLab issue, the > board > +name or Device Tree name, the first kernel version affected, the IGT version > used > +for tests and an approximation of the failure rate. > > They should be provided under the following format:: > > - # Bug Report: $LORE_OR_PATCHWORK_URL > + # Bug Report: $GITLAB_ISSUE ># Board Name: broken-board.dtb ># Linux Version: 6.6-rc1 ># IGT Version: 1.28-gd2af13d9f ># Failure Rate: 100 >flaky-test > > +The GitLab issue must include the logs and the pipeline link. Use the > appropriate > +link below to create an issue. > +https://gitlab.freedesktop.org/drm/i915/kernel/-/issues for i915 driver > +https://gitlab.freedesktop.org/drm/xe/kernel/-/issues for xe driver > +https://gitlab.freedesktop.org/drm/msm/-/issues for msm driver > +https://gitlab.freedesktop.org/drm/misc/kernel/-/issues for other drivers > + I can't comment for the others, but drm-misc at least still requires reporting issues by mail, so, no, sorry, we can't switch to gitlab only for now. Maxime signature.asc Description: PGP signature
Re: [PATCH 0/2] drm: revert some framebuffer API tests
On Wed, Sep 25, 2024 at 09:04:39AM GMT, Guenter Roeck wrote: > On 9/25/24 06:05, Maxime Ripard wrote: > [ ... ] > > > > We've had similar discussions in the past around unit tests that wasted > > > too much cpu time with randomized combinatorial testing, and we've thrown > > > those out too from drm. Test hygiene matters. > > > > We had that discussion because those tests could run for up to around a > > minute on certain platforms. It's not the same issue at all? > > > > I added "kunit.stats_enabled=2 kunit.filter=speed>slow" to the kernel command > line in my tests to avoid long-running tests. That works as long as slow tests > are marked accordingly using KUNIT_CASE_SLOW(), KUNIT_SPEED_SLOW, or > KUNIT_SPEED_VERY_SLOW. Yeah, and I've added a warning some time if a test runs for too long but isn't flagged as slow. Maxime signature.asc Description: PGP signature
Re: [RFC PATCH] drm: allow encoder mode_set even when connectors change for crtc
Hi, On Wed, Sep 11, 2024 at 05:54:44PM GMT, Abhinav Kumar wrote: > On 9/10/2024 1:40 AM, Maxime Ripard wrote: > > Hi, > > > > On Mon, Sep 09, 2024 at 12:59:47PM GMT, Abhinav Kumar wrote: > > > On 9/9/2024 6:37 AM, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Thu, Sep 05, 2024 at 03:11:24PM GMT, Abhinav Kumar wrote: > > > > > In certain use-cases, a CRTC could switch between two encoders > > > > > and because the mode being programmed on the CRTC remains > > > > > the same during this switch, the CRTC's mode_changed remains false. > > > > > In such cases, the encoder's mode_set also gets skipped. > > > > > > > > > > Skipping mode_set on the encoder for such cases could cause an issue > > > > > because even though the same CRTC mode was being used, the encoder > > > > > type could have changed like the CRTC could have switched from a > > > > > real time encoder to a writeback encoder OR vice-versa. > > > > > > > > > > Allow encoder's mode_set to happen even when connectors changed on a > > > > > CRTC and not just when the mode changed. > > > > > > > > > > Signed-off-by: Abhinav Kumar > > > > > > > > The patch and rationale looks sane to me, but we should really add kunit > > > > tests for that scenarii. > > > > > > > > > > Thanks for the review. > > > > > > We have a IGT for recreating this scenario and thats how this issue was > > > captured > > > > > > kms_writeback --run-subtest writeback-check-output -c > > mode> > > > > > > We had added an option ( 'c' - custom mode) a couple of yrs ago to allow > > > writeback to be tested using any mode the user passes in > > > (https://lore.kernel.org/r/all/yujhgkkxah9u6...@platvala-desk.ger.corp.intel.com/T/) > > > > > > If we pass in the same resolution as the primary RT display, this scenario > > > always happens as the CRTC switches between RT encoder and WB encoder. > > > Hope > > > that addresses some of the concern. > > > > Unless it can easily be run in some sort of CI loop by anyone > > contributing to that part of the kernel, it doesn't. > > > > Don't get me wrong, it's a great feature, but it doesn't help making > > sure that issue never creeps back in. > > > > Ack, I understand. > > > > Regarding KUnit tests, I have a couple of questions: > > > > > > 1) This is more of a run-time scenario where CRTC switch happens, does > > > this > > > qualify for a KUnit or perhaps I am missing something. > > > > We've been using kunit to perform integration tests in the kernel too, > > so I would say that it definitely qualifies. > > > > > 2) Is there any existing KUnit test file under drm/tests for validating > > > drm_atomic_helper_commit_modeset_disables() / > > > drm_atomic_helper_commit_modeset_enables() path because this will fall > > > under > > > that bucket. I didnt find any matching case where we can extend this. > > > > We don't have that at the moment, but we shouldn't be too far off. The > > HDMI framework I contributed some months ago for example has all the > > mode checking infrastructure in kunit. So you already have some way to > > create a driver, a new state, modify that state and check it. > > > > The only thing missing in your case is being able to commit it and check > > that it has run, which shouldn't be too hard > > Alright. Yes I reviewed the hdmi infrastructure tests and you seem to have > most of the pieces. I just need to find some cycles to work on this :) so > you can have my name down for it and either me one of our team members or > perhaps with some help from other msm developers we can get it added. > > The reason I was hoping to get this reviewed and added as a "fix" was we had > already run into this scenario with kms_writeback test case and the same > scenario was seen in another msm bug > https://gitlab.freedesktop.org/drm/msm/-/issues/59 leading to a null ptr > crash but we ended up fixing that within msm because that was a better fix > anyway so I was thinking this change would help to resolve these types of > issues for us once for all. > > But if this needs to wait for the KUnit to be added, thats fine, we will > resend this one along with the KUnit once we work on it. Yeah, if it's not urgent I'd rather have the kunit test at the same time. Maxime signature.asc Description: PGP signature
Re: [PATCH 0/2] drm: revert some framebuffer API tests
On Wed, Sep 25, 2024 at 01:52:02PM GMT, Simona Vetter wrote: > On Tue, Sep 24, 2024 at 08:09:38AM -0700, Guenter Roeck wrote: > > On 9/24/24 06:56, Maxime Ripard wrote: > > > On Tue, Sep 24, 2024 at 06:37:59AM GMT, Guenter Roeck wrote: > > > > On 9/24/24 04:54, Maxime Ripard wrote: > > > > > +Guenter > > > > > > > > > > On Tue, Sep 24, 2024 at 12:06:28PM GMT, Simona Vetter wrote: > > > > > > On Tue, Sep 17, 2024 at 08:43:50PM +0300, Jani Nikula wrote: > > > > > > > The tests consistently trigger WARNs in drm_framebuffer code. I'm > > > > > > > not > > > > > > > sure what the point is with type of belts and suspenders tests. > > > > > > > The > > > > > > > warnings *are* the way to flag erroneous API usage. > > > > > > > > > > > > > > Warnings in turn trigger failures in CI. Filtering the warnings > > > > > > > are > > > > > > > error prone, and, crucially, would also filter actual errors in > > > > > > > case the > > > > > > > kunit tests are not run. > > > > > > > > > > > > > > I acknowledge there may be complex test cases where you'd end up > > > > > > > triggering warnings somewhere deep, but these are not it. These > > > > > > > are > > > > > > > simple. > > > > > > > > > > > > > > Revert the tests, back to the drawing board. > > > > > > > > > > > > Yeah I think long-term we might want a kunit framework so that we > > > > > > can > > > > > > catch dmesg warnings we expect and test for those, without those > > > > > > warnings > > > > > > actually going to dmesg. Similar to how the lockdep tests also > > > > > > reroute > > > > > > locking validation, so that the expected positive tests don't wreak > > > > > > lockdep for real. > > > > > > > > > > > > But until that exists, we can't have tests that splat in dmesg when > > > > > > they > > > > > > work as intended. > > > > > > > > > FWIW, that is arguable. More and more tests are added which do add such > > splats, > > and I don't see any hesitance by developers to adding more. So far I counted > > two alone in this commit window, and that does not include new splats from > > tests which I had already disabled. I simply disable those tests or don't > > enable them in the first place if they are new. I did the same with the drm > > unit tests due to the splats generated by the scaling unit tests, so any > > additional drm unit test splats don't make a difference for me since the > > tests are already disabled. > > I think for at least drm the consensus is clear, we won't have kunit tests > that splat. Where was that ever discussed? > Personally I really don't see the point of unit tests that are > somewhere between unecessarily hard or outright too much pain to > deploy in a test rig: Either you don't run them (not great), or you > filter splats and might filter too much (not great either) or you > filter as little as possible and fight false positives (also kinda > suboptimal). Or you don't do any of that, and just rely on the canonical way to run kunit test and trust it's going to pass tests that do indeed pass, and fail / warn on those that don't. > Especially for unit tests this stuff must be totally rock solid. Is there any evidence that those tests were not rock solid? > We've had similar discussions in the past around unit tests that wasted > too much cpu time with randomized combinatorial testing, and we've thrown > those out too from drm. Test hygiene matters. We had that discussion because those tests could run for up to around a minute on certain platforms. It's not the same issue at all? Maxime signature.asc Description: PGP signature
Re: [PATCH 0/2] drm: revert some framebuffer API tests
On Wed, Sep 25, 2024 at 12:41:40PM GMT, Jani Nikula wrote: > On Tue, 24 Sep 2024, Maxime Ripard wrote: > > On Tue, Sep 24, 2024 at 06:56:26PM GMT, Jani Nikula wrote: > >> On Tue, 24 Sep 2024, Guenter Roeck wrote: > >> >>>> On Tue, Sep 24, 2024 at 12:06:28PM GMT, Simona Vetter wrote: > >> >>>>> Yeah I think long-term we might want a kunit framework so that we can > >> >>>>> catch dmesg warnings we expect and test for those, without those > >> >>>>> warnings > >> >>>>> actually going to dmesg. Similar to how the lockdep tests also > >> >>>>> reroute > >> >>>>> locking validation, so that the expected positive tests don't wreak > >> >>>>> lockdep for real. > >> >>>>> > >> >>>>> But until that exists, we can't have tests that splat in dmesg when > >> >>>>> they > >> >>>>> work as intended. > >> > > >> > FWIW, that is arguable. More and more tests are added which do add such > >> > splats, > >> > and I don't see any hesitance by developers to adding more. So far I > >> > counted > >> > two alone in this commit window, and that does not include new splats > >> > from > >> > tests which I had already disabled. I simply disable those tests or don't > >> > enable them in the first place if they are new. I did the same with the > >> > drm > >> > unit tests due to the splats generated by the scaling unit tests, so any > >> > additional drm unit test splats don't make a difference for me since the > >> > tests are already disabled. > >> > >> What's the point of having unit tests that CI systems routinely have to > >> filter out of test runs? Or filter warnings generated by the tests, > >> potentially missing new warnings. Who is going to run the tests if the > >> existing CI systems choose to ignore them? > > > > If we turn this argument around, that means we can't write unit test for > > code that will create a warning. > > The question really is, which warnings we get because of the > functionality being tested, and which warnings are due to an unexpected > and unrelated bug? Is it a pass or fail if the test triggers an > unrelated warning? > > If the developer runs the tests, are they expected to look at dmesg and > inspect every warning to decide? No, there's no such expectation. Yet Intel's CI chose to do so, and chose to treat any warning as a failure, despite the fact that kunit doesn't have the same policy. > > IMO, this creates a bad incentive, and saying that any capable CI system > > should reject them is certainly opiniated. > > All I'm saying it generates an extra burden for current real world CI > systems that run tests on a massive scale and even have paid > maintainers. It's not trivial to sort out expected and unexpected > warnings, and keep the filters updated every time the warnings > change. It's not without a cost. And the end result might just be that > the unit tests won't get run at all. Or warnings just get completely > ignored for kunit tests. I realise it must take a significant amount of resources, but it's also self inflicted. You could also stop looking for warnings when running kunit. > I don't completely disagree with you either. It's just that we don't > have that automation on the kernel side, and in the mean time, perhaps > we should be really conservative about the warnings we create in tests? > > If we can't filter out the warnings kernel side, perhaps we could figure > out a way to annotate the kunit tests that generate warnings on passing > runs? Yeah, I think that would be the best solution. That's what Guenther's series was about :/ Maxime signature.asc Description: PGP signature
Re: [PATCH v2 02/22] drm: Add valid clones check
On Tue, Sep 24, 2024 at 03:59:18PM GMT, Jessica Zhang wrote:
> Check that all encoders attached to a given CRTC are valid
> possible_clones of each other.
>
> Signed-off-by: Jessica Zhang
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 43cdf39019a4..cc4001804fdc 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -574,6 +574,25 @@ mode_valid(struct drm_atomic_state *state)
> return 0;
> }
>
> +static int drm_atomic_check_valid_clones(struct drm_atomic_state *state,
> + struct drm_crtc *crtc)
> +{
> + struct drm_encoder *drm_enc;
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> + crtc);
> +
> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc_state->encoder_mask)
> {
> + if ((crtc_state->encoder_mask & drm_enc->possible_clones) !=
> + crtc_state->encoder_mask) {
> + DRM_DEBUG("crtc%d failed valid clone check for mask
> 0x%x\n",
> + crtc->base.id, crtc_state->encoder_mask);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> /**
> * drm_atomic_helper_check_modeset - validate state object for modeset
> changes
> * @dev: DRM device
> @@ -745,6 +764,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> ret = drm_atomic_add_affected_planes(state, crtc);
> if (ret != 0)
> return ret;
> +
> + ret = drm_atomic_check_valid_clones(state, crtc);
> + if (ret != 0)
> + return ret;
> }
Pretty much the same comment, we should have kunit tests for this.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 01/22] drm: add clone mode check for CRTC
On Wed, Sep 25, 2024 at 02:06:35AM GMT, Dmitry Baryshkov wrote: > On Tue, Sep 24, 2024 at 03:59:17PM GMT, Jessica Zhang wrote: > > Add helper to check if the given CRTC state is in clone mode > > > > Signed-off-by: Jessica Zhang > > --- > > include/drm/drm_crtc.h | 7 +++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 8b48a1974da3..ecb93e2c4afc 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -1323,5 +1323,12 @@ static inline struct drm_crtc *drm_crtc_find(struct > > drm_device *dev, > > > > int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc, > > unsigned int supported_filters); > > Missing kerneldoc Also, a proper commit log to describe what this will be used for, and ideally some kunit tests. Maxime signature.asc Description: PGP signature
Re: [PATCH 0/2] drm: revert some framebuffer API tests
On Tue, Sep 24, 2024 at 06:56:26PM GMT, Jani Nikula wrote: > On Tue, 24 Sep 2024, Guenter Roeck wrote: > On Tue, Sep 24, 2024 at 12:06:28PM GMT, Simona Vetter wrote: > > Yeah I think long-term we might want a kunit framework so that we can > > catch dmesg warnings we expect and test for those, without those > > warnings > > actually going to dmesg. Similar to how the lockdep tests also reroute > > locking validation, so that the expected positive tests don't wreak > > lockdep for real. > > > > But until that exists, we can't have tests that splat in dmesg when they > > work as intended. > > > > FWIW, that is arguable. More and more tests are added which do add such > > splats, > > and I don't see any hesitance by developers to adding more. So far I counted > > two alone in this commit window, and that does not include new splats from > > tests which I had already disabled. I simply disable those tests or don't > > enable them in the first place if they are new. I did the same with the drm > > unit tests due to the splats generated by the scaling unit tests, so any > > additional drm unit test splats don't make a difference for me since the > > tests are already disabled. > > What's the point of having unit tests that CI systems routinely have to > filter out of test runs? Or filter warnings generated by the tests, > potentially missing new warnings. Who is going to run the tests if the > existing CI systems choose to ignore them? If we turn this argument around, that means we can't write unit test for code that will create a warning. IMO, this creates a bad incentive, and saying that any capable CI system should reject them is certainly opiniated. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm: renesas: rcar-du: Add drm_panic support for non-vsp
On Mon, 27 May 2024 15:35:49 +0200, Geert Uytterhoeven wrote: > Add support for the drm_panic module for DU variants not using the > VSP-compositor, to display a message on the screen when a kernel panic > occurs. > > Applied to misc/kernel.git (drm-misc-next). Thanks! Maxime
Re: [PATCH] drm: renesas: shmobile: Add drm_panic support
On Mon, 27 May 2024 15:34:48 +0200, Geert Uytterhoeven wrote: > Add support for the drm_panic module, which displays a message on > the screen when a kernel panic occurs. > > Applied to misc/kernel.git (drm-misc-next). Thanks! Maxime
Re: [PATCH v4] drm/connector: hdmi: Fix writing Dynamic Range Mastering infoframes
On Tue, 27 Aug 2024 11:39:04 -0500, Derek Foreman wrote: > The largest infoframe we create is the DRM (Dynamic Range Mastering) > infoframe which is 26 bytes + a 4 byte header, for a total of 30 > bytes. > > With HDMI_MAX_INFOFRAME_SIZE set to 29 bytes, as it is now, we > allocate too little space to pack a DRM infoframe in > write_device_infoframe(), leading to an ENOSPC return from > hdmi_infoframe_pack(), and never calling the connector's > write_infoframe() vfunc. > > [...] Applied to misc/kernel.git (drm-misc-fixes). Thanks! Maxime
Re: [PATCH v7 3/3] drm/rockchip: Add basic RK3588 HDMI output support
Hi,
On Sat, Sep 14, 2024 at 09:56:53PM GMT, Cristian Ciocaltea wrote:
> The RK3588 SoC family integrates the newer Synopsys DesignWare HDMI 2.1
> Quad-Pixel (QP) TX controller IP and a HDMI/eDP TX Combo PHY based on a
> Samsung IP block.
>
> Add just the basic support for now, i.e. RGB output up to 4K@60Hz,
> without audio, CEC or any of the HDMI 2.1 specific features.
>
> Co-developed-by: Algea Cao
> Signed-off-by: Algea Cao
> Tested-by: Heiko Stuebner
> Signed-off-by: Cristian Ciocaltea
> ---
> drivers/gpu/drm/rockchip/Kconfig | 9 +
> drivers/gpu/drm/rockchip/Makefile | 1 +
> drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 436
> +
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c| 2 +
> drivers/gpu/drm/rockchip/rockchip_drm_drv.h| 1 +
> 5 files changed, 449 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/Kconfig
> b/drivers/gpu/drm/rockchip/Kconfig
> index 23c49e91f1cc..448fadd4ba15 100644
> --- a/drivers/gpu/drm/rockchip/Kconfig
> +++ b/drivers/gpu/drm/rockchip/Kconfig
> @@ -8,6 +8,7 @@ config DRM_ROCKCHIP
> select VIDEOMODE_HELPERS
> select DRM_ANALOGIX_DP if ROCKCHIP_ANALOGIX_DP
> select DRM_DW_HDMI if ROCKCHIP_DW_HDMI
> + select DRM_DW_HDMI_QP if ROCKCHIP_DW_HDMI_QP
> select DRM_DW_MIPI_DSI if ROCKCHIP_DW_MIPI_DSI
> select GENERIC_PHY if ROCKCHIP_DW_MIPI_DSI
> select GENERIC_PHY_MIPI_DPHY if ROCKCHIP_DW_MIPI_DSI
> @@ -63,6 +64,14 @@ config ROCKCHIP_DW_HDMI
> enable HDMI on RK3288 or RK3399 based SoC, you should select
> this option.
>
> +config ROCKCHIP_DW_HDMI_QP
> + bool "Rockchip specific extensions for Synopsys DW HDMI QP"
> + select DRM_BRIDGE_CONNECTOR
> + help
> + This selects support for Rockchip SoC specific extensions
> + for the Synopsys DesignWare HDMI QP driver. If you want to
> + enable HDMI on RK3588 based SoC, you should select this option.
> +
> config ROCKCHIP_DW_MIPI_DSI
> bool "Rockchip specific extensions for Synopsys DW MIPI DSI"
> select GENERIC_PHY_MIPI_DPHY
> diff --git a/drivers/gpu/drm/rockchip/Makefile
> b/drivers/gpu/drm/rockchip/Makefile
> index 3ff7b21c0414..3eab662a5a1d 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -11,6 +11,7 @@ rockchipdrm-$(CONFIG_ROCKCHIP_VOP) += rockchip_drm_vop.o
> rockchip_vop_reg.o
> rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
> +rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI_QP) += dw_hdmi_qp-rockchip.o
> rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi-rockchip.o
> rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
> rockchipdrm-$(CONFIG_ROCKCHIP_LVDS) += rockchip_lvds.o
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> new file mode 100644
> index ..19d407c926bd
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -0,0 +1,436 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021-2022 Rockchip Electronics Co., Ltd.
> + * Copyright (c) 2024 Collabora Ltd.
> + *
> + * Author: Algea Cao
> + * Author: Cristian Ciocaltea
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "rockchip_drm_drv.h"
> +
> +#define RK3588_GRF_SOC_CON2 0x0308
> +#define RK3588_HDMI0_HPD_INT_MSK BIT(13)
> +#define RK3588_HDMI0_HPD_INT_CLR BIT(12)
> +#define RK3588_GRF_SOC_CON7 0x031c
> +#define RK3588_SET_HPD_PATH_MASK GENMASK(13, 12)
> +#define RK3588_GRF_SOC_STATUS1 0x0384
> +#define RK3588_HDMI0_LEVEL_INT BIT(16)
> +#define RK3588_GRF_VO1_CON3 0x000c
> +#define RK3588_SCLIN_MASKBIT(9)
> +#define RK3588_SDAIN_MASKBIT(10)
> +#define RK3588_MODE_MASK BIT(11)
> +#define RK3588_I2S_SEL_MASK BIT(13)
> +#define RK3588_GRF_VO1_CON9 0x0024
> +#define RK3588_HDMI0_GRANT_SEL BIT(10)
> +
> +#define HIWORD_UPDATE(val, mask) ((val) | (mask) << 16)
> +
> +struct rockchip_hdmi_qp {
> + struct device *dev;
> + struct regmap *regmap;
> + struct regmap *vo_regmap;
> + struct rockchip_encoder encoder;
> + struct clk *ref_clk;
> + struct dw_hdmi_qp *hdmi;
> + struct phy *phy;
> + struct gpio_desc *enable_gpio;
> + struct delayed_work hpd_work;
> +};
> +
> +static struct rockchip_hdmi_qp *to_rockchip_hdmi_qp(struct drm_encoder
> *encoder)
> +{
> + struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> +
> + return container_of(rkencoder, struct rockchip_hdmi_qp, encoder);
> +}
> +
> +static void
> +dw_hdmi_qp_rockchip_enco
Re: [PATCH v7 1/3] drm/bridge: synopsys: Add DW HDMI QP TX Controller support library
On Sat, 14 Sep 2024 21:56:51 +0300, Cristian Ciocaltea wrote: > The Synopsys DesignWare HDMI 2.1 Quad-Pixel (QP) TX Controller IP > supports the following features, among others: > > * Fixed Rate Link (FRL) > * Display Stream Compression (DSC) > > [ ... ] Reviewed-by: Maxime Ripard Thanks! Maxime
Re: [PATCH 0/2] drm: revert some framebuffer API tests
On Tue, Sep 24, 2024 at 06:37:59AM GMT, Guenter Roeck wrote: > On 9/24/24 04:54, Maxime Ripard wrote: > > +Guenter > > > > On Tue, Sep 24, 2024 at 12:06:28PM GMT, Simona Vetter wrote: > > > On Tue, Sep 17, 2024 at 08:43:50PM +0300, Jani Nikula wrote: > > > > The tests consistently trigger WARNs in drm_framebuffer code. I'm not > > > > sure what the point is with type of belts and suspenders tests. The > > > > warnings *are* the way to flag erroneous API usage. > > > > > > > > Warnings in turn trigger failures in CI. Filtering the warnings are > > > > error prone, and, crucially, would also filter actual errors in case the > > > > kunit tests are not run. > > > > > > > > I acknowledge there may be complex test cases where you'd end up > > > > triggering warnings somewhere deep, but these are not it. These are > > > > simple. > > > > > > > > Revert the tests, back to the drawing board. > > > > > > Yeah I think long-term we might want a kunit framework so that we can > > > catch dmesg warnings we expect and test for those, without those warnings > > > actually going to dmesg. Similar to how the lockdep tests also reroute > > > locking validation, so that the expected positive tests don't wreak > > > lockdep for real. > > > > > > But until that exists, we can't have tests that splat in dmesg when they > > > work as intended. > > > > It should be pretty soon: > > https://lore.kernel.org/dri-devel/20240403131936.787234-1-li...@roeck-us.net/ > > > > I'm not sure what happened to that series, but it looks like everybody > > was mostly happy with it? > > > > Major subsystem maintainers did not provide any feedback at all, and then > there came the "it is not perfect enough" feedback, so I gave up on it. Well, if that means anything, we're interested even in something imperfect if it solves the above case :) Maxime signature.asc Description: PGP signature
Re: [PATCH 0/2] drm: revert some framebuffer API tests
+Guenter On Tue, Sep 24, 2024 at 12:06:28PM GMT, Simona Vetter wrote: > On Tue, Sep 17, 2024 at 08:43:50PM +0300, Jani Nikula wrote: > > The tests consistently trigger WARNs in drm_framebuffer code. I'm not > > sure what the point is with type of belts and suspenders tests. The > > warnings *are* the way to flag erroneous API usage. > > > > Warnings in turn trigger failures in CI. Filtering the warnings are > > error prone, and, crucially, would also filter actual errors in case the > > kunit tests are not run. > > > > I acknowledge there may be complex test cases where you'd end up > > triggering warnings somewhere deep, but these are not it. These are > > simple. > > > > Revert the tests, back to the drawing board. > > Yeah I think long-term we might want a kunit framework so that we can > catch dmesg warnings we expect and test for those, without those warnings > actually going to dmesg. Similar to how the lockdep tests also reroute > locking validation, so that the expected positive tests don't wreak > lockdep for real. > > But until that exists, we can't have tests that splat in dmesg when they > work as intended. It should be pretty soon: https://lore.kernel.org/dri-devel/20240403131936.787234-1-li...@roeck-us.net/ I'm not sure what happened to that series, but it looks like everybody was mostly happy with it? Maxime signature.asc Description: PGP signature
Re: [PATCH v17 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver
On Tue, Sep 24, 2024 at 03:36:49PM GMT, Sandor Yu wrote:
> +static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device *mhdp)
> +{
> + u8 status;
> + int ret;
> +
> + mutex_lock(&mhdp_mailbox_mutex);
> +
> + ret = cdns_mhdp_mailbox_send(&mhdp->base, MB_MODULE_ID_GENERAL,
> + GENERAL_GET_HPD_STATE, 0, NULL);
> + if (ret)
> + goto err_get_hpd;
> +
> + ret = cdns_mhdp_mailbox_recv_header(&mhdp->base, MB_MODULE_ID_GENERAL,
> + GENERAL_GET_HPD_STATE,
> + sizeof(status));
> + if (ret)
> + goto err_get_hpd;
> +
> + ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, &status, sizeof(status));
> + if (ret)
> + goto err_get_hpd;
> +
> + mutex_unlock(&mhdp_mailbox_mutex);
That's better I guess, but it's still not a good API design. If you
can't have concurrent accesses, then cdns_mhdp_mailbox_send et al.
should take the mutex themselves.
> + return status;
> +
> +err_get_hpd:
> + dev_err(mhdp->dev, "read hpd failed: %d\n", ret);
> + mutex_unlock(&mhdp_mailbox_mutex);
> +
> + return ret;
> +}
> +
> +enum drm_connector_status cdns_mhdp8501_detect(struct cdns_mhdp8501_device
> *mhdp)
> +{
> + u8 hpd = 0xf;
> +
> + hpd = cdns_mhdp8501_read_hpd(mhdp);
> + if (hpd == 1)
> + return connector_status_connected;
> + else if (hpd == 0)
> + return connector_status_disconnected;
> +
> + dev_warn(mhdp->dev, "Unknown cable status, hdp=%u\n", hpd);
This is already logged, there's no need to add a message there.
> + return connector_status_unknown;
> +}
> +
> +static void hotplug_work_func(struct work_struct *work)
> +{
> + struct cdns_mhdp8501_device *mhdp = container_of(work,
> + struct
> cdns_mhdp8501_device,
> + hotplug_work.work);
> + enum drm_connector_status status = cdns_mhdp8501_detect(mhdp);
> +
> + drm_bridge_hpd_notify(&mhdp->bridge, status);
> +
> + if (status == connector_status_connected) {
> + /* Cable connected */
> + DRM_INFO("HDMI/DP Cable Plug In\n");
You might want to log it using drm_dev_debug, but anything else is a big
stretch.
> + enable_irq(mhdp->irq[IRQ_OUT]);
> +
> + /* Reset HDMI/DP link with sink */
> + if (mhdp->connector_type == DRM_MODE_CONNECTOR_HDMIA)
> + cdns_hdmi_reset_link(mhdp);
> + else
> + cdns_dp_check_link_state(mhdp);
> +
> + } else if (status == connector_status_disconnected) {
> + /* Cable Disconnected */
> + DRM_INFO("HDMI/DP Cable Plug Out\n");
> + enable_irq(mhdp->irq[IRQ_IN]);
> + }
> +}
> +
> +static irqreturn_t cdns_mhdp8501_irq_thread(int irq, void *data)
> +{
> + struct cdns_mhdp8501_device *mhdp = data;
> +
> + disable_irq_nosync(irq);
> +
> + mod_delayed_work(system_wq, &mhdp->hotplug_work,
> + msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
> +
> + return IRQ_HANDLED;
> +}
disable_irq_nosync doesn't guarantee that you won't be called before
interrupts are reenabled. What will happen if this handler is called
multiple times?
> +
> +static int cdns_mhdp8501_dt_parse(struct cdns_mhdp8501_device *mhdp,
> + struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *remote;
> +
> + remote = of_graph_get_remote_node(np, 1, 0);
> + if (!remote) {
> + dev_err(dev, "fail to get remote node\n");
> + of_node_put(remote);
> + return -EINVAL;
> + }
> +
> + /* get connector type */
> + if (of_device_is_compatible(remote, "hdmi-connector")) {
> + mhdp->connector_type = DRM_MODE_CONNECTOR_HDMIA;
> +
> + } else if (of_device_is_compatible(remote, "dp-connector")) {
> + mhdp->connector_type = DRM_MODE_CONNECTOR_DisplayPort;
> +
> + } else {
> + dev_err(dev, "Unknown connector type\n");
> + of_node_put(remote);
> + return -EINVAL;
> + }
> +
> + of_node_put(remote);
> +
> + if (of_property_read_u32(np, "lane-mapping", &mhdp->lane_mapping)) {
> + dev_warn(dev, "Failed to get lane_mapping - using default\n");
debug logging as well.
> + mhdp->lane_mapping = LANE_MAPPING_FLIPPED;
> + }
> +
> + return true;
> +}
> +
> +static void cdns_mhdp8501_add_bridge(struct cdns_mhdp8501_device *mhdp)
> +{
> + mhdp->bridge.type = mhdp->connector_type;
> + mhdp->bridge.driver_private = mhdp;
> + mhdp->bridge.of_node = mhdp->dev->of_node;
> + mhdp->bridge.vendor = "NXP";
> + mhdp->bridge.product = "i.MX8";
> + mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM
