From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D0092C433EF for ; Tue, 12 Oct 2021 08:47:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B7B3261056 for ; Tue, 12 Oct 2021 08:47:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235253AbhJLIt4 (ORCPT ); Tue, 12 Oct 2021 04:49:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37632 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235458AbhJLIty (ORCPT ); Tue, 12 Oct 2021 04:49:54 -0400 Received: from mail-wr1-x435.google.com (mail-wr1-x435.google.com [IPv6:2a00:1450:4864:20::435]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4C41FC061570 for ; Tue, 12 Oct 2021 01:47:53 -0700 (PDT) Received: by mail-wr1-x435.google.com with SMTP id r10so64606374wra.12 for ; Tue, 12 Oct 2021 01:47:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=subject:to:cc:references:from:organization:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=zbX6hKlpjnEluaYu/lsbeCfslnUmI3YRbwFY1Jibvtg=; b=EuVHU0mSbR9/OGy5y8viyb3XVqJXl37DhVIe8dX07Z5jQ8Nje2QoKiiFdY6e9PbxLQ dRQDFVnVH0XM1VGpFIuApB4RIfhxIjTIis/Lfggji80pHzTtdvkQCeQ74wIRtZs8Sxg3 bili+uILmV15CCNlbBtPiOPCXeWFK2Q+6ou5yhXQNOhZTdV2qFbZe5UT9eqVi3KHP01k nu1KE0bA5TKLN63b4gle52C/ZYOAjbAQZkdln/9/24Ebg244702t1wIAZBDCxPsG41mT j6+hSRq6XjdOQZuvWkiycFol+sI0GPDzH9KTu+Wi4V0PxqRQlNXCY2oAo5o5UnHDNgZS BpRw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=zbX6hKlpjnEluaYu/lsbeCfslnUmI3YRbwFY1Jibvtg=; b=IRb38b+XMFVE3fa7/7fjMmDwc01S/r1VBDMom/ngHuWPN05z9SNOaMBM6V/3tTy/DN x/Fl7v28JvuHn3HiHcBuOEHwhc9bWSfPDmf0cVcHjTN7Nkxq8CnZVhw2tVPAsIjPUoZd SUm64ozFp7PTJG7l4YWDTVrWTC7xmpaNWtA8e1LkiceaJvGmv3fBX1+rr0f9CPvEpphc zZx4XAzCp3LbRD212Zdcj+QHHpc9DB/ojqmGeBW3PEDcJYYPthcs6GVvt3M7avi5pQP3 E6BXEPYGhPADPuYX7JveY3iX7tUFjKnGZMIJNiYaKY6eKuNxfPQyIc5yxR1f2ruy7lHT vziQ== X-Gm-Message-State: AOAM530kBYMwhHCFXaVCEsRHy/CPqUXERjWws33uT96rguT+MM2auZ3F ZsgCADLEi0cF3Cn0aIsu0Kc+/w== X-Google-Smtp-Source: ABdhPJxOmIo0WWMlshzCTeHU91KlphCPToG02sGDwYhflW9nkfXLZ9f4EUlHyMMolREVzQaYIiFgbQ== X-Received: by 2002:a5d:6c65:: with SMTP id r5mr30137331wrz.26.1634028470054; Tue, 12 Oct 2021 01:47:50 -0700 (PDT) Received: from ?IPv6:2001:861:44c0:66c0:4e93:9fa7:4d66:4f5c? ([2001:861:44c0:66c0:4e93:9fa7:4d66:4f5c]) by smtp.gmail.com with ESMTPSA id p25sm1815128wma.2.2021.10.12.01.47.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 12 Oct 2021 01:47:49 -0700 (PDT) Subject: Re: [PATCH v5 3/8] drm/omap: introduce omap_hw_overlay To: Tomi Valkeinen Cc: linux-omap@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, khilman@baylibre.com, Benoit Parrot References: <20210923070701.145377-1-narmstrong@baylibre.com> <20210923070701.145377-4-narmstrong@baylibre.com> <6e5980a6-218e-b585-3ad7-3e55350929b6@ideasonboard.com> From: Neil Armstrong Organization: Baylibre Message-ID: <654cae73-fe44-022a-ec81-e04979be6f76@baylibre.com> Date: Tue, 12 Oct 2021 10:47:49 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <6e5980a6-218e-b585-3ad7-3e55350929b6@ideasonboard.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org On 12/10/2021 09:59, Tomi Valkeinen wrote: > On 23/09/2021 10:06, Neil Armstrong wrote: >> From: Benoit Parrot >> >> Split out the hardware overlay specifics from omap_plane. >> To start, the hw overlays are statically assigned to planes. >> >> The goal is to eventually assign hw overlays dynamically to planes >> during plane->atomic_check() based on requested caps (scaling, YUV, >> etc). And then perform hw overlay re-assignment if required. >> >> Signed-off-by: Benoit Parrot >> Signed-off-by: Neil Armstrong >> --- >>   drivers/gpu/drm/omapdrm/Makefile       |  1 + >>   drivers/gpu/drm/omapdrm/omap_drv.c     |  9 ++- >>   drivers/gpu/drm/omapdrm/omap_drv.h     |  4 ++ >>   drivers/gpu/drm/omapdrm/omap_overlay.c | 87 ++++++++++++++++++++++++++ >>   drivers/gpu/drm/omapdrm/omap_overlay.h | 31 +++++++++ >>   drivers/gpu/drm/omapdrm/omap_plane.c   | 42 ++++++------- >>   6 files changed, 151 insertions(+), 23 deletions(-) >>   create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.c >>   create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.h >> >> diff --git a/drivers/gpu/drm/omapdrm/Makefile b/drivers/gpu/drm/omapdrm/Makefile >> index 21e8277ff88f..710b4e0abcf0 100644 >> --- a/drivers/gpu/drm/omapdrm/Makefile >> +++ b/drivers/gpu/drm/omapdrm/Makefile >> @@ -9,6 +9,7 @@ omapdrm-y := omap_drv.o \ >>       omap_debugfs.o \ >>       omap_crtc.o \ >>       omap_plane.o \ >> +    omap_overlay.o \ >>       omap_encoder.o \ >>       omap_fb.o \ >>       omap_gem.o \ >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c >> index f86e20578143..b994014b22e8 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c >> @@ -583,10 +583,14 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) >>         omap_gem_init(ddev); >>   +    ret = omap_hwoverlays_init(priv); >> +    if (ret) >> +        goto err_gem_deinit; >> + >>       ret = omap_modeset_init(ddev); >>       if (ret) { >>           dev_err(priv->dev, "omap_modeset_init failed: ret=%d\n", ret); >> -        goto err_gem_deinit; >> +        goto err_free_overlays; >>       } >>         /* Initialize vblank handling, start with all CRTCs disabled. */ >> @@ -618,6 +622,8 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) >>       omap_fbdev_fini(ddev); >>   err_cleanup_modeset: >>       omap_modeset_fini(ddev); >> +err_free_overlays: >> +    omap_hwoverlays_destroy(priv); >>   err_gem_deinit: >>       omap_gem_deinit(ddev); >>       destroy_workqueue(priv->wq); >> @@ -642,6 +648,7 @@ static void omapdrm_cleanup(struct omap_drm_private *priv) >>       drm_atomic_helper_shutdown(ddev); >>         omap_modeset_fini(ddev); >> +    omap_hwoverlays_destroy(priv); >>       omap_gem_deinit(ddev); >>         destroy_workqueue(priv->wq); >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h >> index 591d4c273f02..b4d9c2062723 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_drv.h >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h >> @@ -24,6 +24,7 @@ >>   #include "omap_gem.h" >>   #include "omap_irq.h" >>   #include "omap_plane.h" >> +#include "omap_overlay.h" >>     #define DBG(fmt, ...) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__) >>   #define VERB(fmt, ...) if (0) DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) /* verbose debug */ >> @@ -57,6 +58,9 @@ struct omap_drm_private { >>       unsigned int num_planes; >>       struct drm_plane *planes[8]; >>   +    unsigned int num_ovls; >> +    struct omap_hw_overlay *overlays[8]; >> + >>       struct drm_fb_helper *fbdev; >>         struct workqueue_struct *wq; >> diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c b/drivers/gpu/drm/omapdrm/omap_overlay.c >> new file mode 100644 >> index 000000000000..2b1416d2aad2 >> --- /dev/null >> +++ b/drivers/gpu/drm/omapdrm/omap_overlay.c >> @@ -0,0 +1,87 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2018 Texas Instruments Incorporated -  http://www.ti.com/ >> + * Author: Benoit Parrot, > > Extra comma there, and similar case below. Fixed > >> + */ >> + >> +#include >> +#include >> +#include >> + >> +#include "omap_dmm_tiler.h" >> +#include "omap_drv.h" >> + >> +/* >> + * overlay funcs >> + */ >> +static const char * const overlay_id_to_name[] = { >> +    [OMAP_DSS_GFX] = "gfx", >> +    [OMAP_DSS_VIDEO1] = "vid1", >> +    [OMAP_DSS_VIDEO2] = "vid2", >> +    [OMAP_DSS_VIDEO3] = "vid3", >> +}; > > I was expecting to see the name array to be removed from omap_plane.c as it's moved here, but that's not the case. Why is that? Especially as after this series these names make no sense with the planes. You're right, I'll remove the one from omap_plane.c and where it's used. > >> +static void omap_overlay_destroy(struct omap_hw_overlay *overlay) >> +{ >> +    kfree(overlay); >> +} >> + >> +static struct omap_hw_overlay *omap_overlay_init(enum omap_plane_id overlay_id, >> +                         enum omap_overlay_caps caps) >> +{ >> +    struct omap_hw_overlay *overlay; >> + >> +    overlay = kzalloc(sizeof(*overlay), GFP_KERNEL); >> +    if (!overlay) >> +        return ERR_PTR(-ENOMEM); >> + >> +    overlay->name = overlay_id_to_name[overlay_id]; >> +    overlay->overlay_id = overlay_id; >> +    overlay->caps = caps; >> +    /* >> +     * When this is called priv->num_crtcs is not known yet. >> +     * Use a safe mask value to start with, it will get updated to the >> +     * proper value after the first use. >> +     */ >> +    overlay->possible_crtcs = 0xff; > > This sounds like a hack. Why do we need possible_crtcs anyway? If I'm not mistaken, on all DSS versions any overlay can be used on any ctrtc. On the DRM plane level we need the possible_crtc as the DRM framework needs that (i.e. we just always set all crtcs available for all planes), but why is it needed here? Indeed, in omap_drv.c we have: plane_crtc_mask = (1 << priv->num_pipes) - 1; for all planes, so I'll rework this. > >> +    return overlay; >> +} >> + >> +int omap_hwoverlays_init(struct omap_drm_private *priv) >> +{ >> +    static const enum omap_plane_id hw_plane_ids[] = { >> +            OMAP_DSS_GFX, OMAP_DSS_VIDEO1, >> +            OMAP_DSS_VIDEO2, OMAP_DSS_VIDEO3, >> +    }; >> +    u32 num_overlays = dispc_get_num_ovls(priv->dispc); >> +    enum omap_overlay_caps caps; >> +    int i, ret; >> + >> +    for (i = 0; i < num_overlays; i++) { >> +        struct omap_hw_overlay *overlay; >> + >> +        caps = dispc_ovl_get_caps(priv->dispc, hw_plane_ids[i]); >> +        overlay = omap_overlay_init(hw_plane_ids[i], caps); >> +        if (IS_ERR(overlay)) { >> +            ret = PTR_ERR(overlay); >> +            dev_err(priv->dev, "failed to construct overlay for %s (%d)\n", >> +                overlay_id_to_name[i], ret); >> +            return ret; >> +        } > > I think this leaks memory if omap_overlay_init() fails. Fixed > >> +        overlay->idx = priv->num_ovls; >> +        priv->overlays[priv->num_ovls++] = overlay; >> +    } >> + >> +    return 0; >> +} >> + >> +void omap_hwoverlays_destroy(struct omap_drm_private *priv) >> +{ >> +    int i; >> + >> +    for (i = 0; i < priv->num_ovls; i++) { >> +        omap_overlay_destroy(priv->overlays[i]); >> +        priv->overlays[i] = NULL; >> +    } >> +} >> diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h b/drivers/gpu/drm/omapdrm/omap_overlay.h >> new file mode 100644 >> index 000000000000..892fecb67adb >> --- /dev/null >> +++ b/drivers/gpu/drm/omapdrm/omap_overlay.h >> @@ -0,0 +1,31 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (C) 2018 Texas Instruments Incorporated -  http://www.ti.com/ >> + * Author: Benoit Parrot, >> + */ >> + >> +#ifndef __OMAPDRM_OVERLAY_H__ >> +#define __OMAPDRM_OVERLAY_H__ >> + >> +#include >> + >> +enum drm_plane_type; >> + >> +struct drm_device; >> +struct drm_mode_object; >> +struct drm_plane; >> + >> +/* Used to associate a HW overlay/plane to a plane */ >> +struct omap_hw_overlay { >> +    int idx; > > unsigned int. > >> + >> +    const char *name; >> +    enum omap_plane_id overlay_id; > > Perhaps just "id" is fine. You don't have "overlay_name" there either, but just "name". Ack > >  Tomi Thanks, Neil