RE: [PATCH v3 2/5] drm/kmb: add trailing newlines to drm_dbg msgs
Acked-by: Anitha Chrisanthus
> -Original Message-
> From: Jim Cromie
> Sent: Wednesday, September 6, 2023 12:02 PM
> To: linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; amd-
> g...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; intel-
> g...@lists.freedesktop.org
> Cc: daniel.vet...@ffwll.ch; dan...@ffwll.ch; Nikula, Jani
> ; ville.syrj...@linux.intel.com;
> seanp...@chromium.org; robdcl...@gmail.com; Jim Cromie
> ; Chrisanthus, Anitha
> ; Edmund Dea ;
> David Airlie
> Subject: [PATCH v3 2/5] drm/kmb: add trailing newlines to drm_dbg msgs
>
> By at least strong convention, a print-buffer's trailing newline says
> "message complete, send it". The exception (no TNL, followed by a call
> to pr_cont) proves the general rule.
>
> Most DRM.debug calls already comport with this: 207 DRM_DEV_DEBUG,
> 1288 drm_dbg. Clean up the remainders, in maintainer sized chunks.
>
> No functional changes.
>
> Signed-off-by: Jim Cromie
> ---
> drivers/gpu/drm/kmb/kmb_crtc.c | 10 +-
> drivers/gpu/drm/kmb/kmb_plane.c | 6 +++---
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_crtc.c
> b/drivers/gpu/drm/kmb/kmb_crtc.c
> index 647872f65bff..a58baf25322d 100644
> --- a/drivers/gpu/drm/kmb/kmb_crtc.c
> +++ b/drivers/gpu/drm/kmb/kmb_crtc.c
> @@ -94,7 +94,7 @@ static void kmb_crtc_set_mode(struct drm_crtc *crtc,
> vm.hback_porch = 0;
> vm.hsync_len = 28;
>
> - drm_dbg(dev, "%s : %dactive height= %d vbp=%d vfp=%d vsync-w=%d
> h-active=%d h-bp=%d h-fp=%d hsync-l=%d",
> + drm_dbg(dev, "%s : %dactive height= %d vbp=%d vfp=%d vsync-w=%d
> h-active=%d h-bp=%d h-fp=%d hsync-l=%d\n",
> __func__, __LINE__,
> m->crtc_vdisplay, vm.vback_porch, vm.vfront_porch,
> vm.vsync_len, m->crtc_hdisplay, vm.hback_porch,
> @@ -194,24 +194,24 @@ static enum drm_mode_status
> int vfp = mode->vsync_start - mode->vdisplay;
>
> if (mode->vdisplay < KMB_CRTC_MAX_HEIGHT) {
> - drm_dbg(dev, "height = %d less than %d",
> + drm_dbg(dev, "height = %d less than %d\n",
> mode->vdisplay, KMB_CRTC_MAX_HEIGHT);
> return MODE_BAD_VVALUE;
> }
> if (mode->hdisplay < KMB_CRTC_MAX_WIDTH) {
> - drm_dbg(dev, "width = %d less than %d",
> + drm_dbg(dev, "width = %d less than %d\n",
> mode->hdisplay, KMB_CRTC_MAX_WIDTH);
> return MODE_BAD_HVALUE;
> }
> refresh = drm_mode_vrefresh(mode);
> if (refresh < KMB_MIN_VREFRESH || refresh > KMB_MAX_VREFRESH) {
> - drm_dbg(dev, "refresh = %d less than %d or greater than %d",
> + drm_dbg(dev, "refresh = %d less than %d or greater than
> %d\n",
> refresh, KMB_MIN_VREFRESH, KMB_MAX_VREFRESH);
> return MODE_BAD;
> }
>
> if (vfp < KMB_CRTC_MIN_VFP) {
> - drm_dbg(dev, "vfp = %d less than %d", vfp,
> KMB_CRTC_MIN_VFP);
> + drm_dbg(dev, "vfp = %d less than %d\n", vfp,
> KMB_CRTC_MIN_VFP);
> return MODE_BAD;
> }
>
> diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> index 9e0562aa2bcb..308bd1cb50c8 100644
> --- a/drivers/gpu/drm/kmb/kmb_plane.c
> +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> @@ -78,7 +78,7 @@ static unsigned int check_pixel_format(struct drm_plane
> *plane, u32 format)
>* plane configuration is not supported.
>*/
> if (init_disp_cfg.format && init_disp_cfg.format != format) {
> - drm_dbg(>drm, "Cannot change format after initial
> plane configuration");
> + drm_dbg(>drm, "Cannot change format after initial
> plane configuration\n");
> return -EINVAL;
> }
> for (i = 0; i < plane->format_count; i++) {
> @@ -124,7 +124,7 @@ static int kmb_plane_atomic_check(struct drm_plane
> *plane,
> if ((init_disp_cfg.width && init_disp_cfg.height) &&
> (init_disp_cfg.width != fb->width ||
> init_disp_cfg.height != fb->height)) {
> - drm_dbg(>drm, "Cannot change plane height or width
> after initial configuration");
> + drm_dbg(>drm, "Cannot change plane height or width
> after initial configuration\n");
> return -EINVAL;
> }
> can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
> @@ -375,7 +375,7 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> spin_lock_irq(>irq_lock);
> if (kmb->kmb_under_flow || kmb->kmb_flush_done) {
> spin_unlock_irq(>irq_lock);
> - drm_dbg(>drm, "plane_update:underflow
> returning");
> + drm_dbg(>drm, "plane_update:underflow
> returning\n");
> return;
> }
> spin_unlock_irq(>irq_lock);
> --
> 2.41.0
RE: [PATCH 09/22] drm/kmb: Use GEM DMA fbdev emulation
Acked-by: Anitha Chrisanthus > -Original Message- > From: Thomas Zimmermann > Sent: Wednesday, March 1, 2023 7:31 AM > To: javi...@redhat.com; maarten.lankho...@linux.intel.com; > mrip...@kernel.org; airl...@gmail.com; dan...@ffwll.ch; and...@aj.id.au; > laurentiu.pa...@oss.nxp.com; l.st...@pengutronix.de; > shawn...@kernel.org; s.ha...@pengutronix.de; ker...@pengutronix.de; > feste...@gmail.com; linux-...@nxp.com; p.za...@pengutronix.de; > Chrisanthus, Anitha ; > edmund.j@intel.com; khil...@baylibre.com; jbru...@baylibre.com; > martin.blumensti...@googlemail.com; alain.vol...@foss.st.com; > yannick.fer...@foss.st.com; raphael.gallais-...@foss.st.com; > philippe.co...@foss.st.com; mcoquelin.st...@gmail.com; > alexandre.tor...@foss.st.com; jernej.skra...@gmail.com; > sam...@sholland.org; jyri.sa...@iki.fi; to...@kernel.org; > linus.wall...@linaro.org; hyun.k...@xilinx.com; > laurent.pinch...@ideasonboard.com > Cc: dri-devel@lists.freedesktop.org; linux-asp...@lists.ozlabs.org; linux-arm- > ker...@lists.infradead.org; linux-amlo...@lists.infradead.org; linux- > st...@st-md-mailman.stormreply.com; linux-su...@lists.linux.dev; Thomas > Zimmermann > Subject: [PATCH 09/22] drm/kmb: Use GEM DMA fbdev emulation > > Use the fbdev emulation that is optimized for DMA helpers. Avoids > possible shadow buffering and makes the code simpler. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/kmb/kmb_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c > b/drivers/gpu/drm/kmb/kmb_drv.c > index d29c678f6c91..24035b53441c 100644 > --- a/drivers/gpu/drm/kmb/kmb_drv.c > +++ b/drivers/gpu/drm/kmb/kmb_drv.c > @@ -15,7 +15,7 @@ > > #include > #include > -#include > +#include > #include > #include > #include > @@ -562,7 +562,7 @@ static int kmb_probe(struct platform_device *pdev) > if (ret) > goto err_register; > > - drm_fbdev_generic_setup(>drm, 0); > + drm_fbdev_dma_setup(>drm, 0); > > return 0; > > -- > 2.39.2
RE: [PATCH] drm/kmb: fix dereference before NULL check in kmb_plane_atomic_update()
Agree with Thomas, drm_atomic_commit() will not call kmb_atomic_update() with a
NULL plane. This is not an actual bug.
Thanks,
Anitha
> -Original Message-
> From: Thomas Zimmermann
> Sent: Friday, July 29, 2022 7:26 AM
> To: Zeng Jingxiang ; Chrisanthus, Anitha
> ; edmund.j@intel.com; airl...@linux.ie;
> dan...@ffwll.ch; laurent.pinch...@ideasonboard.com; max...@cerno.tech;
> ville.syrj...@linux.intel.com
> Cc: Zeng Jingxiang ; linux-ker...@vger.kernel.org;
> dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm/kmb: fix dereference before NULL check in
> kmb_plane_atomic_update()
>
> Hi
>
> Am 29.07.22 um 16:23 schrieb Thomas Zimmermann:
> > Hi
> >
> > Am 29.07.22 um 05:07 schrieb Zeng Jingxiang:
> >> From: Zeng Jingxiang
> >>
> >> The "plane" pointer was access before checking if it was NULL.
> >>
> >> The drm_atomic_get_old_plane_state() function will dereference
> >> the pointer "plane".
> >> 345 struct drm_plane_state *old_plane_state =
> >> drm_atomic_get_old_plane_state(state, plane);
> >> 346 struct drm_plane_state *new_plane_state =
> >> drm_atomic_get_new_plane_state(state, plane);
> >>
> >> A NULL check for "plane" indicates that it may be NULL
> >> 363 if (!plane || !new_plane_state || !old_plane_state)
> >
> > Is this an actual bug that happens?
> >
> > All planes should always have a state. Therefore the tests for
> > !new_plane_state and !old_plane_state can be removed, I'd say.
>
> Just to clarify: moving the test for !plane before using one of the
> get_plane_state functions is a correct bugfix.
>
> Best regards
> Thomas
>
> >
> > Best regards
> > Thomas
> >
> >>
> >> Fixes: 977697e20b3d ("drm/atomic: Pass the full state to planes atomic
> >> disable and update")
> >> Fixes: 37418bf14c13 ("drm: Use state helper instead of the plane state
> >> pointer")
> >> Signed-off-by: Zeng Jingxiang
> >> ---
> >> drivers/gpu/drm/kmb/kmb_plane.c | 13 -
> >> 1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> >> b/drivers/gpu/drm/kmb/kmb_plane.c
> >> index 2735b8eb3537..d2bc998b65ce 100644
> >> --- a/drivers/gpu/drm/kmb/kmb_plane.c
> >> +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> >> @@ -342,10 +342,7 @@ static void kmb_plane_set_alpha(struct
> >> kmb_drm_private *kmb,
> >> static void kmb_plane_atomic_update(struct drm_plane *plane,
> >> struct drm_atomic_state *state)
> >> {
> >> - struct drm_plane_state *old_plane_state =
> >> drm_atomic_get_old_plane_state(state,
> >> - plane);
> >> - struct drm_plane_state *new_plane_state =
> >> drm_atomic_get_new_plane_state(state,
> >> - plane);
> >> + struct drm_plane_state *old_plane_state, *new_plane_state;
> >> struct drm_framebuffer *fb;
> >> struct kmb_drm_private *kmb;
> >> unsigned int width;
> >> @@ -360,7 +357,13 @@ static void kmb_plane_atomic_update(struct
> >> drm_plane *plane,
> >> static dma_addr_t addr[MAX_SUB_PLANES];
> >> struct disp_cfg *init_disp_cfg;
> >> - if (!plane || !new_plane_state || !old_plane_state)
> >> + if (!plane)
> >> + return;
> >> +
> >> + old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> >> + new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> >> +
> >> + if (!new_plane_state || !old_plane_state)
> >> return;
> >> fb = new_plane_state->fb;
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
RE: [PATCH] drm/kmb: Fix for build errors with Warray-bounds
Thanks. Fixed line wrapping and change is pushed to drm-misc-fixes.
> -Original Message-
> From: dri-devel On Behalf Of Kees
> Cook
> Sent: Thursday, January 27, 2022 1:14 PM
> To: Chrisanthus, Anitha
> Cc: Dea, Edmund J ; s...@ravnborg.org; dri-
> de...@lists.freedesktop.org; s...@canb.auug.org.au
> Subject: Re: [PATCH] drm/kmb: Fix for build errors with Warray-bounds
>
> On Thu, Jan 27, 2022 at 11:42:27AM -0800, Anitha Chrisanthus wrote:
> > This fixes the following build error
> >
> > drivers/gpu/drm/kmb/kmb_plane.c: In function 'kmb_plane_atomic_disable':
> > drivers/gpu/drm/kmb/kmb_plane.c:165:34: error: array subscript 3 is
> > above array bounds of 'struct layer_status[2]' [-Werror=array-bounds]
> > 165 | kmb->plane_status[plane_id].ctrl =
> > LCD_CTRL_GL2_ENABLE;
> > | ~^~
> > In file included from drivers/gpu/drm/kmb/kmb_plane.c:17:
> > drivers/gpu/drm/kmb/kmb_drv.h:61:41: note: while referencing
> > 'plane_status'
> > 61 | struct layer_status
> > plane_status[KMB_MAX_PLANES];
>
> This would be better without the line wrapping (to match the actual
> output), but otherwise:
>
> Reviewed-by: Kees Cook
>
> > | ^~~~
> > drivers/gpu/drm/kmb/kmb_plane.c:162:34: error: array
> > subscript 2 is above array bounds of 'struct
> > layer_status[2]' [-Werror=array-bounds]
> > 162 |
> > kmb->plane_status[plane_id].ctrl =
> > LCD_CTRL_GL1_ENABLE;
> > | ~^~
> > In file included from
> > drivers/gpu/drm/kmb/kmb_plane.c:17:
> > drivers/gpu/drm/kmb/kmb_drv.h:61:41: note:
> > while referencing 'plane_status'
> > 61 | struct layer_status
> > plane_status[KMB_MAX_PLANES];
> > |
> > ^~~~
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > drivers/gpu/drm/kmb/kmb_plane.c | 6 --
> > 1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> > index 00404ba4126d..2735b8eb3537 100644
> > --- a/drivers/gpu/drm/kmb/kmb_plane.c
> > +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> > @@ -158,12 +158,6 @@ static void kmb_plane_atomic_disable(struct
> drm_plane *plane,
> > case LAYER_1:
> > kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
> > break;
> > - case LAYER_2:
> > - kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
> > - break;
> > - case LAYER_3:
> > - kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
> > - break;
> > }
> >
> > kmb->plane_status[plane_id].disable = true;
> > --
> > 2.25.1
> >
>
> --
> Kees Cook
RE: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console
Hi Thomas,
Thanks for your review.
> -Original Message-
> From: Thomas Zimmermann
> Sent: Thursday, October 14, 2021 5:50 AM
> To: Chrisanthus, Anitha ; dri-
> de...@lists.freedesktop.org
> Cc: s...@ravnborg.org; Dea, Edmund J
> Subject: Re: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console
>
> Hi
>
> Am 14.10.21 um 01:36 schrieb Anitha Chrisanthus:
> > Enable support for fbcon (framebuffer console).
> > The user can initialize fbcon by loading kmb-drm with the parameter
> > console=1.
> >
> > v2: added missing static clk_enable
> >
> > Signed-off-by: Edmund Dea
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index 961ac6fb5fcf..b4e66eac63b5 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -5,6 +5,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -15,6 +16,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -24,6 +26,12 @@
> > #include "kmb_dsi.h"
> > #include "kmb_regs.h"
> >
> > +/* Module Parameters */
> > +static bool console;
> > +module_param(console, bool, 0400);
> > +MODULE_PARM_DESC(console,
> > +"Enable framebuffer console support (0=disable [default],
> 1=on)");
>
> There's already fbdev_emulation in drm_fb_helper.c. No need for a
> separate parameter here.
Good catch, I'll change it in V4.
>
> Best regards
> Thomas
>
> > +
> > static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
> > {
> > int ret = 0;
> > @@ -559,6 +567,9 @@ static int kmb_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_register;
> >
> > + if (console)
> > + drm_fbdev_generic_setup(>drm, 32);
> > +
> > return 0;
> >
> >err_register:
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
RE: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console
Hi Thomas,
Thanks for your review.
> -Original Message-
> From: Thomas Zimmermann
> Sent: Thursday, October 14, 2021 6:08 AM
> To: Chrisanthus, Anitha ; dri-
> de...@lists.freedesktop.org
> Cc: s...@ravnborg.org; Dea, Edmund J
> Subject: Re: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console
>
> Hi
>
> Am 14.10.21 um 01:36 schrieb Anitha Chrisanthus:
> > Enable support for fbcon (framebuffer console).
> > The user can initialize fbcon by loading kmb-drm with the parameter
> > console=1.
> >
> > v2: added missing static clk_enable
> >
> > Signed-off-by: Edmund Dea
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index 961ac6fb5fcf..b4e66eac63b5 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -5,6 +5,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -15,6 +16,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -24,6 +26,12 @@
> > #include "kmb_dsi.h"
> > #include "kmb_regs.h"
> >
> > +/* Module Parameters */
> > +static bool console;
> > +module_param(console, bool, 0400);
> > +MODULE_PARM_DESC(console,
> > +"Enable framebuffer console support (0=disable [default],
> 1=on)");
> > +
> > static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
> > {
> > int ret = 0;
> > @@ -559,6 +567,9 @@ static int kmb_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_register;
> >
> > + if (console)
> > + drm_fbdev_generic_setup(>drm, 32);
>
> The use of the final parameter is somewhat confusing and should probably
> be 0. It's the number of bits per pixel. 32 is the default. But the
> preferred way of selecting color mode is via
> dev->mode_config.preferred_depth, which is the color depth (24!). So
> maybe rather set preferred_depth to 24 and let the fbdev helper figure
> out the details.
Will change it in v4.
>
> Best regards
> Thomas
>
> > +
> > return 0;
> >
> >err_register:
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
RE: [PATCH] drm/kmb: Remove set_test_mode_src_osc_freq_target_{hi,low}_bits()
Thanks for the patch. Looks good.
Acked-by: Anitha Chrisanthus
> -Original Message-
> From: Nathan Chancellor
> Sent: Tuesday, September 28, 2021 12:24 PM
> To: Chrisanthus, Anitha ; Dea, Edmund J
>
> Cc: Geert Uytterhoeven ; Nick Desaulniers
> ; dri-devel@lists.freedesktop.org; linux-
> ker...@vger.kernel.org; l...@lists.linux.dev; Nathan Chancellor
> ; lkp
> Subject: [PATCH] drm/kmb: Remove
> set_test_mode_src_osc_freq_target_{hi,low}_bits()
>
> clang with W=1 warns:
>
> drivers/gpu/drm/kmb/kmb_dsi.c:812:2: error: unused function
> 'set_test_mode_src_osc_freq_target_low_bits' [-Werror,-Wunused-function]
> set_test_mode_src_osc_freq_target_low_bits(struct kmb_dsi *kmb_dsi,
> ^
> drivers/gpu/drm/kmb/kmb_dsi.c:824:2: error: unused function
> 'set_test_mode_src_osc_freq_target_hi_bits' [-Werror,-Wunused-function]
> set_test_mode_src_osc_freq_target_hi_bits(struct kmb_dsi *kmb_dsi,
> ^
> 2 errors generated.
>
> Remove them, as they have been unused since the driver's introduction in
> commit 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver").
>
> Reported-by: kernel test robot
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/kmb/kmb_dsi.c | 28
> 1 file changed, 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c
> b/drivers/gpu/drm/kmb/kmb_dsi.c
> index 1793cd31b117..ae24c5fc35a5 100644
> --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> @@ -808,34 +808,6 @@ static void test_mode_send(struct kmb_dsi
> *kmb_dsi, u32 dphy_no,
> }
> }
>
> -static inline void
> - set_test_mode_src_osc_freq_target_low_bits(struct kmb_dsi
> *kmb_dsi,
> -u32 dphy_no,
> -u32 freq)
> -{
> - /* Typical rise/fall time=166, refer Table 1207 databook,
> - * sr_osc_freq_target[7:0]
> - */
> - test_mode_send(kmb_dsi, dphy_no,
> TEST_CODE_SLEW_RATE_DDL_CYCLES,
> -(freq & 0x7f));
> -}
> -
> -static inline void
> - set_test_mode_src_osc_freq_target_hi_bits(struct kmb_dsi *kmb_dsi,
> - u32 dphy_no,
> - u32 freq)
> -{
> - u32 data;
> -
> - /* Flag this as high nibble */
> - data = ((freq >> 6) & 0x1f) | (1 << 7);
> -
> - /* Typical rise/fall time=166, refer Table 1207 databook,
> - * sr_osc_freq_target[11:7]
> - */
> - test_mode_send(kmb_dsi, dphy_no,
> TEST_CODE_SLEW_RATE_DDL_CYCLES, data);
> -}
> -
> static void mipi_tx_get_vco_params(struct vco_params *vco)
> {
> int i;
>
> base-commit: 93ee1a2c0f08345ab17c51198f725d4c95984f4c
> --
> 2.33.0.591.gddb1055343
RE: [PATCH] drm/kmb: Avoid warnings on impossible plane_id
Hi Kees,
This patch
https://patchwork.kernel.org/project/dri-devel/patch/20210728003126.1425028-13-anitha.chrisant...@intel.com/
is pushed to drm-misc-fixes. This change should fix the below warnings.
I apologize for all the inconveniences.
Thanks,
Anitha
> -Original Message-
> From: Chrisanthus, Anitha
> Sent: Wednesday, August 25, 2021 1:44 PM
> To: Kees Cook
> Cc: Dea, Edmund J ; David Airlie ;
> Daniel Vetter ; dri-devel@lists.freedesktop.org; Sam
> Ravnborg ; linux-ker...@vger.kernel.org; linux-
> harden...@vger.kernel.org
> Subject: RE: [PATCH] drm/kmb: Avoid warnings on impossible plane_id
>
> Hi Kees,
> Thanks for your patch.
> The switch statement is needed for multiple planes which is already approved
> in this patch series.
> https://patchwork.kernel.org/project/dri-
> devel/patch/20210728003126.1425028-13-anitha.chrisant...@intel.com/
>
> This patch has dependencies on the previous patches in this series, and we are
> waiting for reviews on those before this can be checked in.
>
> Thanks,
> Anitha
>
> > -Original Message-
> > From: Kees Cook
> > Sent: Wednesday, August 25, 2021 11:18 AM
> > To: Chrisanthus, Anitha
> > Cc: Kees Cook ; Dea, Edmund J
> > ; David Airlie ; Daniel Vetter
> > ; dri-devel@lists.freedesktop.org; Sam Ravnborg
> > ; linux-ker...@vger.kernel.org; linux-
> > harden...@vger.kernel.org
> > Subject: [PATCH] drm/kmb: Avoid warnings on impossible plane_id
> >
> > KMB_MAX_PLANES is defined as 1, yet kmb_plane_atomic_disable() had
> code
> > for writing beyond 1. It is gated by a WARN_ON() that would skip it,
> > though, but under some compiler versions, poor Dead Code Elimination
> > wasn't optimizing away the unused switch cases, leading to array bounds
> > warnings when building with -Warray-bounds:
> >
> > drivers/gpu/drm/kmb/kmb_plane.c:135:20: warning: array subscript 3 is
> > above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> > drivers/gpu/drm/kmb/kmb_plane.c:132:20: warning: array subscript 2 is
> > above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> > drivers/gpu/drm/kmb/kmb_plane.c:129:20: warning: array subscript 1 is
> > above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> >
> > Instead, just remove the switch statement entirely and adjust the index
> > type to match the struct "id" member.
> >
> > Cc: Anitha Chrisanthus
> > Cc: Edmund Dea
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: dri-devel@lists.freedesktop.org
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Signed-off-by: Kees Cook
> > ---
> > drivers/gpu/drm/kmb/kmb_plane.c | 18 ++
> > 1 file changed, 2 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> > b/drivers/gpu/drm/kmb/kmb_plane.c
> > index ecee6782612d..3d46e756f2fe 100644
> > --- a/drivers/gpu/drm/kmb/kmb_plane.c
> > +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> > @@ -113,7 +113,7 @@ static void kmb_plane_atomic_disable(struct
> > drm_plane *plane,
> > struct drm_atomic_state *state)
> > {
> > struct kmb_plane *kmb_plane = to_kmb_plane(plane);
> > - int plane_id = kmb_plane->id;
> > + unsigned char plane_id = kmb_plane->id;
> > struct kmb_drm_private *kmb;
> >
> > kmb = to_kmb(plane->dev);
> > @@ -121,21 +121,7 @@ static void kmb_plane_atomic_disable(struct
> > drm_plane *plane,
> > if (WARN_ON(plane_id >= KMB_MAX_PLANES))
> > return;
> >
> > - switch (plane_id) {
> > - case LAYER_0:
> > - kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
> > - break;
> > - case LAYER_1:
> > - kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
> > - break;
> > - case LAYER_2:
> > - kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
> > - break;
> > - case LAYER_3:
> > - kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
> > - break;
> > - }
> > -
> > + kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
> > kmb->plane_status[plane_id].disable = true;
> > }
> >
> > --
> > 2.30.2
RE: [PATCH linux-next] : add put_device() after of_find_device_by_node()
Hi Jing,
> -Original Message-
> From: Chrisanthus, Anitha
> Sent: Monday, August 23, 2021 4:25 PM
> To: jing yangyang
> Cc: Dea, Edmund J ; David Airlie ;
> Daniel Vetter ; dri-devel@lists.freedesktop.org; linux-
> ker...@vger.kernel.org; jing yangyang ; Zeal
> Robot
> Subject: RE: [PATCH linux-next] : add put_device() after
> of_find_device_by_node()
>
Thanks for the patch. Please add drm/kmb: to the subject line and also correct
the checkpatch warning.
With that ab.
> Acked-by: Anitha Chrisanthus
>
> > -Original Message-
> > From: jing yangyang
> > Sent: Thursday, August 19, 2021 7:10 PM
> > To: Chrisanthus, Anitha
> > Cc: Dea, Edmund J ; David Airlie
> ;
> > Daniel Vetter ; dri-devel@lists.freedesktop.org; linux-
> > ker...@vger.kernel.org; jing yangyang ; Zeal
> > Robot
> > Subject: [PATCH linux-next] : add put_device() after
> of_find_device_by_node()
> >
> > This was found by coccicheck:
> > ./drivers/gpu/drm/kmb/kmb_drv.c:503:2-8:
> > ERROR missing put_device; call of_find_device_by_node on line 490,
> > but without a corresponding object release within this function.
> >
> > Reported-by: Zeal Robot
> > Signed-off-by: jing yangyang
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> > b/drivers/gpu/drm/kmb/kmb_drv.c
> > index f54392e..58495a9 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -500,8 +500,10 @@ static int kmb_probe(struct platform_device *pdev)
> > ret = kmb_dsi_host_bridge_init(get_device(_pdev->dev));
> >
> > if (ret == -EPROBE_DEFER) {
> > + put_device(_pdev->dev);
> > return -EPROBE_DEFER;
> > } else if (ret) {
> > + put_device(_pdev->dev);
> > DRM_ERROR("probe failed to initialize DSI host bridge\n");
> > return ret;
> > }
> > @@ -509,9 +511,10 @@ static int kmb_probe(struct platform_device *pdev)
> > /* Create DRM device */
> > kmb = devm_drm_dev_alloc(dev, _driver,
> > struct kmb_drm_private, drm);
> > - if (IS_ERR(kmb))
> > + if (IS_ERR(kmb)) {
> > + put_device(_pdev->dev);
> > return PTR_ERR(kmb);
> > -
> > + }
> > dev_set_drvdata(dev, >drm);
> >
> > /* Initialize MIPI DSI */
> > --
> > 1.8.3.1
> >
RE: [PATCH] drm/kmb: Avoid warnings on impossible plane_id
Hi Kees,
Thanks for your patch.
The switch statement is needed for multiple planes which is already approved in
this patch series.
https://patchwork.kernel.org/project/dri-devel/patch/20210728003126.1425028-13-anitha.chrisant...@intel.com/
This patch has dependencies on the previous patches in this series, and we are
waiting for reviews on those before this can be checked in.
Thanks,
Anitha
> -Original Message-
> From: Kees Cook
> Sent: Wednesday, August 25, 2021 11:18 AM
> To: Chrisanthus, Anitha
> Cc: Kees Cook ; Dea, Edmund J
> ; David Airlie ; Daniel Vetter
> ; dri-devel@lists.freedesktop.org; Sam Ravnborg
> ; linux-ker...@vger.kernel.org; linux-
> harden...@vger.kernel.org
> Subject: [PATCH] drm/kmb: Avoid warnings on impossible plane_id
>
> KMB_MAX_PLANES is defined as 1, yet kmb_plane_atomic_disable() had code
> for writing beyond 1. It is gated by a WARN_ON() that would skip it,
> though, but under some compiler versions, poor Dead Code Elimination
> wasn't optimizing away the unused switch cases, leading to array bounds
> warnings when building with -Warray-bounds:
>
> drivers/gpu/drm/kmb/kmb_plane.c:135:20: warning: array subscript 3 is
> above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> drivers/gpu/drm/kmb/kmb_plane.c:132:20: warning: array subscript 2 is
> above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> drivers/gpu/drm/kmb/kmb_plane.c:129:20: warning: array subscript 1 is
> above array bounds of 'struct layer_status[1]' [-Warray-bounds]
>
> Instead, just remove the switch statement entirely and adjust the index
> type to match the struct "id" member.
>
> Cc: Anitha Chrisanthus
> Cc: Edmund Dea
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Kees Cook
> ---
> drivers/gpu/drm/kmb/kmb_plane.c | 18 ++
> 1 file changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> index ecee6782612d..3d46e756f2fe 100644
> --- a/drivers/gpu/drm/kmb/kmb_plane.c
> +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> @@ -113,7 +113,7 @@ static void kmb_plane_atomic_disable(struct
> drm_plane *plane,
>struct drm_atomic_state *state)
> {
> struct kmb_plane *kmb_plane = to_kmb_plane(plane);
> - int plane_id = kmb_plane->id;
> + unsigned char plane_id = kmb_plane->id;
> struct kmb_drm_private *kmb;
>
> kmb = to_kmb(plane->dev);
> @@ -121,21 +121,7 @@ static void kmb_plane_atomic_disable(struct
> drm_plane *plane,
> if (WARN_ON(plane_id >= KMB_MAX_PLANES))
> return;
>
> - switch (plane_id) {
> - case LAYER_0:
> - kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
> - break;
> - case LAYER_1:
> - kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
> - break;
> - case LAYER_2:
> - kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
> - break;
> - case LAYER_3:
> - kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
> - break;
> - }
> -
> + kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
> kmb->plane_status[plane_id].disable = true;
> }
>
> --
> 2.30.2
RE: [PATCH linux-next] : add put_device() after of_find_device_by_node()
Acked-by: Anitha Chrisanthus
> -Original Message-
> From: jing yangyang
> Sent: Thursday, August 19, 2021 7:10 PM
> To: Chrisanthus, Anitha
> Cc: Dea, Edmund J ; David Airlie ;
> Daniel Vetter ; dri-devel@lists.freedesktop.org; linux-
> ker...@vger.kernel.org; jing yangyang ; Zeal
> Robot
> Subject: [PATCH linux-next] : add put_device() after of_find_device_by_node()
>
> This was found by coccicheck:
> ./drivers/gpu/drm/kmb/kmb_drv.c:503:2-8:
> ERROR missing put_device; call of_find_device_by_node on line 490,
> but without a corresponding object release within this function.
>
> Reported-by: Zeal Robot
> Signed-off-by: jing yangyang
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index f54392e..58495a9 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -500,8 +500,10 @@ static int kmb_probe(struct platform_device *pdev)
> ret = kmb_dsi_host_bridge_init(get_device(_pdev->dev));
>
> if (ret == -EPROBE_DEFER) {
> + put_device(_pdev->dev);
> return -EPROBE_DEFER;
> } else if (ret) {
> + put_device(_pdev->dev);
> DRM_ERROR("probe failed to initialize DSI host bridge\n");
> return ret;
> }
> @@ -509,9 +511,10 @@ static int kmb_probe(struct platform_device *pdev)
> /* Create DRM device */
> kmb = devm_drm_dev_alloc(dev, _driver,
>struct kmb_drm_private, drm);
> - if (IS_ERR(kmb))
> + if (IS_ERR(kmb)) {
> + put_device(_pdev->dev);
> return PTR_ERR(kmb);
> -
> + }
> dev_set_drvdata(dev, >drm);
>
> /* Initialize MIPI DSI */
> --
> 1.8.3.1
>
RE: [PATCH v2 00/14] drm: Make DRM's IRQ helpers legacy
Hi Thomas,
> -Original Message-
> From: Thomas Zimmermann
> Sent: Wednesday, August 4, 2021 12:11 AM
> To: Chrisanthus, Anitha ; Sam Ravnborg
>
> Cc: dan...@ffwll.ch; airl...@linux.ie; alexander.deuc...@amd.com;
> christian.koe...@amd.com; liviu.du...@arm.com; brian.star...@arm.com;
> bbrezil...@kernel.org; nicolas.fe...@microchip.com;
> maarten.lankho...@linux.intel.com; mrip...@kernel.org; ste...@agner.ch;
> alison.w...@nxp.com; patrik.r.jakobs...@gmail.com; robdcl...@gmail.com;
> Dea, Edmund J ; s...@poorly.run;
> shawn...@kernel.org; s.ha...@pengutronix.de; ker...@pengutronix.de;
> jyri.sa...@iki.fi; to...@kernel.org; dan.sned...@microchip.com;
> tomi.valkei...@ideasonboard.com; amd-...@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-arm-
> m...@vger.kernel.org; freedr...@lists.freedesktop.org
> Subject: Re: [PATCH v2 00/14] drm: Make DRM's IRQ helpers legacy
>
> Hi
>
> Am 03.08.21 um 20:36 schrieb Chrisanthus, Anitha:
> > Hi Thomas,
> > Can you please hold off on applying the kmb patch, I am seeing some issues
> while testing. Modetest works, but video playback only plays once, and it
> fails
> the second time with this patch.
>
> Sounds a bit like the testing issue at [1]. For testing, you need the
> latest drm-misc-next or drm-tip. Specifically, you need commit
> 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK ioctls").
You are right, with the above patch video plays fine. It's all good now! Sorry
about the confusion.
>
> Let me know whether this works for you.
>
> Best regards
> Thomas
>
> [1] https://patchwork.freedesktop.org/patch/447057/?series=93078=1
>
> >
> > Thanks,
> > Anitha
> >
> >
> >> -Original Message-
> >> From: Sam Ravnborg
> >> Sent: Tuesday, August 3, 2021 8:05 AM
> >> To: Thomas Zimmermann
> >> Cc: dan...@ffwll.ch; airl...@linux.ie; alexander.deuc...@amd.com;
> >> christian.koe...@amd.com; liviu.du...@arm.com;
> brian.star...@arm.com;
> >> bbrezil...@kernel.org; nicolas.fe...@microchip.com;
> >> maarten.lankho...@linux.intel.com; mrip...@kernel.org;
> ste...@agner.ch;
> >> alison.w...@nxp.com; patrik.r.jakobs...@gmail.com; Chrisanthus, Anitha
> >> ; robdcl...@gmail.com; Dea, Edmund J
> >> ; s...@poorly.run; shawn...@kernel.org;
> >> s.ha...@pengutronix.de; ker...@pengutronix.de; jyri.sa...@iki.fi;
> >> to...@kernel.org; dan.sned...@microchip.com;
> >> tomi.valkei...@ideasonboard.com; amd-...@lists.freedesktop.org; dri-
> >> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> arm-
> >> m...@vger.kernel.org; freedr...@lists.freedesktop.org
> >> Subject: Re: [PATCH v2 00/14] drm: Make DRM's IRQ helpers legacy
> >>
> >> Hi Thomas,
> >>
> >> On Tue, Aug 03, 2021 at 11:06:50AM +0200, Thomas Zimmermann wrote:
> >>> DRM's IRQ helpers are only helpful for old, non-KMS drivers. Move
> >>> the code behind CONFIG_DRM_LEGACY. Convert KMS drivers to Linux
> >>> IRQ interfaces.
> >>>
> >>> DRM provides IRQ helpers for setting up, receiving and removing IRQ
> >>> handlers. It's an abstraction over plain Linux functions. The code
> >>> is mid-layerish with several callbacks to hook into the rsp drivers.
> >>> Old UMS driver have their interrupts enabled via ioctl, so these
> >>> abstractions makes some sense. Modern KMS manage all their interrupts
> >>> internally. Using the DRM helpers adds indirection without benefits.
> >>>
> >>> Most KMS drivers already use Linux IRQ functions instead of DRM's
> >>> abstraction layer. Patches 1 to 12 convert the remaining ones.
> >>> The patches also resolve a bug for devices without assigned interrupt
> >>> number. DRM helpers don't test for IRQ_NOTCONNECTED, so drivers do
> >>> not detect if the device has no interrupt assigned.
> >>>
> >>> Patch 13 removes an unused function.
> >>>
> >>> Patch 14 moves the DRM IRQ helpers behind CONFIG_DRM_LEGACY. Only
> >>> the old non-KMS drivers still use the functionality.
> >>>
> >>> v2:
> >>> * drop IRQ_NOTCONNECTED test from atmel-hlcdc (Sam)
> >>> * use devm_request_irq() in atmel-hlcdc (Sam)
> >>> * unify variable names in arm/hlcdc (Sam)
> >>>
> >>> Thomas Zimmermann (14):
> >>
> >> The following patches are all:
> >> Acked-by: Sam Ravnborg
> >>
> >>>drm/fsl-dcu: Convert to Linux IRQ interfaces
> >>>drm/gma500: Convert to Linux IRQ interfaces
> >>>drm/kmb: Convert to Linux IRQ interfaces
> >>>drm/msm: Convert to Linux IRQ interfaces
> >>>drm/mxsfb: Convert to Linux IRQ interfaces
> >>>drm/tidss: Convert to Linux IRQ interfaces
> >>>drm/vc4: Convert to Linux IRQ interfaces
> >>>drm: Remove unused devm_drm_irq_install()
> >>
> >> The remaining patches I either skipped or already had a feedback from
> >> me or I asked a question.
> >>
> >>Sam
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
RE: [PATCH 02/14] drm/kmb: Define driver date and major/minor version
Thanks Thomas, I'll keep this in mind for the next patch.
> -Original Message-
> From: dri-devel On Behalf Of
> Thomas Zimmermann
> Sent: Wednesday, August 4, 2021 11:13 AM
> To: Chrisanthus, Anitha ; dri-
> de...@lists.freedesktop.org; Dea, Edmund J
> Subject: Re: [PATCH 02/14] drm/kmb: Define driver date and major/minor
> version
>
> Hi,
>
> just a friendly reminder that branches that end with -fixes are for
> fixes that are required in upstream ASAP. I found this patch in
> drm-misc-fixes. It's not important, so it should have gone into
> drm-misc-next instead.
>
> Best regards
> Thomas
>
> Am 28.07.21 um 02:31 schrieb Anitha Chrisanthus:
> > From: Edmund Dea
> >
> > Added macros for date and version
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Signed-off-by: Edmund Dea
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 8
> > drivers/gpu/drm/kmb/kmb_drv.h | 5 +
> > 2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index c0b1c6f99249..f54392ec4fab 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -425,10 +425,10 @@ static const struct drm_driver kmb_driver = {
> > .fops = ,
> > DRM_GEM_CMA_DRIVER_OPS_VMAP,
> > .name = "kmb-drm",
> > - .desc = "KEEMBAY DISPLAY DRIVER ",
> > - .date = "20201008",
> > - .major = 1,
> > - .minor = 0,
> > + .desc = "KEEMBAY DISPLAY DRIVER",
> > + .date = DRIVER_DATE,
> > + .major = DRIVER_MAJOR,
> > + .minor = DRIVER_MINOR,
> > };
> >
> > static int kmb_remove(struct platform_device *pdev)
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.h
> b/drivers/gpu/drm/kmb/kmb_drv.h
> > index 02e806712a64..ebbaa5f422d5 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.h
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.h
> > @@ -15,6 +15,11 @@
> > #define KMB_MAX_HEIGHT1080 /*Max height in pixels */
> > #define KMB_MIN_WIDTH 1920 /*Max width in pixels */
> > #define KMB_MIN_HEIGHT 1080 /*Max height in pixels */
> > +
> > +#define DRIVER_DATE"20210223"
> > +#define DRIVER_MAJOR 1
> > +#define DRIVER_MINOR 1
> > +
> > #define KMB_LCD_DEFAULT_CLK 2
> > #define KMB_SYS_CLK_MHZ 500
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
RE: [PATCH v2 00/14] drm: Make DRM's IRQ helpers legacy
Hi Thomas, Can you please hold off on applying the kmb patch, I am seeing some issues while testing. Modetest works, but video playback only plays once, and it fails the second time with this patch. Thanks, Anitha > -Original Message- > From: Sam Ravnborg > Sent: Tuesday, August 3, 2021 8:05 AM > To: Thomas Zimmermann > Cc: dan...@ffwll.ch; airl...@linux.ie; alexander.deuc...@amd.com; > christian.koe...@amd.com; liviu.du...@arm.com; brian.star...@arm.com; > bbrezil...@kernel.org; nicolas.fe...@microchip.com; > maarten.lankho...@linux.intel.com; mrip...@kernel.org; ste...@agner.ch; > alison.w...@nxp.com; patrik.r.jakobs...@gmail.com; Chrisanthus, Anitha > ; robdcl...@gmail.com; Dea, Edmund J > ; s...@poorly.run; shawn...@kernel.org; > s.ha...@pengutronix.de; ker...@pengutronix.de; jyri.sa...@iki.fi; > to...@kernel.org; dan.sned...@microchip.com; > tomi.valkei...@ideasonboard.com; amd-...@lists.freedesktop.org; dri- > de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-arm- > m...@vger.kernel.org; freedr...@lists.freedesktop.org > Subject: Re: [PATCH v2 00/14] drm: Make DRM's IRQ helpers legacy > > Hi Thomas, > > On Tue, Aug 03, 2021 at 11:06:50AM +0200, Thomas Zimmermann wrote: > > DRM's IRQ helpers are only helpful for old, non-KMS drivers. Move > > the code behind CONFIG_DRM_LEGACY. Convert KMS drivers to Linux > > IRQ interfaces. > > > > DRM provides IRQ helpers for setting up, receiving and removing IRQ > > handlers. It's an abstraction over plain Linux functions. The code > > is mid-layerish with several callbacks to hook into the rsp drivers. > > Old UMS driver have their interrupts enabled via ioctl, so these > > abstractions makes some sense. Modern KMS manage all their interrupts > > internally. Using the DRM helpers adds indirection without benefits. > > > > Most KMS drivers already use Linux IRQ functions instead of DRM's > > abstraction layer. Patches 1 to 12 convert the remaining ones. > > The patches also resolve a bug for devices without assigned interrupt > > number. DRM helpers don't test for IRQ_NOTCONNECTED, so drivers do > > not detect if the device has no interrupt assigned. > > > > Patch 13 removes an unused function. > > > > Patch 14 moves the DRM IRQ helpers behind CONFIG_DRM_LEGACY. Only > > the old non-KMS drivers still use the functionality. > > > > v2: > > * drop IRQ_NOTCONNECTED test from atmel-hlcdc (Sam) > > * use devm_request_irq() in atmel-hlcdc (Sam) > > * unify variable names in arm/hlcdc (Sam) > > > > Thomas Zimmermann (14): > > The following patches are all: > Acked-by: Sam Ravnborg > > > drm/fsl-dcu: Convert to Linux IRQ interfaces > > drm/gma500: Convert to Linux IRQ interfaces > > drm/kmb: Convert to Linux IRQ interfaces > > drm/msm: Convert to Linux IRQ interfaces > > drm/mxsfb: Convert to Linux IRQ interfaces > > drm/tidss: Convert to Linux IRQ interfaces > > drm/vc4: Convert to Linux IRQ interfaces > > drm: Remove unused devm_drm_irq_install() > > The remaining patches I either skipped or already had a feedback from > me or I asked a question. > > Sam
RE: [PATCH 13/14] drm/kmb: Enable alpha blended second plane
Hi Sam,
Thanks. Where should this go, drm-misc-fixes or drm-misc-next?
Anitha
> -Original Message-
> From: Sam Ravnborg
> Sent: Wednesday, July 28, 2021 12:29 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J
>
> Subject: Re: [PATCH 13/14] drm/kmb: Enable alpha blended second plane
>
> Hi Anitha,
> On Tue, Jul 27, 2021 at 05:31:25PM -0700, Anitha Chrisanthus wrote:
> > From: Edmund Dea
> >
> > Enable one additional plane that is alpha blended on top
> > of the primary plane.
> >
> > Signed-off-by: Edmund Dea
> Your s-o-b is missing.
>
> With this fixed:
> Acked-by: Sam Ravnborg
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 8 ++--
> > drivers/gpu/drm/kmb/kmb_plane.c | 81 +-
> ---
> > drivers/gpu/drm/kmb/kmb_plane.h | 5 +-
> > drivers/gpu/drm/kmb/kmb_regs.h | 3 ++
> > 4 files changed, 82 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index 12f35c43d838..d0de1db03493 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -173,10 +173,10 @@ static int kmb_setup_mode_config(struct
> drm_device *drm)
> > ret = drmm_mode_config_init(drm);
> > if (ret)
> > return ret;
> > - drm->mode_config.min_width = KMB_MIN_WIDTH;
> > - drm->mode_config.min_height = KMB_MIN_HEIGHT;
> > - drm->mode_config.max_width = KMB_MAX_WIDTH;
> > - drm->mode_config.max_height = KMB_MAX_HEIGHT;
> > + drm->mode_config.min_width = KMB_FB_MIN_WIDTH;
> > + drm->mode_config.min_height = KMB_FB_MIN_HEIGHT;
> > + drm->mode_config.max_width = KMB_FB_MAX_WIDTH;
> > + drm->mode_config.max_height = KMB_FB_MAX_HEIGHT;
> > drm->mode_config.funcs = _mode_config_funcs;
> >
> > ret = kmb_setup_crtc(drm);
> > diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> > index 4523af949ea1..cbe4e981d73e 100644
> > --- a/drivers/gpu/drm/kmb/kmb_plane.c
> > +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> > @@ -118,9 +118,10 @@ static int kmb_plane_atomic_check(struct
> drm_plane *plane,
> > if (ret)
> > return ret;
> >
> > - if (new_plane_state->crtc_w > KMB_MAX_WIDTH || new_plane_state-
> >crtc_h > KMB_MAX_HEIGHT)
> > - return -EINVAL;
> > - if (new_plane_state->crtc_w < KMB_MIN_WIDTH || new_plane_state-
> >crtc_h < KMB_MIN_HEIGHT)
> > + if (new_plane_state->crtc_w > KMB_FB_MAX_WIDTH ||
> > + new_plane_state->crtc_h > KMB_FB_MAX_HEIGHT ||
> > + new_plane_state->crtc_w < KMB_FB_MIN_WIDTH ||
> > + new_plane_state->crtc_h < KMB_FB_MIN_HEIGHT)
> > return -EINVAL;
> >
> > /* Due to HW limitations, changing plane height or width after
> > @@ -311,6 +312,44 @@ static void config_csc(struct kmb_drm_private
> *kmb, int plane_id)
> > kmb_write_lcd(kmb, LCD_LAYERn_CSC_OFF3(plane_id),
> csc_coef_lcd[11]);
> > }
> >
> > +static void kmb_plane_set_alpha(struct kmb_drm_private *kmb,
> > + const struct drm_plane_state *state,
> > + unsigned char plane_id,
> > + unsigned int *val)
> > +{
> > + u16 plane_alpha = state->alpha;
> > + u16 pixel_blend_mode = state->pixel_blend_mode;
> > + int has_alpha = state->fb->format->has_alpha;
> > +
> > + if (plane_alpha != DRM_BLEND_ALPHA_OPAQUE)
> > + *val |= LCD_LAYER_ALPHA_STATIC;
> > +
> > + if (has_alpha) {
> > + switch (pixel_blend_mode) {
> > + case DRM_MODE_BLEND_PIXEL_NONE:
> > + break;
> > + case DRM_MODE_BLEND_PREMULTI:
> > + *val |= LCD_LAYER_ALPHA_EMBED |
> LCD_LAYER_ALPHA_PREMULT;
> > + break;
> > + case DRM_MODE_BLEND_COVERAGE:
> > + *val |= LCD_LAYER_ALPHA_EMBED;
> > + break;
> > + default:
> > + DRM_DEBUG("Missing pixel blend mode case (%s ==
> %ld)\n",
> > + __stringify(pixel_blend_mode),
> > + (long)pixel_blend_mode);
> > + break;
> > + }
> > + }
> > +
> > + if (plane_alpha == DRM_BLEND_ALPHA_OPAQUE && !has_alpha) {
> > + *val &= LCD_LAYER_ALPHA_DISABLED
RE: [PATCH 01/14] drm/kmb: Enable LCD DMA for low TVDDCV
Hi Sam,
Please help! I tried to push the first two patches to drm-misc-fixes using dim
push, but it pushed other things too besides these patches. I am sorry, don't
know what went wrong.
Anitha
> -Original Message-
> From: Sam Ravnborg
> Sent: Wednesday, July 28, 2021 12:05 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J
>
> Subject: Re: [PATCH 01/14] drm/kmb: Enable LCD DMA for low TVDDCV
>
> Hi Anitha,
> On Tue, Jul 27, 2021 at 05:31:13PM -0700, Anitha Chrisanthus wrote:
> > From: Edmund Dea
> >
> > There's an undocumented dependency between LCD layer enable bits [2-5]
> > and the AXI pipelined read enable bit [28] in the LCD_CONTROL register.
> > The proper order of operation is:
> >
> > 1) Clear AXI pipelined read enable bit
> > 2) Set LCD layers
> > 3) Set AXI pipelined read enable bit
> >
> > With this update, LCD can start DMA when TVDDCV is reduced down to
> 700mV.
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Signed-off-by: Edmund Dea
> Patch is missing your s-o-b.
>
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 14 ++
> > drivers/gpu/drm/kmb/kmb_plane.c | 15 +--
> > 2 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index 96ea1a2c11dd..c0b1c6f99249 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -203,6 +203,7 @@ static irqreturn_t handle_lcd_irq(struct drm_device
> *dev)
> > unsigned long status, val, val1;
> > int plane_id, dma0_state, dma1_state;
> > struct kmb_drm_private *kmb = to_kmb(dev);
> > + u32 ctrl = 0;
> >
> > status = kmb_read_lcd(kmb, LCD_INT_STATUS);
> >
> > @@ -227,6 +228,19 @@ static irqreturn_t handle_lcd_irq(struct drm_device
> *dev)
> > kmb_clr_bitmask_lcd(kmb, LCD_CONTROL,
> > kmb-
> >plane_status[plane_id].ctrl);
> >
> > + ctrl = kmb_read_lcd(kmb, LCD_CONTROL);
> > + if (!(ctrl & (LCD_CTRL_VL1_ENABLE |
> > + LCD_CTRL_VL2_ENABLE |
> > + LCD_CTRL_GL1_ENABLE |
> > + LCD_CTRL_GL2_ENABLE))) {
> > + /* If no LCD layers are using DMA,
> > +* then disable DMA pipelined AXI read
> > +* transactions.
> > +*/
> > + kmb_clr_bitmask_lcd(kmb,
> LCD_CONTROL,
> > +
> LCD_CTRL_PIPELINE_DMA);
> > + }
> > +
> This function could benefit from a few helper functions to avoid all the
> indent. But this is un-related to this patch.
>
> > kmb->plane_status[plane_id].disable = false;
> > }
> > }
> > diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> > index d5b6195856d1..2888dd5dcc2c 100644
> > --- a/drivers/gpu/drm/kmb/kmb_plane.c
> > +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> > @@ -427,8 +427,14 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> >
> > kmb_set_bitmask_lcd(kmb, LCD_CONTROL, ctrl);
> >
> > - /* FIXME no doc on how to set output format,these values are
> > -* taken from the Myriadx tests
> > + /* Enable pipeline AXI read transactions for the DMA
> > +* after setting graphics layers. This must be done
> > +* in a separate write cycle.
> > +*/
> > + kmb_set_bitmask_lcd(kmb, LCD_CONTROL,
> LCD_CTRL_PIPELINE_DMA);
> > +
> > + /* FIXME no doc on how to set output format,these values are taken
> ^ add space
> > +* from the Myriadx tests
> > */
> > out_format |= LCD_OUTF_FORMAT_RGB888;
> >
> > @@ -526,6 +532,11 @@ struct kmb_plane *kmb_plane_init(struct
> drm_device *drm)
> > plane->id = i;
> > }
> >
> > + /* Disable pipeline AXI read transactions for the DMA
> > +* prior to setting graphics layers
> > +*/
> > + kmb_clr_bitmask_lcd(kmb, LCD_CONTROL,
> LCD_CTRL_PIPELINE_DMA);
> > +
> > return primary;
> > cleanup:
> > drmm_kfree(drm, plane);
>
> With the two nits fixed:
> Acked-by: Sam Ravnborg
RE: [PATCH 12/14] drm/kmb: Fix possible oops in error handling
Hi Sam,
> -Original Message-
> From: Sam Ravnborg
> Sent: Wednesday, July 28, 2021 12:27 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J
> ; Dan Carpenter
> Subject: Re: [PATCH 12/14] drm/kmb: Fix possible oops in error handling
>
> Hi Anitha,
>
> On Tue, Jul 27, 2021 at 05:31:24PM -0700, Anitha Chrisanthus wrote:
> > If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
> > This can potentially result in kernel panic when kmb_dsi_host_unregister is
> > called.
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
> > Cc: Dan Carpenter
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 9 ++---
> > drivers/gpu/drm/kmb/kmb_dsi.c | 9 +
> > drivers/gpu/drm/kmb/kmb_dsi.h | 3 ++-
> > 3 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index bb7eca9e13ae..12f35c43d838 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -454,8 +454,9 @@ static int kmb_remove(struct platform_device
> *pdev)
> > dev_set_drvdata(dev, NULL);
> >
> > /* Unregister DSI host */
> > - kmb_dsi_host_unregister(kmb->kmb_dsi);
> > + kmb_dsi_host_unregister();
> > drm_atomic_helper_shutdown(drm);
> > + drm_dev_put(drm);
> > return 0;
> > }
> >
> > @@ -519,7 +520,7 @@ static int kmb_probe(struct platform_device *pdev)
> > if (IS_ERR(kmb->kmb_dsi)) {
> > drm_err(>drm, "failed to initialize DSI\n");
> > ret = PTR_ERR(kmb->kmb_dsi);
> > - goto err_free1;
> > + goto err_free2;
> > }
> >
> > kmb->kmb_dsi->dev = _pdev->dev;
> > @@ -555,8 +556,10 @@ static int kmb_probe(struct platform_device *pdev)
> > drm_crtc_cleanup(>crtc);
> > drm_mode_config_cleanup(>drm);
> > err_free1:
> > + kmb_dsi_clk_disable(kmb->kmb_dsi);
> > + err_free2:
> > dev_set_drvdata(dev, NULL);
> > - kmb_dsi_host_unregister(kmb->kmb_dsi);
> > + kmb_dsi_host_unregister();
> >
>
> This really looks like a step backward. There should not be a eed to
> call unregister if kmb_dsi is not a valid pointer in the first place.
> Also drn_dev_put() should not be needed with the use of drmm
> infrastructure.
Agree, I was trying to address issues with Dan's original patch.
I will keep the original code, with only this change
if (IS_ERR(kmb->kmb_dsi)) {
drm_err(>drm, "failed to initialize DSI\n");
- ret = PTR_ERR(kmb->kmb_dsi);
- goto err_free2;
+ dev_set_drvdata(dev, NULL);
+ return PTR_ERR(kmb->kmb_dsi);
Will send v2
>
>
>
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c
> b/drivers/gpu/drm/kmb/kmb_dsi.c
> > index 1cca0fe6f35f..a500172ada87 100644
> > --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> > +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> > @@ -172,17 +172,17 @@
> mipi_hs_freq_range[MIPI_DPHY_DEFAULT_BIT_RATES] = {
> > {.default_bit_rate_mbps = 2500, .hsfreqrange_code = 0x49}
> > };
> >
> > -static void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi)
> > +void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi)
> > {
> > clk_disable_unprepare(kmb_dsi->clk_mipi);
> > clk_disable_unprepare(kmb_dsi->clk_mipi_ecfg);
> > clk_disable_unprepare(kmb_dsi->clk_mipi_cfg);
> > }
> >
> > -void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi)
> > +void kmb_dsi_host_unregister(void)
> > {
> > - kmb_dsi_clk_disable(kmb_dsi);
> > - mipi_dsi_host_unregister(kmb_dsi->host);
> > + if (dsi_host)
> > + mipi_dsi_host_unregister(dsi_host);
> > }
> I thought we had killed the global dsi_host variable??
> Seems some cleanup is till needed here.
>
> Sam
RE: [PATCH 14/14] drm/kmb: Enable support for fbcon (framebuffer console)
Hi Sam,
Thanks for the review.
> -Original Message-
> From: Sam Ravnborg
> Sent: Wednesday, July 28, 2021 12:31 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J
>
> Subject: Re: [PATCH 14/14] drm/kmb: Enable support for fbcon (framebuffer
> console)
>
> Hi Anitha,
>
> On Tue, Jul 27, 2021 at 05:31:26PM -0700, Anitha Chrisanthus wrote:
> > From: Edmund Dea
> >
> > Enable support for fbcon (framebuffer console).
> > The user can initialize fbcon by loading kmb-drm with the parameter
> > console=1.
>
> I do not see the poit of the boot parameter??
> Why is it needed here but not in other drivers?
By default, console is not enabled in this driver, customer only needs it when
they are doing initial setups
and then want it disabled after.
>
> >
> > Signed-off-by: Edmund Dea
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 13 -
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index d0de1db03493..d39d004f513a 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -5,6 +5,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -15,6 +16,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -25,7 +27,13 @@
> > #include "kmb_dsi.h"
> > #include "kmb_regs.h"
> >
> > -static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
> > +/* Module Parameters */
> > +static bool console;
> > +module_param(console, bool, 0400);
> > +MODULE_PARM_DESC(console,
> > +"Enable framebuffer console support (0=disable [default],
> 1=on)");
> > +
> > +int kmb_display_clk_enable(struct kmb_drm_private *kmb)
> kmb_display_clk_enable lost a "static" - it will result in a warning if
> you build with W=1
Will fix it in v2
>
> > {
> > int ret = 0;
> >
> > @@ -546,6 +554,9 @@ static int kmb_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_register;
> >
> > + if (console)
> > + drm_fbdev_generic_setup(>drm, 32);
> > +
> > return 0;
> >
> > err_register:
> > --
> > 2.25.1
RE: [PATCH 04/14] drm/kmb : W/A for 256B cache alignment for video
Hi Sam,
Thanks for you review.
> -Original Message-
> From: Sam Ravnborg
> Sent: Wednesday, July 28, 2021 12:16 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J
>
> Subject: Re: [PATCH 04/14] drm/kmb : W/A for 256B cache alignment for video
>
> Hi Anitha,
> On Tue, Jul 27, 2021 at 05:31:16PM -0700, Anitha Chrisanthus wrote:
> > For B0 silicon, the media driver pads the decoded video dmabufs for 256B
> > alignment. This is the backing buffer of the framebuffer and info in the
> > drm frame buffer is not correct for these buffers as this is done
> > internally in the media driver. This change extracts the meta data info
> > from dmabuf priv structure and uses that info for programming stride and
> > offsets in kmb_plane_atomic_update().
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Signed-off-by: Edmund Dea
> > Signed-off-by: Anitha Chrisanthus
>
> Drop extra space in subject before ':'
ok
>
>
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.h| 1 +
> > drivers/gpu/drm/kmb/kmb_plane.c | 38 ---
> > drivers/gpu/drm/kmb/kmb_vidmem.h | 52
>
> > 3 files changed, 87 insertions(+), 4 deletions(-)
> > create mode 100644 drivers/gpu/drm/kmb/kmb_vidmem.h
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.h
> b/drivers/gpu/drm/kmb/kmb_drv.h
> > index ebbaa5f422d5..0904e6eb2a09 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.h
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.h
> > @@ -49,6 +49,7 @@ struct kmb_drm_private {
> > int kmb_under_flow;
> > int kmb_flush_done;
> > int layer_no;
> > + struct viv_vidmem_metadata *md_info;
> I cannot see this member used in this patch - can it be dropped?
Good catch, yes will remove it.
>
> > };
> >
> > static inline struct kmb_drm_private *to_kmb(const struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> > index 2888dd5dcc2c..e45419d6ed96 100644
> > --- a/drivers/gpu/drm/kmb/kmb_plane.c
> > +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> > @@ -11,12 +11,16 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > +#include
> > +
> > #include "kmb_drv.h"
> > #include "kmb_plane.h"
> > #include "kmb_regs.h"
> > +#include "kmb_vidmem.h"
> >
> > const u32 layer_irqs[] = {
> > LCD_INT_VL0,
> > @@ -294,8 +298,10 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> > unsigned int ctrl = 0, val = 0, out_format = 0;
> > unsigned int src_w, src_h, crtc_x, crtc_y;
> > unsigned char plane_id;
> > - int num_planes;
> > + int num_planes, i;
> > static dma_addr_t addr[MAX_SUB_PLANES];
> > + struct viv_vidmem_metadata *md = NULL;
> > + struct drm_gem_object *gem_obj;
> >
> > if (!plane || !new_plane_state || !old_plane_state)
> > return;
> > @@ -325,6 +331,16 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> > drm_dbg(>drm,
> > "src_w=%d src_h=%d, fb->format->format=0x%x fb-
> >flags=0x%x\n",
> > src_w, src_h, fb->format->format, fb->flags);
> > + gem_obj = drm_gem_fb_get_obj(fb, plane_id);
> > + if (gem_obj && gem_obj->import_attach &&
> > + gem_obj->import_attach->dmabuf &&
> > + gem_obj->import_attach->dmabuf->priv) {
> > + md = gem_obj->import_attach->dmabuf->priv;
> > +
> > + /* Check if metadata is coming from hantro driver */
> > + if (md->magic != HANTRO_IMAGE_VIV_META_DATA_MAGIC)
> > + md = NULL;
> > + }
> >
> > width = fb->width;
> > height = fb->height;
> > @@ -332,6 +348,11 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> > drm_dbg(>drm, "dma_len=%d ", dma_len);
> > kmb_write_lcd(kmb, LCD_LAYERn_DMA_LEN(plane_id), dma_len);
> > kmb_write_lcd(kmb, LCD_LAYERn_DMA_LEN_SHADOW(plane_id),
> dma_len);
> > + if (md) {
> > + for (i = 0; i < 3; i++)
> > + fb->pitches[i] = md->plane[i].stride;
> > + }
> > +
> > kmb_write_lcd(kmb, LCD_LAYERn_DMA_LINE_VSTRIDE(plane_id),
>
RE: [PATCH 09/14] drm/kmb : W/A for planar formats
Please ignore this patch. Will combine this with 256B w/a patch.
> -Original Message-
> From: Chrisanthus, Anitha
> Sent: Tuesday, July 27, 2021 5:31 PM
> To: dri-devel@lists.freedesktop.org; Chrisanthus, Anitha
> ; Dea, Edmund J
> Subject: [PATCH 09/14] drm/kmb : W/A for planar formats
>
> This is a work around for fully planar formats, where color corruption
> was observed for formats like YU12, YU16 etc. Set the DMA Vstride and
> Line width for U and V planes to the same as the Y plane and not the
> actual pitch. For decoded video frames, continue to use the info from
> metadata.
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Anitha Chrisanthus
> ---
> drivers/gpu/drm/kmb/kmb_plane.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> index dacec5c4266f..4523af949ea1 100644
> --- a/drivers/gpu/drm/kmb/kmb_plane.c
> +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> @@ -333,6 +333,7 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> struct disp_cfg *init_disp_cfg;
> struct viv_vidmem_metadata *md = NULL;
> struct drm_gem_object *gem_obj;
> + unsigned int cb_stride, cr_stride;
>
> if (!plane || !new_plane_state || !old_plane_state)
> return;
> @@ -397,8 +398,10 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> val |= get_bits_per_pixel(fb->format);
> /* Program Cb/Cr for planar formats */
> if (num_planes > 1) {
> - kmb_write_lcd(kmb,
> LCD_LAYERn_DMA_CB_LINE_VSTRIDE(plane_id),
> - fb->pitches[1]);
> + cb_stride = md ? fb->pitches[1] : width * fb->format->cpp[0];
> + kmb_write_lcd(kmb,
> + LCD_LAYERn_DMA_CB_LINE_VSTRIDE(plane_id),
> + cb_stride);
> kmb_write_lcd(kmb,
> LCD_LAYERn_DMA_CB_LINE_WIDTH(plane_id),
> (width * fb->format->cpp[0]));
>
> @@ -419,9 +422,11 @@ static void kmb_plane_atomic_update(struct
> drm_plane *plane,
> addr[U_PLANE]);
>
> if (num_planes == 3) {
> + cr_stride = md ? fb->pitches[2] :
> + width * fb->format->cpp[0];
> kmb_write_lcd(kmb,
>
> LCD_LAYERn_DMA_CR_LINE_VSTRIDE(plane_id),
> - fb->pitches[2]);
> + cr_stride);
>
> kmb_write_lcd(kmb,
>
> LCD_LAYERn_DMA_CR_LINE_WIDTH(plane_id),
> --
> 2.25.1
RE: [PATCH 1/1] drm/kmb: Fix error return code in kmb_hw_init()
Thanks for the patch. Patch pushed to drm-misc-fixes.
Anitha
> -Original Message-
> From: Chrisanthus, Anitha
> Sent: Thursday, May 13, 2021 10:12 AM
> To: Zhen Lei ; Dea, Edmund J
> ; David Airlie ; Daniel Vetter
> ; Sam Ravnborg ; dri-devel de...@lists.freedesktop.org>
> Subject: RE: [PATCH 1/1] drm/kmb: Fix error return code in kmb_hw_init()
>
> Thanks for the patch.
>
> Reviewed-by: Anitha Chrisanthus
>
> > -Original Message-
> > From: Zhen Lei
> > Sent: Thursday, May 13, 2021 6:47 AM
> > To: Chrisanthus, Anitha ; Dea, Edmund J
> > ; David Airlie ; Daniel Vetter
> > ; Sam Ravnborg ; dri-devel > de...@lists.freedesktop.org>
> > Cc: Zhen Lei
> > Subject: [PATCH 1/1] drm/kmb: Fix error return code in kmb_hw_init()
> >
> > When call platform_get_irq() to obtain the IRQ of the lcd fails, the
> > returned error code should be propagated. However, we currently do not
> > explicitly assign this error code to 'ret'. As a result, 0 was incorrectly
> > returned.
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Reported-by: Hulk Robot
> > Signed-off-by: Zhen Lei
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> > b/drivers/gpu/drm/kmb/kmb_drv.c
> > index f64e06e1067dd8d..96ea1a2c11dd6a3 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -137,6 +137,7 @@ static int kmb_hw_init(struct drm_device *drm,
> > unsigned long flags)
> > /* Allocate LCD interrupt resources */
> > irq_lcd = platform_get_irq(pdev, 0);
> > if (irq_lcd < 0) {
> > + ret = irq_lcd;
> > drm_err(>drm, "irq_lcd not found");
> > goto setup_fail;
> > }
> > --
> > 2.26.0.106.g9fadedd
> >
RE: [PATCH v2 2/2] drm/kmb: Do not report 0 (success) in case of error
This is already fixed in the patch from Zhen Lei.
> -Original Message-
> From: Christophe JAILLET
> Sent: Wednesday, May 26, 2021 11:10 PM
> To: Chrisanthus, Anitha ; Dea, Edmund J
> ; airl...@linux.ie; dan...@ffwll.ch;
> s...@ravnborg.org
> Cc: dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; kernel-
> janit...@vger.kernel.org; Christophe JAILLET
> Subject: [PATCH v2 2/2] drm/kmb: Do not report 0 (success) in case of error
>
> 'ret' is known to be 0 at this point.
> Reporting the error from the previous 'platform_get_irq()' call is likely,
> so add the missing assignment.
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Christophe JAILLET
> ---
> v2: New patch
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index fa28e42da460..d9e10ac9847c 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -138,6 +138,7 @@ static int kmb_hw_init(struct drm_device *drm,
> unsigned long flags)
> irq_lcd = platform_get_irq(pdev, 0);
> if (irq_lcd < 0) {
> drm_err(>drm, "irq_lcd not found");
> + ret = irq_lcd;
> goto setup_fail;
> }
>
> --
> 2.30.2
RE: [PATCH] drm/kmb: Fix an error handling path
Hi Christophe,
Thanks for the patch, good catch! Patch looks good, few minor comments.
Anitha
> -Original Message-
> From: Christophe JAILLET
> Sent: Wednesday, May 19, 2021 1:47 PM
> To: Chrisanthus, Anitha ; Dea, Edmund J
> ; airl...@linux.ie; dan...@ffwll.ch;
> s...@ravnborg.org
> Cc: dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; kernel-
> janit...@vger.kernel.org; Christophe JAILLET
> Subject: [PATCH] drm/kmb: Fix an error handling path
>
> If 'platform_get_irq()' fails, it is spurious to call
> 'of_reserved_mem_device_release()' in the error handling path, because
> 'of_reserved_mem_device_init() has not been called yet.
>
> Moreover, a previous 'kmb_initialize_clocks()' is unbalanced by a
> corresponding 'kmb_display_clk_disable()' call, has already done in the
> remove function.
>
> It is likely that 'kmb_display_clk_disable()' is expected in the error
> handling path, instead of 'kmb_display_clk_disable()'.
You mean instead of of_reserved_mem_device_release()
>
>
> Also, it is spurious to return directly if 'of_reserved_mem_device_init()'
> fails.
> Goto the error handling path instead to free some resources.
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Christophe JAILLET
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index f64e06e1067d..b41b8789fe57 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -138,13 +138,13 @@ static int kmb_hw_init(struct drm_device *drm,
> unsigned long flags)
> irq_lcd = platform_get_irq(pdev, 0);
> if (irq_lcd < 0) {
> drm_err(>drm, "irq_lcd not found");
> - goto setup_fail;
> + goto disable_clk_err;
Keep setup_fail label or something like err_free_clocks
> }
>
> /* Get the optional framebuffer memory resource */
> ret = of_reserved_mem_device_init(drm->dev);
> if (ret && ret != -ENODEV)
> - return ret;
> + goto disable_clk_err;
>
> spin_lock_init(>irq_lock);
>
> @@ -152,8 +152,8 @@ static int kmb_hw_init(struct drm_device *drm,
> unsigned long flags)
>
> return 0;
>
> - setup_fail:
> - of_reserved_mem_device_release(drm->dev);
> + disable_clk_err:
> + kmb_display_clk_disable(kmb);
>
> return ret;
> }
> --
> 2.30.2
RE: [PATCH 1/1] drm/kmb: Fix error return code in kmb_hw_init()
Thanks for the patch.
Reviewed-by: Anitha Chrisanthus
> -Original Message-
> From: Zhen Lei
> Sent: Thursday, May 13, 2021 6:47 AM
> To: Chrisanthus, Anitha ; Dea, Edmund J
> ; David Airlie ; Daniel Vetter
> ; Sam Ravnborg ; dri-devel de...@lists.freedesktop.org>
> Cc: Zhen Lei
> Subject: [PATCH 1/1] drm/kmb: Fix error return code in kmb_hw_init()
>
> When call platform_get_irq() to obtain the IRQ of the lcd fails, the
> returned error code should be propagated. However, we currently do not
> explicitly assign this error code to 'ret'. As a result, 0 was incorrectly
> returned.
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Reported-by: Hulk Robot
> Signed-off-by: Zhen Lei
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index f64e06e1067dd8d..96ea1a2c11dd6a3 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -137,6 +137,7 @@ static int kmb_hw_init(struct drm_device *drm,
> unsigned long flags)
> /* Allocate LCD interrupt resources */
> irq_lcd = platform_get_irq(pdev, 0);
> if (irq_lcd < 0) {
> + ret = irq_lcd;
> drm_err(>drm, "irq_lcd not found");
> goto setup_fail;
> }
> --
> 2.26.0.106.g9fadedd
>
RE: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
Thanks Dan.
> -Original Message-
> From: Dan Carpenter
> Sent: Sunday, November 29, 2020 11:48 PM
> To: Chrisanthus, Anitha
> Cc: Dea, Edmund J ; David Airlie ;
> Daniel Vetter ; Sam Ravnborg ; dri-
> de...@lists.freedesktop.org; kernel-janit...@vger.kernel.org
> Subject: Re: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
>
> On Fri, Nov 20, 2020 at 10:15:57PM +, Chrisanthus, Anitha wrote:
> > Hi Dan,
> > I see the problem now, thanks for the patch.
> >
> > > -Original Message-
> > > From: Dan Carpenter
> > > Sent: Friday, November 20, 2020 12:11 AM
> > > To: Chrisanthus, Anitha
> > > Cc: Dea, Edmund J ; David Airlie
> ;
> > > Daniel Vetter ; Sam Ravnborg ; dri-
> > > de...@lists.freedesktop.org; kernel-janit...@vger.kernel.org
> > > Subject: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
> > >
> > > If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
> > > The kernel will Oops when we pass it to kmb_dsi_host_unregister().
> > >
> > > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > > Signed-off-by: Dan Carpenter
> > > ---
> > > v2: write a better commit message
> > >
> > > drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> > > b/drivers/gpu/drm/kmb/kmb_drv.c
> > > index a31a840ce634..8c43b136765c 100644
> > > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > > @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device
> *pdev)
> > > if (IS_ERR(kmb->kmb_dsi)) {
> > > drm_err(>drm, "failed to initialize DSI\n");
> > > ret = PTR_ERR(kmb->kmb_dsi);
> > > - goto err_free1;
> > > + goto err_clear_drvdata;
> > > }
> > >
> > > kmb->kmb_dsi->dev = _pdev->dev;
> > > @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device
> *pdev)
> > > drm_crtc_cleanup(>crtc);
> > > drm_mode_config_cleanup(>drm);
> > > err_free1:
> > > - dev_set_drvdata(dev, NULL);
> > > kmb_dsi_host_unregister(kmb->kmb_dsi);
> > > + err_clear_drvdata:
> > We still need to unregister the dsi_host that was registered in this call
> kmb_dsi_host_bridge_init.
> > This will require more changes in kmb_dsi_host_unregister and/or separate
> out mipi_dsi_host_unregister.
> > FYI - I will be out all of next week, will be back the next Monday.
>
> Hm... Yes. Now that you point it out, there are several bugs related
> to kmb_dsi_host_bridge_init()...
>
>182 void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi)
>183 {
>184 kmb_dsi_clk_disable(kmb_dsi);
>185 mipi_dsi_host_unregister(kmb_dsi->host);
> ^
> kmb_dsi->host is dsi_host.
>
> Every user unregisters it, but only the first user registers it. So
> if there are multiple users it will be unregistered prematurely. Should
> there be a kfree to prevent a leak?
>
> kfree(kmb_dsi->host);
> dsi_host = NULL;
>
>186 }
>
> [ snip ]
>
>216 int kmb_dsi_host_bridge_init(struct device *dev)
>217 {
>218 struct device_node *encoder_node, *dsi_out;
>219
>220 /* Create and register MIPI DSI host */
>221 if (!dsi_host) {
>
> This is only allocated for the first user.
>
>222 dsi_host = kzalloc(sizeof(*dsi_host), GFP_KERNEL);
>223 if (!dsi_host)
>224 return -ENOMEM;
>225
>226 dsi_host->ops = _dsi_host_ops;
>227
>228 if (!dsi_device) {
>229 dsi_device = kzalloc(sizeof(*dsi_device),
> GFP_KERNEL);
>230 if (!dsi_device) {
>231 kfree(dsi_host);
> ^^^
> But now it is non-NULL but it is a freed pointer. dsi_host = NULL;
>
>232 return -ENOMEM;
>233 }
>234 }
>235
>236 dsi_host->dev = dev;
>237 mipi_dsi_host_register(dsi_host);
>238 }
>239
>
RE: [PATCH] drm/kmb: fix array bounds warning
Hi Arnd,
Thanks for your patch.
> -Original Message-
> From: Arnd Bergmann
> Sent: Sunday, November 29, 2020 12:09 PM
> To: Chrisanthus, Anitha ; Dea, Edmund J
> ; David Airlie ; Daniel Vetter
> ; Sam Ravnborg
> Cc: Arnd Bergmann ; lkp ; dri-
> de...@lists.freedesktop.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] drm/kmb: fix array bounds warning
>
> From: Arnd Bergmann
>
> gcc warns about an out-of-bounds access when the using nonzero
> values for 'plane_id' on kmb->plane_status:
>
> drivers/gpu/drm/kmb/kmb_plane.c: In function 'kmb_plane_atomic_disable':
> drivers/gpu/drm/kmb/kmb_plane.c:128:20: warning: array subscript 3 is
> above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> 128 | kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
> | ~^~
> drivers/gpu/drm/kmb/kmb_plane.c:125:20: warning: array subscript 2 is
> above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> 125 | kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
> | ~^~
> drivers/gpu/drm/kmb/kmb_plane.c:122:20: warning: array subscript 1 is
> above array bounds of 'struct layer_status[1]' [-Warray-bounds]
> 122 | kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
>
> Having the array truncated to one entry seems intentional, so add
> a range check before indexing it to make it clearer what is going
> on and shut up the warning.
>
> I received the warning from the kernel test robot after a private
> patch that turns on Warray-bounds unconditionally.
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Reported-by: kernel test robot
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/kmb/kmb_plane.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_plane.c
> b/drivers/gpu/drm/kmb/kmb_plane.c
> index 8448d1edb553..be8eea3830c1 100644
> --- a/drivers/gpu/drm/kmb/kmb_plane.c
> +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> @@ -114,6 +114,9 @@ static void kmb_plane_atomic_disable(struct
> drm_plane *plane,
>
> kmb = to_kmb(plane->dev);
>
> + if (WARN_ON(plane_id >= KMB_MAX_PLANES))
> + return;
> +
Looks good.
Reviewed-by: Anitha Chrisanthus
> switch (plane_id) {
> case LAYER_0:
> kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
> --
> 2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
Hi Dan,
I see the problem now, thanks for the patch.
> -Original Message-
> From: Dan Carpenter
> Sent: Friday, November 20, 2020 12:11 AM
> To: Chrisanthus, Anitha
> Cc: Dea, Edmund J ; David Airlie ;
> Daniel Vetter ; Sam Ravnborg ; dri-
> de...@lists.freedesktop.org; kernel-janit...@vger.kernel.org
> Subject: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
>
> If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
> The kernel will Oops when we pass it to kmb_dsi_host_unregister().
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Dan Carpenter
> ---
> v2: write a better commit message
>
> drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index a31a840ce634..8c43b136765c 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
> if (IS_ERR(kmb->kmb_dsi)) {
> drm_err(>drm, "failed to initialize DSI\n");
> ret = PTR_ERR(kmb->kmb_dsi);
> - goto err_free1;
> + goto err_clear_drvdata;
> }
>
> kmb->kmb_dsi->dev = _pdev->dev;
> @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
> drm_crtc_cleanup(>crtc);
> drm_mode_config_cleanup(>drm);
> err_free1:
> - dev_set_drvdata(dev, NULL);
> kmb_dsi_host_unregister(kmb->kmb_dsi);
> + err_clear_drvdata:
We still need to unregister the dsi_host that was registered in this call
kmb_dsi_host_bridge_init.
This will require more changes in kmb_dsi_host_unregister and/or separate out
mipi_dsi_host_unregister.
FYI - I will be out all of next week, will be back the next Monday.
> + dev_set_drvdata(dev, NULL);
>
> return ret;
> }
> --
> 2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH] drm/kmb: Remove an unnecessary NULL check
> -Original Message- > From: Thomas Zimmermann > Sent: Friday, November 20, 2020 12:34 AM > To: Sam Ravnborg ; Chrisanthus, Anitha > > Cc: David Airlie ; Dea, Edmund J ; > kernel-janit...@vger.kernel.org; dri-devel@lists.freedesktop.org; Dan > Carpenter > Subject: Re: [PATCH] drm/kmb: Remove an unnecessary NULL check > > Hi > > Am 20.11.20 um 09:21 schrieb Sam Ravnborg: > > Hi Anitha. > > > > On Fri, Nov 20, 2020 at 01:19:06AM +, Chrisanthus, Anitha wrote: > >> Looks good to me. > > > > Can we get either an "Acked-by:" or "Reviewed-by:"? > > Then we can use this while applying. Sorry, forgot that. Reviewed-by: Anitha Chrisanthus > > > > Any news on gettting commit access yourself? > > If not, then try to ping on the open ticket. > > It's been acked a while ago. I sent out a reminder to Daniel Stone. I did get access today, thank you! I will have to get familiar with dim and the whole process before I start pushing patches. > > Best regards > Thomas > > > > > > > Sam > > ___ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH] drm/kmb: Fix possible oops in probe error handling
Hi Dan,
> -Original Message-
> From: Dan Carpenter
> Sent: Monday, November 16, 2020 11:21 PM
> To: Chrisanthus, Anitha
> Cc: Dea, Edmund J ; David Airlie ;
> Daniel Vetter ; Sam Ravnborg ; dri-
> de...@lists.freedesktop.org; kernel-janit...@vger.kernel.org
> Subject: [PATCH] drm/kmb: Fix possible oops in probe error handling
>
> If kmb_dsi_init() fails the error handling will dereference an error
> pointer which will cause an Oops.
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Dan Carpenter
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index a31a840ce634..8c43b136765c 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
> if (IS_ERR(kmb->kmb_dsi)) {
> drm_err(>drm, "failed to initialize DSI\n");
> ret = PTR_ERR(kmb->kmb_dsi);
> - goto err_free1;
> + goto err_clear_drvdata;
> }
dsi host is registered earlier, it needs to be unregistered, original code is
correct.
Anitha
>
> kmb->kmb_dsi->dev = _pdev->dev;
> @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
> drm_crtc_cleanup(>crtc);
> drm_mode_config_cleanup(>drm);
> err_free1:
> - dev_set_drvdata(dev, NULL);
> kmb_dsi_host_unregister(kmb->kmb_dsi);
> + err_clear_drvdata:
> + dev_set_drvdata(dev, NULL);
>
> return ret;
> }
> --
> 2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH] drm/kmb: Remove an unnecessary NULL check
Looks good to me.
Anitha
> -Original Message-
> From: Dan Carpenter
> Sent: Monday, November 16, 2020 11:22 PM
> To: Chrisanthus, Anitha
> Cc: Dea, Edmund J ; David Airlie ;
> Daniel Vetter ; dri-devel@lists.freedesktop.org; kernel-
> janit...@vger.kernel.org
> Subject: [PATCH] drm/kmb: Remove an unnecessary NULL check
>
> The NULL checking isn't done consistently in this function and it leads
> to a static checker warning:
>
> drivers/gpu/drm/kmb/kmb_drv.c:561 kmb_pm_suspend()
> error: we previously assumed 'drm' could be null (see line 559)
>
> Fortunately "drm" cannot be NULL at this point so the check can just be
> removed.
>
> Signed-off-by: Dan Carpenter
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index 8c43b136765c..5ff392644603 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -557,7 +557,7 @@ MODULE_DEVICE_TABLE(of, kmb_of_match);
> static int __maybe_unused kmb_pm_suspend(struct device *dev)
> {
> struct drm_device *drm = dev_get_drvdata(dev);
> - struct kmb_drm_private *kmb = drm ? to_kmb(drm) : NULL;
> + struct kmb_drm_private *kmb = to_kmb(drm);
>
> drm_kms_helper_poll_disable(drm);
>
> --
> 2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH][next] drm/kmb: fix array out-of-bounds writes to kmb->plane_status[]
Hi Sam and Colin,
> -Original Message-
> From: Sam Ravnborg
> Sent: Friday, November 13, 2020 10:02 AM
> To: Colin Ian King
> Cc: Chrisanthus, Anitha ; Dea, Edmund J
> ; David Airlie ; Daniel Vetter
> ; dri-devel@lists.freedesktop.org; kernel-
> janit...@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH][next] drm/kmb: fix array out-of-bounds writes to kmb-
> >plane_status[]
>
> Hi Colin.
> On Fri, Nov 13, 2020 at 03:04:34PM +, Colin Ian King wrote:
> > On 13/11/2020 14:55, Sam Ravnborg wrote:
> > > Hi Colin.
> > >
> > > On Fri, Nov 13, 2020 at 12:01:21PM +, Colin King wrote:
> > >> From: Colin Ian King
> > >>
> > >> Writes to elements in the kmb->plane_status array in function
> > >> kmb_plane_atomic_disable are overrunning the array when plane_id is
> > >> more than 1 because currently the array is KMB_MAX_PLANES elements
> > >> in size and this is currently #defined as 1. Fix this by defining
> > >> KMB_MAX_PLANES to 4.
> > >
> > > I fail to follow you here.
> > > In kmb_plane_init() only one plane is allocated - with id set to 0.
> > > So for now only one plane is allocated thus kmb_plane_atomic_disable()
> > > is only called for this plane.
> > >
> > > With your change we will start allocating four planes, something that is
> > > not tested.
> > >
> > > Do I miss something?
> > >
> > > Sam
> > >
> >
> > The static analysis from coverity on linux-next suggested that there was
> > an array overflow as follows:
> >
> > 108 static void kmb_plane_atomic_disable(struct drm_plane *plane,
> > 109 struct drm_plane_state *state)
> > 110 {
> >
> >1. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> > !__builtin_types_compatible_p()) */, taking false branch.
> >
> > 111struct kmb_plane *kmb_plane = to_kmb_plane(plane);
> >
> >2. assignment: Assigning: plane_id = kmb_plane->id.
> >
> > 112int plane_id = kmb_plane->id;
> > 113struct kmb_drm_private *kmb;
> > 114
> > 115kmb = to_kmb(plane->dev);
> > 116
> >
> >3. Switch case value LAYER_3.
> >
> > 117switch (plane_id) {
> > 118case LAYER_0:
> > 119kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
> > 120break;
>
> With the current code this is the only case that hits.
> So coverity is right that if we hit other cases that would result in a
> bug. But kmb_plane->id will for now not have other values than 0.
>
> So it is a subtle false positive.
> There is some "dead" code here - but this is in preparation for more
> than one layer and we will keep the code for now, unless Anitha chimes
> in and says otherwise.
Thanks Sam, I was out on Friday. Agree with Sam, let's keep the current code
for now. Kmb->plane_id will not have non-zero values now.
Only one plane is supported and tested currently, the extra code is in
preparation for multiple planes.
Thanks,
Anitha
>
> Sam
>
> > 121case LAYER_1:
> >
> >(#2 of 4): Out-of-bounds write (OVERRUN)
> >
> > 122kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
> > 123break;
> > 124case LAYER_2:
> >
> >(#3 of 4): Out-of-bounds write (OVERRUN)
> >
> > 125kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
> > 126break;
> >
> >4. equality_cond: Jumping to case LAYER_3.
> >
> > 127case LAYER_3:
> >
> >(#1 of 4): Out-of-bounds write (OVERRUN)
> >5. overrun-local: Overrunning array kmb->plane_status of 1 8-byte
> > elements at element index 3 (byte offset 31) using index plane_id (which
> > evaluates to 3).
> >
> > 128kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
> > 129break;
> > 130}
> > 131
> >
> >(#4 of 4): Out-of-bounds write (OVERRUN)
> >
> > 132kmb->plane_status[plane_id].disable = true;
> > 133 }
> > 134
> >
> > So it seems the assignments to kmb->plane_status[plane_id] are
> > overrunning the array since plane_status is allocated as 1 element and
> > yet plane_id can be 0..3
> >
> > I could be misunderstanding this, or it may be a false positive.
> >
> > Colin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v9 1/5] dt-bindings: display: Add support for Intel KeemBay Display
Hi Neil,
Thanks for your review, please see my reply inline.
> -Original Message-
> From: Neil Armstrong
> Sent: Friday, October 9, 2020 2:10 AM
> To: Chrisanthus, Anitha ; dri-
> de...@lists.freedesktop.org; devicet...@vger.kernel.org; Vetter, Daniel
>
> Cc: Dea, Edmund J ; s...@ravnborg.org
> Subject: Re: [PATCH v9 1/5] dt-bindings: display: Add support for Intel
> KeemBay Display
>
> Hi,
>
> On 09/10/2020 03:03, Anitha Chrisanthus wrote:
> > This patch adds bindings for Intel KeemBay Display
> >
> > v2: review changes from Rob Herring
> >
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > .../bindings/display/intel,keembay-display.yaml| 99
> ++
> > 1 file changed, 99 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/intel,keembay-
> display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-
> display.yaml
> > new file mode 100644
> > index 000..a38493d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/intel,keembay-
> display.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/intel,keembay-
> display.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Devicetree bindings for Intel Keem Bay display controller
> > +
> > +maintainers:
> > + - Anitha Chrisanthus
> > + - Edmond J Dea
> > +
> > +properties:
> > + compatible:
> > +const: intel,kmb_display
> > +
> > + reg:
> > +items:
> > + - description: Lcd registers range
> > + - description: Mipi registers range
>
> Looking at the registers, the MIPI transceiver seems to be a separate IP,
> same for D-PHY which should have a proper PHY driver instead of beeing
> handled
> here.
>
The LCD, MIPI DSI, DPHY and MSSCAM as a group, are considered the display
subsystem for Keem Bay. As such, there are several interdependencies that make
splitting them up next to impossible and currently we do not have the resources
available for that effort.
> > + - description: Msscam registers range
>
> MSScam here seems to be a clock and reset controller for the LCD and MIPI
> IPs,
> thus should be handler out of DRM.
>
> > +
> > + reg-names:
> > +items:
> > + - const: lcd
> > + - const: mipi
> > + - const: msscam
> > +
> > + clocks:
> > +items:
> > + - description: LCD controller clock
> > + - description: Mipi DSI clock
> > + - description: Mipi DSI econfig clock
> > + - description: Mipi DSI config clock
> > + - description: System clock or pll0 clock
> > +
> > + clock-names:
> > +items:
> > + - const: clk_lcd
> > + - const: clk_mipi
> > + - const: clk_mipi_ecfg
> > + - const: clk_mipi_cfg
> > + - const: clk_pll0
> > +
> > + interrupts:
> > +maxItems: 1
> > +
> > + encoder-slave:
> > +description: bridge node entry for mipi to hdmi converter
> > +
> > + port:
> > +type: object
> > +description: >
> > + Port node with one endpoint connected to mipi to hdmi converter
> node.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reg-names
> > + - clocks
> > + - clock-names
> > + - interrupts
> > + - encoder-slave
> > + - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +#define MOVISOC_KMB_MSS_AUX_LCD
> > +#define MOVISOC_KMB_MSS_AUX_MIPI_TX0
> > +#define MOVISOC_KMB_MSS_AUX_MIPI_ECFG
> > +#define MOVISOC_KMB_MSS_AUX_MIPI_CFG
> > +#define MOVISOC_KMB_A53_PLL_0_OUT_0
> > +display@2090 {
> > + compatible = "intel,keembay-display";
> > + reg = <0x2093 0x3000>,
> > +<0x2090 0x4000>,
> > +<0x2091 0x30>;
> > + reg-names = "lcd", "mipi", "msscam";
> > + interrupts = ;
> > + clocks = <_clk MOVISOC_KMB_MSS_AUX_LCD>,
> > + <_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>,
> > + <_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>,
> > + <_clk MOVISOC_KMB_MSS_AUX_M
RE: [PATCH v9 1/5] dt-bindings: display: Add support for Intel KeemBay Display
> -Original Message-
> From: Neil Armstrong
> Sent: Friday, October 9, 2020 2:10 AM
> To: Chrisanthus, Anitha ; dri-
> de...@lists.freedesktop.org; devicet...@vger.kernel.org; Vetter, Daniel
>
> Cc: Dea, Edmund J ; s...@ravnborg.org
> Subject: Re: [PATCH v9 1/5] dt-bindings: display: Add support for Intel
> KeemBay Display
>
> Hi,
>
> On 09/10/2020 03:03, Anitha Chrisanthus wrote:
> > This patch adds bindings for Intel KeemBay Display
> >
> > v2: review changes from Rob Herring
> >
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > .../bindings/display/intel,keembay-display.yaml| 99
> ++
> > 1 file changed, 99 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/intel,keembay-
> display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-
> display.yaml
> > new file mode 100644
> > index 000..a38493d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/intel,keembay-
> display.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/intel,keembay-
> display.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Devicetree bindings for Intel Keem Bay display controller
> > +
> > +maintainers:
> > + - Anitha Chrisanthus
> > + - Edmond J Dea
> > +
> > +properties:
> > + compatible:
> > +const: intel,kmb_display
> > +
> > + reg:
> > +items:
> > + - description: Lcd registers range
> > + - description: Mipi registers range
>
> Looking at the registers, the MIPI transceiver seems to be a separate IP,
> same for D-PHY which should have a proper PHY driver instead of beeing
> handled
> here.
Mipi is not a separate IP, it is all part of one sub system in the Intel
Movidius Soc.
>
> > + - description: Msscam registers range
>
> MSScam here seems to be a clock and reset controller for the LCD and MIPI
> IPs,
> thus should be handler out of DRM.
>
> > +
> > + reg-names:
> > +items:
> > + - const: lcd
> > + - const: mipi
> > + - const: msscam
> > +
> > + clocks:
> > +items:
> > + - description: LCD controller clock
> > + - description: Mipi DSI clock
> > + - description: Mipi DSI econfig clock
> > + - description: Mipi DSI config clock
> > + - description: System clock or pll0 clock
> > +
> > + clock-names:
> > +items:
> > + - const: clk_lcd
> > + - const: clk_mipi
> > + - const: clk_mipi_ecfg
> > + - const: clk_mipi_cfg
> > + - const: clk_pll0
> > +
> > + interrupts:
> > +maxItems: 1
> > +
> > + encoder-slave:
> > +description: bridge node entry for mipi to hdmi converter
> > +
> > + port:
> > +type: object
> > +description: >
> > + Port node with one endpoint connected to mipi to hdmi converter
> node.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reg-names
> > + - clocks
> > + - clock-names
> > + - interrupts
> > + - encoder-slave
> > + - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +#define MOVISOC_KMB_MSS_AUX_LCD
> > +#define MOVISOC_KMB_MSS_AUX_MIPI_TX0
> > +#define MOVISOC_KMB_MSS_AUX_MIPI_ECFG
> > +#define MOVISOC_KMB_MSS_AUX_MIPI_CFG
> > +#define MOVISOC_KMB_A53_PLL_0_OUT_0
> > +display@2090 {
> > + compatible = "intel,keembay-display";
> > + reg = <0x2093 0x3000>,
> > +<0x2090 0x4000>,
> > +<0x2091 0x30>;
> > + reg-names = "lcd", "mipi", "msscam";
> > + interrupts = ;
> > + clocks = <_clk MOVISOC_KMB_MSS_AUX_LCD>,
> > + <_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>,
> > + <_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>,
> > + <_clk MOVISOC_KMB_MSS_AUX_MIPI_CFG>,
> > + <_clk MOVISOC_KMB_A53_PLL_0_OUT_0>;
> > + clock-names = "clk_lcd", "clk_mipi", "clk_mipi_ecfg",
> > +"clk_mipi_cfg", "clk_pll0";
> > +
> > + encoder-slave = <>;
> > +
> > + port {
> > +dsi_output: endpoint {
> > +remote-endpoint = <_input>;
> > +};
> > + };
> > +};
> >
>
> Anitha, Daniel, this keembay driver should be architectured like other ARM-
> like display
> controllers, with separate drivers for MIPI DSI bridge and msscam clock &
> reset controller.
> Neil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v8 1/4] drm/kmb: Keem Bay driver register definition
> -Original Message- > From: Rob Herring > Sent: Wednesday, October 7, 2020 6:45 AM > To: Chrisanthus, Anitha > Cc: dri-devel ; Paauwe, Bob J > ; Dea, Edmund J ; > Vetter, Daniel ; Sam Ravnborg > > Subject: Re: [PATCH v8 1/4] drm/kmb: Keem Bay driver register definition > > On Fri, Oct 2, 2020 at 9:17 PM Anitha Chrisanthus > wrote: > > > > Register definitions for Keem Bay display driver > > > > v2: removed license text (Sam) > > v3: Squashed all 59 commits to one > > v4: review changes from Sam Ravnborg > > renamed dev_p to kmb > > v5: corrected spellings > > v6: corrected checkpatch warnings > > > > Cc: Sam Ravnborg > > Signed-off-by: Anitha Chrisanthus > > Reviewed-by: Bob Paauwe > > --- > > drivers/gpu/drm/kmb/kmb_regs.h | 748 > + > > 1 file changed, 748 insertions(+) > > create mode 100644 drivers/gpu/drm/kmb/kmb_regs.h > > > > diff --git a/drivers/gpu/drm/kmb/kmb_regs.h > b/drivers/gpu/drm/kmb/kmb_regs.h > > new file mode 100644 > > index 000..f794ac3 > > --- /dev/null > > +++ b/drivers/gpu/drm/kmb/kmb_regs.h > > @@ -0,0 +1,748 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only > > + * > > + * Copyright © 2018-2020 Intel Corporation > > + */ > > + > > +#ifndef __KMB_REGS_H__ > > +#define __KMB_REGS_H__ > > + > > +#define ENABLE 1 > > +#define DISABLE 0 > > +/*from Data Book section 12.5.8.1 page 4322 */ > > +#define CPR_BASE_ADDR (0x2081) > > +#define MIPI_BASE_ADDR (0x2090) > > +/*from Data Book section 12.11.6.1 page 4972 */ > > +#define LCD_BASE_ADDR (0x2093) > > +#define MSS_CAM_BASE_ADDR (MIPI_BASE_ADDR + > 0x1) > > +#define LCD_MMIO_SIZE (0x3000) > > +#define MIPI_MMIO_SIZE (0x4000) > > +#define MSS_CAM_MMIO_SIZE (0x30) > > Why are these defines here? They all come from DT. Will remove, will send v9 soon. > > Rob ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v1] dt-bindings: display: Add support for Intel KeemBay Display
Hi Rob, Thanks for the your prompt review. Please see my comments/questions inline. For everything else, I can incorporate the changes in v2. Anitha > -Original Message- > From: Rob Herring > Sent: Tuesday, October 6, 2020 2:08 PM > To: Chrisanthus, Anitha > Cc: devicet...@vger.kernel.org; Paauwe, Bob J ; > Dea, Edmund J ; s...@ravnborg.org; > narmstr...@baylibre.com > Subject: Re: [PATCH v1] dt-bindings: display: Add support for Intel KeemBay > Display > > On Fri, Oct 02, 2020 at 07:21:02PM -0700, Anitha Chrisanthus wrote: > > This patch adds bindings for Intel KeemBay Display > > > > Signed-off-by: Anitha Chrisanthus > > --- > > .../bindings/display/intel,kmb_display.yaml| 106 > + > > 1 file changed, 106 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/display/intel,kmb_display.yaml > > > > diff --git > a/Documentation/devicetree/bindings/display/intel,kmb_display.yaml > b/Documentation/devicetree/bindings/display/intel,kmb_display.yaml > > new file mode 100644 > > index 000..65835cb > > --- /dev/null > > +++ > b/Documentation/devicetree/bindings/display/intel,kmb_display.yaml > > @@ -0,0 +1,106 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > check checkpatch.pl > > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/intel,kmb_display.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Devicetree bindings for Intel Keem Bay display controller > > + > > +maintainers: > > + - Anitha Chrisanthus > > + - Edmond J Dea > > + > > +properties: > > + compatible: > > +const: intel,kmb_display > > 'keembay' was used elsewhere. Please be consistent. > > Don't use '_' either. Please note that I cannot change the name at this point as there is a dependency on the u-boot firmware which loads the device tree. I can change the name to kmb-drm or keembay-display when updated firmware becomes available. > > > + > > + reg: > > +maxItems: 3 > > Can drop, implied. > > > +items: > > + - description: Lcd registers range > > + - description: Mipi registers range > > + - description: Msscam registers range > > Is this really 1 h/w block? Don't really seem like it given addresses > aren't adjacent, separate clocks, and MIPI blocks are often licensed IP. Yes, these are part of the camera subsystem block of Intel Movidius Keembay SOC. Please see https://lwn.net/Articles/833540/ > > > + > > + reg-names: > > +items: > > + - const: lcd_regs > > + - const: mipi_regs > > + - const: msscam_regs > > '_regs' is redundant. > > > + > > + clocks: > > +items: > > + - description: LCD controller clock > > + - description: Mipi DSI clock > > + - description: Mipi DSI econfig clock > > + - description: Mipi DSI config clock > > + - description: System clock or pll0 clock > > + > > + clock-names: > > +items: > > + - const: clk_lcd > > + - const: clk_mipi > > + - const: clk_mipi_ecfg > > + - const: clk_mipi_cfg > > + - const: clk_pll0 > > + > > + interrupts: > > +maxItems: 1 > > + > > + interrupt-names: > > +items: > > + - const: irq_lcd > > You don't really need *-names when there's only 1 entry. > > > + > > + encoder-slave: > > +description: bridge node entry for mipi to hdmi converter > > No, this is what 'port' is for. Driver calls this encoder_node = of_parse_phandle(dev->of_node, "encoder-slave", 0) And bridge = of_drm_find_bridge(encoder_node); to locate the bridge driver. How do I do this without this entry? Please advise. This was tested and it works. > > > + > > + port: > > +type: object > > +description: > > > + Port node with one endpoint connected to mipi to hdmi converter > node. > > + > > +required: > > + - compatible > > + - reg > > + - reg-names > > + - clocks > > + - clock-names > > + - interrupts > > + - interrupt-names > > + - encoder-slave > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > +#include > > +#define GIC_SPI > > There's a header for this. > > > +#define MOVISOC_KMB_MSS_AUX_LCD > > +#define MOVISOC_KMB_MSS_AUX_MIPI_TX0 > > +#define MOVISOC_K
RE: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
> -Original Message- > From: Daniel Vetter > Sent: Monday, October 5, 2020 6:49 AM > To: Chrisanthus, Anitha > Cc: Daniel Vetter ; dri-devel@lists.freedesktop.org; > Paauwe, Bob J ; Dea, Edmund J > ; Vetter, Daniel > Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display > > On Sat, Oct 03, 2020 at 04:58:51AM +, Chrisanthus, Anitha wrote: > > > > > > > -Original Message- > > > From: Daniel Vetter > > > Sent: Friday, October 2, 2020 2:19 AM > > > To: Chrisanthus, Anitha > > > Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J > > > ; Dea, Edmund J ; > > > Vetter, Daniel > > > Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay > Display > > > > > > On Thu, Oct 1, 2020 at 8:02 PM Chrisanthus, Anitha > > > wrote: > > > > > > > > Hi Daniel, > > > > I was finally able to test the driver with 5.9 kernel with minor changes > in > > > the driver. > > > > Ran the igt_vblank test and it passed/skipped all the subtests except > the > > > pipe-A-accuracy-idle test. > > > > Results are attached. Investigated the failing test and it seems like > > > drm_handle_vblank() is taking too long sometimes. I can work on this > after > > > we merge. > > > > > > kms_flip is the one that should check whether vblank events and page > > > flip events agree. Which I think isn't the case with your current > > > code. > > Thanks. I will run that test and work on the failures. I did send v8 > > with 5.9 testing changes today and also sent the YAML file v1 today to > > devicet...@vger.kernel.org, not sure if I should cc dri-devel,, should > > I? > > I think usually people send out the entire series, first device tree yaml, > then bindings, then drivers, to everyone. It helps with context I think > (But I don't review dt stuff myself). Thanks, I will wait for review for DT YAML and will include dri-devel if v2 is needed. Also attaching dt binding yaml patch. I am still awaiting approval for push access to drm-misc, I think there is some confusion on what this driver is. This was requested for v7. Can you grant access? https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/291 Anitha > -Daniel > > > > > Anitha > > > -Daniel > > > > > > > > > > > Will send out V8 with the minor changes and device tree YAML > binding > > > file ASAP. Will work on the rest of the review comments after. > > > > > > > > Thanks, > > > > Anitha > > > > > > > > > From: Daniel Vetter > > > > > Sent: Thursday, September 10, 2020 1:31 AM > > > > > To: Chrisanthus, Anitha > > > > > Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J > > > > > ; Dea, Edmund J > ; > > > > > Vetter, Daniel > > > > > Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay > > > Display > > > > > > > > > > On Mon, Aug 31, 2020 at 01:02:50PM -0700, Anitha Chrisanthus > > > wrote: > > > > > > This is a basic KMS atomic modesetting display driver for KeemBay > > > family > > > > > of > > > > > > SOCs. Driver has no 2D or 3D graphics.It calls into the ADV bridge > > > > > > driver at the connector level. > > > > > > > > > > > > Single CRTC with LCD controller->mipi DSI-> ADV bridge > > > > > > > > > > > > Only 1080p resolution and single plane is supported at this time. > > > > > > > > > > > > v2: moved extern to .h, removed license text > > > > > > use drm_dev_init, upclassed dev_private, removed > > > HAVE_IRQ.(Sam) > > > > > > > > > > > > v3: Squashed all 59 commits to one > > > > > > > > > > > > v4: review changes from Sam Ravnborg > > > > > > renamed dev_p to kmb > > > > > > moved clocks under kmb_clock, consolidated clk initializations > > > > > > use drmm functions > > > > > > use DRM_GEM_CMA_DRIVER_OPS_VMAP > > > > > > > > > > > > v5: corrected spellings > > > > > > v6: corrected checkpatch warnings > > > > > > v7: review changes Sam Ravnborg and Thomas Zimmerman > > > > > > removed kmb_crtc.h kmb_crtc_cleanup (Thomas) > > > > > >
RE: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
> -Original Message- > From: Daniel Vetter > Sent: Friday, October 2, 2020 2:19 AM > To: Chrisanthus, Anitha > Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J > ; Dea, Edmund J ; > Vetter, Daniel > Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display > > On Thu, Oct 1, 2020 at 8:02 PM Chrisanthus, Anitha > wrote: > > > > Hi Daniel, > > I was finally able to test the driver with 5.9 kernel with minor changes in > the driver. > > Ran the igt_vblank test and it passed/skipped all the subtests except the > pipe-A-accuracy-idle test. > > Results are attached. Investigated the failing test and it seems like > drm_handle_vblank() is taking too long sometimes. I can work on this after > we merge. > > kms_flip is the one that should check whether vblank events and page > flip events agree. Which I think isn't the case with your current > code. Thanks. I will run that test and work on the failures. I did send v8 with 5.9 testing changes today and also sent the YAML file v1 today to devicet...@vger.kernel.org, not sure if I should cc dri-devel,, should I? Anitha > -Daniel > > > > > Will send out V8 with the minor changes and device tree YAML binding > file ASAP. Will work on the rest of the review comments after. > > > > Thanks, > > Anitha > > > > > From: Daniel Vetter > > > Sent: Thursday, September 10, 2020 1:31 AM > > > To: Chrisanthus, Anitha > > > Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J > > > ; Dea, Edmund J ; > > > Vetter, Daniel > > > Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay > Display > > > > > > On Mon, Aug 31, 2020 at 01:02:50PM -0700, Anitha Chrisanthus > wrote: > > > > This is a basic KMS atomic modesetting display driver for KeemBay > family > > > of > > > > SOCs. Driver has no 2D or 3D graphics.It calls into the ADV bridge > > > > driver at the connector level. > > > > > > > > Single CRTC with LCD controller->mipi DSI-> ADV bridge > > > > > > > > Only 1080p resolution and single plane is supported at this time. > > > > > > > > v2: moved extern to .h, removed license text > > > > use drm_dev_init, upclassed dev_private, removed > HAVE_IRQ.(Sam) > > > > > > > > v3: Squashed all 59 commits to one > > > > > > > > v4: review changes from Sam Ravnborg > > > > renamed dev_p to kmb > > > > moved clocks under kmb_clock, consolidated clk initializations > > > > use drmm functions > > > > use DRM_GEM_CMA_DRIVER_OPS_VMAP > > > > > > > > v5: corrected spellings > > > > v6: corrected checkpatch warnings > > > > v7: review changes Sam Ravnborg and Thomas Zimmerman > > > > removed kmb_crtc.h kmb_crtc_cleanup (Thomas) > > > > renamed mode_set, kmb_load, inlined unload (Thomas) > > > > moved remaining logging to drm_*(Thomas) > > > > re-orged driver initialization (Thomas) > > > > moved plane_status to drm_private (Sam) > > > > removed unnecessary logs and defines and ifdef codes (Sam) > > > > call helper_check in plane_atomic_check (Sam) > > > > renamed set to get for bpp and format functions(Sam) > > > > use drm helper functions for reset, duplicate/destroy state instead > > > > of kmb functions (Sam) > > > > removed kmb_priv from kmb_plane and removed > kmb_plane_state > > > (Sam) > > > > > > > > Cc: Sam Ravnborg > > > > Signed-off-by: Anitha Chrisanthus > > > > Reviewed-by: Bob Paauwe > > > > > > Sam asked me to take a look at this too, looks reasonable overall. I've > > > spotted a few small things plus a potential functional issue around > > > vblank/event handling. I strongly recommend running the igt test suite > > > over the driver to see whether it all works reasonably ok. See below for > > > details. > > > > > > > --- > > > > drivers/gpu/drm/kmb/kmb_crtc.c | 224 + > > > > drivers/gpu/drm/kmb/kmb_drv.c | 676 > > > > > > > drivers/gpu/drm/kmb/kmb_drv.h | 170 ++ > > > > drivers/gpu/drm/kmb/kmb_plane.c | 480 > > > > > > > drivers/gpu/drm/kmb/kmb_plane.h | 110 +++ > > > > 5 files changed, 1660 insertions(+) > > >
RE: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
Hi Daniel,
I was finally able to test the driver with 5.9 kernel with minor changes in the
driver.
Ran the igt_vblank test and it passed/skipped all the subtests except the
pipe-A-accuracy-idle test.
Results are attached. Investigated the failing test and it seems like
drm_handle_vblank() is taking too long sometimes. I can work on this after we
merge.
Will send out V8 with the minor changes and device tree YAML binding file ASAP.
Will work on the rest of the review comments after.
Thanks,
Anitha
> From: Daniel Vetter
> Sent: Thursday, September 10, 2020 1:31 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J
> ; Dea, Edmund J ;
> Vetter, Daniel
> Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
>
> On Mon, Aug 31, 2020 at 01:02:50PM -0700, Anitha Chrisanthus wrote:
> > This is a basic KMS atomic modesetting display driver for KeemBay family
> of
> > SOCs. Driver has no 2D or 3D graphics.It calls into the ADV bridge
> > driver at the connector level.
> >
> > Single CRTC with LCD controller->mipi DSI-> ADV bridge
> >
> > Only 1080p resolution and single plane is supported at this time.
> >
> > v2: moved extern to .h, removed license text
> > use drm_dev_init, upclassed dev_private, removed HAVE_IRQ.(Sam)
> >
> > v3: Squashed all 59 commits to one
> >
> > v4: review changes from Sam Ravnborg
> > renamed dev_p to kmb
> > moved clocks under kmb_clock, consolidated clk initializations
> > use drmm functions
> > use DRM_GEM_CMA_DRIVER_OPS_VMAP
> >
> > v5: corrected spellings
> > v6: corrected checkpatch warnings
> > v7: review changes Sam Ravnborg and Thomas Zimmerman
> > removed kmb_crtc.h kmb_crtc_cleanup (Thomas)
> > renamed mode_set, kmb_load, inlined unload (Thomas)
> > moved remaining logging to drm_*(Thomas)
> > re-orged driver initialization (Thomas)
> > moved plane_status to drm_private (Sam)
> > removed unnecessary logs and defines and ifdef codes (Sam)
> > call helper_check in plane_atomic_check (Sam)
> > renamed set to get for bpp and format functions(Sam)
> > use drm helper functions for reset, duplicate/destroy state instead
> > of kmb functions (Sam)
> > removed kmb_priv from kmb_plane and removed kmb_plane_state
> (Sam)
> >
> > Cc: Sam Ravnborg
> > Signed-off-by: Anitha Chrisanthus
> > Reviewed-by: Bob Paauwe
>
> Sam asked me to take a look at this too, looks reasonable overall. I've
> spotted a few small things plus a potential functional issue around
> vblank/event handling. I strongly recommend running the igt test suite
> over the driver to see whether it all works reasonably ok. See below for
> details.
>
> > ---
> > drivers/gpu/drm/kmb/kmb_crtc.c | 224 +
> > drivers/gpu/drm/kmb/kmb_drv.c | 676
>
> > drivers/gpu/drm/kmb/kmb_drv.h | 170 ++
> > drivers/gpu/drm/kmb/kmb_plane.c | 480
>
> > drivers/gpu/drm/kmb/kmb_plane.h | 110 +++
> > 5 files changed, 1660 insertions(+)
> > create mode 100644 drivers/gpu/drm/kmb/kmb_crtc.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_drv.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_drv.h
> > create mode 100644 drivers/gpu/drm/kmb/kmb_plane.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_plane.h
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_crtc.c
> b/drivers/gpu/drm/kmb/kmb_crtc.c
> > new file mode 100644
> > index 000..a684331
> > --- /dev/null
> > +++ b/drivers/gpu/drm/kmb/kmb_crtc.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2018-2020 Intel Corporation
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "kmb_drv.h"
> > +#include "kmb_dsi.h"
> > +#include "kmb_plane.h"
> > +#include "kmb_regs.h"
> > +
> > +struct kmb_crtc_timing {
> > + u32 vfront_porch;
> > + u32 vback_porch;
> > + u32 vsync_len;
> > + u32 hfront_porch;
> > + u32 hback_porch;
> > + u32 hsync_len;
> > +};
> > +
> > +static int kmb_crtc_enable_vbla
RE: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
Hi Daniel,
Thank you for your review. Keem Bay is developed on kernel 5.45 (Yocto) and I
am working with the program to get a working 5.9 kernel for Keem Bay and will
test this driver when that becomes available.
Thanks,
Anitha
> -Original Message-
> From: Daniel Vetter
> Sent: Thursday, September 10, 2020 1:31 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J
> ; Dea, Edmund J ;
> Vetter, Daniel
> Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
>
> On Mon, Aug 31, 2020 at 01:02:50PM -0700, Anitha Chrisanthus wrote:
> > This is a basic KMS atomic modesetting display driver for KeemBay family
> of
> > SOCs. Driver has no 2D or 3D graphics.It calls into the ADV bridge
> > driver at the connector level.
> >
> > Single CRTC with LCD controller->mipi DSI-> ADV bridge
> >
> > Only 1080p resolution and single plane is supported at this time.
> >
> > v2: moved extern to .h, removed license text
> > use drm_dev_init, upclassed dev_private, removed HAVE_IRQ.(Sam)
> >
> > v3: Squashed all 59 commits to one
> >
> > v4: review changes from Sam Ravnborg
> > renamed dev_p to kmb
> > moved clocks under kmb_clock, consolidated clk initializations
> > use drmm functions
> > use DRM_GEM_CMA_DRIVER_OPS_VMAP
> >
> > v5: corrected spellings
> > v6: corrected checkpatch warnings
> > v7: review changes Sam Ravnborg and Thomas Zimmerman
> > removed kmb_crtc.h kmb_crtc_cleanup (Thomas)
> > renamed mode_set, kmb_load, inlined unload (Thomas)
> > moved remaining logging to drm_*(Thomas)
> > re-orged driver initialization (Thomas)
> > moved plane_status to drm_private (Sam)
> > removed unnecessary logs and defines and ifdef codes (Sam)
> > call helper_check in plane_atomic_check (Sam)
> > renamed set to get for bpp and format functions(Sam)
> > use drm helper functions for reset, duplicate/destroy state instead
> > of kmb functions (Sam)
> > removed kmb_priv from kmb_plane and removed kmb_plane_state
> (Sam)
> >
> > Cc: Sam Ravnborg
> > Signed-off-by: Anitha Chrisanthus
> > Reviewed-by: Bob Paauwe
>
> Sam asked me to take a look at this too, looks reasonable overall. I've
> spotted a few small things plus a potential functional issue around
> vblank/event handling. I strongly recommend running the igt test suite
> over the driver to see whether it all works reasonably ok. See below for
> details.
>
> > ---
> > drivers/gpu/drm/kmb/kmb_crtc.c | 224 +
> > drivers/gpu/drm/kmb/kmb_drv.c | 676
>
> > drivers/gpu/drm/kmb/kmb_drv.h | 170 ++
> > drivers/gpu/drm/kmb/kmb_plane.c | 480
>
> > drivers/gpu/drm/kmb/kmb_plane.h | 110 +++
> > 5 files changed, 1660 insertions(+)
> > create mode 100644 drivers/gpu/drm/kmb/kmb_crtc.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_drv.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_drv.h
> > create mode 100644 drivers/gpu/drm/kmb/kmb_plane.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_plane.h
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_crtc.c
> b/drivers/gpu/drm/kmb/kmb_crtc.c
> > new file mode 100644
> > index 000..a684331
> > --- /dev/null
> > +++ b/drivers/gpu/drm/kmb/kmb_crtc.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2018-2020 Intel Corporation
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "kmb_drv.h"
> > +#include "kmb_dsi.h"
> > +#include "kmb_plane.h"
> > +#include "kmb_regs.h"
> > +
> > +struct kmb_crtc_timing {
> > + u32 vfront_porch;
> > + u32 vback_porch;
> > + u32 vsync_len;
> > + u32 hfront_porch;
> > + u32 hback_porch;
> > + u32 hsync_len;
> > +};
> > +
> > +static int kmb_crtc_enable_vblank(struct drm_crtc *crtc)
> > +{
> > + struct drm_device *dev = crtc->dev;
> > + struct kmb_drm_private *kmb = to_kmb(dev);
> > +
> > + /* Clear interrupt */
> > + kmb_write
RE: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
Hi Neil,
Thanks for your review. Is a device tree binding document like this one enough?
Entries for kmb-drm are similar to this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/gpu/brcm%2Cbcm-v3d.txt
How do I submit it once I have it ready?
Thanks,
Anitha
> -Original Message-
> From: Neil Armstrong
> Sent: Thursday, September 10, 2020 2:33 AM
> To: Chrisanthus, Anitha ; dri-
> de...@lists.freedesktop.org; Paauwe, Bob J ;
> Dea, Edmund J
> Cc: Vetter, Daniel
> Subject: Re: [PATCH v7 2/4] drm/kmb: Add support for KeemBay Display
>
> On 31/08/2020 22:02, Anitha Chrisanthus wrote:
> > This is a basic KMS atomic modesetting display driver for KeemBay family
> of
> > SOCs. Driver has no 2D or 3D graphics.It calls into the ADV bridge
> > driver at the connector level.
> >
> > Single CRTC with LCD controller->mipi DSI-> ADV bridge
> >
> > Only 1080p resolution and single plane is supported at this time.
> >
> > v2: moved extern to .h, removed license text
> > use drm_dev_init, upclassed dev_private, removed HAVE_IRQ.(Sam)
> >
> > v3: Squashed all 59 commits to one
> >
> > v4: review changes from Sam Ravnborg
> > renamed dev_p to kmb
> > moved clocks under kmb_clock, consolidated clk initializations
> > use drmm functions
> > use DRM_GEM_CMA_DRIVER_OPS_VMAP
> >
> > v5: corrected spellings
> > v6: corrected checkpatch warnings
> > v7: review changes Sam Ravnborg and Thomas Zimmerman
> > removed kmb_crtc.h kmb_crtc_cleanup (Thomas)
> > renamed mode_set, kmb_load, inlined unload (Thomas)
> > moved remaining logging to drm_*(Thomas)
> > re-orged driver initialization (Thomas)
> > moved plane_status to drm_private (Sam)
> > removed unnecessary logs and defines and ifdef codes (Sam)
> > call helper_check in plane_atomic_check (Sam)
> > renamed set to get for bpp and format functions(Sam)
> > use drm helper functions for reset, duplicate/destroy state instead
> > of kmb functions (Sam)
> > removed kmb_priv from kmb_plane and removed kmb_plane_state
> (Sam)
> >
> > Cc: Sam Ravnborg
> > Signed-off-by: Anitha Chrisanthus
> > Reviewed-by: Bob Paauwe
> > ---
> > drivers/gpu/drm/kmb/kmb_crtc.c | 224 +
> > drivers/gpu/drm/kmb/kmb_drv.c | 676
>
> > drivers/gpu/drm/kmb/kmb_drv.h | 170 ++
> > drivers/gpu/drm/kmb/kmb_plane.c | 480
>
> > drivers/gpu/drm/kmb/kmb_plane.h | 110 +++
> > 5 files changed, 1660 insertions(+)
> > create mode 100644 drivers/gpu/drm/kmb/kmb_crtc.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_drv.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_drv.h
> > create mode 100644 drivers/gpu/drm/kmb/kmb_plane.c
> > create mode 100644 drivers/gpu/drm/kmb/kmb_plane.h
> >
>
> [...]
>
> > +
> > +static const struct of_device_id kmb_of_match[] = {
> > + {.compatible = "intel,kmb_display"},
> > + {},
> > +};
>
> As I already commented on v1, a proper YAML bindings files
> is mandatory here, to check if the bindings are correct and if
> the drivers uses them correctly (port/endpoints, etc..)
>
> Neil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v7 0/4] Add support for KeemBay DRM driver
Hi Daniel, Thanks for your review. I have requested drm-next access here https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/291 Will address your comments in next emails. thanks, Anitha > -Original Message- > From: Daniel Vetter > Sent: Thursday, September 10, 2020 1:42 AM > To: Chrisanthus, Anitha > Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J > ; Dea, Edmund J ; > Vetter, Daniel > Subject: Re: [PATCH v7 0/4] Add support for KeemBay DRM driver > > On Mon, Aug 31, 2020 at 01:02:48PM -0700, Anitha Chrisanthus wrote: > > This is a new DRM driver for Intel's KeemBay SOC. > > The SoC couples an ARM Cortex A53 CPU with an Intel > > Movidius VPU. > > > > This driver is tested with the KMB EVM board which is the refernce baord > > for Keem Bay SOC. The SOC's display pipeline is as follows > > > > +--++-++---+ > > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > > +--++-++---+ > > > > LCD controller and Mipi DSI transmitter are part of the SOC and > > mipi to HDMI converter is ADV7535 for KMB EVM board. > > > > The DRM driver is a basic KMS atomic modesetting display driver and > > has no 2D or 3D graphics.It calls into the ADV bridge driver at > > the connector level. > > > > Only 1080p resolution and single plane is supported at this time. > > The usecase is for debugging video and camera outputs. > > > > Device tree patches are under review here > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1- > daniele.alessandre...@linux.intel.com/T/ > > I've looked at the code, and imo looks fairly reasonable. I think the > remaining things can also be polished in-tree, and we could start merging > the code already. For that 2 things are missing: > > - Need a MAINTAINERS entry for this (I'm assuming you're volunteering) > - You need drm-misc commit rights set up so you can push patches, see > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating- > changes-with-igt > > The docs there also have howto and everything. > > Cheers, Daniel > > > > > Changes since v1: > > - Removed redundant license text, updated license > > - Rearranged include blocks > > - renamed global vars and removed extern in c > > - Used upclassing for dev_private > > - Used drm_dev_init in drm device create > > - minor cleanups > > > > Changes since v2: > > - squashed all commits to a single commit > > - logging changed to drm_info, drm_dbg etc. > > - used devm_drm_dev_alloc() > > - removed commented out sections and general cleanup > > > > Changes since v3: > > - renamed dev_p to kmb > > - moved clocks under kmb_clock, consolidated clk initializations > > - use drmm functions > > - use DRM_GEM_CMA_DRIVER_OPS_VMAP > > - more cleanups > > > > Changes since v4: > > - corrected spellings > > > > Changes since v5: > > - corrected checkpatch warnings/checks > >-Please ignore checkpatch checks on Camelcase - this is how it is > >named in the databook > >- Please ignore checkpatch warnings on misspelled for hsa, dout, > >widthn etc. - they are spelled as in the databook > >- Please ignore checkpatch checks on macro arguments reuse - > >its confirmed ok > > > > Changes since v6: > > - review changes Sam Ravnborg and Thomas Zimmerman > > split patch into 4 parts, part1 register definitions, part2 display > > driver files, part3 mipi dsi, part4 build files (Sam) > > removed kmb_crtc.h kmb_crtc_cleanup (Thomas) > > renamed mode_set, kmb_load, inlined unload (Thomas) > > moved remaining logging to drm_*(Thomas) > > re-orged driver initialization (Thomas) > > moved plane_status to drm_private (Sam) > > removed unnecessary logs and defines and ifdef codes (Sam) > > split dphy_init_sequence smaller (Sam) > > removed redundant checks in kmb_dsi (Sam) > > changed kmb_dsi_init to drm_bridge_connector_init and > > drm_connector_attach_encoder to bridge's connector (Sam) > > call helper_check in plane_atomic_check (Sam) > > renamed set to get for bpp and format functions(Sam) > > use drm helper functions for reset, duplicate/destroy state instead > > of kmb functions (Sam) > > removed kmb_priv from kmb_plane and removed kmb_plane_state > (Sam) > > > > Anitha Chrisanthus (4): > > drm/kmb: Keem Bay driver regis
RE: [PATCH v6] drm/kmb: Add support for KeemBay Display
Hi Sam,
Thanks a lot for the review. I will address your comments in v7.
For those that are not addressed, please see my reply inline.
Regards,
Anitha
> -Original Message-
> From: Sam Ravnborg
> Sent: Thursday, August 20, 2020 1:10 PM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J
> ; Dea, Edmund J ;
> Vetter, Daniel ; intel-...@lists.freedesktop.org
> Subject: Re: [PATCH v6] drm/kmb: Add support for KeemBay Display
>
> Hi Anitha.
>
> Feedback on kmb_dsi.
>
> The main feedback can be found after the kmb_dsi_init function.
> The highligt of the feedback is that, in my opinion, the
> best would be to use the drm_bridge abstraction for the kmb_dsi.
> Maybe because I am biased - and this is just overhead.
> But it just looks simpler to me.
Mipi dsi is very time sensitive and any delay can cause problems.
As soon as it's initialized it expects LCD controller to be up and running
especially the LCD_TIMING_GEN_TRIG, a slight delay and lots of issues.
It is tightly coupled in the hardware with LCD and let's keep it that
way in the software too.
> There are several chunks of code surrounded by #ifdef.
> It would be good if they could all be handled so no more #ifdef are
> required.
>
> There is also a lot of debug prints. Maybe this can be trimmed now that
> the driver works?
>
> There is a lot of variables that should all be included in a struct
> kmb_dsi that should be allocated in the probe function (if this becomes
> a bridge driver).
>
> I hope this does not scare you away! If I am right about the ocnversion
> to a drm_bridge, then I hope it is semi trivial to do.
> All the hard HW init stuff is done.
>
> Sam
>
> > diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c
> b/drivers/gpu/drm/kmb/kmb_dsi.c
> > new file mode 100644
> > index 000..21745ae
> > --- /dev/null
> > +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> > @@ -0,0 +1,1828 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2019-2020 Intel Corporation
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "kmb_drv.h"
> > +#include "kmb_dsi.h"
> > +#include "kmb_regs.h"
> > +
> > +static int hw_initialized;
> > +/* #define MIPI_TX_TEST_PATTERN_GENERATION */
> > +/* #define DPHY_GET_FSM */
> > +/* #define DPHY_READ_TESTCODE */
> > +
> > +static struct mipi_dsi_host *dsi_host;
> > +static struct mipi_dsi_device *dsi_device;
> > +
> > +/* Default setting is 1080p, 4 lanes */
> > +#define IMG_HEIGHT_LINES 1080
> > +#define IMG_WIDTH_PX 1920
> > +#define MIPI_TX_ACTIVE_LANES 4
> > +
> > +struct mipi_tx_frame_section_cfg mipi_tx_frame0_sect_cfg = {
> > + .width_pixels = IMG_WIDTH_PX,
> > + .height_lines = IMG_HEIGHT_LINES,
> > + .data_type = DSI_LP_DT_PPS_RGB888_24B,
> > + .data_mode = MIPI_DATA_MODE1,
> > + .dma_packed = 0
> > +};
> > +
> > +struct mipi_tx_frame_cfg mipitx_frame0_cfg = {
> > + .sections[0] = _tx_frame0_sect_cfg,
> > + .sections[1] = NULL,
> > + .sections[2] = NULL,
> > + .sections[3] = NULL,
> > + .vsync_width = 5,
> > + .v_backporch = 36,
> > + .v_frontporch = 4,
> > + .hsync_width = 44,
> > + .h_backporch = 148,
> > + .h_frontporch = 88
> > +};
> > +
> > +struct mipi_tx_dsi_cfg mipitx_dsi_cfg = {
> > + .hfp_blank_en = 0,
> > + .eotp_en = 0,
> > + .lpm_last_vfp_line = 0,
> > + .lpm_first_vsa_line = 0,
> > + .sync_pulse_eventn = DSI_VIDEO_MODE_NO_BURST_EVENT,
> > + .hfp_blanking = SEND_BLANK_PACKET,
> > + .hbp_blanking = SEND_BLANK_PACKET,
> > + .hsa_blanking = SEND_BLANK_PACKET,
> > + .v_blanking = SEND_BLANK_PACKET,
> > +};
> > +
> > +struct mipi_ctrl_cfg mipi_tx_init_cfg = {
> > + .index = MIPI_CTRL6,
> > + .type = MIPI_DSI,
> > + .dir = MIPI_TX,
> > + .active_lanes = MIPI_TX_ACTIVE_LANES,
> > + .lane_rate_mbps = MIPI_TX_LANE_DATA_RATE_MBPS,
> > + .ref_clk_khz = MIPI_TX_REF_CLK_KHZ,
> > + .cfg_clk_khz = MIPI_TX_CFG_CLK_KHZ,
> > + .data_if = MIPI_IF_PARALLEL,
> > + .tx_ctrl_cfg = {
> > + .frames[0] = _frame0_cfg,
> > + .frames[1] = NULL,
> > + .frames[2] = NULL,
> > +
RE: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver
Hi Sam and Daniel, Thank you very much for reviewing the code. I will squish all the commits and send version 3, so it will be easier for you to review. Anitha > -Original Message- > From: Sam Ravnborg > Sent: Wednesday, July 15, 2020 10:07 AM > To: Daniel Vetter > Cc: Chrisanthus, Anitha ; Vetter, Daniel > ; intel-...@lists.freedesktop.org; Dea, Edmund J > ; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver > > On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote: > > Hi Anitha > > > > On Tue, Jul 14, 2020 at 01:56:46PM -0700, Anitha Chrisanthus wrote: > > > This is a new DRM driver for Intel's KeemBay SOC. > > > The SoC couples an ARM Cortex A53 CPU with an Intel > > > Movidius VPU. > > > > > > This driver is tested with the KMB EVM board which is the refernce baord > > > for Keem Bay SOC. The SOC's display pipeline is as follows > > > > > > +--++-++---+ > > > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > > > +--++-++---+ > > > > > > LCD controller and Mipi DSI transmitter are part of the SOC and > > > mipi to HDMI converter is ADV7535 for KMB EVM board. > > > > > > The DRM driver is a basic KMS atomic modesetting display driver and > > > has no 2D or 3D graphics.It calls into the ADV bridge driver at > > > the connector level. > > > > > > Only 1080p resolution and single plane is supported at this time. > > > The usecase is for debugging video and camera outputs. > > > > > > Device tree patches are under review here > > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1- > daniele.alessandre...@linux.intel.com/T/ > > > > Cool, new driver, thanks a lot for submitting. > > > > > Changes since v1: > > > - Removed redundant license text, updated license > > > - Rearranged include blocks > > > - renamed global vars and removed extern in c > > > - Used upclassing for dev_private > > > - Used drm_dev_init in drm device create (will be updated to use > > > devm_drm_dev_alloc() in a separate patch later as kmb driver is > > > currently > > > developed on 5.4 kernel) > > > > drm moves fairly quickly, please develop the upstream submission on top of > > linux-next or similar. We constantly add new helpers to simplify drivers, > > and we expect new driver submissions to be up to date with all that. > Seconded! > > > > > Another thing: From your description it sounds like it's a very simple > > driver, just a single plane/crtc, nothing fancy, plus adv bridge output. > > Is the driver already using simple display pipeline helpers? I think that > > would be an ideal fit and probably greatly simplifies the code. > > > > > - minor cleanups > > > > The patch series looks like it contains the entire development history, or > > at least large chunks of it. That's useful for you, but for upstreaming > > the main focues (especially for smaller drivers) is whether your driver > > uses all the available helpers and integrations correctly. And for that > > it's much easier if the history is cleaned up, and all intermediate steps > > removed. > And also agree on this point. > The submission could be split up in a few patches where the split is > file based. So only with the latest patch, containing Makefile + > Kconfig,the driver i buildable. > This would ease review as one looses focus when trying to review 1000+ > lines in one mail. > > You will loose some of the internal history - but if important keep > relevant parts in sensible comments. > > Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 00/59] Add support for Keem Bay DRM driver
Thanks much Sam for reviewing the code so quickly. I'll address your reviews in v2. Anitha > -Original Message- > From: Sam Ravnborg > Sent: Wednesday, July 1, 2020 12:01 AM > To: Chrisanthus, Anitha > Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J ; > Dea, Edmund J ; Vetter, Daniel > ; intel-...@lists.freedesktop.org; Vivi, Rodrigo > > Subject: Re: [PATCH 00/59] Add support for Keem Bay DRM driver > > Hi Anitha. > > On Tue, Jun 30, 2020 at 02:27:12PM -0700, Anitha Chrisanthus wrote: > > This is a new DRM driver for Intel's KeemBay SOC. > > The SoC couples an ARM Cortex A53 CPU with an Intel > > Movidius VPU. > ... > Nice and informative intro - thanks. > > The patchset looks more like a dump of current state and less like a > logical set of changes - as the few I looked at changed code introduced > in earlier patches. > So I ended up applying all patches to a local branch. > Very good to post an WIP version so you can capture some early > feedback. > It is never fun to think something is finished and then address a lot of > feedback. > > > Some general remarks after reading/browsing some of the code: > - Embeds drm_device - good > - Uses SPDX, good. But includes also a license text. Can we > get rid of the redundandt license text? > - Some of the inline functions in kmb_drv.h is too large > (kmb_write_bits_mipi()) > - There is stray code commented out using "//" here and there - shall be > dropped. > - Please arrange include files in blocks: > #include > > #include > > #include > > #include "kmb_*" > > Within each block sort alphabetially. > > - Use a kmb_ prefix or similar for global variables - like under_flow > - no extern in .c files - plane_status > - Consider using an array for the clk's > - In general use drm_info(), drm_err() for logging. > Yes, you will need to pass kmb_drm_private around to do so > - Do not use drm_device->dev_private, it is deprecated. Use upclassing > - kmb_driver can be slimmed a lot. See what recent drivers uses. There is > some nice defines so it is obvious what is not standard. > DRIVER_HAVE_IRQ is not relevant btw. > - Start using drmm_* support. The way kmb_drm_private is allocated > is sub-optimal > > The above was my fist drive-by comments - but do not be discouraged. > For the most part they should be easy to address. > Looking forward for other feedback and for v2. > > Let me know if the above triggers any questions. > > > +--++-++---+ > > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter | > > +--++-++---+ > > Question to dri-devel people: > Would the Mipi DSI be better represented outside the display driver? > If yes, how? > > Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
