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@arm.com 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