Re: [PATCH 20/21] drm/msm: Add SDM845 DPU support

[Rob Clark <robdclark@gmail.com>]

On Tue, Jul 10, 2018 at 11:45 AM, Sean Paul <seanpaul@chromium.org> wrote:
> The patch was rejected for being too big. Please refer to
> https://gitlab.freedesktop.org/seanpaul/dpu-staging/commit/b4215cf040d1978287bd1403832ffc610659652b
>

heh, and also seems to be too big for gmail to reply to..

That said, +30k is a nice improvement over the +110k where this
started.  But a few comments..

+static void dpu_kms_preclose(struct msm_kms *kms, struct drm_file *file)
+{
+       struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+       struct drm_device *dev = dpu_kms->dev;
+       struct msm_drm_private *priv = dev->dev_private;
+       unsigned int i;
+
+       for (i = 0; i < priv->num_crtcs; i++)
+               dpu_crtc_cancel_pending_flip(priv->crtcs[i], file);
+}

So, not totally a fan of bringing back preclose() from the dead, esp.
since it is only supposed to be used by legacy drivers.  Since all
this does is fire off pending events, I think it could be dropped
entirely... drm_events_release() should unlink unsent events from the
drm_file, and if that isn't working properly, that seems like a bug
that should be fixed in drm core.

+static int _dpu_core_irq_enable(struct dpu_kms *dpu_kms, int irq_idx)
+{
+       unsigned long irq_flags;
+       int ret = 0, enable_count;
+
+       if (!dpu_kms || !dpu_kms->hw_intr ||
+                       !dpu_kms->irq_obj.enable_counts ||
+                       !dpu_kms->irq_obj.irq_counts) {
+               DPU_ERROR("invalid params\n");
+               return -EINVAL;
+       }
+
+       if (irq_idx < 0 || irq_idx >= dpu_kms->hw_intr->irq_idx_tbl_size) {
+               DPU_ERROR("invalid IRQ index: [%d]\n", irq_idx);
+               return -EINVAL;
+       }
...
+int dpu_core_irq_enable(struct dpu_kms *dpu_kms, int *irq_idxs, u32 irq_count)
+{
+       int i, ret = 0, counts;
+
+       if (!dpu_kms || !irq_idxs || !irq_count) {
+               DPU_ERROR("invalid params\n");
+               return -EINVAL;
+       }
...


there is a *whole* lot of multiple levels of null checks like this for
things that are only ever called internally (ie. not something that
can be triggered by parameters passed from userspace).. which is
really unnecessary.

(maybe something that someone who knows coccinelle better than I,
could automate cleaning up?)


+/**
+ * dpu_core_irq_disable_nolock - disable core interrupt given by the index
+ *                               without lock
+ * @dpu_kms:           Pointer to dpu kms context
+ * @irq_idx:           interrupt index
+ */
+int dpu_core_irq_disable_nolock(struct dpu_kms *dpu_kms, int irq_idx)
+{

appears to be unused?  dpu_core_irq_read_nolock() also seems unused,
and maybe some others.



diff --git a/include/uapi/media/msm_media_info.h
b/include/uapi/media/msm_media_info.h
new file mode 100644
index 000000000000..4f12e5c534c8
--- /dev/null
+++ b/include/uapi/media/msm_media_info.h
@@ -0,0 +1,1376 @@
+#ifndef __MEDIA_INFO_H__
+#define __MEDIA_INFO_H__
+
...


Not quite sure where this belongs, but (at least for now)
include/uapi/media seems very wrong.

Maybe just move into drm/msm for now?  We can always move it back to
include/uapi later (but the other direction isn't so cool)


------------------

Anyways, if I notice anything else to complain about, I'll send more
replies.  Overall it looks much better than where we started, and I'm
open to the idea of pulling it in to msm-next soon.  I think moving
msm_media_info.h out of uapi, and probably nuking preclose, should
happen first.


BR,
-R
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel