Re: [RFC PATCHv2 4/9] drm/tidss: add new driver for TI Keystone platforms

Hi Tomi,

(CC'ing Jacopo Mondi for a comment about bus_formats in bridge drivers)

Thank you for the patch.

On Monday, 18 June 2018 16:22:37 EEST Tomi Valkeinen wrote:

This patch adds a new DRM driver for Texas Instruments DSS6 IP used on Texas Instruments Keystone K2G SoC. The DSS6 IP is a major change to the older DSS IP versions, which are supported by the omapdrm driver, and while on higher level the DSS6 resembles the older DSS versions, the registers and the internal pipelines differ a lot.

DSS6 IP on K2G is a "ultra-light" version, and has only a single plane and a single output. The driver will also support future DSS versions, which support multiple planes and outputs, so the driver already has support for those.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/tidss/Kconfig | 10 + drivers/gpu/drm/tidss/Makefile | 11 + drivers/gpu/drm/tidss/tidss_crtc.c | 390 +++++++ drivers/gpu/drm/tidss/tidss_crtc.h | 49 + drivers/gpu/drm/tidss/tidss_dispc.h | 145 +++ drivers/gpu/drm/tidss/tidss_dispc6.c | 1450 +++++++++++++++++++++++++ drivers/gpu/drm/tidss/tidss_dispc6.h | 109 ++ drivers/gpu/drm/tidss/tidss_drv.c | 333 ++++++ drivers/gpu/drm/tidss/tidss_drv.h | 41 + drivers/gpu/drm/tidss/tidss_encoder.c | 101 ++ drivers/gpu/drm/tidss/tidss_encoder.h | 22 + drivers/gpu/drm/tidss/tidss_irq.c | 193 ++++ drivers/gpu/drm/tidss/tidss_irq.h | 25 + drivers/gpu/drm/tidss/tidss_kms.c | 85 ++ drivers/gpu/drm/tidss/tidss_kms.h | 14 + drivers/gpu/drm/tidss/tidss_plane.c | 186 ++++ drivers/gpu/drm/tidss/tidss_plane.h | 25 + 19 files changed, 3192 insertions(+) create mode 100644 drivers/gpu/drm/tidss/Kconfig create mode 100644 drivers/gpu/drm/tidss/Makefile create mode 100644 drivers/gpu/drm/tidss/tidss_crtc.c create mode 100644 drivers/gpu/drm/tidss/tidss_crtc.h create mode 100644 drivers/gpu/drm/tidss/tidss_dispc.h create mode 100644 drivers/gpu/drm/tidss/tidss_dispc6.c create mode 100644 drivers/gpu/drm/tidss/tidss_dispc6.h create mode 100644 drivers/gpu/drm/tidss/tidss_drv.c create mode 100644 drivers/gpu/drm/tidss/tidss_drv.h create mode 100644 drivers/gpu/drm/tidss/tidss_encoder.c create mode 100644 drivers/gpu/drm/tidss/tidss_encoder.h create mode 100644 drivers/gpu/drm/tidss/tidss_irq.c create mode 100644 drivers/gpu/drm/tidss/tidss_irq.h create mode 100644 drivers/gpu/drm/tidss/tidss_kms.c create mode 100644 drivers/gpu/drm/tidss/tidss_kms.h create mode 100644 drivers/gpu/drm/tidss/tidss_plane.c create mode 100644 drivers/gpu/drm/tidss/tidss_plane.h

[snip]

diff --git a/drivers/gpu/drm/tidss/Kconfig b/drivers/gpu/drm/tidss/Kconfig new file mode 100644 index 000000000000..818666db08a4 --- /dev/null +++ b/drivers/gpu/drm/tidss/Kconfig @@ -0,0 +1,10 @@ +config DRM_TIDSS

Isn't the name TIDSS a bit too generic ? Previous platforms also had a DSS.

• tristate "DRM Support for TI Keystone"
• depends on DRM && OF
• depends on ARM || ARM64 || COMPILE_TEST
• select DRM_KMS_HELPER
• select DRM_KMS_CMA_HELPER
• select DRM_GEM_CMA_HELPER
• select VIDEOMODE_HELPERS
• help

•  TI Keystone
  

This is a bit short :)

diff --git a/drivers/gpu/drm/tidss/Makefile b/drivers/gpu/drm/tidss/Makefile new file mode 100644 index 000000000000..ee6b24db0441 --- /dev/null +++ b/drivers/gpu/drm/tidss/Makefile @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0

+tidss-y := tidss_drv.o \

• tidss_kms.o \
• tidss_crtc.o \
• tidss_plane.o \
• tidss_encoder.o \
• tidss_dispc6.o \
• tidss_irq.o

How about keeping these alphabetically sorted?

+obj-$(CONFIG_DRM_TIDSS) += tidss.o diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c new file mode 100644 index 000000000000..22c11f1e3318 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_crtc.c @@ -0,0 +1,390 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#include <drm/drmP.h> +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_fb_cma_helper.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_plane_helper.h> +#include <uapi/linux/media-bus-format.h>

Can't you include <linux/media-bus-format.h> ? I think the uapi/ prefix isn't meant to be used within the kernel.

+#include "tidss_crtc.h" +#include "tidss_dispc.h" +#include "tidss_drv.h" +#include "tidss_irq.h"

+/* ------------------------------------------------------------------------

• • Page Flip
• */

+static void tidss_crtc_finish_page_flip(struct tidss_crtc *tcrtc) +{

• struct drm_device *ddev = tcrtc->crtc.dev;
• struct tidss_device *tidss = ddev->dev_private;
• struct drm_pending_vblank_event *event;
• unsigned long flags;
• bool busy;
• /*

• * New settings are taken into use at VFP, and GO bit is cleared at
  

• * the same time. This happens before the vertical blank interrupt.
  

• * So there is a small change that the driver sets GO bit after VFP, but
  

• * before vblank, and we have to check for that case here.
  

• */
  

• busy = tidss->dispc_ops->vp_go_busy(tidss->dispc, tcrtc->hw_videoport);
• if (busy)

• return;
  

• spin_lock_irqsave(&ddev->event_lock, flags);
• event = tcrtc->event;
• tcrtc->event = NULL;
• if (!event) {

• spin_unlock_irqrestore(&ddev->event_lock, flags);
  

• return;
  

• }
• dev_dbg(ddev->dev, "%s\n", __func__);

Debugging leftover ?

• drm_crtc_send_vblank_event(&tcrtc->crtc, event);
• spin_unlock_irqrestore(&ddev->event_lock, flags);
• drm_crtc_vblank_put(&tcrtc->crtc);

+}

+void tidss_crtc_vblank_irq(struct drm_crtc *crtc) +{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• drm_crtc_handle_vblank(crtc);
• tidss_crtc_finish_page_flip(tcrtc);

Shouldn't page flip be completed at frame done time ? What's the relationship between the vblank and frame done IRQs ?

+}

+void tidss_crtc_framedone_irq(struct drm_crtc *crtc) +{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• complete(&tcrtc->framedone_completion);

+}

+void tidss_crtc_error_irq(struct drm_crtc *crtc, u64 irqstatus) +{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• dev_err_ratelimited(crtc->dev->dev, "CRTC%u SYNC LOST: (irq %llx)\n",

• 	    tcrtc->hw_videoport, irqstatus);
  

+}

Extra blank line.

+/* ------------------------------------------------------------------------

• • CRTC Functions
• */

+static int tidss_crtc_atomic_check(struct drm_crtc *crtc,

• 		   struct drm_crtc_state *state)
  

+{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• const struct drm_display_mode *mode = &state->adjusted_mode;
• struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(state);
• int r;
• dev_dbg(ddev->dev, "%s\n", __func__);
• if (!state->enable)

• return 0;
  

• r = tidss->dispc_ops->vp_check_config(tidss->dispc, tcrtc->hw_videoport,

• 			      mode,
  

• 			      tcrtc_state->bus_format,
  

• 			      tcrtc_state->bus_flags);
  

• if (r == 0)

• return 0;
  

• dev_err(ddev->dev, "%s: failed (%ux%u pclk %u kHz)\n",

• __func__, mode->hdisplay, mode->vdisplay, mode->clock);
  

I think we shouldn't give userspace yet another way to spam the kernel log. This should be a debugging message at most, but I believe we should just remove it, as it doesn't give much information. If you want to add debugging messages to help userspace find out what goes wrong, let's push them down to the dispc ops where you could print the cause of the error.

• return r;

+}

+static void tidss_crtc_atomic_begin(struct drm_crtc *crtc,

• 		    struct drm_crtc_state *old_crtc_state)
  

+{

• struct drm_device *ddev = crtc->dev;
• dev_dbg(ddev->dev, "%s\n", __func__);

Is this useful? Same comment for the other debugging messages below that serve as function tracers.

If the function becomes empty you could just drop it.

+}

+static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,

• 		    struct drm_crtc_state *old_crtc_state)
  

+{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• dev_dbg(ddev->dev, "%s, crtc enabled %d, event %p\n",

• __func__, tcrtc->enabled, crtc->state->event);
  

• /* Only flush the CRTC if it is currently enabled. */
• if (!tcrtc->enabled)

In atomic drivers state should be stored in state structures. You could check old_crtc_state for this and remove the enabled field in struct tidss_crtc.

• return;
  

• WARN_ON(tidss->dispc_ops->vp_go_busy(tidss->dispc,

• 			     tcrtc->hw_videoport));
  

What will then happen ?

• // I think we always need the event to signal flip done
• WARN_ON(!crtc->state->event);

When using drm_atomic_helper_commit() the helper allocates an event internal if none is requested by userspace, except for legacy cursor updates if the driver implements atomic_°async_check and atomic_async_commit. You will this always have an event. I think you can drop this WARN_ON(), the driver will crash later when dereferencing the event anyway.

• if (crtc->state->color_mgmt_changed) {

• struct drm_color_lut *lut = NULL;
  

• uint length = 0;
  

unsigned int ? uint doesn't seem to be a common type in the kernel.

• if (crtc->state->gamma_lut) {
  

• 	lut = (struct drm_color_lut *)
  

• 	      crtc->state->gamma_lut->data;
  

• 	length = crtc->state->gamma_lut->length /
  

• 		 sizeof(*lut);
  

• }
  

• tidss->dispc_ops->vp_set_gamma(tidss->dispc,
  

• 			       tcrtc->hw_videoport,
  

• 			       lut, length);
  

• }
• WARN_ON(drm_crtc_vblank_get(crtc) != 0);

When can this fail ?

• spin_lock_irq(&ddev->event_lock);
• tidss->dispc_ops->vp_go(tidss->dispc, tcrtc->hw_videoport);
• if (crtc->state->event) {

• tcrtc->event = crtc->state->event;
  

• crtc->state->event = NULL;
  

• }
• spin_unlock_irq(&ddev->event_lock);

+}

+static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,

• 		     struct drm_crtc_state *old_state)
  

+{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc->state);
• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
• int r;
• dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
• tidss->dispc_ops->runtime_get(tidss->dispc);
• r = tidss->dispc_ops->vp_set_clk_rate(tidss->dispc, tcrtc->hw_videoport,

• 			      mode->clock * 1000);
  

• WARN_ON(r);
• r = tidss->dispc_ops->vp_enable_clk(tidss->dispc, tcrtc->hw_videoport);
• WARN_ON(r);

That doesn't seem to be good error handling :-/

• /* Turn vertical blanking interrupt reporting on. */
• drm_crtc_vblank_on(crtc);
• tcrtc->enabled = true;
• tidss->dispc_ops->vp_enable(tidss->dispc, tcrtc->hw_videoport,

• 		    mode,
  

• 		    tcrtc_state->bus_format,
  

• 		    tcrtc_state->bus_flags);
  

• spin_lock_irq(&ddev->event_lock);
• if (crtc->state->event) {

• drm_crtc_send_vblank_event(crtc, crtc->state->event);
  

• crtc->state->event = NULL;
  

• }

Why do you report vblank when enabling the CRTC ?

• spin_unlock_irq(&ddev->event_lock);

+}

+static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,

• 		      struct drm_crtc_state *old_state)
  

+{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
• reinit_completion(&tcrtc->framedone_completion);
• tidss->dispc_ops->vp_disable(tidss->dispc, tcrtc->hw_videoport);
• if (!wait_for_completion_timeout(&tcrtc->framedone_completion,

• 			 msecs_to_jiffies(500)))
  

• dev_err(tidss->dev, "Timeout waiting for framedone on crtc %d",
  

• 	tcrtc->hw_videoport);
  

Without detailed knowledge of the hardware I can't tell for sure, but couldn't this be racy ? Could a vblank event be reported right before the reinit_completion() call, and ->vp_disable() called before the start of the next frame, resulting in no further vblank event being generated ? A wake_up()/wait_for_event() with an explicit test on whether a page flip is pending (if possible at the hardware level ?) might be better.

• spin_lock_irq(&ddev->event_lock);
• if (crtc->state->event) {

• drm_crtc_send_vblank_event(crtc, crtc->state->event);
  

Is this needed only when waiting for completion times out, or is it needed in the general case ? I would assume the event to be signaled by the interrupt handler.

• crtc->state->event = NULL;
  

• }
• spin_unlock_irq(&ddev->event_lock);
• tcrtc->enabled = false;
• drm_crtc_vblank_off(crtc);
• tidss->dispc_ops->vp_disable_clk(tidss->dispc, tcrtc->hw_videoport);
• tidss->dispc_ops->runtime_put(tidss->dispc);
• spin_lock_irq(&crtc->dev->event_lock);
• if (crtc->state->event) {

• drm_crtc_send_vblank_event(crtc, crtc->state->event);
  

• crtc->state->event = NULL;
  

A second one ?

• }
• spin_unlock_irq(&crtc->dev->event_lock);

+}

+static +enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,

• 			   const struct drm_display_mode *mode)
  

+{

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• return tidss->dispc_ops->vp_check_mode(tidss->dispc,

• 			       tcrtc->hw_videoport,
  

• 			       mode);
  

+}

+static const struct drm_crtc_helper_funcs crtc_helper_funcs = {

• .atomic_check = tidss_crtc_atomic_check,
• .atomic_begin = tidss_crtc_atomic_begin,
• .atomic_flush = tidss_crtc_atomic_flush,
• .atomic_enable = tidss_crtc_atomic_enable,
• .atomic_disable = tidss_crtc_atomic_disable,
• .mode_valid = tidss_crtc_mode_valid,

+};

+static void tidss_crtc_reset(struct drm_crtc *crtc) +{

• if (crtc->state)

• __drm_atomic_helper_crtc_destroy_state(crtc->state);
  

• kfree(crtc->state);

As you're testing for crtc->state above you could put this in the same code block. It would avoid a double check in the case where crtc->state is NULL.

• crtc->state = kzalloc(sizeof(struct tidss_crtc_state), GFP_KERNEL);

Please don't rely on drm_crtc_state being the first field of tidss_crtc_state, it's not very error-proof.

Also, sizeof(struct name) isn't favoured. If you stop relying on the field order you will need to add a variable for the custom state pointer, so you will be able to use sizeof(*var).

• if (crtc->state)

• crtc->state->crtc = crtc;
  

+}

+static struct drm_crtc_state * +tidss_crtc_duplicate_state(struct drm_crtc *crtc) +{

• struct tidss_crtc_state *state, *current_state;
• if (WARN_ON(!crtc->state))

• return NULL;
  

Can this happen ?

• current_state = to_tidss_crtc_state(crtc->state);
• state = kmalloc(sizeof(*state), GFP_KERNEL);
• if (!state)

• return NULL;
  

• __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
• state->bus_format = current_state->bus_format;
• state->bus_flags = current_state->bus_flags;
• return &state->base;

+}

+static int tidss_crtc_enable_vblank(struct drm_crtc *crtc) +{

• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• dev_dbg(ddev->dev, "%s\n", __func__);
• tidss->dispc_ops->runtime_get(tidss->dispc);
• tidss_irq_enable_vblank(crtc);
• return 0;

+}

+static void tidss_crtc_disable_vblank(struct drm_crtc *crtc) +{

• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• dev_dbg(ddev->dev, "%s\n", __func__);
• tidss_irq_disable_vblank(crtc);
• tidss->dispc_ops->runtime_put(tidss->dispc);

+}

+static const struct drm_crtc_funcs crtc_funcs = {

• .reset = tidss_crtc_reset,
• .destroy = drm_crtc_cleanup,
• .set_config = drm_atomic_helper_set_config,
• .page_flip = drm_atomic_helper_page_flip,
• .atomic_duplicate_state = tidss_crtc_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
• .enable_vblank = tidss_crtc_enable_vblank,
• .disable_vblank = tidss_crtc_disable_vblank,

+};

+struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss, u32 hw_videoport,

• 		     struct drm_plane *primary, struct device_node *epnode)
  

epnode isn't used.What do you need the epnode for ?

+{

• struct tidss_crtc *tcrtc;
• struct drm_crtc *crtc;
• int ret;
• tcrtc = devm_kzalloc(tidss->dev, sizeof(*tcrtc), GFP_KERNEL);
• if (!tcrtc)

• return ERR_PTR(-ENOMEM);
  

• tcrtc->hw_videoport = hw_videoport;
• init_completion(&tcrtc->framedone_completion);
• crtc = &tcrtc->crtc;
• ret = drm_crtc_init_with_planes(tidss->ddev, crtc, primary,

• 			NULL, &crtc_funcs, NULL);
  

• if (ret < 0)

• return ERR_PTR(ret);
  

• drm_crtc_helper_add(crtc, &crtc_helper_funcs);
• /* The dispc API adapts to what ever size we ask from in no

For multiline comments the first line usually contains just /*

s/from in/form it/ ?

• * matter what HW supports. X-server assumes 256 element gamma
  

• * tables so lets use that. Size of HW gamma table can be
  

• * extracted with dispc_vp_gamma_size(). If it returns 0
  

• * gamma table is not supprted.
  

• */
  

• if (tidss->dispc_ops->vp_gamma_size(tidss->dispc, hw_videoport)) {

• uint gamma_lut_size = 256;
  

unsigned int ? Same for other locations below, if any.

• drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
  

• drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
  

• }
• return tcrtc;

+} diff --git a/drivers/gpu/drm/tidss/tidss_crtc.h b/drivers/gpu/drm/tidss/tidss_crtc.h new file mode 100644 index 000000000000..9f32d81e55d9 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_crtc.h @@ -0,0 +1,49 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#ifndef __TIDSS_CRTC_H__ +#define __TIDSS_CRTC_H__

+#include <linux/wait.h> +#include <linux/completion.h> +#include <drm/drm_crtc.h>

Let's try to keep headers alphabetically sorted ?

+#include "tidss_dispc.h"

+#define to_tidss_crtc(c) container_of((c), struct tidss_crtc, crtc)

+struct tidss_crtc {

• struct drm_crtc crtc;
• u32 hw_videoport;
• struct drm_pending_vblank_event *event;
• /* has crtc_atomic_enable been called? */
• bool enabled;
• struct completion framedone_completion;

+};

+#define to_tidss_crtc_state(x) container_of(x, struct tidss_crtc_state, base)

+struct tidss_crtc_state {

• /* Must be first. */
• struct drm_crtc_state base;
• u32 bus_format;
• u32 bus_flags;

+};

+struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss, u32 hw_videoport,

• 		     struct drm_plane *primary, struct device_node *epnode);
  

+void tidss_crtc_vblank_irq(struct drm_crtc *crtc); +void tidss_crtc_framedone_irq(struct drm_crtc *crtc); +void tidss_crtc_error_irq(struct drm_crtc *crtc, u64 irqstatus);

+#endif diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h new file mode 100644 index 000000000000..38ee3d75404e --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_dispc.h @@ -0,0 +1,145 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#ifndef __TIDSS_DISPC_H__ +#define __TIDSS_DISPC_H__

+struct tidss_device; +struct dispc_device; +struct drm_color_lut;

You could try to keep structure declarations sorted too.

+#define DSS_MAX_CHANNELS 8 +#define DSS_MAX_PLANES 8

+/*

• • Based on the above 2 defines the bellow defines describe following
• • u64 IRQ bits:
• • bit group |dev |mrg0|mrg1|mrg2|mrg3|mrg4|mrg5|mrg6|mrg7|plane

0-7|<unused> |

• • bit use |Dfou|FEOL|FEOL|FEOL|FEOL|FEOL|FEOL|FEOL|FEOL|UUUU|UUUU| | | |

| |

• • bit number|0-3 |4-7 |8-11| 12-35 | 36-43 |

44-63 |

• • device bits: D = OCP error
• • WB bits: f = frame done wb, o = wb buffer overflow,

• • u = wb buffer uncomplete
    

• • vp bits: F = frame done, E = vsync even, O = vsync odd, L = sync lost
• • plane bits: U = fifo underflow
• */

+#define DSS_IRQ_DEVICE_OCP_ERR BIT_ULL(0)

+#define DSS_IRQ_DEVICE_FRAMEDONEWB BIT_ULL(1) +#define DSS_IRQ_DEVICE_WBBUFFEROVERFLOW BIT_ULL(2) +#define DSS_IRQ_DEVICE_WBUNCOMPLETEERROR BIT_ULL(3) +#define DSS_IRQ_DEVICE_WB_MASK GENMASK_ULL(3, 1)

+#define DSS_IRQ_VP_BIT_N(ch, bit) (4 + 4 * ch + bit)

Please enclose macro arguments in parentheses. Doesn't checkpatch warn about that ?

+#define DSS_IRQ_PLANE_BIT_N(plane, bit) \

• (DSS_IRQ_VP_BIT_N(DSS_MAX_CHANNELS, 0) + 1 * plane + bit)

+#define DSS_IRQ_VP_BIT(ch, bit) BIT_ULL(DSS_IRQ_VP_BIT_N(ch, bit)) +#define DSS_IRQ_PLANE_BIT(plane, bit) BIT_ULL(DSS_IRQ_PLANE_BIT_N(plane, bit)) + +#define DSS_IRQ_VP_MASK(ch) \

• GENMASK_ULL(DSS_IRQ_VP_BIT_N(ch, 3), DSS_IRQ_VP_BIT_N(ch, 0))

+#define DSS_IRQ_PLANE_MASK(plane) \

• GENMASK_ULL(DSS_IRQ_PLANE_BIT_N(plane, 0), DSS_IRQ_PLANE_BIT_N(plane, 0))

+#define DSS_IRQ_VP_FRAME_DONE(ch) DSS_IRQ_VP_BIT(ch, 0) +#define DSS_IRQ_VP_VSYNC_EVEN(ch) DSS_IRQ_VP_BIT(ch, 1) +#define DSS_IRQ_VP_VSYNC_ODD(ch) DSS_IRQ_VP_BIT(ch, 2) +#define DSS_IRQ_VP_SYNC_LOST(ch) DSS_IRQ_VP_BIT(ch, 3)

+#define DSS_IRQ_PLANE_FIFO_UNDERFLOW(plane) DSS_IRQ_PLANE_BIT(plane, 0)

+struct tidss_vp_info {

• u32 default_color;

+};

+struct tidss_plane_info {

This structure would be worth documenting.

• dma_addr_t paddr;
• dma_addr_t p_uv_addr; /* for NV12 format */
• u16 fb_width;
• u16 width;
• u16 height;
• u32 fourcc;
• u16 pos_x;
• u16 pos_y;
• u16 out_width; /* if 0, out_width == width */
• u16 out_height; /* if 0, out_height == height */
• u8 global_alpha;
• u8 pre_mult_alpha;
• u8 zorder;

+};

+struct dispc_ops {

tidss_dispc_ops ?

• u64 (*read_and_clear_irqstatus)(struct dispc_device *dispc);
• void (*write_irqenable)(struct dispc_device *dispc, u64 enable);
• int (*runtime_get)(struct dispc_device *dispc);
• void (*runtime_put)(struct dispc_device *dispc);
• int (*get_num_planes)(struct dispc_device *dispc);
• int (*get_num_vps)(struct dispc_device *dispc);
• const char *(*plane_name)(struct dispc_device *dispc,

• 		  u32 hw_plane);
  

• const char *(*vp_name)(struct dispc_device *dispc,

• 	       u32 hw_videoport);
  

Do the hardware IDs need to be exactly and explicitly 32-bit wide ? Can't we use unsigned int instead ?

• u32 (*get_memory_bandwidth_limit)(struct dispc_device *dispc);
• void (*vp_enable)(struct dispc_device *dispc, u32 hw_videoport,

• 	  const struct drm_display_mode *mode,
  

• 	  u32 bus_fmt, u32 bus_flags);
  

• void (*vp_disable)(struct dispc_device *dispc, u32 hw_videoport);
• bool (*vp_go_busy)(struct dispc_device *dispc,

• 	   u32 hw_videoport);
  

• void (*vp_go)(struct dispc_device *dispc, u32 hw_videoport);
• void (*vp_setup)(struct dispc_device *dispc, u32 hw_videoport,

• 	 const struct tidss_vp_info *info);
  

• enum drm_mode_status (*vp_check_mode)(struct dispc_device *dispc,

• 			      u32 hw_videoport,
  

• 			      const struct drm_display_mode *mode);
  

How about calling this vp_mode_Valid to use the DRM terminology ?

• int (*vp_check_config)(struct dispc_device *dispc, u32 hw_videoport,

• 	       const struct drm_display_mode *mode,
  

• 	       u32 bus_fmt, u32 bus_flags);
  

• u32 (*vp_gamma_size)(struct dispc_device *dispc,

• 	     u32 hw_videoport);
  

• void (*vp_set_gamma)(struct dispc_device *dispc,

• 	     u32 hw_videoport,
  

• 	     const struct drm_color_lut *lut,
  

• 	     unsigned int length);
  

• int (*plane_enable)(struct dispc_device *dispc, u32 hw_plane,

• 	    bool enable);
  

• int (*plane_setup)(struct dispc_device *dispc, u32 hw_plane,

• 	   const struct tidss_plane_info *oi,
  

• 	   u32 hw_videoport);
  

• int (*vp_set_clk_rate)(struct dispc_device *dispc,

• 	       u32 hw_videoport, unsigned long rate);
  

• int (*vp_enable_clk)(struct dispc_device *dispc, u32 hw_videoport);
• void (*vp_disable_clk)(struct dispc_device *dispc, u32 hw_videoport);
• int (*runtime_suspend)(struct dispc_device *dispc);
• int (*runtime_resume)(struct dispc_device *dispc);
• void (*remove)(struct dispc_device *dispc);
• int (*modeset_init)(struct dispc_device *dispc);

+};

I think I would sort the functions by category: dispc, vp, planes, ...

+int dispc6_init(struct tidss_device *tidss);

+#endif diff --git a/drivers/gpu/drm/tidss/tidss_dispc6.c b/drivers/gpu/drm/tidss/tidss_dispc6.c new file mode 100644 index 000000000000..7772d4484a6e --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_dispc6.c

[snip]

diff --git a/drivers/gpu/drm/tidss/tidss_dispc6.h b/drivers/gpu/drm/tidss/tidss_dispc6.h new file mode 100644 index 000000000000..80197c812acd --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_dispc6.h

I'll review the dispc6 code separately.

[snip]

diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c new file mode 100644 index 000000000000..8002766c4640 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_drv.c @@ -0,0 +1,333 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#include <linux/console.h> +#include <linux/of_device.h> +#include <linux/pm_runtime.h>

+#include <drm/drmP.h> +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_fb_cma_helper.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_gem_cma_helper.h>

+#include "tidss_dispc.h" +#include "tidss_drv.h" +#include "tidss_irq.h" +#include "tidss_kms.h"

+/* ------------------------------------------------------------------------

• • Device Information
• */

+static const struct tidss_features tidss_k2g_feats = {

• .dispc_init = dispc6_init,

+};

+static const struct of_device_id tidss_of_table[] = {

• { .compatible = "ti,k2g-dss", .data = &tidss_k2g_feats },
• { }

+};

How about moving this a bit closer to where tidss_of_table is used ? Just before the probe function possibly ?

As you use block comments to mark sections, a section named "DRM Driver" would be useful here.

+DEFINE_DRM_GEM_CMA_FOPS(tidss_fops);

+static struct drm_driver tidss_driver = {

• .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME

• 		| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
  

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
• .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
• .gem_prime_import = drm_gem_prime_import,
• .gem_prime_export = drm_gem_prime_export,
• .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,
• .dumb_create = drm_gem_cma_dumb_create,
• .fops = &tidss_fops,
• .name = "tidss",
• .desc = "TI Keystone DSS",
• .date = "20180215",

So this date will be remembers forever as the birthday of the driver :-)

• .major = 1,
• .minor = 0,
• .irq_preinstall = tidss_irq_preinstall,
• .irq_postinstall = tidss_irq_postinstall,
• .irq_handler = tidss_irq_handler,
• .irq_uninstall = tidss_irq_uninstall,

I personally find the way the DRM framework manages IRQ to lead to more complex code than doing it manually. Up to you.

+};

+#ifdef CONFIG_PM +/* ------------------------------------------------------------------------

• • Power management
• */

+static int tidss_pm_runtime_suspend(struct device *dev) +{

• struct tidss_device *tidss = dev_get_drvdata(dev);
• dev_dbg(dev, "%s\n", __func__);
• return tidss->dispc_ops->runtime_suspend(tidss->dispc);

+}

+static int tidss_pm_runtime_resume(struct device *dev) +{

• struct tidss_device *tidss = dev_get_drvdata(dev);
• int r;
• dev_dbg(dev, "%s\n", __func__);
• r = tidss->dispc_ops->runtime_resume(tidss->dispc);
• if (r)

• return r;
  

• tidss_irq_resume(tidss->ddev);
• return 0;

+}

+#ifdef CONFIG_PM_SLEEP +static int tidss_suspend(struct device *dev) +{

• struct tidss_device *tidss = dev_get_drvdata(dev);
• struct drm_atomic_state *state;
• drm_kms_helper_poll_disable(tidss->ddev);
• console_lock();
• drm_fbdev_cma_set_suspend(tidss->fbdev, 1);
• console_unlock();
• state = drm_atomic_helper_suspend(tidss->ddev);
• if (IS_ERR(state)) {

• console_lock();
  

• drm_fbdev_cma_set_suspend(tidss->fbdev, 0);
  

• console_unlock();
  

• drm_kms_helper_poll_enable(tidss->ddev);
  

• return PTR_ERR(state);
  

• }
• tidss->saved_state = state;

We have drm_mode_config_helper_suspend() and drm_mode_config_helper_resume(), could they be used ?

• return 0;

+}

+static int tidss_resume(struct device *dev) +{

• struct tidss_device *tidss = dev_get_drvdata(dev);
• int ret = 0;
• if (tidss->saved_state)

• ret = drm_atomic_helper_resume(tidss->ddev, tidss->saved_state);
  

• console_lock();
• drm_fbdev_cma_set_suspend(tidss->fbdev, 0);
• console_unlock();
• drm_kms_helper_poll_enable(tidss->ddev);
• return ret;

+} +#endif /* CONFIG_PM_SLEEP */

+static const struct dev_pm_ops tidss_pm_ops = {

• .runtime_suspend = tidss_pm_runtime_suspend,
• .runtime_resume = tidss_pm_runtime_resume,
• SET_SYSTEM_SLEEP_PM_OPS(tidss_suspend, tidss_resume)

+};

+#endif /* CONFIG_PM */

+/* ------------------------------------------------------------------------

• • Platform driver
• */

+static int tidss_probe(struct platform_device *pdev) +{

• struct device *dev = &pdev->dev;
• struct tidss_device *tidss;
• struct drm_device *ddev;
• int ret;
• int irq;
• dev_dbg(dev, "%s\n", __func__);
• tidss = devm_kzalloc(dev, sizeof(*tidss), GFP_KERNEL);
• if (tidss == NULL)

• return -ENOMEM;
  

• tidss->pdev = pdev;
• tidss->dev = dev;
• tidss->feat = of_device_get_match_data(dev);

I'd spell it features instead of feat if that's not too long.

• platform_set_drvdata(pdev, tidss);
• ddev = drm_dev_alloc(&tidss_driver, dev);
• if (IS_ERR(ddev))

• return PTR_ERR(ddev);
  

• tidss->ddev = ddev;
• ddev->dev_private = tidss;
• pm_runtime_enable(dev);
• ret = tidss->feat->dispc_init(tidss);
• if (ret) {

• dev_err(dev, "failed to initialize dispc: %d\n", ret);
  

• goto err_disable_pm;
  

• }

+#ifndef CONFIG_PM_SLEEP

• /* no PM, so force enable DISPC */
• tidss->dispc_ops->runtime_resume(tidss->dispc);

+#endif

This should really be hidden somewhere in the PM framework, it's pretty ugly otherwise :( Or should we depend on CONFIG_PM_SLEEP ?

• ret = tidss_modeset_init(tidss);
• if (ret < 0) {

• if (ret != -EPROBE_DEFER)
  

• 	dev_err(dev, "failed to init DRM/KMS (%d)\n", ret);
  

• goto err_runtime_suspend;
  

• }
• irq = platform_get_irq(tidss->pdev, 0);
• if (irq < 0) {

• ret = irq;
  

• dev_err(dev, "platform_get_irq failed: %d\n", ret);
  

• goto err_modeset_cleanup;
  

• }
• ret = drm_irq_install(ddev, irq);
• if (ret) {

• dev_err(dev, "drm_irq_install failed: %d\n", ret);
  

• goto err_modeset_cleanup;
  

• }

+#ifdef CONFIG_DRM_FBDEV_EMULATION

• if (ddev->mode_config.num_connector) {

• struct drm_fbdev_cma *fbdev;
  

• fbdev = drm_fbdev_cma_init(ddev, 32,
  

• 			   ddev->mode_config.num_connector);
  

• if (IS_ERR(fbdev)) {
  

• 	dev_err(tidss->dev, "fbdev init failed\n");
  

• 	ret =  PTR_ERR(fbdev);
  

• 	goto err_irq_uninstall;
  

• }
  

Should this be a non-fatal error ? We can still use the display even if the fbdev emulation isn't available.

• tidss->fbdev = fbdev;
  

• }

+#endif

• drm_kms_helper_poll_init(ddev);
• ret = drm_dev_register(ddev, 0);
• if (ret) {

• dev_err(dev, "failed to register DRM device\n");
  

• goto err_poll_fini;
  

• }
• dev_dbg(dev, "%s done\n", __func__);
• return 0;

+err_poll_fini:

• drm_kms_helper_poll_fini(ddev);
• if (tidss->fbdev)

• drm_fbdev_cma_fini(tidss->fbdev);
  

• drm_atomic_helper_shutdown(ddev);

+#ifdef CONFIG_DRM_FBDEV_EMULATION +err_irq_uninstall: +#endif

• drm_irq_uninstall(ddev);

+err_modeset_cleanup:

• drm_mode_config_cleanup(ddev);

+err_runtime_suspend: +#ifndef CONFIG_PM_SLEEP

• /* no PM, so force disable DISPC */
• tidss->dispc_ops->runtime_suspend(tidss->dispc);

+#endif

• tidss->dispc_ops->remove(tidss->dispc);

+err_disable_pm:

• pm_runtime_disable(dev);
• drm_dev_put(ddev);
• return ret;

+}

+static int tidss_remove(struct platform_device *pdev) +{

• struct device *dev = &pdev->dev;
• struct tidss_device *tidss = platform_get_drvdata(pdev);
• struct drm_device *ddev = tidss->ddev;
• dev_dbg(dev, "%s\n", __func__);
• drm_dev_unregister(ddev);
• drm_kms_helper_poll_fini(ddev);
• if (tidss->fbdev)

• drm_fbdev_cma_fini(tidss->fbdev);
  

• drm_atomic_helper_shutdown(ddev);
• drm_irq_uninstall(ddev);
• drm_mode_config_cleanup(ddev);

+#ifndef CONFIG_PM_SLEEP

• /* no PM, so force disable DISPC */
• tidss->dispc_ops->runtime_suspend(tidss->dispc);

+#endif

• tidss->dispc_ops->remove(tidss->dispc);
• pm_runtime_disable(dev);
• drm_dev_put(ddev);
• dev_dbg(dev, "%s done\n", __func__);
• return 0;

+}

+MODULE_DEVICE_TABLE(of, tidss_of_table);

+static struct platform_driver tidss_platform_driver = {

• .probe = tidss_probe,
• .remove = tidss_remove,
• .driver = {

• .name	= "tidss",
  

+#ifdef CONFIG_PM

• .pm	= &tidss_pm_ops,
  

+#endif

• .of_match_table = tidss_of_table,
  

• .suppress_bind_attrs = true,
  

• },

+};

+module_platform_driver(tidss_platform_driver);

+MODULE_AUTHOR("Tomi Valkeinen tomi.valkeinen@ti.com"); +MODULE_DESCRIPTION("TI Keystone DSS Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h new file mode 100644 index 000000000000..fd1e767e9308 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_drv.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#ifndef __TIDSS_DRV_H__ +#define __TIDSS_DRV_H__

+#include <linux/spinlock.h>

+struct tidss_device {

• struct device *dev; /* Underlying DSS device */
• struct platform_device *pdev; /* Underlying DSS platform device */

Do we need to store both ? Can't we store dev only, and cast it to platform_device in the only location that needs it ?

• struct drm_device *ddev; /* DRM device for DSS */
• struct drm_fbdev_cma *fbdev;
• struct dispc_device *dispc;
• const struct dispc_ops *dispc_ops;
• const struct tidss_features *feat;
• u32 num_crtcs;

I think unsigned_int should be more than enough.

• struct drm_crtc *crtcs[8];

Do you plan to support platforms with 8 CRTCs ?

• u32 num_planes;
• struct drm_plane *planes[8];
• spinlock_t wait_lock; /* protects the irq masks */
• u64 irq_mask; /* enabled irqs in addition to wait_list */
• u64 irq_uf_mask; /* underflow irq bits for all planes */
• struct drm_atomic_state *saved_state;

+};

+struct tidss_features {

• int (*dispc_init)(struct tidss_device *tidss);

+};

+#endif diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c new file mode 100644 index 000000000000..fb7bc3fc0fe8 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_encoder.c @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#include <linux/export.h>

+#include <drm/drmP.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_panel.h> +#include <drm/drm_of.h>

+#include "tidss_drv.h" +#include "tidss_encoder.h" +#include "tidss_crtc.h"

+/* ------------------------------------------------------------------------

• • Encoder
• */

+static void tidss_encoder_disable(struct drm_encoder *encoder) +{

• struct drm_device *ddev = encoder->dev;
• dev_dbg(ddev->dev, "%s\n", __func__);

Extra blank line.

+}

+static void tidss_encoder_enable(struct drm_encoder *encoder) +{

• struct drm_device *ddev = encoder->dev;
• dev_dbg(ddev->dev, "%s\n", __func__);

Here too.

+}

The encoder enable and disable functions are optional.

+static int tidss_encoder_atomic_check(struct drm_encoder *encoder,

• 		      struct drm_crtc_state *crtc_state,
  

• 		      struct drm_connector_state *conn_state)
  

+{

• struct drm_device *ddev = encoder->dev;
• struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
• struct drm_display_info *di = &conn_state->connector->display_info;
• dev_dbg(ddev->dev, "%s\n", __func__);
• // XXX any cleaner way to set bus format and flags?

Not that I know of :-/ Jacopo (CC'ed) started working on support for bus formats in bridge drivers, which you might be interested in.

This being said, why do you need to store them in the CRTC state, can't you access them directly in the two locations where they're used ?

• tcrtc_state->bus_format = di->bus_formats[0];
• tcrtc_state->bus_flags = di->bus_flags;
• return 0;

+}

+static void tidss_encoder_mode_set(struct drm_encoder *encoder,

• 		   struct drm_crtc_state *crtc_state,
  

• 		   struct drm_connector_state *conn_state)
  

+{

• struct drm_device *ddev = encoder->dev;
• dev_dbg(ddev->dev, "%s\n", __func__);

Extra blank line.

+}

This function is optional too.

+static const struct drm_encoder_helper_funcs encoder_helper_funcs = {

• .atomic_mode_set = tidss_encoder_mode_set,
• .disable = tidss_encoder_disable,
• .enable = tidss_encoder_enable,
• .atomic_check = tidss_encoder_atomic_check,

+};

+static const struct drm_encoder_funcs encoder_funcs = {

• .destroy = drm_encoder_cleanup,

+};

+struct tidss_encoder *tidss_encoder_create(struct tidss_device *tidss,

• 			   u32 encoder_type, u32 possible_crtcs)
  

+{

• struct tidss_encoder *tenc;
• struct drm_encoder *enc;
• int ret;
• tenc = devm_kzalloc(tidss->dev, sizeof(*tenc), GFP_KERNEL);
• if (!tenc)

• return ERR_PTR(-ENOMEM);
  

• enc = &tenc->encoder;
• enc->possible_crtcs = possible_crtcs;
• ret = drm_encoder_init(tidss->ddev, enc, &encoder_funcs,

• 	       encoder_type, NULL);
  

• if (ret < 0)

• return ERR_PTR(ret);
  

• drm_encoder_helper_add(enc, &encoder_helper_funcs);
• dev_dbg(tidss->dev, "Encoder create done\n");
• return tenc;

+} diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h new file mode 100644 index 000000000000..f1bd05a3f611 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_encoder.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#ifndef __TIDSS_ENCODER_H__ +#define __TIDSS_ENCODER_H__

+#include <drm/drm_encoder.h>

+struct tidss_device; +struct tidss_crtc;

+struct tidss_encoder {

• struct drm_encoder encoder;

+};

Do you need a tidss_encoder structure at all ?

+struct tidss_encoder *tidss_encoder_create(struct tidss_device *tidss,

• 			   u32 encoder_type, u32 possible_crtcs);
  

+#endif diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c new file mode 100644 index 000000000000..07c4e1f0924e --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_irq.c @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#include <drm/drmP.h>

+#include "tidss_irq.h" +#include "tidss_drv.h" +#include "tidss_dispc.h" +#include "tidss_crtc.h" +#include "tidss_plane.h"

+/* call with wait_lock and dispc runtime held */ +static void tidss_irq_update(struct drm_device *ddev) +{

• struct tidss_device *tidss = ddev->dev_private;
• assert_spin_locked(&tidss->wait_lock);
• tidss->dispc_ops->write_irqenable(tidss->dispc, tidss->irq_mask);

+}

+void tidss_irq_enable_vblank(struct drm_crtc *crtc) +{

• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• u32 hw_videoport = tcrtc->hw_videoport;
• unsigned long flags;
• spin_lock_irqsave(&tidss->wait_lock, flags);
• tidss->irq_mask |= DSS_IRQ_VP_VSYNC_EVEN(hw_videoport) |

• 	   DSS_IRQ_VP_VSYNC_ODD(hw_videoport);
  

• tidss_irq_update(ddev);
• spin_unlock_irqrestore(&tidss->wait_lock, flags);

+}

+void tidss_irq_disable_vblank(struct drm_crtc *crtc) +{

• struct drm_device *ddev = crtc->dev;
• struct tidss_device *tidss = ddev->dev_private;
• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
• u32 hw_videoport = tcrtc->hw_videoport;
• unsigned long flags;
• spin_lock_irqsave(&tidss->wait_lock, flags);
• tidss->irq_mask &= ~(DSS_IRQ_VP_VSYNC_EVEN(hw_videoport) |

• 	     DSS_IRQ_VP_VSYNC_ODD(hw_videoport));
  

• tidss_irq_update(ddev);
• spin_unlock_irqrestore(&tidss->wait_lock, flags);

+}

+static void tidss_irq_fifo_underflow(struct tidss_device *tidss,

• 		     u64 irqstatus)
  

+{

• static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,

• 		      DEFAULT_RATELIMIT_BURST);
  

• unsigned int i;
• u64 masked;
• spin_lock(&tidss->wait_lock);
• masked = irqstatus & tidss->irq_uf_mask & tidss->irq_mask;

Can you have irq_uf_mask bits that are not set in irq_mask ?

• spin_unlock(&tidss->wait_lock);
• if (!masked)

• return;
  

• if (!__ratelimit(&_rs))

• return;
  

• DRM_ERROR("FIFO underflow on ");
• for (i = 0; i < DSS_MAX_PLANES; ++i) {

• if (masked & DSS_IRQ_PLANE_FIFO_UNDERFLOW(i))
  

• 	pr_cont("%u:%s ", i,
  

• 		tidss->dispc_ops->plane_name(tidss->dispc, i));
  

• }
• pr_cont("(%016llx)\n", irqstatus);

+}

+static void tidss_irq_ocp_error_handler(struct drm_device *ddev,

• 			u64 irqstatus)
  

+{

• if (irqstatus & DSS_IRQ_DEVICE_OCP_ERR)

• dev_err_ratelimited(ddev->dev, "OCP error\n");
  

+}

+irqreturn_t tidss_irq_handler(int irq, void *arg) +{

• struct drm_device *ddev = (struct drm_device *) arg;
• struct tidss_device *tidss = ddev->dev_private;
• unsigned int id;
• u64 irqstatus;
• if (WARN_ON(!ddev->irq_enabled))

• return IRQ_NONE;
  

• irqstatus = tidss->dispc_ops->read_and_clear_irqstatus(tidss->dispc);
• for (id = 0; id < tidss->num_crtcs; id++) {

• struct drm_crtc *crtc = tidss->crtcs[id];
  

• struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
  

• u32 hw_videoport = tcrtc->hw_videoport;
  

• if (irqstatus & (DSS_IRQ_VP_VSYNC_EVEN(hw_videoport) |
  

• 		 DSS_IRQ_VP_VSYNC_ODD(hw_videoport)))
  

• 	tidss_crtc_vblank_irq(crtc);
  

• if (irqstatus & (DSS_IRQ_VP_FRAME_DONE(hw_videoport)))
  

• 	tidss_crtc_framedone_irq(crtc);
  

• if (irqstatus & DSS_IRQ_VP_SYNC_LOST(hw_videoport))
  

• 	tidss_crtc_error_irq(crtc, irqstatus);
  

• }
• tidss_irq_ocp_error_handler(ddev, irqstatus);
• tidss_irq_fifo_underflow(tidss, irqstatus);
• return IRQ_HANDLED;

+}

+void tidss_irq_preinstall(struct drm_device *ddev) +{

• struct tidss_device *tidss = ddev->dev_private;
• spin_lock_init(&tidss->wait_lock);
• tidss->dispc_ops->runtime_get(tidss->dispc);
• tidss->dispc_ops->write_irqenable(tidss->dispc, 0);
• tidss->dispc_ops->read_and_clear_irqstatus(tidss->dispc);
• tidss->dispc_ops->runtime_put(tidss->dispc);

+}

+int tidss_irq_postinstall(struct drm_device *ddev) +{

• struct tidss_device *tidss = ddev->dev_private;
• unsigned int i;
• unsigned long flags;
• tidss->dispc_ops->runtime_get(tidss->dispc);
• spin_lock_irqsave(&tidss->wait_lock, flags);
• tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
• tidss->irq_uf_mask = 0;
• for (i = 0; i < tidss->num_planes; ++i) {

• struct tidss_plane *tplane = to_tidss_plane(tidss->planes[i]);
  

• tidss->irq_uf_mask |= DSS_IRQ_PLANE_FIFO_UNDERFLOW(tplane->hw_plane_id);
  

• }
• tidss->irq_mask |= tidss->irq_uf_mask;
• for (i = 0; i < tidss->num_crtcs; ++i) {

• struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
  

• tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
  

• tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
  

• }
• tidss_irq_update(ddev);
• spin_unlock_irqrestore(&tidss->wait_lock, flags);
• tidss->dispc_ops->runtime_put(tidss->dispc);
• return 0;

+}

+void tidss_irq_uninstall(struct drm_device *ddev) +{

• struct tidss_device *tidss = ddev->dev_private;
• tidss->dispc_ops->runtime_get(tidss->dispc);
• tidss->dispc_ops->write_irqenable(tidss->dispc, 0);
• tidss->dispc_ops->runtime_put(tidss->dispc);

+}

+void tidss_irq_resume(struct drm_device *ddev) +{

• struct tidss_device *tidss = ddev->dev_private;
• unsigned long flags;
• spin_lock_irqsave(&tidss->wait_lock, flags);
• tidss_irq_update(ddev);
• spin_unlock_irqrestore(&tidss->wait_lock, flags);

+} diff --git a/drivers/gpu/drm/tidss/tidss_irq.h b/drivers/gpu/drm/tidss/tidss_irq.h new file mode 100644 index 000000000000..e1507298c9d8 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_irq.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#ifndef __TIDSS_IRQ_H__ +#define __TIDSS_IRQ_H__

+#include <linux/types.h>

+struct drm_crtc; +struct drm_device;

+void tidss_irq_enable_vblank(struct drm_crtc *crtc); +void tidss_irq_disable_vblank(struct drm_crtc *crtc);

+void tidss_irq_preinstall(struct drm_device *ddev); +int tidss_irq_postinstall(struct drm_device *ddev); +void tidss_irq_uninstall(struct drm_device *ddev); +irqreturn_t tidss_irq_handler(int irq, void *arg);

+void tidss_irq_resume(struct drm_device *ddev);

+#endif diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c new file mode 100644 index 000000000000..cd003f1569cd --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_kms.c @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_fb_cma_helper.h>

This is nearly alphabetically ordered :-)

+#include <drm/drm_gem_framebuffer_helper.h>

+#include "tidss_crtc.h" +#include "tidss_drv.h" +#include "tidss_encoder.h" +#include "tidss_kms.h" +#include "tidss_plane.h"

+static void tidss_atomic_commit_tail(struct drm_atomic_state *old_state) +{

• struct drm_device *ddev = old_state->dev;
• struct tidss_device *tidss = ddev->dev_private;
• dev_dbg(ddev->dev, "%s\n", __func__);
• tidss->dispc_ops->runtime_get(tidss->dispc);
• drm_atomic_helper_commit_modeset_disables(ddev, old_state);
• drm_atomic_helper_commit_planes(ddev, old_state, 0);
• drm_atomic_helper_commit_modeset_enables(ddev, old_state);
• drm_atomic_helper_commit_hw_done(old_state);
• drm_atomic_helper_wait_for_flip_done(ddev, old_state);
• drm_atomic_helper_cleanup_planes(ddev, old_state);
• tidss->dispc_ops->runtime_put(tidss->dispc);

+}

+static const struct drm_mode_config_helper_funcs mode_config_helper_funcs = { + .atomic_commit_tail = tidss_atomic_commit_tail, +};

+static const struct drm_mode_config_funcs mode_config_funcs = {

• .fb_create = drm_gem_fb_create,
• .atomic_check = drm_atomic_helper_check,
• .atomic_commit = drm_atomic_helper_commit,

+};

+int tidss_modeset_init(struct tidss_device *tidss) +{

• struct drm_device *ddev = tidss->ddev;
• int ret;
• int i;

i never takes negative values, you can make it an unsigned int.

• dev_dbg(tidss->dev, "%s\n", __func__);
• drm_mode_config_init(ddev);
• ddev->mode_config.min_width = 8;
• ddev->mode_config.min_height = 8;
• ddev->mode_config.max_width = 8096;
• ddev->mode_config.max_height = 8096;
• ddev->mode_config.funcs = &mode_config_funcs;
• ddev->mode_config.helper_private = &mode_config_helper_funcs;
• ret = tidss->dispc_ops->modeset_init(tidss->dispc);

I'm not too fond of hiding the DRM encoder and DRM bridge instantiation code in the dispc layer. Would there be a way to move it here ?

• if (ret)

• return ret;
  

• ret = drm_vblank_init(ddev, tidss->num_crtcs);
• if (ret)

• return ret;
  

• /* Start with vertical blanking interrupt reporting disabled. */
• for (i = 0; i < tidss->num_crtcs; ++i)

• drm_crtc_vblank_reset(tidss->crtcs[i]);
  

• drm_mode_config_reset(ddev);
• dev_dbg(tidss->dev, "%s done\n", __func__);
• return 0;

+} diff --git a/drivers/gpu/drm/tidss/tidss_kms.h b/drivers/gpu/drm/tidss/tidss_kms.h new file mode 100644 index 000000000000..927b57b05827 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_kms.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#ifndef __TIDSS_KMS_H__ +#define __TIDSS_KMS_H__

+struct tidss_device;

+int tidss_modeset_init(struct tidss_device *rcdu);

Now I wonder which driver you used as a model to develop this one ;-)

+#endif diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c new file mode 100644 index 000000000000..539c3ae706de --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_plane.c @@ -0,0 +1,186 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#include <drm/drmP.h> +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_fb_cma_helper.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_plane_helper.h>

+#include "tidss_crtc.h" +#include "tidss_drv.h" +#include "tidss_plane.h"

+static int tidss_plane_atomic_check(struct drm_plane *plane,

• 		    struct drm_plane_state *state)
  

+{

• struct drm_device *ddev = plane->dev;
• struct drm_crtc_state *crtc_state;
• int ret;
• dev_dbg(ddev->dev, "%s\n", __func__);
• if (!state->crtc) {

• /*
  

•  * The visible field is not reset by the DRM core but only
  

•  * updated by drm_plane_helper_check_state(), set it manually.
  

•  */
  

So it's at least two drivers suffering from this. Could we fix it in the DRM core ?

• state->visible = false;
  

• return 0;
  

• }
• crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
• if (IS_ERR(crtc_state))

• return PTR_ERR(crtc_state);
  

• /* XXX TODO: check scaling via dispc_ops */
• ret = drm_atomic_helper_check_plane_state(state, crtc_state,

• 				  0,
  

• 				  INT_MAX,
  

• 				  true, true);
  

• if (ret < 0)

• return ret;
  

• if (!state->visible)

• return 0;
  

This doesn't seem to be needed. You could then just return drm_atomic_helper_check_plane_state(...);.

• return 0;

+}

+static void tidss_plane_atomic_update(struct drm_plane *plane,

• 		      struct drm_plane_state *old_state)
  

+{

• struct drm_device *ddev = plane->dev;
• struct tidss_device *tidss = ddev->dev_private;
• struct tidss_plane *tplane = to_tidss_plane(plane);
• struct tidss_plane_info info;
• int ret;
• u32 hw_videoport;

How about moving these two down after the struct variables ?

• struct drm_plane_state *state = plane->state;
• struct drm_framebuffer *fb = state->fb;
• uint32_t x, y;

u32 ?

• struct drm_gem_cma_object *gem;
• dev_dbg(ddev->dev, "%s\n", __func__);
• if (!state->visible) {

• tidss->dispc_ops->plane_enable(tidss->dispc, tplane->hw_plane_id,
  

• 			       false);
  

• return;
  

• }

This too might be a candidate for handling in the DRM core.

• hw_videoport = to_tidss_crtc(state->crtc)->hw_videoport;
• memset(&info, 0, sizeof(info));

You can get rid of this by zeroing the variable when declaring it.

• info.fourcc = fb->format->format;
• info.pos_x = state->crtc_x;
• info.pos_y = state->crtc_y;
• info.out_width = state->crtc_w;
• info.out_height = state->crtc_h;
• info.width = state->src_w >> 16;
• info.height = state->src_h >> 16;
• info.zorder = state->zpos;
• x = state->src_x >> 16;
• y = state->src_y >> 16;
• gem = drm_fb_cma_get_gem_obj(fb, 0);
• info.paddr = gem->paddr + fb->offsets[0] + x * fb->format->cpp[0] +

•      y * fb->pitches[0];
  

• info.fb_width = fb->pitches[0] / fb->format->cpp[0];
• if (fb->format->num_planes == 2) {

• gem = drm_fb_cma_get_gem_obj(fb, 1);
  

• info.p_uv_addr = gem->paddr +
  

• 		 fb->offsets[0] +
  

Shouldn't this be offsets[1] ?

• 		 (x * fb->format->cpp[0] / fb->format->hsub) +
  

• 		 (y * fb->pitches[0] / fb->format->vsub);
  

And if you use cpp[1] and pitches[1], can't you get rid of the divisions by hsub and vsub ?

• }
• ret = tidss->dispc_ops->plane_setup(tidss->dispc, tplane->hw_plane_id,

• 			    &info, hw_videoport);
  

• if (ret) {

• dev_err(plane->dev->dev, "Failed to setup plane %d\n",
  

• 	tplane->hw_plane_id);
  

We should really make sure this never happens by validating parameters at atomic check time.

• tidss->dispc_ops->plane_enable(tidss->dispc, tplane->hw_plane_id,
  

• 			       false);
  

• return;
  

• }
• tidss->dispc_ops->plane_enable(tidss->dispc, tplane->hw_plane_id, true);

+}

+static void tidss_plane_atomic_disable(struct drm_plane *plane,

• 		       struct drm_plane_state *old_state)
  

+{

• struct drm_device *ddev = plane->dev;
• struct tidss_device *tidss = ddev->dev_private;
• struct tidss_plane *tplane = to_tidss_plane(plane);
• dev_dbg(ddev->dev, "%s\n", __func__);
• tidss->dispc_ops->plane_enable(tidss->dispc, tplane->hw_plane_id, false);

+}

+static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = {

• .atomic_check = tidss_plane_atomic_check,
• .atomic_update = tidss_plane_atomic_update,
• .atomic_disable = tidss_plane_atomic_disable,

+};

+static const struct drm_plane_funcs tidss_plane_funcs = {

• .update_plane = drm_atomic_helper_update_plane,
• .disable_plane = drm_atomic_helper_disable_plane,
• .reset = drm_atomic_helper_plane_reset,
• .destroy = drm_plane_cleanup,
• .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,

+};

+struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,

• 		       u32 hw_plane_id,	u32 plane_type,
  

• 		       u32 crtc_mask, const u32 *formats,
  

• 		       u32 num_formats)
  

+{

• enum drm_plane_type type;
• uint32_t possible_crtcs;
• uint num_planes = tidss->dispc_ops->get_num_planes(tidss->dispc);

unsigned int ?

• int ret;
• struct tidss_plane *tplane;
• tplane = devm_kzalloc(tidss->dev, sizeof(*tplane), GFP_KERNEL);
• if (!tplane)

• return ERR_PTR(-ENOMEM);
  

• tplane->hw_plane_id = hw_plane_id;
• possible_crtcs = crtc_mask;
• type = plane_type;

You can use crtc_mask and plane_type directly below, no need for local variables.

• ret = drm_universal_plane_init(tidss->ddev, &tplane->plane,

• 		       possible_crtcs,
  

• 		       &tidss_plane_funcs,
  

• 		       formats, num_formats,
  

• 		       NULL, type, NULL);
  

• if (ret < 0)

• return ERR_PTR(ret);
  

• drm_plane_helper_add(&tplane->plane, &tidss_plane_helper_funcs);
• if (num_planes > 1)

• drm_plane_create_zpos_property(&tplane->plane, hw_plane_id, 0,
  

• 			       num_planes - 1);
  

• return tplane;

+} diff --git a/drivers/gpu/drm/tidss/tidss_plane.h b/drivers/gpu/drm/tidss/tidss_plane.h new file mode 100644 index 000000000000..d865e14cb5f9 --- /dev/null +++ b/drivers/gpu/drm/tidss/tidss_plane.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
• • Author: Tomi Valkeinen tomi.valkeinen@ti.com
• */

+#ifndef __TIDSS_PLANE_H__ +#define __TIDSS_PLANE_H__

+#include "tidss_dispc.h"

You don't need to pull the whole dispc header in here, you can just include the header for drm_plane and forward-declare struct tidss_device.

+#define to_tidss_plane(p) container_of((p), struct tidss_plane, plane)

+struct tidss_plane {

• struct drm_plane plane;
• u32 hw_plane_id;

Extra blank line here.

+};

+struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,

• 		       u32 hw_plane_id,	u32 plane_type,
  

• 		       u32 crtc_mask, const u32 *formats,
  

• 		       u32 num_formats);
  

+#endif