Re: [PATCH v2 2/3] drm/arm: Add support for Mali Display Processors

[Emil Velikov]

Hi Liviu,

Humble request: For the future please split things into manageable
hunks. I doubt you wrote the whole thing in one go, right ?

At the very minimum you could have introduced DP500 support initially
and then DP550 and DP650 as follow up commits.

On 25 April 2016 at 15:19, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:

> --- /dev/null
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -0,0 +1,259 @@

> +static void malidp_crtc_enable(struct drm_crtc *crtc)
> +{
> +       struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
> +       struct malidp_hw_device *hwdev = malidp->dev;
> +       struct videomode vm;
> +
> +       drm_display_mode_to_videomode(&crtc->state->adjusted_mode, &vm);
> +
> +       clk_prepare_enable(hwdev->pxlclk);
> +
> +       /* mclk needs to be set to the same or higher rate than pxlclk */
> +       clk_set_rate(hwdev->mclk, crtc->state->adjusted_mode.crtc_clock * 1000);
> +       clk_set_rate(hwdev->pxlclk, crtc->state->adjusted_mode.crtc_clock * 1000);
> +
> +       hwdev->modeset(hwdev, &vm);
> +       hwdev->leave_config_mode(hwdev);
> +}
> +
> +static void malidp_crtc_disable(struct drm_crtc *crtc)
> +{
> +       struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
> +       struct malidp_hw_device *hwdev = malidp->dev;
> +
> +       /*
> +        * avoid disabling already disabled clocks and hardware
> +        * (as is the case at device probe time)
> +        */
Ideally one should lower the pxlclk, correct ?

> +       if (crtc->state->active) {
> +               hwdev->enter_config_mode(hwdev);
> +               clk_disable_unprepare(hwdev->pxlclk);
> +       }
> +}
> +


> +int malidp_crtc_init(struct drm_device *drm)
> +{
> +       struct malidp_drm *malidp = drm->dev_private;
> +       struct drm_plane *primary = NULL, *plane;
> +       int ret;
> +
You want malidp_de_planes_init() in here.

> +       drm_for_each_plane(plane, drm) {
> +               if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> +                       primary = plane;
> +                       break;
> +               }
> +       }
> +
> +       if (!primary) {
> +               DRM_ERROR("no primary plane found\n");
... and _destroy() here.


> --- /dev/null
> +++ b/drivers/gpu/drm/arm/malidp_drv.c

> +static int malidp_init(struct drm_device *drm)
> +{
> +       int ret;
> +       struct malidp_drm *malidp = drm->dev_private;
> +       struct malidp_hw_device *hwdev = malidp->dev;
> +
> +       drm_mode_config_init(drm);
> +
> +       drm->mode_config.min_width = hwdev->min_line_size;
> +       drm->mode_config.min_height = hwdev->min_line_size;
> +       drm->mode_config.max_width = hwdev->max_line_size;
> +       drm->mode_config.max_height = hwdev->max_line_size;
> +       drm->mode_config.funcs = &malidp_mode_config_funcs;
> +
> +       ret = malidp_de_planes_init(drm);
Move this in malidp_crtc_init() ...

> +       if (ret < 0) {
> +               DRM_ERROR("Failed to initialise planes\n");
> +               goto plane_init_fail;
> +       }
> +
> +       ret = malidp_crtc_init(drm);
> +       if (ret) {
> +               DRM_ERROR("Failed to initialise CRTC\n");
> +               goto crtc_init_fail;
> +       }
> +
> +       return 0;
> +
> +crtc_init_fail:
> +       malidp_de_planes_destroy(drm);
... and drop this ?

> +plane_init_fail:
Nitpick: there is/was the idea that labels should be called after what
they do, rather than what fails. Personally I'm in favour of it as it
makes things clearer... sadly I might be one of the few.

> +       drm_mode_config_cleanup(drm);
> +
> +       return ret;
> +}
> +
Add a malidp_fini() helper to complement the above ?

> +static int malidp_irq_init(struct platform_device *pdev)

Ditto - add malidp_irq_fini() to complement the _init() function ?


> +static int malidp_bind(struct device *dev)
> +{

> +       /*
> +        * copy the associated data from malidp_drm_of_match to avoid
> +        * having to keep a reference to the OF node after binding
> +        */
This feels a bit strange. Is keeping a reference that bad ?



> --- /dev/null
> +++ b/drivers/gpu/drm/arm/malidp_hw.c

> +#define MALIDP_COMMON_FORMATS \
> +       /*    layers supporting the format,      internal id,      fourcc */ \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 0), DRM_FORMAT_ARGB2101010 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 1), DRM_FORMAT_ABGR2101010 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 2), DRM_FORMAT_RGBA1010102 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 3), DRM_FORMAT_BGRA1010102 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 0), DRM_FORMAT_ARGB8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 1), DRM_FORMAT_ABGR8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 2), DRM_FORMAT_RGBA8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 3), DRM_FORMAT_BGRA8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 0), DRM_FORMAT_XRGB8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 1), DRM_FORMAT_XBGR8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 2), DRM_FORMAT_RGBX8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 3), DRM_FORMAT_BGRX8888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 0), DRM_FORMAT_RGB888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 1), DRM_FORMAT_BGR888 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 0), DRM_FORMAT_RGBA5551 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 1), DRM_FORMAT_ABGR1555 }, \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 2), DRM_FORMAT_RGB565 },   \
> +       { DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 3), DRM_FORMAT_BGR565 },   \
> +       { DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 2), DRM_FORMAT_YUYV }, \
> +       { DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 3), DRM_FORMAT_UYVY }, \
> +       { DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 6), DRM_FORMAT_NV12 }, \
> +       { DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7), DRM_FORMAT_YUV420 }
> +
> +static const struct malidp_input_format malidp550_de_formats[] = {
> +       MALIDP_COMMON_FORMATS,
> +};
> +
> +static const struct malidp_input_format malidp650_de_formats[] = {
> +       MALIDP_COMMON_FORMATS,
> +};
> +
Pretty sure that using the define here will lead to the exact same
code existing twice in the driver. Just kill off malidp650_de_formats
and use malidp550_de_formats instead ?


> +void malidp500_enter_config_mode(struct malidp_hw_device *hwdev)
Many/most of the functions in this file should be static.

> +{
> +       u32 status, count = 100;
> +
Any particular reason behind the asymmetric (vs the leave_config_mode)
'count' ? Worth adding a comment ?

> +       malidp_hw_setbits(hwdev, MALIDP500_DC_CONFIG_REQ, MALIDP500_DC_CONTROL);
> +       while (count) {
> +               status = malidp_hw_read(hwdev, hwdev->map.dc_base + MALIDP_REG_STATUS);
> +               if ((status & MALIDP500_DC_CONFIG_REQ) == MALIDP500_DC_CONFIG_REQ)
> +                       break;
> +               /*
> +                * entering config mode can take as long as the rendering
> +                * of a full frame, hence the long sleep here
> +                */
> +               usleep_range(1000, 10000);
> +               count--;
> +       }
> +       WARN(count == 0, "timeout while entering config mode");
> +}
> +
> +void malidp500_leave_config_mode(struct malidp_hw_device *hwdev)
> +{
> +       u32 status, count = 30;
> +
...


> +u8 malidp_hw_get_format_id(const struct malidp_hw_regmap *map,
> +                          u8 layer_id, u32 format)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < map->n_input_formats; i++) {
> +               if (((map->input_formats[i].layer & layer_id) == layer_id) &&
> +                   (map->input_formats[i].format == format))
> +                       return map->input_formats[i].id;
> +       }
> +
> +       return (u8)-1;
This feels very strange to read. Use 0xff (here and below) instead ?

> +}
> +
> +
> +u32 malidp_hw_read(struct malidp_hw_device *hwdev, u32 reg)
> +{
> +       u32 value = readl(hwdev->regs + reg);
> +       return value;
> +}
> +
> +void malidp_hw_write(struct malidp_hw_device *hwdev, u32 value, u32 reg)
> +{
> +       writel(value, hwdev->regs + reg);
> +}
> +
> +void malidp_hw_setbits(struct malidp_hw_device *hwdev, u32 mask, u32 reg)
> +{
> +       u32 data = malidp_hw_read(hwdev, reg);
> +
> +       data |= mask;
> +       malidp_hw_write(hwdev, data, reg);
> +}
> +
> +void malidp_hw_clearbits(struct malidp_hw_device *hwdev, u32 mask, u32 reg)
> +{
> +       u32 data = malidp_hw_read(hwdev, reg);
> +
> +       data &= ~mask;
> +       malidp_hw_write(hwdev, data, reg);
> +}
> +
Just declare the above 4 as static inline ? Or the read/write ones at least.


> +void malidp_hw_clear_irq(struct malidp_hw_device *hwdev, u8 block, u32 irq)
> +{
> +       u32 base = 0;
> +
> +       switch (block) {
> +       case MALIDP_DE_BLOCK:
> +               base = 0;
> +               break;
> +       case MALIDP_SE_BLOCK:
> +               base = hwdev->map.se_base;
> +               break;
> +       case MALIDP_DC_BLOCK:
> +               base = hwdev->map.dc_base;
> +               break;
> +       }
> +
Move the above switch into a helper function, instead of having three
copies of it ?


> +int malidp_de_irq_init(struct drm_device *drm, int irq)
> +{
> +       struct malidp_drm *malidp = drm->dev_private;
> +       struct malidp_hw_device *hwdev = malidp->dev;
> +       int ret;
> +
> +       /* ensure interrupts are disabled */
> +       malidp_hw_disable_irq(hwdev, MALIDP_DC_BLOCK, 0xffffffff);
> +       malidp_hw_clear_irq(hwdev, MALIDP_DC_BLOCK, 0xffffffff);
> +       malidp_hw_disable_irq(hwdev, MALIDP_DE_BLOCK, 0xffffffff);
> +       malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, 0xffffffff);
> +
Shouldn't we disable/clear DE before DC ? This way It aligns with
_cleanup and is the inverse of enable a couple of lines below.

> +       ret = devm_request_threaded_irq(drm->dev, irq, malidp_de_irq,
> +                                       malidp_de_irq_thread_handler,
> +                                       IRQF_SHARED, "malidp-de", drm);
> +       if (ret < 0) {
> +               DRM_ERROR("failed to install DE IRQ handler\n");
> +               return ret;
> +       }
> +
> +       /* first enable the DC block IRQs */
> +       malidp_hw_enable_irq(hwdev, MALIDP_DC_BLOCK,
> +                            hwdev->map.dc_irq_map.irq_mask);
> +
> +       /* now enable the DE block IRQs */
> +       malidp_hw_enable_irq(hwdev, MALIDP_DE_BLOCK,
> +                            hwdev->map.de_irq_map.irq_mask);
> +
> +       return 0;
> +}


> --- /dev/null
> +++ b/drivers/gpu/drm/arm/malidp_hw.h
> @@ -0,0 +1,189 @@

> +#include <drm/drm_fourcc.h>
Afaict the header is not used here. Please include it only where needed.


> +struct malidp_hw_device {
> +       const struct malidp_hw_regmap map;
> +       void __iomem *regs;
> +
> +       /* APB clock */
> +       struct clk *pclk;
> +       /* AXI clock */
> +       struct clk *aclk;
> +       /* main clock for display core */
> +       struct clk *mclk;
> +       /* pixel clock for display core */
> +       struct clk *pxlclk;
> +
> +       /*
> +        * Validate the driver instance against the hardware bits
> +        */
> +       int (*query_hw)(struct malidp_hw_device *hwdev);
> +
> +       /*
> +        * Set the hardware into config mode, ready to accept mode changes
> +        */
> +       void (*enter_config_mode)(struct malidp_hw_device *hwdev);
> +
> +       /*
> +        * Tell hardware to exit configuration mode
> +        */
> +       void (*leave_config_mode)(struct malidp_hw_device *hwdev);
> +
> +       /*
> +        * Query if hardware is in configuration mode
> +        */
> +       bool (*in_config_mode)(struct malidp_hw_device *hwdev);
> +
> +       /*
> +        * Set configuration valid flag for hardware parameters that can
> +        * be changed outside the configuration mode. Hardware will use
> +        * the new settings when config valid is set after the end of the
> +        * current buffer scanout
> +        */
> +       void (*set_config_valid)(struct malidp_hw_device *hwdev);
> +
> +       /*
> +        * Set a new mode in hardware. Requires the hardware to be in
> +        * configuration mode before this function is called.
> +        */
> +       void (*modeset)(struct malidp_hw_device *hwdev, struct videomode *m);
> +
> +       /*
> +        * Calculate the required rotation memory given the active area
> +        * and the buffer format.
> +        */
> +       int (*rotmem_required)(struct malidp_hw_device *hwdev, u16 w, u16 h, u32 fmt);
> +
Nitpick: We normally create use "const struct foo *funcs" for vfuncs.

Many of the structs in this file have holes in them. Worth checking
with pahole and reordering ?

> +/*
> + * background color components are defined as 12bits values,
> + * they will be shifted right when stored on hardware that
> + * supports only 8bits per channel
> + */
> +#define MALIDP_BGND_COLOR_R            0x000
> +#define MALIDP_BGND_COLOR_G            0x000
> +#define MALIDP_BGND_COLOR_B            0x000
> +
Something feels very wrong here. Are you sure what all three are zero ?


> --- /dev/null
> +++ b/drivers/gpu/drm/arm/malidp_planes.c

> +static int malidp_de_atomic_update_plane(struct drm_plane *plane,
> +                                        struct drm_crtc *crtc,
> +                                        struct drm_framebuffer *fb,
> +                                        int crtc_x, int crtc_y,
> +                                        unsigned int crtc_w,
> +                                        unsigned int crtc_h,
> +                                        uint32_t src_x, uint32_t src_y,
> +                                        uint32_t src_w, uint32_t src_h)
> +{
> +       return drm_atomic_helper_update_plane(plane, crtc, fb, crtc_x, crtc_y,
> +                                             crtc_w, crtc_h, src_x, src_y,
> +                                             src_w, src_h);
Just drop the wrapper and reference the helper directly into
drm_plane_funcs below ?


> +static void malidp_de_plane_update(struct drm_plane *plane,
> +                                  struct drm_plane_state *old_state)
> +{
> +       struct drm_gem_cma_object *obj;
> +       struct malidp_plane *mp;
> +       const struct malidp_hw_regmap *map;
> +       u8 format_id;
> +       u16 ptr;
> +       u32 format, src_w, src_h, dest_w, dest_h, val = 0;
> +       int num_planes, i;
> +
> +       mp = to_malidp_plane(plane);
> +
> +#ifdef MALIDP_ENABLE_BGND_COLOR_AS_PRIMARY_PLANE
> +       /* skip the primary plane, it is using the background color */
> +       if (!mp->layer || !mp->layer->id)
> +               return;
> +#endif
> +
Afaict the above define is not set anywhere - neither explicitly
(#define foo, -DFOO) nor implicitly (via kconfig toggle). As such it
should go. Same goes for the other instances of it.


> +       format_id = malidp_hw_get_format_id(map, mp->layer->id, format);
> +       if (format_id == (u8)-1)
> +               return;
> +
We should move this to from _update to _check.


> +int malidp_de_planes_init(struct drm_device *drm)
> +{
...
> +       for (i = 0; i < map->n_layers; i++) {
> +               u8 id = map->layers[i].id;
> +
> +               plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
Either use the non-devm function here and in _destroy or drop the one
in _destroy ? Afaict we could get a double-free with the latter in
place. Personally, I'd drop the devm_.


The best thing is that I did not spot even one use of deprecated
functions. Nicely done gents :-)

Regards,
Emil