Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

Paul Cercueil
2019-03-01T23:33:29Z

Hi,

Le ven. 1 mars 2019 à 18:00, Sam Ravnborg <sam@ravnborg.org> a écrit :
> Hi Paul.
>
> Driver looks good and is a very nice piece of work.
> In the following a number of minor issues.
>
> One area that jumped at me was framedesc and the use of
> dma_alloc_coherent()
> I hope someone that knows the memory handling better can give some
> advice here.
> To me it looks like something drm should be able to help you with.
>
> Sam
>
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
>> obj-$(CONFIG_DRM_STM) += stm/
>> obj-$(CONFIG_DRM_STI) += sti/
>> obj-$(CONFIG_DRM_IMX) += imx/
>> +obj-y += ingenic/
>
> To avoid visiting ingenic/ dir for every build use:
> obj-$(CONFIG_DRM_INGENIC) += ingennic/
>
> And accept that you need to do this also in ingenic/Makefile

OK.

>> obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
>> obj-$(CONFIG_DRM_MESON) += meson/
>> obj-y += i2c/
>> +#include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_fb_cma_helper.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <drm/drm_fourcc.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_irq.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drm_plane.h>
>> +#include <drm/drm_plane_helper.h>
>> +#include <drm/drm_probe_helper.h>
>> +#include <drm/drm_vblank.h>
>> +
>> +#include <dt-bindings/display/ingenic,drm.h>
>> +
>> +#include "../drm_internal.h"
> No other drivers needs drm_internal.h - what makes this driver
> special?
> Or do we have something in drm_internal that should be moved to
> include/drm/ ?

I needed to include it for drm_vblank_cleanup().
But I guess I don't need to call that function? It's not obvious to me
how the error handling and driver removal should be done.

>> +struct ingenic_framedesc {
>> + uint32_t next;
>> + uint32_t addr;
>> + uint32_t id;
>> + uint32_t cmd;
>> +} __packed;
> For internel types u32 is the typical use.
> uint32_t is usually used in uapi.
>
> Consider to use dma_addr_t for addresses - we see this in other
> drivers.
> If there are alignemnt constraints then add these.

So 'ingenic_framedesc' represents a DMA descriptor, in the format that
the hardware expects it. The fields must be 32-bit. Is a dma_addr_t
the best option in that case?

>> +
>> +struct jz_soc_info {
>> + bool needs_dev_clk;
>> +};
>> +
>> +struct ingenic_drm {
>> + struct device *dev;
>> + void __iomem *base;
>> + struct regmap *map;
>> + struct clk *lcd_clk, *pix_clk;
>> +
>> + u32 lcd_mode;
>> +
>> + struct ingenic_framedesc *framedesc;
>> + dma_addr_t framedesc_phys;
>> +
>> + struct drm_device *drm;
>> + struct drm_plane primary;
>> + struct drm_crtc crtc;
>> + struct drm_connector connector;
>> + struct drm_encoder encoder;
>> + struct drm_panel *panel;
>> +
>> + struct device_node *panel_node;
> panel_node is not used outside ingenic_drm_probe() so no need
> to have it here.

Noted.

>
>> +
>> + unsigned short ps_start, ps_end, cls_start,
>> + cls_end, spl_start, spl_end, rev_start;
> I do not see these used, they can all be dropped.
>
>> +};
>> +
>> +
>> +static const struct regmap_config ingenic_drm_regmap_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> +
>> + .max_register = JZ_REG_LCD_CMD1,
>> + .writeable_reg = ingenic_drm_writeable_reg,
>> +};
>
> +1 for using regmap.
>
>> +
>> +static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
>> +{
>> + switch (mode) {
>> + case JZ_DRM_LCD_SPECIAL_TFT_1:
>> + case JZ_DRM_LCD_SPECIAL_TFT_2:
>> + case JZ_DRM_LCD_SPECIAL_TFT_3:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
> Does it make sense to support these modes today?
> If it is not used in practice then no need to carry it in the driver.

Yes, I have a board that uses such a panel.

>> +
>> +static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
>> +{
>> + switch (mode) {
>> + case JZ_DRM_LCD_SINGLE_COLOR_STN:
>> + case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
>> + case JZ_DRM_LCD_DUAL_COLOR_STN:
>> + case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
> This function is not used and can be deleted.
> stn display are not really worth it these days anyway.
>
>
>> +static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>> + struct drm_crtc_state *oldstate)
>> +{
>> + struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>> + struct drm_crtc_state *state = crtc->state;
>> + struct drm_pending_vblank_event *event = state->event;
>> + struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
>> + const struct drm_format_info *finfo;
>> + unsigned int width, height;
>> +
>> + if (drm_atomic_crtc_needs_modeset(state)) {
>> + finfo = drm_format_info(drm_fb->format->format);
>> + width = state->adjusted_mode.hdisplay;
>> + height = state->adjusted_mode.vdisplay;
>
> width and height are unused and can be dropped.
>
>> +
>> + ingenic_drm_crtc_update_timings(priv, &state->mode);
>> +
>> + ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
>> + ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
>> +
>> + clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
>> +
>> + regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
>> + }
>> +
>> + if (event) {
>> + state->event = NULL;
>> +
>> + spin_lock_irq(&crtc->dev->event_lock);
>> + if (drm_crtc_vblank_get(crtc) == 0)
>> + drm_crtc_arm_vblank_event(crtc, event);
>> + else
>> + drm_crtc_send_vblank_event(crtc, event);
>> + spin_unlock_irq(&crtc->dev->event_lock);
>> + }
>> +}
>> +
>> +static void ingenic_drm_plane_atomic_update(struct drm_plane
>> *plane,
>> + struct drm_plane_state *oldstate)
>> +{
>> + struct ingenic_drm *priv = drm_plane_get_priv(plane);
>> + struct drm_plane_state *state = plane->state;
>> + const struct drm_format_info *finfo;
>> + unsigned int width, height;
>> +
>> + finfo = drm_format_info(state->fb->format->format);
>> + width = state->crtc->state->adjusted_mode.hdisplay;
>> + height = state->crtc->state->adjusted_mode.vdisplay;
> adjusted_mode->{h,v}display are both ints. So there is a hidden
> conversion to unsigned int here.
> and framedesc->cmd is uint32_t then this is maybe fine.
> Noticed so added a comment about it.
>
>> +
>> + priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state,
>> 0);
>> +
>> + priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8)
>> / 4;
>> + priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
>> +}
>> +
>> +static struct drm_driver ingenic_drm_driver_data = {
>> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
>> + | DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
>
> DRIVER_HAVE_IRQ are only for legacy drivers these days.
> You can drop it.
>
>> + .name = "ingenic-drm",
>> + .desc = "DRM module for Ingenic SoCs",
>> + .date = "20190228",
>> + .major = 1,
>> + .minor = 0,
>> + .patchlevel = 0,
>> +
>> + .fops = &ingenic_drm_fops,
>> +
>> + .dumb_create = drm_gem_cma_dumb_create,
>> + .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,
> The two assignments above are not needed. This is the default
> behaviour. See drm_drv.h
> (from drm-misc-next)
>
>> +static int ingenic_drm_probe(struct platform_device *pdev)
>> +{
>> + const struct jz_soc_info *soc_info;
>> + struct device *dev = &pdev->dev;
>> + struct ingenic_drm *priv;
>> + struct clk *parent_clk;
>> + struct drm_device *drm;
>> + struct resource *mem;
>> + void __iomem *base;
>> + long parent_rate;
>> + int ret, irq;
>> +
>> + soc_info = device_get_match_data(dev);
>> + if (!soc_info)
>> + return -EINVAL;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->dev = dev;
>> +
>> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + priv->base = base = devm_ioremap_resource(dev, mem);
>> + if (IS_ERR(base))
>> + return PTR_ERR(base);
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + dev_err(dev, "Failed to get platform irq\n");
>> + return -ENOENT;
>> + }
>> +
>> + priv->map = devm_regmap_init_mmio(dev, base,
>> + &ingenic_drm_regmap_config);
>> + if (IS_ERR(priv->map)) {
>> + dev_err(dev, "Failed to create regmap\n");
>> + return PTR_ERR(priv->map);
>> + }
>> +
>> + if (soc_info->needs_dev_clk) {
>> + priv->lcd_clk = devm_clk_get(dev, "lcd");
>> + if (IS_ERR(priv->lcd_clk)) {
>> + dev_err(dev, "Failed to get lcd clock\n");
>> + return PTR_ERR(priv->lcd_clk);
>> + }
>> + }
>> +
>> + priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
>> + if (IS_ERR(priv->pix_clk)) {
>> + dev_err(dev, "Failed to get pixel clock\n");
>> + return PTR_ERR(priv->pix_clk);
>> + }
>> +
>> + priv->panel_node = of_parse_phandle(dev->of_node,
>> "ingenic,panel", 0);
>> + if (!priv->panel_node) {
>> + DRM_INFO("No panel found");
>> + } else {
>> + priv->panel = of_drm_find_panel(priv->panel_node);
>> + of_node_put(priv->panel_node);
>> +
>> + if (IS_ERR(priv->panel)) {
>> + if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> +
>> + priv->panel = NULL;
>> + } else {
>> + ret = drm_panel_prepare(priv->panel);
>> + if (ret && ret != -ENOSYS)
>> + return ret;
>> + }
>> + }
>> +
>> + if (priv->panel) {
>> + ret = device_property_read_u32(dev, "ingenic,lcd-mode",
>> + &priv->lcd_mode);
>> + if (ret) {
>> + dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
>> + return ret;
>> + }
>> + }
>> +
>> + priv->framedesc = dma_alloc_coherent(dev,
>> sizeof(*priv->framedesc),
>> + &priv->framedesc_phys, GFP_KERNEL);
>> + if (!priv->framedesc)
>> + return -ENOMEM;
>> +
>> + priv->framedesc->next = priv->framedesc_phys;
>> + priv->framedesc->id = 0xdeafbead;
> I did not really follow this code.
> But you have framedesc that include a next pointer which is uint32_t,
> and you assign to it framedesc_phys which is dmaaddr_t.
> This looks fishy to me, but maybe it is fine.

As explained above, the framedesc is the hardware DMA descriptor, its
fields
must be 32-bit, that's why it's done this way.

> When browsing other drivers I see uses of for example
> {dma,dmam}_pool_create()
>
> When lookign at the origianl fbdev code it is obvious where this
> comes from.
> Notice that in the fbdev code JZ_REG_LCD_DA0 register is updated in
> the enable function.
> This is done different in this driver.
> Just an observation, what is right I dunno.

JZ_REG_LCD_DA0 should contain the address of the DMA descriptor. In
this driver,
I create a single DMA descriptor that commands the transfer of a full
frame.
It only has to be set once, so it could even be written in the probe
function,
provided the clocks are enabled.

>> +static int ingenic_drm_remove(struct platform_device *pdev)
>> +{
>> + struct drm_device *drm = platform_get_drvdata(pdev);
>> + struct ingenic_drm *priv = drm->dev_private;
>> +
>> + drm_dev_unregister(drm);
>> + drm_mode_config_cleanup(drm);
>> +
>> + if (priv->lcd_clk)
>> + clk_disable_unprepare(priv->lcd_clk);
>> + clk_disable_unprepare(priv->pix_clk);
>> +
>> + drm_vblank_cleanup(drm);
>> + drm_irq_uninstall(drm);
>> +
>> + drm_encoder_cleanup(&priv->encoder);
>> + drm_connector_cleanup(&priv->connector);
>> + drm_crtc_cleanup(&priv->crtc);
>> + drm_plane_cleanup(&priv->primary);
>> +
>> + drm_dev_put(drm);
>> +
>> + dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
>> + priv->framedesc, priv->framedesc_phys);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct jz_soc_info jz4740_soc_info = {
>> + .needs_dev_clk = true,
>> +};
>> +
>> +static const struct jz_soc_info jz4725b_soc_info = {
>> + .needs_dev_clk = false,
>> +};
>> +
>> +static const struct of_device_id ingenic_drm_of_match[] = {
>> + { .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
>> + { .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info
>> },
>> + {},
> Use /* sentinel */ like in most drivers.

For this and other embedded comments I didn't directly reply: duly
noted.

Thanks for the review.

Greetings,
-Paul