Re: [PATCH v14 0/6] drm/imx: Introduce i.MX8qm/qxp DPU DRM
Hi, On Tue, Aug 22, 2023 at 05:36:14AM +, Ying Liu wrote: > Hi, > > > On Friday, January 6, 2023 1:50 PM Ying Liu wrote: > > > > Hi, > > > > > > This is the v14 series to introduce i.MX8qm/qxp Display Processing Unit(DPU) > > DRM support. > > > > DPU is comprised of a blit engine for 2D graphics, a display controller > > and a command sequencer. Outside of DPU, optional prefetch engines can > > fetch data from memory prior to some DPU fetchunits of blit engine and > > display controller. The pre-fetchers support linear formats and Vivante > > GPU tile formats. > > > > Reference manual can be found at: > > https://www.nxp.com/webapp/Download?colCode=IMX8DQXPRM > > > > > > This patch set adds kernel modesetting support for the display controller > > part. > > It supports two CRTCs per display controller, several planes, prefetch > > engines and some properties of CRTC and plane. Currently, the registers of > > the controller is accessed without command sequencer involved, instead just > > by > > using CPU. DRM connectors would be created from the DPU KMS driver. > > > > > > Patch 1 ~ 3 add dt-bindings for DPU and prefetch engines. > > Patch 4 is a minor improvement of a macro to suppress warning as the KMS > > driver > > uses it. > > Patch 5 introduces the DPU DRM support. > > Patch 6 updates MAINTAINERS. > > > > Welcome comments, thanks. > > > > v13->v14: > > * Rebase the patch series to the latest drm-misc-next branch(v6.1-rc6 > > based). > > * Include drm_fbdev_generic.h in dpu_drv.c due to the rebase. > > * Fix dpu drm driver suspend/resume by properly get drm device through > > dev_get_drvdata(). > > * Use pm_ptr() macro for dpu core driver PM operations. > > * Use pm_sleep_ptr() macro for dpu drm driver PM operations. > > * Use DEFINE_SIMPLE_DEV_PM_OPS() macro to define dpu drm driver PM > > operations, > > instead of SIMPLE_DEV_PM_OPS(). > > * Update year of Copyright. > > * Add SoC series name 'i.MX8'/'IMX8'/'imx8' to dpu driver module decription, > > Kconfig name, dpu driver names and dpu driver object name. > > > > v12->v13: > > * Drop 'drm->irq_enabled = true;' from patch 5/6 to fix a potential build > > break reported by 'kernel test robot '. drm->irq_enabled > > should not be used by imx-dpu drm as it is only used by legacy drivers > > with userspace modesetting. > > > > v11->v12: > > * Rebase the series upon v6.1-rc1. > > * Minor update on Kconfigs, struct names and macro names for patch 5/6 > > due to the rebase. > > > > v10->v11: > > * Rebase the series upon v6.0-rc1. > > * Include drm_blend.h and drm_framebuffer.h in dpu-kms.c and dpu- > > plane.c > > to fix build errors due to the rebase. > > * Fix a checkpatch warning for dpu-crtc.c. > > * Properly use dev_err_probe() to return it's return value directly where > > possible. > > > > v9->v10: > > * Rebase the series upon v5.18-rc1. > > * Make 'checkpatch.pl --strict' happier for patch 5/6. > > * Add Rob's R-b tag on patch 3/6. > > * Add Laurentiu's R-b tag on patch 5/6. > > * Add Laurentiu's A-b tag on patch 6/6. > > > > v8->v9: > > * Use drm_atomic_get_new_plane_state() in dpu_plane_atomic_update() > > for > > patch 5/6. (Laurentiu) > > * Drop getting DPU DT alias ID for patch 5/6, as it is unused. > > * Reference 'interrupts-extended' schema instead of 'interrupts' for patch > > 3/6 > > to require an additional DPR interrupt(r_rtram_stall) because the > > reference > > manual does mention it, though the driver doesn't get/use it for now. > > Reference 'interrupt-names' schema to define the two DPR interrupt names > > - > > 'dpr_wrap' and 'r_rtram_stall'. Accordingly, patch 5/6 gets the > > 'dpr_wrap' > > interrupt by name. > > * Drop Rob's R-b tag on patch 3/6, as review is needed. > > > > v7->v8: > > * Rebase this series up onto the latest drm-misc-next branch, due to DRM > > plane > > helper functions API change(atomic_check and atomic_update) from DRM > > atomic > > core. So, dpu_plane_atomic_check() and dpu_plane_atomic_update() are > > updated > > accordingly in patch 5/6. Also, rename plane->state variables and > > relevant > > DPU plane state variables in those two functions to reflect they are new > > states, like the patch 'drm: Rename plane->state variables in atomic > > update > > and disable' recently landed in drm-misc-next. > > * Replace drm_gem_fb_prepare_fb() with > > drm_gem_plane_helper_prepare_fb() in > > patch 5/6, due to DRM core API change. > > * Improve DPR burst length for GPU standard tile and 32bpp GPU super tile in > > patch 5/6 to align with the latest version of internal HW documention. > > > > v6->v7: > > * Fix return value of dpu_get_irqs() if platform_get_irq() fails. > > (Laurentiu) > > * Use the function array dpu_irq_handler[] to store individual DPU irq > > handlers. > > (Laurentiu) > > * Call get/put() hooks directly to get/put DPU fetchunits for DPU plane > > groups. > > (Laurentiu) > > * Shorten the names of indi
Re: [PATCH v4 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU
Hi, To expand on what Krzysztof said On Tue, Jul 18, 2023 at 01:10:14PM +0200, Krzysztof Kozlowski wrote: > On 18/07/2023 13:08, Frank Binns wrote: > >> And this > >> items: > >> - const: gpu > >> can just be > >> const: gpu > >> > >> Although, if there is only one interrupt this is probably not > >> particularly helpful. Are there other implementations of this IP that > >> have more interrupts? > > > > No, all our current GPUs just have a single interrupt. I assume it's more > > future > > proof to keep the name in case that ever changes? > > Why do you need name in the first place? If there is single entry, the > name is pointless, especially if it repeats the name of the IP block. Generally speaking, if there's only one resource (interrupt, clock, etc) we don't put any discriminant. If you need to extend it later on for some reason, then you'll add an interrupt-names property and you can either require it for that new GPU or whatever, or fallback on the nameless on if no name was found. > > As in, by having the name now > > we can make it a required property, which I guess we won't be able to do at > > some > > later point. > > Why even making it required? There's no issue with introducing a new property later on if a GPU needs it. Then, you can either make it required only for that particular generation, or you can have some fallback case. Both are fine and easy to do. Maxime signature.asc Description: PGP signature
Re: Re: Re: [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
Hi, On Wed, Jan 31, 2024 at 05:27:14AM +, Jason-JH Lin (林睿祥) wrote: > > On Sun, 2024-01-28 at 10:24 +0100, Maxime Ripard wrote: > > On Thu, Jan 25, 2024 at 07:17:21PM +0100, Daniel Vetter wrote: > > > On Tue, Jan 23, 2024 at 06:09:05AM +, Jason-JH Lin (林睿祥) wrote: > > > > Hi Maxime, Daniel, > > > > > > > > We encountered similar issue with mediatek SoCs. > > > > > > > > We have found that in drm_atomic_helper_commit_rpm(), when > > > > disabling > > > > the cursor plane, the old_state->legacy_cursor_update in > > > > drm_atomic_wait_for_vblank() is set to true. > > > > As the result, we are not actually waiting for a vlbank to wait > > > > for our > > > > hardware to close the cursor plane. Subsequently, the execution > > > > proceeds to drm_atomic_helper_cleanup_planes() to free the > > > > cursor > > > > buffer. This can lead to use-after-free issues with our hardware. > > > > > > > > Could you please apply this patch to fix our problem? > > > > Or are there any considerations for not applying this patch? > > > > > > Mostly it needs someone to collect a pile of acks/tested-by and > > > then land > > > it. > > > > > > I'd be _very_ happy if someone else can take care of that ... > > > > > > There's also the potential issue that it might slow down some of > > > the > > > legacy X11 use-cases that really needed a non-blocking cursor, but > > > I think > > > all the drivers where this matters have switched over to the async > > > plane > > > update stuff meanwhile. So hopefully that's good. > > > > I think there was also a regression with msm no one really figured > > out? > > OK... > But I am only available on MediaTek platform. I think most of us are in that situation, and which is part of the reason it kind of stalled :) > Does it also causes a regression with msn if I re-send a patch for > drm_atomic_helper.c only? Yes, that's my recollection at least. Fortunately, Dmitry might be able to clear that up. Maxime signature.asc Description: PGP signature
Re: [PATCH v6 3/6] drm/display: Add missing aux less alpm wake related bits
On Tue, May 28, 2024 at 10:04:06AM GMT, Manna, Animesh wrote: > + drm-core maintainer > > Hi, > > Could you please have a look and ack this patch. > Received r-b from Jouni on 0th patch for the whole patch series. Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH 02/17] drm/panel replay: Add edp1.5 Panel Replay bits and register
On Tue, May 28, 2024 at 11:25:41AM GMT, Hogander, Jouni wrote: > On Thu, 2024-05-16 at 11:53 +0300, Jouni Högander wrote: > > Add PANEL_REPLAY_CONFIGURATION_2 register and some missing Panel > > Replay > > bits. > > Hello drm-core maintainers, > > Could you please consider providing your ack on this patch? I'm > planning to merge it via drm-intel tree. I have already r-b tag. Acked-by: Maxime Ripard Maxime signature.asc Description: PGP signature
Re: [PATCH v6 0/7] Add mediate-drm secure flow for SVP
Hi, On Tue, Jun 11, 2024 at 09:13:03AM GMT, Jason-JH Lin (林睿祥) wrote: > Hi Maxime, > > [snip] > > > > > > > --- > > > > > > TODO: > > > > > > 1) Drop MTK_DRM_IOCTL_GEM_CREATE and use DMA_HEAP_IOCTL_ALLOC > > > > > > in > > > > > > userspace > > > > > > 2) DRM driver use secure mailbox channel to handle normal and > > > > > > secure flow > > > > > > 3) Implement setting mmsys routing table in the secure world > > > > > > series > > > > > > > > > > I'm not sure what you mean here. Why are you trying to upstream > > > > > something that still needs to be removed from your patch > > > > > series? > > > > > > > > > > > > > Because their is too much patches need to be fixed in this > > > > series, > > > > so I > > > > list down the remaining TODO items and send to review for the > > > > other > > > > patches. > > > > > > > > Sorry for the bothering, I'll drop this at the next version. > > > > > > If you don't intend to use it, we just shouldn't add it. Removing > > > the > > > TODO item doesn't make sense, even more so if heaps should be the > > > way > > > you handle this. > > > > > > > Sorry for this misunderstanding. > > > > I mean I'll remove the DRM_IOCTL_GEM_CREATE patch and then change > > user > > space calling DMA_HEAP_IOCTL_ALLOC to allocate buffer from secure > > heap. > > > > I have changed user space to use DMA_HEAP_IOCTL_ALLOC to allocate > secure buffer, but I still encounter the problem of determining whether > the buffer is secure in mediatek-drm driver to add some secure > configure for hardware. > > > As the comment in [1], dma driver won't provide API for use. > [1]: > https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/#25857255 > > > So I use name checking at [PATCH v6 3/7] like this currently: > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device > *dev, > struct dma_buf_attachment *attach, struct sg_table *sg) > { > struct mtk_gem_obj *mtk_gem; > > /* check if the entries in the sg_table are contiguous */ > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > DRM_ERROR("sg_table is not contiguous"); > return ERR_PTR(-EINVAL); > } > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > if (IS_ERR(mtk_gem)) > return ERR_CAST(mtk_gem); > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > + mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > return &mtk_gem->base; > } > > But I want to change this name checking to the information brought from > user space. > I tried to use arg->flags to append the secure flag in user space and > call drmPrimeHandleToFD() to pass it to DRM driver, but it will be > blocked by at the beginning of the drm_prime_handle_to_fd_ioctl(). I agree with you, it's something to discuss mostly with the dma-buf maintainers but it would be better to just set a flag on the dma-buf, and use that flag whenever necessary. It might be related to the recent work I did to introduce allocation flags too: https://lore.kernel.org/linux-media/[email protected]/ Maxime signature.asc Description: PGP signature
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > External email : Please do not click links or open attachments > > until > > > you have verified the sender or the content. > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > https://lore.kernel.org/linux-media/[email protected]/ > > > > > > > > > I think it would be better to have the user configurable > > > private > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > requirement can get their private information from dma-buf > > > directly > > > > > > > and > > > > > > > no need to change or add the interface. > > > > > > > > > What's your opinion in this point? > > > > > > > Well of hand I don't see the need for that. > > > > > > > What happens if you get a non-secure buffer imported in your > > > secure > > > > > > device? > > > > > > > > We use the same mediatek-drm driver for secure and > > non-secure > > > buffer. > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > the > > > > > normal flow with normal hardware settings. > > > > > > > > We use different configurations to make hardware have > > different > > > > > permission to access the buffer it should access. > > > > > > > > So if we can't get the information of "the buffer is > > allocated > > > from > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > won't > > > > > be able to configure the hardware correctly. > > > > > > Why can't you get this information from userspace? > > > > Same reason amd and i915/xe also pass this around internally in the > > > kernel, it's just that for those gpus the render and kms node are the > > > same > > > driver so this is easy. > > > > > The reason I ask is that encryption here looks just like another parameter > for the buffer, e.g. like format, stride, tilling etc.. > > So instead of this during buffer import: > > mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > You can trivially say during use hey this buffer is encrypted. > > At least that's my 10 mile high view, maybe I'm missing some extensive key > exchange or something like that. That doesn't work in all cases, unfortunately. If you're doing secure video playback, the firmware is typically in charge of the frame decryption/decoding, and you'd get dma-buf back that aren't accessible by the CPU (or at least, not at the execution level Linux runs with). So nobody can map that buffer, and the firmware driver is the one who knows that this buffer cannot be accessed by anyone. Putting this on the userspace to know would be pretty weird, and wouldn't solve the case where the kernel would try to map it. Maxime signature.asc Description: PGP signature
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Fri, Jun 28, 2024 at 01:47:01PM GMT, Thierry Reding wrote: > On Thu, Jun 27, 2024 at 04:40:02PM GMT, [email protected] wrote: > > On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > > > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > > External email : Please do not click links or open attachments > > > > until > > > > > you have verified the sender or the content. > > > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > https://lore.kernel.org/linux-media/[email protected]/ > > > > > > > > > > > I think it would be better to have the user configurable > > > > > private > > > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > > > requirement can get their private information from dma-buf > > > > > directly > > > > > > > > > and > > > > > > > > > no need to change or add the interface. > > > > > > > > > > > What's your opinion in this point? > > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > > What happens if you get a non-secure buffer imported in your > > > > > secure > > > > > > > > device? > > > > > > > > > > We use the same mediatek-drm driver for secure and > > > > non-secure > > > > > buffer. > > > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > > > the > > > > > > > normal flow with normal hardware settings. > > > > > > > > > > We use different configurations to make hardware have > > > > different > > > > > > > permission to access the buffer it should access. > > > > > > > > > > So if we can't get the information of "the buffer is > > > > allocated > > > > > from > > > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > > > won't > > > > > > > be able to configure the hardware correctly. > > > > > > > > Why can't you get this information from userspace? > > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > > kernel, it's just that for those gpus the render and kms node are the > > > > > same > > > > > driver so this is easy. > > > > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > So instead of this during buffer import: > > > > > > mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > mtk_gem->size = attach->dmabuf->size; > > > mtk_gem->sg = sg; > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > exchange or something like that. > > > > That doesn't work in all cases, unfortunately. > > > > If you're doing secure video playback, the firmware is typically in > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > aren't accessible by the CPU (or at least, not at the execution level > > Linux runs with). > > Can you clarify which firmware you're talking about? Is this secure > firmware, or firmware running on the video decoding hardware? Secure firmware > > So nobody can map that buffer, and the firmware driver is the one who > > knows that this buffer cannot be accessed by anyone. Putting this on the > > userspace to know would be pretty weird, and wouldn't solve the case > > where the kernel would try to map it. > > Doesn't userspace need to know from the start whether it's trying to do >
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
On Fri, Jun 28, 2024 at 01:42:27PM GMT, Christian König wrote: > Am 27.06.24 um 16:40 schrieb [email protected]: > > [SNIP] > > > > > > > > Why can't you get this information from userspace? > > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > > kernel, it's just that for those gpus the render and kms node are the > > > > > same > > > > > driver so this is easy. > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > So instead of this during buffer import: > > > > > > mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); > > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > > mtk_gem->size = attach->dmabuf->size; > > > mtk_gem->sg = sg; > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > exchange or something like that. > > That doesn't work in all cases, unfortunately. > > > > If you're doing secure video playback, the firmware is typically in > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > aren't accessible by the CPU (or at least, not at the execution level > > Linux runs with). > > Yeah, that's perfectly fine. At least the AMD encryption solution > works exactly like that as well. > > So nobody can map that buffer, and the firmware driver is the one who > > knows that this buffer cannot be accessed by anyone. > > On most hw I know you can actually map that buffer, it's just that the > CPU sees only garbage in it because you don't have the necessary > decryption keys around. So you can always map and access the buffer, but only if you're in the right "context" the content would be correct? I think that part is pretty different than what ARM SoCs are doing, where they would typically prevent any CPU access and fault on access. > > Putting this on the userspace to know would be pretty weird, and > > wouldn't solve the case where the kernel would try to map it. > > But that's exactly how all other implementations work as far as I know. I > mean what do you do if the kernel maps the encrypted buffer? > > On AMD we also block userspace and kernel CPU accesses, but that is only > done to make it easier to find bugs not for correctness. > > And userspace absolutely needs to be aware that a buffer is encrypted, cause > otherwise it could potentially try to access it with the CPU. I absolutely agree. I guess our discussion is whether it's something that should be implicit and understood by applications, or if it should be explicit and discoverable. Maxime signature.asc Description: PGP signature
Re: [v3 3/6] drm/vs: Register DRM device
Hi,
On Mon, Dec 11, 2023 at 05:00:04PM +0800, Keith Zhao wrote:
> >> +static int vs_drm_device_init_clocks(struct vs_drm_device *priv)
> >> +{
> >> + struct drm_device *dev = &priv->base;
> >> + struct platform_device *pdev = to_platform_device(dev->dev);
> >> + struct device_node *of_node = pdev->dev.of_node;
> >> + struct clk *clock;
> >> + unsigned int i;
> >> + int ret;
> >> +
> >> + if (dev_get_platdata(&pdev->dev) || !of_node)
> >> + return 0;
> >> +
> >> + priv->nrsts = ARRAY_SIZE(priv->rst_vout);
> >> + for (int i = 0; i < priv->nrsts; ++i)
> >> + priv->rst_vout[i].id = vout_resets[i];
> >> + ret = devm_reset_control_bulk_get_shared(dev->dev, priv->nrsts,
> >> + priv->rst_vout);
> >
> > I would request resets and clocks in _probe().
>
> >
> > If component_bind_all() returns -EPROBE_DEFER because of a still
> > missing DSI panel backlight or similar, this doesn't have to be done
> > multiple times.
> I got what you mean. component_bind_all should be done multiple times
> to prevent the dsi panel driver from lagging load.
No. component_bind_all only needs to be called once.
> in my drm subsystem , there are 2 pipeline
>
> +--+
> | |
> | |
> ++ | +---+ | +---+ +--+ +--+
> |+->+ dc controller 0 +--->->+HDMICtl| ->+ PHY +-->+PANEL0+
> |AXI | | +---+ | +---+ +--+ +--+
> || | |
> || | |
> || | |
> || | |
> |APB | | +---+ +-++--+ +---+
> |+->+ dc controller 1 +--->>+ dsiTx +--->+DPHY +->+ PANEL1+
> || | +---+ +-++--+ +---+
> ++ | |
> +--+
>
>
> component_bind_all will bind the hdmi encoder and dsi encoder .
> binding the hdmi encoder will always return ok .
>
> binging the dsi encoder has a question :
> I used the panel-raspberrypi-touchscreen.c as panel driver ,
> this driver is a i2c device and it use a i2c command to read reg ID
> if read success , it will do drm_panel_add.
>
> if I disconnect the panel ,it will not do drm_panel_add.
> dsiTx will fail to find panel , The consequence is that the inputbridge
> cannot be created ,
> also outputbridge cannot be created.
> for encoder bind , it will fail to find the input bridge of dsi.
> Under this premise, although returning -EPROBE_DEFER allows bind to be
> executed multiple times,
> the final result is that the entire bind fails.
>
> returning -EPROBE_DEFER can solve panel driver from lagging load ,
> but for no panel case , it will destory all pipeline (include hdmi and dsi).
Yes, that's expected.
> I did two things:
> late_initcall_sync(vs_drm_init); to make sure the panel drive has been probed;
> dsi encoder bind always return ok to make sure hdmi pipeline ok at lease.
> component_bind_all do once .
You should have a look at
https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v6 0/7] Add mediate-drm secure flow for SVP
On Tue, May 28, 2024 at 07:15:34AM GMT, Jason-JH Lin (林睿祥) wrote: > Hi Maxime, > > On Mon, 2024-05-27 at 16:06 +0200, Maxime Ripard wrote: > > Hi, > > > > On Sun, May 26, 2024 at 07:29:21AM GMT, Jason-JH.Lin wrote: > > > From: Jason-jh Lin > > > > > > Memory Definitions: > > > secure memory - Memory allocated in the TEE (Trusted Execution > > > Environment) which is inaccessible in the REE (Rich Execution > > > Environment, i.e. linux kernel/userspace). > > > secure handle - Integer value which acts as reference to 'secure > > > memory'. Used in communication between TEE and REE to reference > > > 'secure memory'. > > > secure buffer - 'secure memory' that is used to store decrypted, > > > compressed video or for other general purposes in the TEE. > > > secure surface - 'secure memory' that is used to store graphic > > > buffers. > > > > > > Memory Usage in SVP: > > > The overall flow of SVP starts with encrypted video coming in from > > > an > > > outside source into the REE. The REE will then allocate a 'secure > > > buffer' and send the corresponding 'secure handle' along with the > > > encrypted, compressed video data to the TEE. The TEE will then > > > decrypt > > > the video and store the result in the 'secure buffer'. The REE will > > > then allocate a 'secure surface'. The REE will pass the 'secure > > > handles' for both the 'secure buffer' and 'secure surface' into the > > > TEE for video decoding. The video decoder HW will then decode the > > > contents of the 'secure buffer' and place the result in the 'secure > > > surface'. The REE will then attach the 'secure surface' to the > > > overlay > > > plane for rendering of the video. > > > > > > Everything relating to ensuring security of the actual contents of > > > the > > > 'secure buffer' and 'secure surface' is out of scope for the REE > > > and > > > is the responsibility of the TEE. > > > > > > DRM driver handles allocation of gem objects that are backed by a > > > 'secure > > > surface' and for displaying a 'secure surface' on the overlay > > > plane. > > > This introduces a new flag for object creation called > > > DRM_MTK_GEM_CREATE_RESTRICTED which indicates it should be a > > > 'secure > > > surface'. All changes here are in MediaTek specific code. > > > --- > > > TODO: > > > 1) Drop MTK_DRM_IOCTL_GEM_CREATE and use DMA_HEAP_IOCTL_ALLOC in > > > userspace > > > 2) DRM driver use secure mailbox channel to handle normal and > > > secure flow > > > 3) Implement setting mmsys routing table in the secure world series > > > > I'm not sure what you mean here. Why are you trying to upstream > > something that still needs to be removed from your patch series? > > > Because their is too much patches need to be fixed in this series, so I > list down the remaining TODO items and send to review for the other > patches. > > Sorry for the bothering, I'll drop this at the next version. If you don't intend to use it, we just shouldn't add it. Removing the TODO item doesn't make sense, even more so if heaps should be the way you handle this. > > Also, I made some comments on the previous version that have been > > entirely ignored and still apply on this version: > > > https://lore.kernel.org/dri-devel/20240415-guppy-of-perpetual-current-3a7974@houat/ > > > > I lost that mail in my mailbox, so I didn't reply at that time. > I have imported that mail and replied to you. Hope you don't mind :) I haven't received that answer Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/mediatek: Drop dependency on ARM
On Wed, Oct 30, 2024 at 04:52:17PM +0800, Chen-Yu Tsai wrote: > On Wed, Oct 30, 2024 at 4:48 PM CK Hu (胡俊光) wrote: > > > > On Wed, 2024-10-30 at 09:25 +0100, [email protected] wrote: > > > On Wed, Oct 30, 2024 at 03:30:34AM +, CK Hu (胡俊光) wrote: > > > > Hi, Chen-yu: > > > > > > > > On Tue, 2024-10-29 at 19:13 +0800, Chen-Yu Tsai wrote: > > > > > External email : Please do not click links or open attachments until > > > > > you have verified the sender or the content. > > > > > > > > > > > > > > > The recent attempt to make the MediaTek DRM driver build for non-ARM > > > > > compile tests made the driver unbuildable for arm64 platforms. Since > > > > > this is used on both ARM and arm64 platforms, just drop the dependency > > > > > on ARM. > > > > > > > > Reviewed-by: CK Hu > > > > > > > > I find this days ago, but I don't know there is someone who apply it. > > > > Let this patch go through drm-misc tree which already has the bug patch. > > > > > > If you are ok with this patch, why didn't you apply it yourself? > > > > > > I think that's very much the expectation, so it's probably took a while > > > to merge. > > > > That's ok for me to apply it if drm-misc has no plan to apply it. > > I'm confused. The culprit patch is already in drm-misc. So this one has > to go in drm-misc as well. > > I can try to apply it to drm-misc myself, or have a colleague assist with > that. I'll let it sit for another day in case anyone has something to say > about it. Sorry, I was under the assumption that CK had drm-misc commit rights? It should go through drm-misc indeed. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/mediatek: Drop dependency on ARM
On Wed, Oct 30, 2024 at 03:30:34AM +, CK Hu (胡俊光) wrote: > Hi, Chen-yu: > > On Tue, 2024-10-29 at 19:13 +0800, Chen-Yu Tsai wrote: > > External email : Please do not click links or open attachments until you > > have verified the sender or the content. > > > > > > The recent attempt to make the MediaTek DRM driver build for non-ARM > > compile tests made the driver unbuildable for arm64 platforms. Since > > this is used on both ARM and arm64 platforms, just drop the dependency > > on ARM. > > Reviewed-by: CK Hu > > I find this days ago, but I don't know there is someone who apply it. > Let this patch go through drm-misc tree which already has the bug patch. If you are ok with this patch, why didn't you apply it yourself? I think that's very much the expectation, so it's probably took a while to merge. Maxime signature.asc Description: PGP signature
Re: [PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Sat, Aug 17, 2024 at 11:52:22AM GMT, Aditya Garg wrote: > From: Kerem Karabay > > The Touch Bars found on x86 Macs support two USB configurations: one > where the device presents itself as a HID keyboard and can display > predefined sets of keys, and one where the operating system has full > control over what is displayed. This commit adds support for the display > functionality of the second configuration. > > Note that this driver has only been tested on T2 Macs, and only includes > the USB device ID for these devices. Testing on T1 Macs would be > appreciated. > > Credit goes to @imbushuo on GitHub for reverse engineering most of the > protocol. > > Signed-off-by: Kerem Karabay > Signed-off-by: Aditya Garg > --- > MAINTAINERS | 6 + > drivers/gpu/drm/tiny/Kconfig | 12 + > drivers/gpu/drm/tiny/Makefile | 1 + > drivers/gpu/drm/tiny/appletbdrm.c | 624 ++ > 4 files changed, 643 insertions(+) > create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 8766f3e5e..2665e6c57 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6889,6 +6889,12 @@ S: Supported > T: git https://gitlab.freedesktop.org/drm/misc/kernel.git > F: drivers/gpu/drm/sun4i/sun8i* > > +DRM DRIVER FOR APPLE TOUCH BARS > +M: Kerem Karabay > +L: [email protected] > +S: Maintained > +F: drivers/gpu/drm/tiny/appletbdrm.c How do you plan on maintaining it? If you want to do so through drm-misc (and you should), you need to list the gitlab repo here. Also, I haven't seen Kerem take part of the discussion at all. Are they ok with taking on the maintainer's role? It's really clear to me either why this needs to be going through hid at all. Is it not standalone? Maxime signature.asc Description: PGP signature
Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure
Hi, On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote: > On Thu, Aug 14, 2025 at 05:13:54PM +0100, [email protected] wrote: > > Hi, > > > > On Wed, Aug 13, 2025 at 10:04:22AM +, Kandpal, Suraj wrote: > > > > > > }; > > > > > > > > > > I still don't like that. This really doesn't belong here. If anything, > > > > > the drm_connector for writeback belongs to drm_crtc. > > > > > > > > Why? We already have generic HDMI field inside drm_connector. I am > > > > really > > > > hoping to be able to land DP parts next to it. In theory we can have a > > > > DVI- > > > > specific entry there (e.g. with the subconnector type). > > > > The idea is not to limit how the drivers subclass those structures. > > > > > > > > I don't see a good case why WB should deviate from that design. > > > > > > > > > If the issue is that some drivers need a custom drm_connector > > > > > subclass, then I'd rather turn the connector field of > > > > > drm_writeback_connector into a pointer. > > > > > > > > Having a pointer requires additional ops in order to get drm_connector > > > > from > > > > WB code and vice versa. Having drm_connector_wb inside drm_connector > > > > saves us from those ops (which don't manifest for any other kind of > > > > structure). > > > > Nor will it take any more space since union will reuse space already > > > > taken up by > > > > HDMI part. > > > > > > > > > > > > > > > Seems like this thread has died. We need to get a conclusion on the > > > design. > > > Laurent do you have any issue with the design given Dmitry's explanation > > > as to why this > > > Design is good for drm_writeback_connector. > > > > I'm with Laurent here. The idea for drm_connector (and a lot of drm > > structures) are to > > be used as base "classes" for extended structures. I don't know why HDMI > > connector ended > > up inside drm_connector as not all connectors have HDMI functionality, but > > that's a cleanup > > for another day. > > Maybe Maxime can better comment on it, but I think it was made exactly > for the purpose of not limiting the driver's design. For example, a lot > of drivers subclass drm_connector via drm_bridge_connector. If > struct drm_connector_hdmi was a wrapper around struct drm_connector, > then it would have been impossible to use HDMI helpers for bridge > drivers, while current design freely allows any driver to utilize > corresponding library code. That's exactly why we ended up like this. With that design, we wouldn't have been able to "inherit" two connector "classes": bridge_connector is one, intel_connector another one. See here for the rationale: https://lore.kernel.org/dri-devel/[email protected]/ I don't think the "but we'll bloat drm_connector" makes sense either. There's already a *lot* of things that aren't useful to every connector (fwnode, display_info, edid in general, scaling, vrr, etc.) And it's not like we allocate more than a handful of them during a system's life. Maxime signature.asc Description: PGP signature
Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure
On Tue, Aug 26, 2025 at 07:08:17PM +0300, Dmitry Baryshkov wrote: > On Tue, Aug 26, 2025 at 05:48:18PM +0200, [email protected] wrote: > > On Mon, Aug 25, 2025 at 06:26:48AM +, Kandpal, Suraj wrote: > > > > Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor > > > > drm_writeback_connector structure > > > > > > > > Hi, > > > > > > > > On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote: > > > > > On Thu, Aug 14, 2025 at 05:13:54PM +0100, [email protected] wrote: > > > > > > Hi, > > > > > > > > > > > > On Wed, Aug 13, 2025 at 10:04:22AM +, Kandpal, Suraj wrote: > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > I still don't like that. This really doesn't belong here. If > > > > > > > > > anything, the drm_connector for writeback belongs to drm_crtc. > > > > > > > > > > > > > > > > Why? We already have generic HDMI field inside drm_connector. I > > > > > > > > am really hoping to be able to land DP parts next to it. In > > > > > > > > theory we can have a DVI- specific entry there (e.g. with the > > > > subconnector type). > > > > > > > > The idea is not to limit how the drivers subclass those > > > > > > > > structures. > > > > > > > > > > > > > > > > I don't see a good case why WB should deviate from that design. > > > > > > > > > > > > > > > > > If the issue is that some drivers need a custom drm_connector > > > > > > > > > subclass, then I'd rather turn the connector field of > > > > > > > > > drm_writeback_connector into a pointer. > > > > > > > > > > > > > > > > Having a pointer requires additional ops in order to get > > > > > > > > drm_connector from WB code and vice versa. Having > > > > > > > > drm_connector_wb inside drm_connector saves us from those ops > > > > (which don't manifest for any other kind of structure). > > > > > > > > Nor will it take any more space since union will reuse space > > > > > > > > already taken up by HDMI part. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Seems like this thread has died. We need to get a conclusion on > > > > > > > the > > > > design. > > > > > > > Laurent do you have any issue with the design given Dmitry's > > > > > > > explanation as to why this Design is good for > > > > > > > drm_writeback_connector. > > > > > > > > > > > > I'm with Laurent here. The idea for drm_connector (and a lot of drm > > > > > > structures) are to be used as base "classes" for extended > > > > > > structures. I don't know why HDMI connector ended up inside > > > > > > drm_connector as not all connectors have HDMI functionality, but > > > > > > that's a > > > > cleanup for another day. > > > > > > > > > > Maybe Maxime can better comment on it, but I think it was made exactly > > > > > for the purpose of not limiting the driver's design. For example, a > > > > > lot of drivers subclass drm_connector via drm_bridge_connector. If > > > > > struct drm_connector_hdmi was a wrapper around struct drm_connector, > > > > > then it would have been impossible to use HDMI helpers for bridge > > > > > drivers, while current design freely allows any driver to utilize > > > > > corresponding library code. > > > > > > > > That's exactly why we ended up like this. With that design, we wouldn't > > > > have > > > > been able to "inherit" two connector "classes": bridge_connector is one, > > > > intel_connector another one. > > > > > > > > See here for the rationale: > > > > https://lore.kernel.org/dri-devel/[email protected]/ > > > > > > > > I don't think the "but we'll bloat drm_connector" makes sense either. > > > > There's already a *l
Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure
On Mon, Aug 25, 2025 at 06:26:48AM +, Kandpal, Suraj wrote: > > Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor > > drm_writeback_connector structure > > > > Hi, > > > > On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote: > > > On Thu, Aug 14, 2025 at 05:13:54PM +0100, [email protected] wrote: > > > > Hi, > > > > > > > > On Wed, Aug 13, 2025 at 10:04:22AM +, Kandpal, Suraj wrote: > > > > > > > > }; > > > > > > > > > > > > > > I still don't like that. This really doesn't belong here. If > > > > > > > anything, the drm_connector for writeback belongs to drm_crtc. > > > > > > > > > > > > Why? We already have generic HDMI field inside drm_connector. I > > > > > > am really hoping to be able to land DP parts next to it. In > > > > > > theory we can have a DVI- specific entry there (e.g. with the > > subconnector type). > > > > > > The idea is not to limit how the drivers subclass those structures. > > > > > > > > > > > > I don't see a good case why WB should deviate from that design. > > > > > > > > > > > > > If the issue is that some drivers need a custom drm_connector > > > > > > > subclass, then I'd rather turn the connector field of > > > > > > > drm_writeback_connector into a pointer. > > > > > > > > > > > > Having a pointer requires additional ops in order to get > > > > > > drm_connector from WB code and vice versa. Having > > > > > > drm_connector_wb inside drm_connector saves us from those ops > > (which don't manifest for any other kind of structure). > > > > > > Nor will it take any more space since union will reuse space > > > > > > already taken up by HDMI part. > > > > > > > > > > > > > > > > > > > > > > > Seems like this thread has died. We need to get a conclusion on the > > design. > > > > > Laurent do you have any issue with the design given Dmitry's > > > > > explanation as to why this Design is good for drm_writeback_connector. > > > > > > > > I'm with Laurent here. The idea for drm_connector (and a lot of drm > > > > structures) are to be used as base "classes" for extended > > > > structures. I don't know why HDMI connector ended up inside > > > > drm_connector as not all connectors have HDMI functionality, but that's > > > > a > > cleanup for another day. > > > > > > Maybe Maxime can better comment on it, but I think it was made exactly > > > for the purpose of not limiting the driver's design. For example, a > > > lot of drivers subclass drm_connector via drm_bridge_connector. If > > > struct drm_connector_hdmi was a wrapper around struct drm_connector, > > > then it would have been impossible to use HDMI helpers for bridge > > > drivers, while current design freely allows any driver to utilize > > > corresponding library code. > > > > That's exactly why we ended up like this. With that design, we wouldn't have > > been able to "inherit" two connector "classes": bridge_connector is one, > > intel_connector another one. > > > > See here for the rationale: > > https://lore.kernel.org/dri-devel/[email protected]/ > > > > I don't think the "but we'll bloat drm_connector" makes sense either. > > There's already a *lot* of things that aren't useful to every connector > > (fwnode, > > display_info, edid in general, scaling, vrr, etc.) > > > > And it's not like we allocate more than a handful of them during a system's > > life. > > So Are we okay with the approach mentioned here with the changes that have > been proposed here like > Having drm_writeback_connector in union with drm_hdmi_connector I don't think we need a union here. It artificially creates the same issue: we can't have two types for a connector if we do so. > Also one more thing I would like to clarify here is how everyone would > like the patches patches where each patch changes both the drm core > and all related drivers (ensures buildability but then review is tough > for each driver). Or patches where we have initial drm core changes > and then each patch does the all changes in a driver in its own > respective patch. The latter should be preferred, but if you can't maintain bisectability that way, then it's the most important and you should fall back to the former. Maxime signature.asc Description: PGP signature
