From patchwork Mon Jul 16 14:03:58 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rob Clark X-Patchwork-Id: 10526853 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 9BB906020A for ; Mon, 16 Jul 2018 14:04:02 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 904CF28BB6 for ; Mon, 16 Jul 2018 14:04:02 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8E05828BCA; Mon, 16 Jul 2018 14:04:02 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, FREEMAIL_FROM, MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 37CE228BC0 for ; Mon, 16 Jul 2018 14:04:02 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 46D276E4AE; Mon, 16 Jul 2018 14:04:01 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-io0-x244.google.com (mail-io0-x244.google.com [IPv6:2607:f8b0:4001:c06::244]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0E2E76E4AE for ; Mon, 16 Jul 2018 14:04:00 +0000 (UTC) Received: by mail-io0-x244.google.com with SMTP id z20-v6so37902224iol.0 for ; Mon, 16 Jul 2018 07:04:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=z2x/PdIZK3oPjRSBWAm6y5l+M/dxBT0NHv7IXVDN4oA=; b=gexuN8bgRdlvORvnTHrNFKmCbHG+AKmfVBlSrhPwAWvMcZBwFtYIJwY3wqQki3KLkE g8bWW23nS6lbz+hBCLGzzqd1mc4LV4apfuco7sqcy279xW+BUa8u+J2L/7nzsYiv6TUx LBUq5JQDtiTORoGTskXpInQUbSnDgIAiw7/gAd+W+0kejsUUXNjhsBua8vK//VkFXcp7 J4M+NqB5QZcD0ZvRCZB/sKyPOY441HE7u8iMkw59sqlU9CkqqEvLIv5PymAwlmd297Mh EfyUFr7L0O2/C7D5dt3oW78616L1qC/ohK7Y3WK0wGyxIqQIaxwrfVumNfYTYozehNEe 2Wiw== X-Gm-Message-State: APt69E0dQ0axuaq9aoyyAMqfi+XfLJk8DX27ENNZg1t4Vbdng7mDl4UT ihsppJ4nmQVZmtSwzPaHp106sWOFXLcCwmPcm/A= X-Google-Smtp-Source: AAOMgpdqmJiBriud7ZXQNNg05dcN1HBYHLk2jiVmmkLLNPrAOqhsI12pD3ITnjF8DXxgBZp1Q8s9Bs1U9sLFDhZD/Gs= X-Received: by 2002:a6b:3954:: with SMTP id g81-v6mr33335910ioa.225.1531749839270; Mon, 16 Jul 2018 07:03:59 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ad5:40cb:0:0:0:0:0 with HTTP; Mon, 16 Jul 2018 07:03:58 -0700 (PDT) In-Reply-To: <20180710154505.GT20303@art_vandelay> References: <20180709173200.238457-1-seanpaul@chromium.org> <20180710154505.GT20303@art_vandelay> From: Rob Clark Date: Mon, 16 Jul 2018 10:03:58 -0400 Message-ID: Subject: Re: [PATCH 20/21] drm/msm: Add SDM845 DPU support To: Sean Paul X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: dri-devel Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jul 10, 2018 at 11:45 AM, Sean Paul 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