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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
