Messages in this thread | | | Date | Fri, 01 Mar 2019 20:33:29 -0300 | From | Paul Cercueil <> | Subject | Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs |
| |
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
| |