RE: [PATCH v2] drm: xlnx: zynqmp_dpsub: Enable plane in atomic update
> -Original Message-
> From: Tomi Valkeinen
> Sent: Monday, June 17, 2024 12:44 AM
> To: Klymenko, Anatoliy ; Laurent Pinchart
> ; Maarten Lankhorst
> ; Maxime Ripard
> ; Thomas Zimmermann ;
> David Airlie ; Daniel Vetter ;
> Simek, Michal
> Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org;
> linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v2] drm: xlnx: zynqmp_dpsub: Enable plane in atomic
> update
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> On 24/05/2024 02:49, Anatoliy Klymenko wrote:
> > Unconditionally enable the DPSUB layer in the corresponding atomic
> plane
> > update callback. Setting the new display mode may require disabling
> and
> > re-enabling the CRTC. This effectively resets DPSUB to the default state
> > with all layers disabled. The original implementation of the plane
> atomic
> > update enables the corresponding DPSUB layer only if the framebuffer
> > format has changed. This would leave the layer disabled after switching
> to
> > a different display mode with the same framebuffer format.
> >
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > Changes in v2:
> > - Added comment around DPSUB layer enablement explaining why it
> should be
> >done unconditionally.
> > - Link to v1: https://lore.kernel.org/r/20240520-dp-layer-enable-v1-1-
> c9b481209...@amd.com
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_kms.c | 10 +++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > index 43bf416b33d5..0b57ab5451a9 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > @@ -120,9 +120,13 @@ static void
> zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> > zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> > plane->state->alpha >> 8);
> >
> > - /* Enable or re-enable the plane if the format has changed. */
> > - if (format_changed)
> > - zynqmp_disp_layer_enable(layer);
> > + /*
> > + * Unconditionally enable the layer, as it may have been disabled
> > + * previously either explicitly to reconfigure layer format, or
> > + * implicitly after DPSUB reset during display mode change. DRM
> > + * framework calls this callback for enabled planes only.
> > + */
> > + zynqmp_disp_layer_enable(layer);
> > }
> >
> > static const struct drm_plane_helper_funcs
> zynqmp_dpsub_plane_helper_funcs = {
> >
> > ---
> > base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> > change-id: 20240520-dp-layer-enable-7b561af29ca8
> >
> > Best regards,
>
> Thanks, I have pushed this to drm-misc-next.
>
> Tomi
Thank you,
Anatoliy
RE: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic update
> -Original Message-
> From: Laurent Pinchart
> Sent: Wednesday, May 22, 2024 11:11 AM
> To: Klymenko, Anatoliy
> Cc: Tomi Valkeinen ; Maarten
> Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ;
> David Airlie ; Daniel Vetter ;
> Simek, Michal ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic
> update
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Wed, May 22, 2024 at 05:52:56PM +, Klymenko, Anatoliy wrote:
> > On Wednesday, May 22, 2024 8:32 AM, Laurent Pinchart wrote:
> > > On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko
> wrote:
> > > > Unconditionally enable the DPSUB layer in the corresponding
> atomic plane
> > > > update callback. Setting the new display mode may require
> disabling and
> > > > re-enabling the CRTC. This effectively resets DPSUB to the default
> state
> > > > with all layers disabled.
> > >
> > > Ah, I went through the code and I see that. Oops.
> > >
> > > > The original implementation of the plane atomic
> > > > update enables the corresponding DPSUB layer only if the
> framebuffer
> > > > format has changed. This would leave the layer disabled after
> switching to
> > > > a different display mode with the same framebuffer format.
> > >
> > > Do we need a Fixes: tag or has this issue been there since the
> beginning
> > > ?
> >
> > Yes, this was introduced in the initial driver commit.
> >
> > > > Signed-off-by: Anatoliy Klymenko
> > > > ---
> > > > drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
> > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > > index 43bf416b33d5..c4f038e34814 100644
> > > > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > > @@ -120,9 +120,8 @@ static void
> > > zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> > > > zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> > > > plane->state->alpha >>
> > > > 8);
> > > >
> > > > - /* Enable or re-enable the plane if the format has changed. */
> > > > - if (format_changed)
> > > > - zynqmp_disp_layer_enable(layer);
> > > > + /* Enable or re-enable the plane. */
> > > > + zynqmp_disp_layer_enable(layer);
> > >
> > > This should be safe for now, as the function will just write the plane
> > > registers with identical values. The waste of CPU cycles may not be a
> > > big issue, even if it would be best to avoid it.
> >
> > The CPU time wasted on doubling down layer enablement is
> neglectable
> > compared to DP link training time.
>
> Good point.
>
> > > What bothers me more is that we may be working around a larger
> > > problem.
> > > Resetting the CRTC when disabling it affects the hardware state of the
> > > whole device, and thus the state of all software DRM objects. The
> > > hardware and software states lose sync. Maybe this patch is all that is
> > > needed for now, but other similar issues could pop up in the future.
> >
> > I had similar thoughts about proper HW state tracking, but that would
> be
> > rather large rework.
> >
> > > Would it be possible, at atomic check time, to detect the objects
> whose
> > > hardware state will need to be synced, and marked that in their state
> ?
> > > Then in zynqmp_dpsub_plane_atomic_update() you could re-enable
> the
> > > layer
> > > based on that. You may need to subclass the drm_plane_state if
> there's
> > > no field in that structure that is suitable to store the information.
> > > The format_changed local variable would move there.
> >
> > Thank you for the idea! I'll check it out.
>
> If it ends up being overkill I think this patch is probably OK. A
> comment to explain the reasoning in the code would be nice though.
>
Sure, I'll add this.
> > > > }
> > > >
> > > > static const struct drm_plane_helper_funcs
> > > zynqmp_dpsub_plane_helper_funcs = {
> > > >
> > > > ---
> > > > base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> > > > change-id: 20240520-dp-layer-enable-7b561af29ca8
>
> --
> Regards,
>
> Laurent Pinchart
--
Thank you,
Anatoliy
RE: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic update
Hi Laurent,
Thanks a lot for the review.
> -Original Message-
> From: Laurent Pinchart
> Sent: Wednesday, May 22, 2024 8:32 AM
> To: Klymenko, Anatoliy
> Cc: Tomi Valkeinen ; Maarten
> Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ;
> David Airlie ; Daniel Vetter ;
> Simek, Michal ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic
> update
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko wrote:
> > Unconditionally enable the DPSUB layer in the corresponding atomic
> plane
> > update callback. Setting the new display mode may require disabling
> and
> > re-enabling the CRTC. This effectively resets DPSUB to the default state
> > with all layers disabled.
>
> Ah, I went through the code and I see that. Oops.
>
> > The original implementation of the plane atomic
> > update enables the corresponding DPSUB layer only if the framebuffer
> > format has changed. This would leave the layer disabled after switching
> to
> > a different display mode with the same framebuffer format.
>
> Do we need a Fixes: tag or has this issue been there since the beginning
> ?
Yes, this was introduced in the initial driver commit.
>
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > index 43bf416b33d5..c4f038e34814 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > @@ -120,9 +120,8 @@ static void
> zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> > zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> > plane->state->alpha >> 8);
> >
> > - /* Enable or re-enable the plane if the format has changed. */
> > - if (format_changed)
> > - zynqmp_disp_layer_enable(layer);
> > + /* Enable or re-enable the plane. */
> > + zynqmp_disp_layer_enable(layer);
>
> This should be safe for now, as the function will just write the plane
> registers with identical values. The waste of CPU cycles may not be a
> big issue, even if it would be best to avoid it.
>
The CPU time wasted on doubling down layer enablement is neglectable
compared to DP link training time.
> What bothers me more is that we may be working around a larger
> problem.
> Resetting the CRTC when disabling it affects the hardware state of the
> whole device, and thus the state of all software DRM objects. The
> hardware and software states lose sync. Maybe this patch is all that is
> needed for now, but other similar issues could pop up in the future.
>
I had similar thoughts about proper HW state tracking, but that would be
rather large rework.
> Would it be possible, at atomic check time, to detect the objects whose
> hardware state will need to be synced, and marked that in their state ?
> Then in zynqmp_dpsub_plane_atomic_update() you could re-enable the
> layer
> based on that. You may need to subclass the drm_plane_state if there's
> no field in that structure that is suitable to store the information.
> The format_changed local variable would move there.
>
Thank you for the idea! I'll check it out.
> > }
> >
> > static const struct drm_plane_helper_funcs
> zynqmp_dpsub_plane_helper_funcs = {
> >
> > ---
> > base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> > change-id: 20240520-dp-layer-enable-7b561af29ca8
>
> --
> Regards,
>
> Laurent Pinchart
--
Thank you,
Anatoliy
RE: [PATCH] drm: xlnx: zynqmp_disp: Fix WARN_ON build warning
Hi Laurent and Palmer,
> -Original Message-
> From: Laurent Pinchart
> Sent: Wednesday, May 22, 2024 7:44 AM
> To: Palmer Dabbelt
> Cc: tomi.valkei...@ideasonboard.com;
> maarten.lankho...@linux.intel.com; mrip...@kernel.org;
> tzimmerm...@suse.de; airl...@gmail.com; dan...@ffwll.ch; Simek,
> Michal ; dri-devel@lists.freedesktop.org;
> linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org;
> kernel test robot ; Klymenko, Anatoliy
>
> Subject: Re: [PATCH] drm: xlnx: zynqmp_disp: Fix WARN_ON build
> warning
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Hi Palmer,
>
> (CC'ing Anatoliy)
>
> Thank you for the patch.
>
> On Tue, May 21, 2024 at 07:28:15AM -0700, Palmer Dabbelt wrote:
> > From: Palmer Dabbelt
> >
> > Without this I get warnings along the lines of
> >
> > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: error: logical not is
> only applied to the left hand side of this comparison [-Werror,-Wlogical-
> not-parentheses]
> > 949 | if (WARN_ON(!layer->mode ==
> ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > | ^~~
> > arch/s390/include/asm/bug.h:54:25: note: expanded from macro
> 'WARN_ON'
> >54 | int __ret_warn_on = !!(x); \
> > |^
> > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses
> after the '!' to evaluate the comparison first
> > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses
> around left hand side expression to silence this warning
> >
> > which get promoted to errors in my test builds. Adding the suggested
> > parens elides those warnings.
>
> I think this should have
>
> Fixes: b0f0469ab662 ("drm: xlnx: zynqmp_dpsub: Anounce supported
> input formats")
>
> > Reported-by: kernel test robot
> > Closes: https://lore.kernel.org/oe-kbuild-all/202405080553.tfH9EmS8-
> l...@intel.com/
> > Signed-off-by: Palmer Dabbelt
> > ---
> > I couldn't find a patch for this in Linus' tree or on the lists, sorry
> > if someone's already fixed it. No rush on my end, I'll just stash this
> > in a local branch for the tester.
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index 13157da0089e..d37b4a9c99ea 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -981,7 +981,7 @@ u32 *zynqmp_disp_layer_drm_formats(struct
> zynqmp_disp_layer *layer,
> > unsigned int i;
> > u32 *formats;
> >
> > - if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > + if (WARN_ON((!layer->mode) == ZYNQMP_DPSUB_LAYER_NONLIVE))
> {
>
> That doesn't seem right. layer->mode isn't a boolean, it's an enum. The
> right fix seems to be
>
> if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE)) {
>
Yes, this is how it should be.
> Anatoliy, could you check this ? Palmer, do you plan to submit a new
> version of the patch, or should I send the right fix separately ?
>
Actually, this has been fixed here:
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c72211751870ffa2cff5d91834059456cfa7cbd5.
Looks like the fix just missed the merge window.
> > *num_formats = 0;
> > return NULL;
> > }
>
> --
> Regards,
>
> Laurent Pinchart
--
Thank you,
Anatoliy
RE: [PATCH v2 2/2] drm: xlnx: zynqmp_dpsub: Fix compilation error
> -Original Message-
> From: Tomi Valkeinen
> Sent: Thursday, April 25, 2024 11:36 PM
> To: Klymenko, Anatoliy
> Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org;
> linux-ker...@vger.kernel.org; kernel test robot ; Laurent
> Pinchart ; Maarten Lankhorst
> ; Maxime Ripard
> ; Thomas Zimmermann ;
> David Airlie ; Daniel Vetter ;
> Simek, Michal
> Subject: Re: [PATCH v2 2/2] drm: xlnx: zynqmp_dpsub: Fix compilation
> error
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On 26/04/2024 04:46, Anatoliy Klymenko wrote:
> > Fix W=1 clang 19 compilation error in
> zynqmp_disp_layer_drm_formats().
> >
> > Reported-by: kernel test robot
> > Closes: https://lore.kernel.org/oe-kbuild-all/202404260946.4oZXvHD2-
> l...@intel.com/
> > ---
>
> This is missing your signed-off-by.
>
> Tomi
Thank you!
>
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index 423f5f4943cc..c9fb432d4cbd 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -981,7 +981,7 @@ u32 *zynqmp_disp_layer_drm_formats(struct
> zynqmp_disp_layer *layer,
> > unsigned int i;
> > u32 *formats;
> >
> > - if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > + if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > *num_formats = 0;
> > return NULL;
> > }
> >
RE: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
> -Original Message-
> From: Tomi Valkeinen
> Sent: Friday, April 5, 2024 5:31 AM
> To: Klymenko, Anatoliy
> Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org;
> linux-ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> me...@vger.kernel.org; Laurent Pinchart
> ; Maarten Lankhorst
> ; Maxime Ripard
> ; Thomas Zimmermann ;
> David Airlie ; Daniel Vetter ;
> Simek, Michal ; Andrzej Hajda
> ; Neil Armstrong
> ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec
> ; Rob Herring ;
> Krzysztof Kozlowski ; Conor Dooley
> ; Mauro Carvalho Chehab
>
> Subject: Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode
> during creation
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On 21/03/2024 22:43, Anatoliy Klymenko wrote:
> > Set layer mode of operation (live or dma-based) during layer creation.
> >
> > Each DPSUB layer mode of operation is defined by corresponding DT
> node port
> > connection, so it is possible to assign it during layer object creation.
> > Previously it was set in layer enable functions, although it is too late
> > as setting layer format depends on layer mode, and should be done
> before
> > given layer enabled.
> >
> > Signed-off-by: Anatoliy Klymenko
> > Reviewed-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 20
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> > 4 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index 8a39b3accce5..e6d26ef60e89 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -64,6 +64,16 @@
> >
> > #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
> >
> > +/**
> > + * enum zynqmp_dpsub_layer_mode - Layer mode
> > + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> > + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> > + */
> > +enum zynqmp_dpsub_layer_mode {
> > + ZYNQMP_DPSUB_LAYER_NONLIVE,
> > + ZYNQMP_DPSUB_LAYER_LIVE,
> > +};
> > +
> > /**
> >* struct zynqmp_disp_format - Display subsystem format information
> >* @drm_fmt: DRM format (4CC)
> > @@ -902,15 +912,12 @@ u32
> *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> > /**
> >* zynqmp_disp_layer_enable - Enable a layer
> >* @layer: The layer
> > - * @mode: Operating mode of layer
> >*
> >* Enable the @layer in the audio/video buffer manager and the
> blender. DMA
> >* channels are started separately by zynqmp_disp_layer_update().
> >*/
> > -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> > - enum zynqmp_dpsub_layer_mode mode)
> > +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
> > {
> > - layer->mode = mode;
> > zynqmp_disp_avbuf_enable_video(layer->disp, layer);
> > zynqmp_disp_blend_layer_enable(layer->disp, layer);
> > }
> > @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
> > layer->id = i;
> > layer->disp = disp;
> > layer->info = &layer_info[i];
> > + /* For now assume dpsub works in either live or non-live
> mode for both layers.
> > + * Hybrid mode is not supported yet.
> > + */
>
> This comment style is not according to the style guide, and in fact you
> fix it in the patch 4. So please fix it here instead.
>
Thanks for catching it.
> Tomi
>
> > + layer->mode = disp->dpsub->dma_enabled ?
> ZYNQMP_DPSUB_LAYER_NONLIVE
> > +:
> > ZYNQMP_DPSUB_LAYER_LIVE;
> >
> > ret = zynqmp_disp_layer_request_dma(disp, layer);
> > if (ret)
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > index 123cffac08be..9b8b202224d9 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> > @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
> > ZYNQMP_DPSUB_LAYER_GFX,
> > };
> >
> > -/**
> > - * enum
RE: [PATCH v3 2/9] drm: xlnx: zynqmp_dpsub: Update live format defines
> -Original Message- > From: Tomi Valkeinen > Sent: Friday, April 5, 2024 5:10 AM > To: Klymenko, Anatoliy > Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; > linux-ker...@vger.kernel.org; devicet...@vger.kernel.org; linux- > me...@vger.kernel.org; Laurent Pinchart > ; Maarten Lankhorst > ; Maxime Ripard > ; Thomas Zimmermann ; > David Airlie ; Daniel Vetter ; > Simek, Michal ; Andrzej Hajda > ; Neil Armstrong > ; Robert Foss ; Jonas > Karlman ; Jernej Skrabec > ; Rob Herring ; > Krzysztof Kozlowski ; Conor Dooley > ; Mauro Carvalho Chehab > > Subject: Re: [PATCH v3 2/9] drm: xlnx: zynqmp_dpsub: Update live format > defines > > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > On 21/03/2024 22:43, Anatoliy Klymenko wrote: > > Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG > register > > layout. > > I think this description needs a bit more. Mention that the defines are > not currently used, so we can change them like this without any other > change. > Makes sense. I'll update this. > Tomi > > > Reviewed-by: Laurent Pinchart > > Signed-off-by: Anatoliy Klymenko > > --- > > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > > index f92a006d5070..fa3935384834 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > > @@ -165,10 +165,10 @@ > > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 > > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 > > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK > GENMASK(2, 0) > > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 > > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x1 > > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x2 > > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 > > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 > << 4) > > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444(0x1 << > 4) > > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422(0x2 << > 4) > > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << > 4) > > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK > GENMASK(5, 4) > > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) > > #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400 > >
RE: [PATCH v2 4/4] drm: xlnx: zynqmp_dpsub: Add DP audio support
Hi Tomi,
> -Original Message-
> From: Tomi Valkeinen
> Sent: Tuesday, March 19, 2024 1:23 AM
> To: Lars-Peter Clausen ; Jaroslav Kysela
> ; Takashi Iwai ; Liam Girdwood
> ; Mark Brown ; Laurent
> Pinchart ; Maarten Lankhorst
> ; Maxime Ripard
> ; Thomas Zimmermann ;
> David Airlie ; Daniel Vetter ; Rob
> Herring ; Krzysztof Kozlowski
> ; Conor Dooley
> ; Simek, Michal
> Cc: linux-so...@vger.kernel.org; linux-ker...@vger.kernel.org; dri-
> de...@lists.freedesktop.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; Sagar, Vishal ;
> Klymenko, Anatoliy ; Péter Ujfalusi
> ; Tomi Valkeinen
>
> Subject: [PATCH v2 4/4] drm: xlnx: zynqmp_dpsub: Add DP audio support
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Add basic DisplayPort audio support.
>
> Support non-live audio playback from two PCMs (DMA channels), and
> the
> volume control in the audio mixer.
>
> As older dtb files may not have the audio DMA channels defined, the
> driver will just mark the audio support as disabled if the audio DMA is
> missing, and will continue with only display support.
>
> Note: Reset doesn't seem to work (ZYNQMP_DISP_AUD_SOFT_RESET). If
> we do
> a reset, audio playback won't start again even if, afaics, we do set up
> all the necessary registers. So, at the moment, resetting the audio
> block in dp_dai_hw_free() is commented out.
>
> Signed-off-by: Tomi Valkeinen
Tested-by: Anatoliy Klymenko
> ---
> drivers/gpu/drm/xlnx/Kconfig| 9 +
> drivers/gpu/drm/xlnx/Makefile | 1 +
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 50
> drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 7 +-
> drivers/gpu/drm/xlnx/zynqmp_dp.c| 54 ++--
> drivers/gpu/drm/xlnx/zynqmp_dp.h| 7 +
> drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461
>
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 39 +--
> drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 15 +-
> 9 files changed, 540 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> index 68ee897de9d7..d88cfbaf2863 100644
> --- a/drivers/gpu/drm/xlnx/Kconfig
> +++ b/drivers/gpu/drm/xlnx/Kconfig
> @@ -15,3 +15,12 @@ config DRM_ZYNQMP_DPSUB
> This is a DRM/KMS driver for ZynqMP DisplayPort controller.
> Choose
> this option if you have a Xilinx ZynqMP SoC with DisplayPort
> subsystem.
> +
> +config DRM_ZYNQMP_DPSUB_AUDIO
> + bool "ZynqMP DisplayPort Audio Support"
> + depends on DRM_ZYNQMP_DPSUB
> + depends on SND && SND_SOC
> + select SND_SOC_GENERIC_DMAENGINE_PCM
> + help
> + Choose this option to enable DisplayPort audio support in the
> ZynqMP
> + DisplayPort driver.
> diff --git a/drivers/gpu/drm/xlnx/Makefile
> b/drivers/gpu/drm/xlnx/Makefile
> index ea1422a39502..ab6e2ffd7e8d 100644
> --- a/drivers/gpu/drm/xlnx/Makefile
> +++ b/drivers/gpu/drm/xlnx/Makefile
> @@ -1,2 +1,3 @@
> zynqmp-dpsub-y := zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
> zynqmp_kms.o
> +zynqmp-dpsub-$(CONFIG_DRM_ZYNQMP_DPSUB_AUDIO) +=
> zynqmp_dp_audio.o
> obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 407bc07cec69..d2bf0e2d0135 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -130,7 +130,6 @@ struct zynqmp_disp_layer {
> * @dpsub: Display subsystem
> * @blend.base: Register I/O base address for the blender
> * @avbuf.base: Register I/O base address for the audio/video buffer
> manager
> - * @audio.base: Registers I/O base address for the audio mixer
> * @layers: Layers (planes)
> */
> struct zynqmp_disp {
> @@ -143,9 +142,6 @@ struct zynqmp_disp {
> struct {
> void __iomem *base;
> } avbuf;
> - struct {
> - void __iomem *base;
> - } audio;
>
> struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS];
> };
> @@ -807,42 +803,6 @@ static void
> zynqmp_disp_blend_layer_disable(struct zynqmp_disp *disp,
> csc_zero_offsets);
> }
>
> -/*
> -
> - * Audio Mixer
> - */
> -
> -static void zynqmp_disp_audio_write(struct zynqmp_disp *disp, int reg,
> u32 val)
> -{
> - writel(val, disp->audio.base + reg);
> -}
> -
> -/**
> - * zynqmp_disp_audio_enable - Enab
RE: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings
> -Original Message- > From: Conor Dooley > Sent: Friday, March 29, 2024 8:47 AM > To: Klymenko, Anatoliy > Cc: Krzysztof Kozlowski ; Laurent Pinchart > ; Maarten Lankhorst > ; Maxime Ripard > ; Thomas Zimmermann ; > David Airlie ; Daniel Vetter ; Simek, > Michal ; Andrzej Hajda > ; Neil Armstrong ; > Robert Foss ; Jonas Karlman ; Jernej > Skrabec ; Rob Herring ; > Krzysztof Kozlowski ; Conor Dooley > ; Mauro Carvalho Chehab ; > Tomi Valkeinen ; dri- > de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; devicet...@vger.kernel.org; linux- > me...@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > On Fri, Mar 29, 2024 at 12:38:33AM +, Klymenko, Anatoliy wrote: > > Thank you for the feedback. > > > From: Krzysztof Kozlowski > > > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > > >> From: Krzysztof Kozlowski > > > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > > > >>> b/include/dt- > > > >> bindings/media/media-bus-format.h > > > >>> new file mode 100644 > > > >>> index ..60fc6e11dabc > > > >>> --- /dev/null > > > >>> +++ b/include/dt-bindings/media/media-bus-format.h > > > >>> @@ -0,0 +1,177 @@ > > > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > > >>> +/* > > > >>> + * Media Bus API header > > > >>> + * > > > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > > > >>> + > > > >>> + * > > > >>> + * This program is free software; you can redistribute it and/or > > > >>> +modify > > > >>> + * it under the terms of the GNU General Public License version 2 > > > >>> +as > > > >>> + * published by the Free Software Foundation. > > > >> > > > >> That's not true. Your SPDX tells something entirely different. > > > >> > > > > > > > > Thank you - I'll see how to fix it. > > > > > > > >> Anyway, you did not explain why you need to copy anything anywhere. > > > >> > > > >> Specifically, random hex values *are not bindings*. > > > >> > > > > > > > > The same media bus format values are being used by the reference > > > > driver in patch #9. And, as far as I know, we cannot use headers from > > > > Linux API headers directly (at least I > > > > > > I don't understand what does it mean. You can use in your driver > whatever > > > headers you wish, I don't care about them. > > > > > > > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > > > What would be the best approach to reusing the same defines on DT and > > > driver sides from your point of view? Symlink maybe? > > > > > > > > > > Wrap your messages to match mailing list discussion style. There are no > > > defines used in DT. If there are, show me them in *THIS* or other > > > *upstreamed* (being upstreamed) patchset. > > > > > > > Sorry, I didn't explain properly what I'm trying to achieve. I need to > > create a DT node property that represents video signal format, one of > > MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would > be > > nice to reuse the same symbolic values in the device tree. What is the > > best approach here? Should I create a separate header in > > include/dt-bindings with the same or similar (to avoid multiple > > definition errors) defines, or is it better to create a symlink to > > media-bus-format.h like include/dt-bindings/linux-event-codes.h? > > Isn't there already a property for this, described in > Documentation/devicetree/bindings/media/xilinx/video.txt > ? Those properties are very similar, indeed but not exactly the same. The one you noticed maps Xilinx video data format on AXI4-Stream transport that covers color space and chroma subsampling only. The rest of the signal attributes are either conventional or left out. MEDIA_BUS_FMT_* collection is more specific and embeds additional information about video signals, like bits per component and component ordering.
RE: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings
Hi Krzysztof, Thank you for the feedback. > -Original Message- > From: Krzysztof Kozlowski > Sent: Saturday, March 23, 2024 3:21 AM > To: Klymenko, Anatoliy ; Laurent Pinchart > ; Maarten Lankhorst > ; Maxime Ripard > ; Thomas Zimmermann ; > David Airlie ; Daniel Vetter ; Simek, > Michal ; Andrzej Hajda > ; Neil Armstrong ; > Robert Foss ; Jonas Karlman ; Jernej > Skrabec ; Rob Herring ; > Krzysztof Kozlowski ; Conor Dooley > ; Mauro Carvalho Chehab > Cc: Tomi Valkeinen ; dri- > de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; devicet...@vger.kernel.org; linux- > me...@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > On 22/03/2024 20:12, Klymenko, Anatoliy wrote: > > Hi Krzysztof, > > > > Thanks a lot for the review. > > > >> -Original Message----- > >> From: Krzysztof Kozlowski > >> Sent: Thursday, March 21, 2024 10:59 PM > >> To: Klymenko, Anatoliy ; Laurent Pinchart > >> ; Maarten Lankhorst > >> ; Maxime Ripard > >> ; Thomas Zimmermann ; > David > >> Airlie ; Daniel Vetter ; Simek, > >> Michal ; Andrzej Hajda > >> ; Neil Armstrong > >> ; Robert Foss ; Jonas > >> Karlman ; Jernej Skrabec > ; > >> Rob Herring ; Krzysztof Kozlowski > >> ; Conor Dooley > >> ; Mauro Carvalho Chehab > >> Cc: Tomi Valkeinen ; dri- > >> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; > >> linux- ker...@vger.kernel.org; devicet...@vger.kernel.org; linux- > >> me...@vger.kernel.org > >> Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG > >> bindings > >> > >> Caution: This message originated from an External Source. Use proper > >> caution when opening attachments, clicking links, or responding. > >> > >> > >> On 21/03/2024 21:43, Anatoliy Klymenko wrote: > >>> diff --git a/include/dt-bindings/media/media-bus-format.h > >>> b/include/dt- > >> bindings/media/media-bus-format.h > >>> new file mode 100644 > >>> index ..60fc6e11dabc > >>> --- /dev/null > >>> +++ b/include/dt-bindings/media/media-bus-format.h > >>> @@ -0,0 +1,177 @@ > >>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > >>> +/* > >>> + * Media Bus API header > >>> + * > >>> + * Copyright (C) 2009, Guennadi Liakhovetski > >>> + > >>> + * > >>> + * This program is free software; you can redistribute it and/or > >>> +modify > >>> + * it under the terms of the GNU General Public License version 2 > >>> +as > >>> + * published by the Free Software Foundation. > >> > >> That's not true. Your SPDX tells something entirely different. > >> > > > > Thank you - I'll see how to fix it. > > > >> Anyway, you did not explain why you need to copy anything anywhere. > >> > >> Specifically, random hex values *are not bindings*. > >> > > > > The same media bus format values are being used by the reference > > driver in patch #9. And, as far as I know, we cannot use headers from > > Linux API headers directly (at least I > > I don't understand what does it mean. You can use in your driver whatever > headers you wish, I don't care about them. > > > noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). > What would be the best approach to reusing the same defines on DT and > driver sides from your point of view? Symlink maybe? > > > > Wrap your messages to match mailing list discussion style. There are no > defines used in DT. If there are, show me them in *THIS* or other > *upstreamed* (being upstreamed) patchset. > Sorry, I didn't explain properly what I'm trying to achieve. I need to create a DT node property that represents video signal format, one of MEDIA_BUS_FMT_* from include/uapi/linux/media-bus-format.h. It would be nice to reuse the same symbolic values in the device tree. What is the best approach here? Should I create a separate header in include/dt-bindings with the same or similar (to avoid multiple definition errors) defines, or is it better to create a symlink to media-bus-format.h like include/dt-bindings/linux-event-codes.h? > Whatever you have out of tree or "DO NOT MERGE" does not matter and > does not justify anything. > > > Best regards, > Krzysztof Thank you, Anatoliy
RE: [PATCH v3 7/9] drm/atomic-helper: Add select_output_bus_format callback
Hi Maxime,
Thank you for the review.
> -Original Message-
> From: Maxime Ripard
> Sent: Friday, March 22, 2024 2:45 AM
> To: Klymenko, Anatoliy
> Cc: Laurent Pinchart ; Maarten Lankhorst
> ; Thomas Zimmermann
> ; David Airlie ; Daniel Vetter
> ; Simek, Michal ; Andrzej Hajda
> ; Neil Armstrong ; Robert
> Foss ; Jonas Karlman ; Jernej Skrabec
> ; Rob Herring ; Krzysztof
> Kozlowski ; Conor Dooley
> ; Mauro Carvalho Chehab ; Tomi
> Valkeinen ; dri-devel@lists.freedesktop.org;
> linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org;
> devicet...@vger.kernel.org; linux-me...@vger.kernel.org
> Subject: Re: [PATCH v3 7/9] drm/atomic-helper: Add select_output_bus_format
> callback
>
> On Thu, Mar 21, 2024 at 01:43:45PM -0700, Anatoliy Klymenko wrote:
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> > b/drivers/gpu/drm/drm_crtc_helper.c
> > index 2dafc39a27cb..f2e12a3c4e5f 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -1055,3 +1055,39 @@ int drm_helper_force_disable_all(struct
> drm_device *dev)
> > return ret;
> > }
> > EXPORT_SYMBOL(drm_helper_force_disable_all);
> > +
> > +/**
> > + * drm_helper_crtc_select_output_bus_format - Select output media bus
> > +format
> > + * @crtc: The CRTC to query
> > + * @crtc_state: The new CRTC state
> > + * @supported_fmts: List of media bus format options to pick from
> > + * @num_supported_fmts: Number of media bus formats in
> > +@supported_fmts list
> > + *
> > + * Encoder drivers may call this helper to give the connected CRTC a
> > +chance to
> > + * select compatible or preffered media bus format to use over the
> > +CRTC encoder
> > + * link. Encoders should provide list of supported input
> > +MEDIA_BUS_FMT_* for
> > + * CRTC to pick from. CRTC driver is expected to select preferred
> > +media bus
> > + * format from the list and, once enabled, generate the signal accordingly.
> > + *
> > + * Returns:
> > + * Selected preferred media bus format or 0 if CRTC does not support
> > +any from
> > + * @supported_fmts list.
> > + */
> > +u32 drm_helper_crtc_select_output_bus_format(struct drm_crtc *crtc,
> > +struct drm_crtc_state *crtc_state,
> > +const u32 *supported_fmts,
> > +unsigned int num_supported_fmts) {
> > + if (!crtc || !supported_fmts || !num_supported_fmts)
> > + return 0;
> > +
> > + if (!crtc->helper_private ||
> > + !crtc->helper_private->select_output_bus_format)
> > + return supported_fmts[0];
> > +
> > + return crtc->helper_private->select_output_bus_format(crtc,
> > + crtc_state,
> > + supported_fmts,
> > + num_supported_fmts);
> > +}
>
> Again, the output of that selection must be found in the CRTC state, otherwise
> CRTCs have no way to know which have been selected.
>
Yes, now I got it - thank you. I'll fix this in the next version.
> Maxime
Thank you,
Anatoliy
RE: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings
Hi Krzysztof, Thanks a lot for the review. > -Original Message- > From: Krzysztof Kozlowski > Sent: Thursday, March 21, 2024 10:59 PM > To: Klymenko, Anatoliy ; Laurent Pinchart > ; Maarten Lankhorst > ; Maxime Ripard ; > Thomas Zimmermann ; David Airlie > ; Daniel Vetter ; Simek, Michal > ; Andrzej Hajda ; Neil > Armstrong ; Robert Foss ; Jonas > Karlman ; Jernej Skrabec ; Rob > Herring ; Krzysztof Kozlowski > ; Conor Dooley ; > Mauro Carvalho Chehab > Cc: Tomi Valkeinen ; dri- > de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org; devicet...@vger.kernel.org; linux- > me...@vger.kernel.org > Subject: Re: [PATCH v3 8/9] dt-bindings: xlnx: Add VTC and TPG bindings > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On 21/03/2024 21:43, Anatoliy Klymenko wrote: > > diff --git a/include/dt-bindings/media/media-bus-format.h b/include/dt- > bindings/media/media-bus-format.h > > new file mode 100644 > > index ..60fc6e11dabc > > --- /dev/null > > +++ b/include/dt-bindings/media/media-bus-format.h > > @@ -0,0 +1,177 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */ > > +/* > > + * Media Bus API header > > + * > > + * Copyright (C) 2009, Guennadi Liakhovetski > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > That's not true. Your SPDX tells something entirely different. > Thank you - I'll see how to fix it. > Anyway, you did not explain why you need to copy anything anywhere. > > Specifically, random hex values *are not bindings*. > The same media bus format values are being used by the reference driver in patch #9. And, as far as I know, we cannot use headers from Linux API headers directly (at least I noticed the same pattern in ../dt-bindings/sdtv-standarts.h for instance). What would be the best approach to reusing the same defines on DT and driver sides from your point of view? Symlink maybe? > Best regards, > Krzysztof Thank you, Anatoliy
RE: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
Hi Laurent,
Thanks a lot for your review.
> -Original Message-
> From: Laurent Pinchart
> Sent: Monday, March 18, 2024 5:35 PM
> To: Klymenko, Anatoliy
> Cc: Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ; Simek, Michal
> ; Andrzej Hajda ; Neil
> Armstrong ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec ; Rob
> Herring ; Krzysztof Kozlowski
> ; Conor Dooley ;
> Mauro Carvalho Chehab ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> me...@vger.kernel.org
> Subject: Re: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Tue, Mar 12, 2024 at 05:55:02PM -0700, Anatoliy Klymenko wrote:
> > Program live video input format according to selected media bus format.
> >
> > In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
> > almost certainly supports a single media bus format as its output.
> > Expect this to be delivered via the new bridge atomic state. Program
> > DPSUB registers accordingly.
>
> No line breaks within paragraphs. Add a blank line if you want to paragraphs,
> remove the line break otherwise.
>
Got it - will fix. Thank you.
> > Update zynqmp_disp_layer_set_format() API to fit both live and
> > non-live layer types.
> >
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 93
> > +++---
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +-
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 --
> > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> > 4 files changed, 87 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index dd48fa60fa9a..0cacd597f4b8 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -383,7 +383,7 @@ static const struct zynqmp_disp_format
> avbuf_live_fmts[] = {
> > ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> > .sf = scaling_factors_666,
> > }, {
> > - .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X24,
> > + .bus_fmt= MEDIA_BUS_FMT_RBG888_1X24,
>
> Does this belong to a previous patch ?
Yep, slipped between my fingers during the rebase. I will update this in the
next version.
>
> > .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> > ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> > .sf = scaling_factors_888,
> > @@ -433,19 +433,28 @@ static void zynqmp_disp_avbuf_set_format(struct
> zynqmp_disp *disp,
> >const struct zynqmp_disp_format
> > *fmt) {
> > unsigned int i;
> > - u32 val;
> > + u32 val, reg;
> >
> > - val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
> > - val &= zynqmp_disp_layer_is_video(layer)
> > - ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
> > - : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
> > - val |= fmt->buf_fmt;
> > - zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val);
> > + layer->disp_fmt = fmt;
> > + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
> > + reg = ZYNQMP_DISP_AV_BUF_FMT;
> > + val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
> > + val &= zynqmp_disp_layer_is_video(layer)
> > + ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
> > + : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
> > + val |= fmt->buf_fmt;
> > + } else {
> > + reg = zynqmp_disp_layer_is_video(layer)
> > + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
> > + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
> > + val = fmt->buf_fmt;
> > + }
> > + zynqmp_disp_avbuf_write(disp, reg, val);
> >
> > for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
> > - unsigned int reg = zynqmp_disp_layer_is_video(layer)
> > - ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
> > - : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
> > + reg = zynqmp_disp_layer_is_video(laye
RE: [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag
Hi Laurent,
Thank you for the review.
> -Original Message-
> From: Laurent Pinchart
> Sent: Monday, March 18, 2024 5:13 PM
> To: Klymenko, Anatoliy
> Cc: Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ; Simek, Michal
> ; Andrzej Hajda ; Neil
> Armstrong ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec ; Rob
> Herring ; Krzysztof Kozlowski
> ; Conor Dooley ;
> Mauro Carvalho Chehab ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> me...@vger.kernel.org
> Subject: Re: [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global
> flag
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Tue, Mar 12, 2024 at 05:55:01PM -0700, Anatoliy Klymenko wrote:
> > Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer
> > context. This flag signals whether the driver runs in DRM CRTC or DRM
> > bridge mode, assuming that all display layers share the same live or
> > non-live mode of operation. Using per-layer mode instead of global
> > flag will siplify future support of the hybrid scenario.
>
> s/siplify/simplify/
>
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 11 ---
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index af851190f447..dd48fa60fa9a 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -975,7 +975,7 @@ void zynqmp_disp_layer_disable(struct
> > zynqmp_disp_layer *layer) {
> > unsigned int i;
> >
> > - if (layer->disp->dpsub->dma_enabled) {
> > + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
> > for (i = 0; i < layer->drm_fmt->num_planes; i++)
> > dmaengine_terminate_sync(layer->dmas[i].chan);
> > }
> > @@ -1001,7 +1001,7 @@ void zynqmp_disp_layer_set_format(struct
> > zynqmp_disp_layer *layer,
> >
> > zynqmp_disp_avbuf_set_format(layer->disp, layer,
> > layer->disp_fmt);
> >
> > - if (!layer->disp->dpsub->dma_enabled)
> > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> > return;
> >
> > /*
> > @@ -1039,7 +1039,7 @@ int zynqmp_disp_layer_update(struct
> zynqmp_disp_layer *layer,
> > const struct drm_format_info *info = layer->drm_fmt;
> > unsigned int i;
> >
> > - if (!layer->disp->dpsub->dma_enabled)
> > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> > return 0;
>
> The above changes look nice.
>
> >
> > for (i = 0; i < info->num_planes; i++) { @@ -1089,7 +1089,7 @@
> > static void zynqmp_disp_layer_release_dma(struct zynqmp_disp *disp, {
> > unsigned int i;
> >
> > - if (!layer->info || !disp->dpsub->dma_enabled)
> > + if (!layer->info)
>
> This, however, doesn't seem right, as this function is called unconditionally
> from
> the remove path. The change below seems weird too.
> If I'm missing something, it should at least be explained in the commit
> message.
>
Actually, this whole condition should be removed, as now we're setting
layer.info for all types of layers. On top of that, we're setting the number of
DMA channels to zero for the live layers, which in turn prevents any DMA
channel initialization or release. You are right - that probably should be
mentioned explicitly in the commit message. I'll update it.
> > return;
> >
> > for (i = 0; i < layer->info->num_channels; i++) { @@ -1132,9
> > +1132,6 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp
> *disp,
> > unsigned int i;
> > int ret;
> >
> > - if (!disp->dpsub->dma_enabled)
> > - return 0;
> > -
> > for (i = 0; i < layer->info->num_channels; i++) {
> > struct zynqmp_disp_layer_dma *dma = &layer->dmas[i];
> > char dma_channel_name[16];
> >
>
> --
> Regards,
>
> Laurent Pinchart
Thank you,
Anatoliy
RE: [PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
Hi Laurent,
Thanks a lot for the review.
> -Original Message-
> From: Laurent Pinchart
> Sent: Monday, March 18, 2024 5:05 PM
> To: Klymenko, Anatoliy
> Cc: Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ; Simek, Michal
> ; Andrzej Hajda ; Neil
> Armstrong ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec ; Rob
> Herring ; Krzysztof Kozlowski
> ; Conor Dooley ;
> Mauro Carvalho Chehab ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> me...@vger.kernel.org
> Subject: Re: [PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input
> formats
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Tue, Mar 12, 2024 at 05:55:00PM -0700, Anatoliy Klymenko wrote:
> > DPSUB in bridge mode supports multiple input media bus formats.
> >
> > Announce the list of supported input media bus formats via
> > drm_bridge.atomic_get_input_bus_fmts callback.
> > Introduce a set of live input formats, supported by DPSUB.
> > Rename zynqmp_disp_layer_drm_formats() to zynqmp_disp_layer_formats()
> > to reflect semantics for both live and non-live layer format lists.
> >
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 67
> > +-
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 4 +--
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 26 +++
> > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> > 4 files changed, 88 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index e6d26ef60e89..af851190f447 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -18,6 +18,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -77,12 +78,16 @@ enum zynqmp_dpsub_layer_mode {
> > /**
> > * struct zynqmp_disp_format - Display subsystem format information
> > * @drm_fmt: DRM format (4CC)
> > + * @bus_fmt: Media bus format
> > * @buf_fmt: AV buffer format
> > * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
> > * @sf: Scaling factors for color components
> > */
> > struct zynqmp_disp_format {
> > - u32 drm_fmt;
> > + union {
> > + u32 drm_fmt;
> > + u32 bus_fmt;
> > + };
>
> I'm not a big fan of the union, but I can live with it.
>
I'm trying to represent the duality of the layer formats - non-live described
by the DRM fourcc, and live by the bus format.
> > u32 buf_fmt;
> > bool swap;
> > const u32 *sf;
> > @@ -182,6 +187,12 @@ static const u32 scaling_factors_565[] = {
> > ZYNQMP_DISP_AV_BUF_5BIT_SF,
> > };
> >
> > +static const u32 scaling_factors_666[] = {
> > + ZYNQMP_DISP_AV_BUF_6BIT_SF,
> > + ZYNQMP_DISP_AV_BUF_6BIT_SF,
> > + ZYNQMP_DISP_AV_BUF_6BIT_SF,
> > +};
> > +
> > static const u32 scaling_factors_888[] = {
> > ZYNQMP_DISP_AV_BUF_8BIT_SF,
> > ZYNQMP_DISP_AV_BUF_8BIT_SF,
> > @@ -364,6 +375,36 @@ static const struct zynqmp_disp_format
> avbuf_gfx_fmts[] = {
> > },
> > };
> >
> > +/* List of live video layer formats */ static const struct
> > +zynqmp_disp_format avbuf_live_fmts[] = {
> > + {
> > + .bus_fmt= MEDIA_BUS_FMT_RGB666_1X18,
> > + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 |
> > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> > + .sf = scaling_factors_666,
> > + }, {
> > + .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X24,
> > + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> > + .sf = scaling_factors_888,
> > + }, {
> > + .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16,
> > + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> > + .sf = scaling_factors_888,
> > + }, {
> > +
RE: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver
Hi Maxime,
Thank you for the review.
> -Original Message-
> From: Maxime Ripard
> Sent: Thursday, March 14, 2024 5:05 AM
> To: Klymenko, Anatoliy
> Cc: Laurent Pinchart ; Maarten Lankhorst
> ; Thomas Zimmermann
> ; David Airlie ; Daniel Vetter
> ; Simek, Michal ; Andrzej Hajda
> ; Neil Armstrong ; Robert
> Foss ; Jonas Karlman ; Jernej Skrabec
> ; Rob Herring ; Krzysztof
> Kozlowski ; Conor Dooley
> ; Mauro Carvalho Chehab ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> me...@vger.kernel.org
> Subject: Re: [PATCH v2 8/8] drm: xlnx: Intoduce TPG CRTC driver
>
> Hi,
>
> On Tue, Mar 12, 2024 at 05:55:05PM -0700, Anatoliy Klymenko wrote:
> > DO NOT MERGE. REFERENCE ONLY.
> >
> > Add CRTC driver based on AMD/Xilinx Video Test Pattern Generator IP.
> > TPG based FPGA design represents minimalistic harness useful for
> > testing links between FPGA based CRTC and external DRM encoders, both
> > FPGA and hardened IP based.
> >
> > Add driver for AMD/Xilinx Video Timing Controller. The VTC, working in
> > generator mode, suplements TPG with video timing signals.
> >
> > Signed-off-by: Anatoliy Klymenko
>
> As I said previously, we don't want to have unused APIs, so this patch should
> be in
> a good enough state to be merged if we want to merge the whole API.
>
This is understandable, but even having this API just reviewed by the community
will open the path forward for aligning AMD/Xilinx downstream DRM drivers with
the upstream kernel.
> > +/*
> > +-
> > +
> > + * DRM CRTC
> > + */
> > +
> > +static enum drm_mode_status xlnx_tpg_crtc_mode_valid(struct drm_crtc
> *crtc,
> > +const struct
> drm_display_mode *mode) {
> > + return MODE_OK;
> > +}
> > +
> > +static int xlnx_tpg_crtc_check(struct drm_crtc *crtc,
> > + struct drm_atomic_state *state) {
> > + struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> > + int ret;
> > +
> > + if (!crtc_state->enable)
> > + goto out;
> > +
> > + ret = drm_atomic_helper_check_crtc_primary_plane(crtc_state);
> > + if (ret)
> > + return ret;
> > +
> > +out:
> > + return drm_atomic_add_affected_planes(state, crtc); }
> > +
>
> [...]
>
> > +
> > +static u32 xlnx_tpg_crtc_select_output_bus_format(struct drm_crtc *crtc,
> > + struct drm_crtc_state
> *crtc_state,
> > + const u32 *in_bus_fmts,
> > + unsigned int
> num_in_bus_fmts) {
> > + struct xlnx_tpg *tpg = crtc_to_tpg(crtc);
> > + unsigned int i;
> > +
> > + for (i = 0; i < num_in_bus_fmts; ++i)
> > + if (in_bus_fmts[i] == tpg->output_bus_format)
> > + return tpg->output_bus_format;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct drm_crtc_helper_funcs xlnx_tpg_crtc_helper_funcs = {
> > + .mode_valid = xlnx_tpg_crtc_mode_valid,
> > + .atomic_check = xlnx_tpg_crtc_check,
> > + .atomic_enable = xlnx_tpg_crtc_enable,
> > + .atomic_disable = xlnx_tpg_crtc_disable,
> > + .select_output_bus_format = xlnx_tpg_crtc_select_output_bus_format,
> > +};
>
> From that code, it's not clear to me how the CRTC is going to be able to get
> what
> the format is.
>
It's coming from DT "bus-format" property. The idea is that this property will
reflect the FPGA design variation synthesized.
> It looks like you hardcode it here, but what if there's several that would
> fit the
> bill? Is the CRTC expected to store it into its private structure?
>
It's impractical from the resources utilization point of view to support
multiple runtime options for FPGA-based CRTCs output signal format, so the
bus-format will be runtime fixed but can vary between differently synthesized
instances.
> If so, I would expect it to be in the crtc state, and atomic_enable to just
> reuse
> whatever is in the state.
>
This could be totally valid for different kinds of CRTCs, although for this
particular case, the bus-fomat choice is runtime immutable.
> Maxime
Thank you,
Anatoliy
RE: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
Hi Laurent,
Thanks for the review.
> -Original Message-
> From: dri-devel On Behalf Of Laurent
> Pinchart
> Sent: Wednesday, February 28, 2024 7:58 AM
> To: Klymenko, Anatoliy
> Cc: Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ; Simek, Michal
> ; Andrzej Hajda ; Neil
> Armstrong ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Anounce supported input
> formats
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Mon, Feb 26, 2024 at 08:44:43PM -0800, Anatoliy Klymenko wrote:
> > DPSUB in bridge mode supports multiple input media bus formats.
> >
> > Announce the list of supported input media bus formats via
> > drm_bridge.atomic_get_input_bus_fmts callback.
> >
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 37
> > +
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 +
> > 3 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index e6d26ef60e89..ee99aad915ba 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -18,6 +18,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
> > /**
> > * struct zynqmp_disp_format - Display subsystem format information
> > * @drm_fmt: DRM format (4CC)
> > + * @bus_fmt: Media bus format
> > * @buf_fmt: AV buffer format
> > * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
> > * @sf: Scaling factors for color components
> > */
> > struct zynqmp_disp_format {
> > u32 drm_fmt;
> > + u32 bus_fmt;
> > u32 buf_fmt;
> > bool swap;
> > const u32 *sf;
> > @@ -364,6 +367,40 @@ static const struct zynqmp_disp_format
> avbuf_gfx_fmts[] = {
> > },
> > };
> >
> > +/* List of live video layer formats */ static const struct
> > +zynqmp_disp_format avbuf_live_fmts[] = {
> > + {
> > + .drm_fmt= DRM_FORMAT_VYUY,
> > + .bus_fmt= MEDIA_BUS_FMT_VYUY8_1X16,
> > + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> > + .sf = scaling_factors_888,
>
> Is there a reason to have a separate array, instead of populating .bus_fmt in
> the
> existing arrays for the formats that can be supported with the live input,
> and only
> reporting those from
> zynqmp_disp_get_input_bus_fmts() ?
>
There are multiple reasons for this: 1) those formats share some similarities,
although they are different in nature, e.g. memory layout formats vs. video
signal formats; 2) corresponding IP registers are different with incompatible
layouts; 3) ZynqMP DPSUB documentation clearly distinguishes 3 sets of formats:
live video, video packer and graphics packer, ref.
https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Video-Formats.
I think, having separate format arrays will help to avoid ambiguity.
> > + },
> > +};
> > +
> > +u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state,
> > + u32 output_fmt,
> > + unsigned int *num_input_fmts) {
> > + int i;
> > + u32 *input_fmts;
> > +
> > + input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts),
> GFP_KERNEL);
> > + if (!input_fmts) {
> > + *num_input_fmts = 0;
> > + return input_fmts;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i)
> > + input_fmts[i] = avbuf_live_fmts[i].bus_fmt;
>
> Extra space.
>
ACK. Thank you.
> > + *num_input_fmts =
RE: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format
Hi Laurent,
Thanks a lot for the review.
> -Original Message-
> From: Laurent Pinchart
> Sent: Wednesday, February 28, 2024 8:08 AM
> To: Klymenko, Anatoliy
> Cc: Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ; Simek, Michal
> ; Andrzej Hajda ; Neil
> Armstrong ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Mon, Feb 26, 2024 at 08:44:44PM -0800, Anatoliy Klymenko wrote:
> > Program live video input format according to selected media bus format.
> >
> > In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
> > almost certainly supports a single media bus format as its output.
> > Expect this to be delivered via the new bridge atomic state. Program
> > DPSUB registers accordingly.
> >
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 52
> +
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 ++
> > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 ++---
> > drivers/gpu/drm/xlnx/zynqmp_dp.c| 13 ++---
> > 4 files changed, 67 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index ee99aad915ba..1c3ffdee6b8e 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -416,6 +416,34 @@ static bool zynqmp_disp_layer_is_video(const struct
> zynqmp_disp_layer *layer)
> > return layer->id == ZYNQMP_DPSUB_LAYER_VID; }
> >
> > +/**
> > + * zynqmp_disp_avbuf_set_live_format - Set live input format for a
> > +layer
> > + * @disp: Display controller
> > + * @layer: The layer
> > + * @fmt: The format information
> > + *
> > + * Set the live video input format for @layer to @fmt.
> > + */
> > +static void zynqmp_disp_avbuf_set_live_format(struct zynqmp_disp *disp,
> > + struct zynqmp_disp_layer *layer,
> > + const struct
> > +zynqmp_disp_format *fmt) {
> > + u32 reg, i;
> > +
> > + reg = zynqmp_disp_layer_is_video(layer)
> > + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
> > + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
> > + zynqmp_disp_avbuf_write(disp, reg, fmt->buf_fmt);
> > +
> > + for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; ++i) {
> > + reg = zynqmp_disp_layer_is_video(layer)
> > + ? ZYNQMP_DISP_AV_BUF_LIVD_VID_COMP_SF(i)
> > + : ZYNQMP_DISP_AV_BUF_LIVD_GFX_COMP_SF(i);
> > + zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
> > + }
>
> This is identical to zynqmp_disp_avbuf_set_format(), you should avoid
> duplicating
> code.
>
Yeah, there are similarities - let me think on how to refactor this properly.
> > + layer->disp_fmt = fmt;
> > +}
> > +
> > /**
> > * zynqmp_disp_avbuf_set_format - Set the input format for a layer
> > * @disp: Display controller
> > @@ -979,6 +1007,30 @@ void zynqmp_disp_layer_disable(struct
> zynqmp_disp_layer *layer)
> > zynqmp_disp_blend_layer_disable(layer->disp, layer); }
> >
> > +/**
> > + * zynqmp_disp_layer_set_live_format - Set live layer input format
> > + * @layer: The layer
> > + * @info: Input media bus format
> > + *
> > + * Set the live @layer input bus format. The layer must be disabled.
> > + */
> > +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> > +u32 bus_format)
>
> I'd prefer reusing zynqmp_disp_layer_set_format(), and handling the
> differences
> between live and non-live input there. There's already a dma_enabled check in
> that function.
>
There is a difference between setting format for dma-backed layer vs live input
layer. In the first case we have memory layout in fourcc format, but in the
second case we have video signal described by media bus format. Anyways, let me
check if I can unify both cases.
> > +{
> > + int i;
> > + const struct zynqmp_disp_format *fmt;
> > +
> > + for
RE: [PATCH 4/4] drm/atomic-helper: Add select_output_bus_format callback
Hi Maxime,
Thanks for the review.
> -Original Message-
> From: Maxime Ripard
> Sent: Wednesday, February 28, 2024 7:30 AM
> To: Klymenko, Anatoliy
> Cc: Laurent Pinchart ; Maarten Lankhorst
> ; Thomas Zimmermann
> ; David Airlie ; Daniel Vetter
> ; Simek, Michal ; Andrzej Hajda
> ; Neil Armstrong ; Robert
> Foss ; Jonas Karlman ; Jernej Skrabec
> ; dri-devel@lists.freedesktop.org; linux-arm-
> ker...@lists.infradead.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH 4/4] drm/atomic-helper: Add select_output_bus_format
> callback
>
> Hi,
>
> On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote:
> > Add select_output_bus_format to CRTC atomic helpers callbacks. This
> > callback Will allow CRTC to participate in media bus format
> > negotiation over connected DRM bridge chain and impose CRTC-specific
> restrictions.
> > A good example is CRTC implemented as FPGA soft IP. This kind of CRTC
> > will most certainly support a single output media bus format, as
> > supporting multiple runtime options consumes extra FPGA resources. A
> > variety of options for FPGA are usually achieved by synthesizing IP
> > with different parameters.
> >
> > Incorporate select_output_bus_format callback into the format
> > negotiation stage to fix the input bus format of the first DRM bridge in the
> chain.
> >
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/drm_bridge.c | 19 +--
> > include/drm/drm_modeset_helper_vtables.h | 31
> > +++
> > 2 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c
> > b/drivers/gpu/drm/drm_bridge.c index 521a71c61b16..453ae3d174b4 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -32,6 +32,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge
> *first_bridge,
> > unsigned int i, num_in_bus_fmts = 0;
> > struct drm_bridge_state *cur_state;
> > struct drm_bridge *prev_bridge;
> > - u32 *in_bus_fmts;
> > + struct drm_crtc *crtc = crtc_state->crtc;
> > + u32 *in_bus_fmts, in_fmt;
> > int ret;
> >
> > prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
> > @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge
> *first_bridge,
> > return -ENOMEM;
> >
> > if (first_bridge == cur_bridge) {
> > - cur_state->input_bus_cfg.format = in_bus_fmts[0];
> > + in_fmt = in_bus_fmts[0];
> > + if (crtc->helper_private &&
> > + crtc->helper_private->select_output_bus_format) {
> > + in_fmt = crtc->helper_private-
> >select_output_bus_format(
> > + crtc,
> > + crtc->state,
> > + in_bus_fmts,
> > + num_in_bus_fmts);
> > + if (!in_fmt) {
> > + kfree(in_bus_fmts);
> > + return -ENOTSUPP;
> > + }
> > + }
> > + cur_state->input_bus_cfg.format = in_fmt;
>
> I don't think we should start poking at the CRTC internals, but we should
> rather
> provide a helper here.
Makes sense, thank you. ACK.
>
> > cur_state->output_bus_cfg.format = out_bus_fmt;
> > kfree(in_bus_fmts);
> > return 0;
> > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 881b03e4dc28..7c21ae1fe3ad 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> > bool in_vblank_irq, int *vpos, int *hpos,
> > ktime_t *stime, ktime_t *etime,
> > const struct drm_display_mode *mode);
> > +
> > + /**
> > +* @select_output_bus_format
> > +*
> > +* Called by the first connected DRM bridge to negotiate input media
> > +* bus format. CRTC is expected to pick preferable media formats from
> > +* the list supported by the DRM
RE: [PATCH v3 2/5] drm: xlnx: zynqmp_dpsub: Fix timing for live mode
Hi Laurent, Thanks a lot for the review. > -Original Message- > From: Laurent Pinchart > Sent: Monday, February 5, 2024 12:08 AM > To: Klymenko, Anatoliy > Cc: maarten.lankho...@linux.intel.com; mrip...@kernel.org; > tzimmerm...@suse.de; airl...@gmail.com; dan...@ffwll.ch; Simek, Michal > ; dri-devel@lists.freedesktop.org; linux-arm- > ker...@lists.infradead.org; linux-ker...@vger.kernel.org > Subject: Re: [PATCH v3 2/5] drm: xlnx: zynqmp_dpsub: Fix timing for live mode > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > Hi Anatoliy, > > Thank you for the patch. > > On Tue, Jan 23, 2024 at 06:53:59PM -0800, Anatoliy Klymenko wrote: > > Expect external video timing in live video input mode, program DPSUB > > acordingly. > > Are there no designs where the DPSUB operates in non-live mode, but uses a > video clock from the PL, for instance to use a different clock frequency ? > > I don't think that use case is very common, so I'm fine with this patch in > order to > properly support the more common use case of live input, and leave the PL > clock > without live input use case for later. > Theoretically, we can create such a design, but it wouldn't make too much sense since the PS clock is the best choice here. Probably, in some complicated scenarios like hybrid live-in/DMA or live-out, we would consider using the PL video clock, but the DPSUB driver is not supporting them yet. > > > > Reviewed-by: Tomi Valkeinen > > > > No need for a blank line here. > Sure, I'll fix this in the new version. Thank you. > > Signed-off-by: Anatoliy Klymenko > > Reviewed-by: Laurent Pinchart > > > --- > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > index 407bc07cec69..8a39b3accce5 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > > @@ -1166,7 +1166,7 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp) > > /* Choose clock source based on the DT clock handle. */ > > zynqmp_disp_avbuf_set_clocks_sources(disp, disp->dpsub- > >vid_clk_from_ps, > >disp->dpsub->aud_clk_from_ps, > > - true); > > + > > + disp->dpsub->vid_clk_from_ps); > > zynqmp_disp_avbuf_enable_channels(disp); > > zynqmp_disp_avbuf_enable_audio(disp); > > > > -- > Regards, > > Laurent Pinchart
RE: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB
> -Original Message- > From: Maxime Ripard > Sent: Friday, January 26, 2024 4:26 AM > To: Laurent Pinchart > Cc: Klymenko, Anatoliy ; > maarten.lankho...@linux.intel.com; tzimmerm...@suse.de; airl...@gmail.com; > dan...@ffwll.ch; Simek, Michal ; dri- > de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org > Subject: Re: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB > > On Wed, Jan 17, 2024 at 04:23:43PM +0200, Laurent Pinchart wrote: > > On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote: > > > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote: > > > > Patches 1/4,2/4,3/4 are minor fixes. > > > > > > > > DPSUB requires input live video format to be configured. > > > > Patch 4/4: The DP Subsystem requires the input live video format to be > > > > configured. In this patch we are assuming that the CRTC's bus format is > > > > fixed > > > > and comes from the device tree. This is a proposed solution, as there > > > > are no > api > > > > to query CRTC output bus format. > > > > > > > > Is this a good approach to go with? > > > > > > I guess you would need to expand a bit on what "live video input" is? Is > > > it some kind of mechanism to bypass memory and take your pixels straight > > > from a FIFO from another device, or something else? > > > > Yes and no. > > > > The DPSUB integrates DMA engines, a blending engine (two planes), and a > > DP encoder. The dpsub driver supports all of this, and creates a DRM > > device. The DP encoder hardware always takes its input data from the > > output of the blending engine. > > > > The blending engine can optionally take input data from a bus connected > > to the FPGA fabric, instead of taking it from the DPSUB internal DMA > > engines. When operating in that mode, the dpsub driver exposes the DP > > encoder as a bridge, and internally programs the blending engine to > > disable blending. Typically, the FPGA fabric will then contain a CRTC of > > some sort, with a driver that will acquire the DP encoder bridge as > > usually done. > > > > In this mode of operation, it is typical for the IP cores in FPGA fabric > > to be synthesized with a fixed format (as that saves resources), while > > the DPSUB supports multiple input formats. > > Where is that CRTC driver? It's not clear to me why the format would > need to be in the device tree at all. Format negociation between the > CRTC and whatever comes next is already done in a number of drivers so > it would be useful to have that kind of API outside of the bridge > support. > One example of such CRTC driver: https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xlnx/xlnx_mixer.c It's not upstreamed yet. Bus format negotiations here are handled by utilizing Xilinx-specific bridge framework. Ideally, it would be nice to rework this to comply with the upstream DRM bridge framework. > > Bridge drivers in the upstream kernel work the other way around, with > > the bridge hardware supporting a limited set of formats, and the CRTC > > then being programmed with whatever the bridges chain needs. Here, the > > negotiation needs to go the other way around, as the CRTC is the > > limiting factor, not the bridge. > > Sounds like there's something to rework in the API then? > Adding an optional CRTC callback imposing CRTC specific bus format restrictions, which may be called from here https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L935 would solve the problem. > Maxime -- Regards, Anatoliy
RE: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
Hi Laurent, > -Original Message- > From: Laurent Pinchart > Sent: Friday, January 19, 2024 4:30 AM > To: Klymenko, Anatoliy > Cc: Tomi Valkeinen ; > aarten.lankho...@linux.intel.com; mrip...@kernel.org; tzimmerm...@suse.de; > airl...@gmail.com; dan...@ffwll.ch; Simek, Michal ; > dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in > live > mode > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > Hi Anatoliy, > > On Fri, Jan 19, 2024 at 05:53:30AM +, Klymenko, Anatoliy wrote: > > On Wed, 17 Jan 2024 16:20:10 +0200, Tomi Valkeinen wrote: > > > On 13/01/2024 01:42, Anatoliy Klymenko wrote: > > > > Filter out status register against interrupts' mask. > > > > Some events are being reported via DP status register, even if > > > > corresponding interrupts have been disabled. Avoid processing of > > > > such events in interrupt handler context. > > > > > > The subject talks about vblank and live mode, the the description > > > doesn't. Can you elaborate in the desc a bit about when this is an > > > issue and why it wasn't before? > > > > Yes, I should make patch comment more consistent and elaborate. I will fix > > it in > the next version. Thank you. > > > > > > > > > Signed-off-by: Anatoliy Klymenko > > > > --- > > > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > > index d60b7431603f..571c5dbc97e5 100644 > > > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > > @@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int > irq, void *data) > > > > u32 status, mask; > > > > > > > > status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS); > > > > + zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > > > > mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); > > > > - if (!(status & ~mask)) > > > > + > > > > + /* > > > > + * Status register may report some events, which corresponding > interrupts > > > > + * have been disabled. Filter out those events against > > > > interrupts' mask. > > > > + */ > > > > + status &= ~mask; > > > > + > > > > + if (!status) > > > > return IRQ_NONE; > > > > > > > > /* dbg for diagnostic, but not much that the driver can do > > > > */ @@ -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int > irq, void *data) > > > > if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK) > > > > dev_dbg_ratelimited(dp->dev, "overflow > > > > interrupt\n"); > > > > > > > > - zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > > > > > > > > if (status & ZYNQMP_DP_INT_VBLANK_START) > > > > zynqmp_dpsub_drm_handle_vblank(dp->dpsub); > > > > > > Moving the zynqmp_dp_write() is not related to this fix, is it? I > > > think it should be in a separate patch. > > > > The sole purpose of zynqmp_dp_write() is to clear status register. The > > sooner we do it the better (after reading status in the local variable > > of course). > > No disagreement about that. Tomi's point is that it's a good change, but it > should > be done in a separate patch, by itself, not bundled with other changes. The > usual > rule in the kernel is "one change, one commit". > Sure, I will move this into a separate commit in the next version. Thank you. > > We can also reuse status variable for in-place filtering against the > > interrupt mask, which makes this change dependent on > > zynqmp_dp_write() reordering. I will add a comment explaining this. Is > > it ok? > > -- > Regards, > > Laurent Pinchart Thank you, Anatoliy
Re: [PATCH 4/4] drm: xlnx: zynqmp_dpsub: Set live video in format
Hi Tommy,
Thank you for the comments.
> Date: Wed, 17 Jan 2024 17:32:57 +0200
> From: Tomi Valkeinen
> To: Anatoliy Klymenko ,
> laurent.pinch...@ideasonboard.com, maarten.lankho...@linux.intel.com,
> mrip...@kernel.org, tzimmerm...@suse.de, airl...@gmail.com,
> dan...@ffwll.ch, michal.si...@amd.com,
> dri-devel@lists.freedesktop.org, linux-arm-ker...@lists.infradead.org,
> linux-ker...@vger.kernel.org
> Subject: Re: [PATCH 4/4] drm: xlnx: zynqmp_dpsub: Set live video in
> format
> Message-ID:
> Content-Type: text/plain; charset=UTF-8; format=flowed
>
> Hi Anatoliy,
>
> On 13/01/2024 01:42, Anatoliy Klymenko wrote:
> > Live video input format is expected to be set as "bus-format" property
> > in connected remote endpoint.
> > Program live video input format DPSUB registers.
> > Set display layer mode in layer creation context.
>
> Some comments inline below. But one thing to improve is the commit desc.
>
> I think this needs more explanation on what's the issue here. So basically
> something like what's the feature in question, why it's not working or what's
> missing, and what does this patch do to get it working.
>
> And while often it's reasonable to expect some level of understanding of the
> HW
> in question, it doesn't hurt to give some clarifications on the names used
> (here the
> "live video input").
>
Sure, good point, how about something like
"ZynqMP DPSUB supports 2 modes of operations in regard to video data
input.
In the first mode, DPSUB uses DMA engine to pull video data from memory
buffers. To support this the driver implements CRTC and DRM bridge
representing DP encoder.
In the second mode, DPSUB acquires video data pushed from FPGA and
passes it downstream to DP output. This mode of operation is modeled in
the driver as a DRM bridge that should be attached to some external
CRTC. DPSUB supports multiple input data formats. In order to properly
operate exact media bus format should be negotiated between external
CRTC and DPSUB bridge. DRM framework provides a mechanism to negotiate
media bus formats between bridges connected into a chain, but not
between CRTC and encoder (first bridge in the chain). This change
mitigates the issue for FPGA based CRTC, which would typically have a
single fixed media bus format.
Expect live video input format to be set as "bus-format" property in
connected remote endpoint.
Set display layer mode in the layer creation context."?
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 109 ++--
> > drivers/gpu/drm/xlnx/zynqmp_disp.h | 3 +-
> > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +-
> > drivers/gpu/drm/xlnx/zynqmp_dp.c| 2 +-
> > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> > 5 files changed, 107 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index 8a39b3accce5..83af3ad9cdb5 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -20,8 +20,10 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > +#include
>
> Alphabetical order, please.
Got it - will fix in the next version.
>
> > #include "zynqmp_disp.h"
> > #include "zynqmp_disp_regs.h"
> > @@ -67,12 +69,16 @@
> > /**
> >* struct zynqmp_disp_format - Display subsystem format information
> >* @drm_fmt: DRM format (4CC)
> > + * @bus_fmt: Live video media bus format
> >* @buf_fmt: AV buffer format
> >* @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
> >* @sf: Scaling factors for color components
> >*/
> > struct zynqmp_disp_format {
> > - u32 drm_fmt;
> > + union {
> > + u32 drm_fmt;
> > + u32 bus_fmt;
> > + };
> > u32 buf_fmt;
> > bool swap;
> > const u32 *sf;
> > @@ -354,6 +360,16 @@ static const struct zynqmp_disp_format
> avbuf_gfx_fmts[] = {
> > },
> > };
> >
> > +/* TODO: add support for different formats */ static const struct
> > +zynqmp_disp_format avbuf_live_vid_fmts[] = {
> > + {
> > + .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16,
> > + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> > + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> > + .sf = scaling_factors_888,
> > + }
> > +};
> > +
> > static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
> > {
> > return readl(disp->avbuf.base + reg); @@ -369,6 +385,34 @@
> > static bool zynqmp_disp_layer_is_video(const struct zynqmp_disp_layer
> > *layer)
> > return layer->id == ZYNQMP_DPSUB_LAYER_VID;
> > }
> >
> > +/**
> > + * zynqmp_disp_avbuf_set_live_format - Set live input format for a
> > +layer
> > + *
Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
Hi Tomi, Thank you for the review. > Date: Wed, 17 Jan 2024 16:20:10 +0200 > From: Tomi Valkeinen > To: Anatoliy Klymenko , > laurent.pinch...@ideasonboard.com, maarten.lankho...@linux.intel.com, > mrip...@kernel.org, tzimmerm...@suse.de, airl...@gmail.com, > dan...@ffwll.ch, michal.si...@amd.com, > dri-devel@lists.freedesktop.org, linux-arm-ker...@lists.infradead.org, > linux-ker...@vger.kernel.org > Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate > vblank in live mode > Message-ID: > Content-Type: text/plain; charset=UTF-8; format=flowed > > On 13/01/2024 01:42, Anatoliy Klymenko wrote: > > Filter out status register against interrupts' mask. > > Some events are being reported via DP status register, even if > > corresponding interrupts have been disabled. Avoid processing of such > > events in interrupt handler context. > > The subject talks about vblank and live mode, the the description doesn't. Can > you elaborate in the desc a bit about when this is an issue and why it wasn't > before? Yes, I should make patch comment more consistent and elaborate. I will fix it in the next version. Thank you. > > > Signed-off-by: Anatoliy Klymenko > > --- > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > index d60b7431603f..571c5dbc97e5 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > @@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, > void *data) > > u32 status, mask; > > > > status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS); > > + zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > > mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); > > - if (!(status & ~mask)) > > + > > + /* > > + * Status register may report some events, which corresponding > > interrupts > > + * have been disabled. Filter out those events against interrupts' > > mask. > > + */ > > + status &= ~mask; > > + > > + if (!status) > > return IRQ_NONE; > > > > /* dbg for diagnostic, but not much that the driver can do */ @@ > > -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void > *data) > > if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK) > > dev_dbg_ratelimited(dp->dev, "overflow interrupt\n"); > > > > - zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > > > > if (status & ZYNQMP_DP_INT_VBLANK_START) > > zynqmp_dpsub_drm_handle_vblank(dp->dpsub); > > Moving the zynqmp_dp_write() is not related to this fix, is it? I think it > should be in > a separate patch. > The sole purpose of zynqmp_dp_write() is to clear status register. The sooner we do it the better (after reading status in the local variable of course). We can also reuse status variable for in-place filtering against the interrupt mask, which makes this change dependent on zynqmp_dp_write() reordering. I will add a comment explaining this. Is it ok? > Tomi > Thank you, Anatoliy
Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge
Hi Tomi, Thanks for your response. > Date: Wed, 17 Jan 2024 16:06:31 +0200 > From: Tomi Valkeinen > To: Anatoliy Klymenko , > laurent.pinch...@ideasonboard.com, maarten.lankho...@linux.intel.com, > mrip...@kernel.org, tzimmerm...@suse.de, airl...@gmail.com, > dan...@ffwll.ch, michal.si...@amd.com, > dri-devel@lists.freedesktop.org, linux-arm-ker...@lists.infradead.org, > linux-ker...@vger.kernel.org > Subject: Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge > discoverable > Message-ID: <98a9f4f1-dd55-47c3-bb1b-07e201b29...@ideasonboard.com> > Content-Type: text/plain; charset=UTF-8; format=flowed > > On 13/01/2024 01:42, Anatoliy Klymenko wrote: > > Assign device of node to bridge prior registering it. This will make > > said bridge discoverable by separate crtc driver. > > I think a few words on why this is needed (and why it wasn't needed > before) would be nice. Sure. I will update. " ZynqMP DPSUB supports 2 input modes: DMA based and live video. In the first mode, the driver implements CRTC and DP encoder DRM bridge to model the complete display pipeline. In this case, DRM bridge is being directly instantiated within the driver, not using any bridge discovery mechanisms. In the live video input mode video signal is generated by FPGA fabric and passed into DPSUB over the connected bus. In this mode driver exposes the DP encoder as a DRM bridge, expecting external CRTC to discover it via drm_of_find_panel_or_bridge() or a similar call. This discovery relies on drm_bridge.of_node being properly set. Assign device OF node to the bridge prior to registering it. This will make said bridge discoverable by an external CRTC driver.". How does this sound? > > Other than that: > > Reviewed-by: Tomi Valkeinen > > Tomi > > > Signed-off-by: Anatoliy Klymenko > > --- > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > index a0606fab0e22..d60b7431603f 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > @@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) > > bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID > > | DRM_BRIDGE_OP_HPD; > > bridge->type = DRM_MODE_CONNECTOR_DisplayPort; > > + bridge->of_node = dp->dev->of_node; > > dpsub->bridge = bridge; > > > > /* > > Regards Anatoliy
Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable
Hi Laurent, Thank you for the reply. > Date: Wed, 17 Jan 2024 16:24:20 +0200 > From: Laurent Pinchart > To: Tomi Valkeinen > Cc: Anatoliy Klymenko , > maarten.lankho...@linux.intel.com, mrip...@kernel.org, > tzimmerm...@suse.de, airl...@gmail.com, dan...@ffwll.ch, > michal.si...@amd.com, dri-devel@lists.freedesktop.org, > linux-arm-ker...@lists.infradead.org, linux-ker...@vger.kernel.org > Subject: Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge > discoverable > Message-ID: <20240117142420.ge17...@pendragon.ideasonboard.com> > Content-Type: text/plain; charset=utf-8 > > On Wed, Jan 17, 2024 at 04:06:31PM +0200, Tomi Valkeinen wrote: > > On 13/01/2024 01:42, Anatoliy Klymenko wrote: > > > Assign device of node to bridge prior registering it. This will make > > > said bridge discoverable by separate crtc driver. > > > > I think a few words on why this is needed (and why it wasn't needed > > before) would be nice. > > > > Other than that: > > > > Reviewed-by: Tomi Valkeinen > > Agreed. With a better commit message, > Sure, I will update commit message in the next version. > Reviewed-by: Laurent Pinchart > > > > Signed-off-by: Anatoliy Klymenko > > > --- > > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > index a0606fab0e22..d60b7431603f 100644 > > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > @@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub > *dpsub) > > > bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID > > > | DRM_BRIDGE_OP_HPD; > > > bridge->type = DRM_MODE_CONNECTOR_DisplayPort; > > > + bridge->of_node = dp->dev->of_node; > > > dpsub->bridge = bridge; > > > > > > /* > > -- > Regards, > > Laurent Pinchart > > Thank you, Anatoliy
Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB
Hi Laurent and Maxime, Laurent, thank you very much for clear and comprehensive description of the "live video input" feature. Maxime, sure, I will elaborate more in the next version of cover letter. > Date: Wed, 17 Jan 2024 16:23:43 +0200 > From: Laurent Pinchart > To: Maxime Ripard > Cc: Anatoliy Klymenko , > maarten.lankho...@linux.intel.com, tzimmerm...@suse.de, > airl...@gmail.com, dan...@ffwll.ch, michal.si...@amd.com, > dri-devel@lists.freedesktop.org, linux-arm-ker...@lists.infradead.org, > linux-ker...@vger.kernel.org > Subject: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB > Message-ID: <20240117142343.gd17...@pendragon.ideasonboard.com> > Content-Type: text/plain; charset=utf-8 > > On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote: > > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote: > > > Patches 1/4,2/4,3/4 are minor fixes. > > > > > > DPSUB requires input live video format to be configured. > > > Patch 4/4: The DP Subsystem requires the input live video format to > > > be configured. In this patch we are assuming that the CRTC's bus > > > format is fixed and comes from the device tree. This is a proposed > > > solution, as there are no api to query CRTC output bus format. > > > > > > Is this a good approach to go with? > > > > I guess you would need to expand a bit on what "live video input" is? > > Is it some kind of mechanism to bypass memory and take your pixels > > straight from a FIFO from another device, or something else? > > Yes and no. > > The DPSUB integrates DMA engines, a blending engine (two planes), and a DP > encoder. The dpsub driver supports all of this, and creates a DRM device. The > DP > encoder hardware always takes its input data from the output of the blending > engine. > > The blending engine can optionally take input data from a bus connected to the > FPGA fabric, instead of taking it from the DPSUB internal DMA engines. When > operating in that mode, the dpsub driver exposes the DP encoder as a bridge, > and > internally programs the blending engine to disable blending. Typically, the > FPGA > fabric will then contain a CRTC of some sort, with a driver that will acquire > the DP > encoder bridge as usually done. > > In this mode of operation, it is typical for the IP cores in FPGA fabric to be > synthesized with a fixed format (as that saves resources), while the DPSUB > supports multiple input formats. Bridge drivers in the upstream kernel work > the > other way around, with the bridge hardware supporting a limited set of > formats, > and the CRTC then being programmed with whatever the bridges chain needs. > Here, the negotiation needs to go the other way around, as the CRTC is the > limiting factor, not the bridge. > > Is this explanation clear ? > > -- > Regards, > > Laurent Pinchart > > Thank you, Anatoliy
