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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8074EC433F5 for ; Wed, 20 Apr 2022 22:06:25 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8A9D510E45E; Wed, 20 Apr 2022 22:06:24 +0000 (UTC) Received: from alexa-out.qualcomm.com (alexa-out.qualcomm.com [129.46.98.28]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7B21310E45E; Wed, 20 Apr 2022 22:06:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; i=@quicinc.com; q=dns/txt; s=qcdkim; t=1650492384; x=1682028384; h=message-id:date:mime-version:subject:from:to:cc: references:in-reply-to:content-transfer-encoding; bh=0QnibYr3NFcW5sTW6E6yE4DyfD2Su+C1a+cyfvZvoac=; b=V6dk1T6le9GVodOKHUWqjaT7j/7NcTWmGHwpTQaObIeCGp4PIfSiBhMS vphE6XGQtWEPuIepVqF1MntxvKbQxsM47X/OLcbqIxQUE5O8aSoj2LGdB eYeDV6n+gSBHdCggedCHam4YNHNmjZGJgZBh10ntJGKjf62zpFA+XUZ54 8=; Received: from ironmsg09-lv.qualcomm.com ([10.47.202.153]) by alexa-out.qualcomm.com with ESMTP; 20 Apr 2022 15:06:23 -0700 X-QCInternal: smtphost Received: from nasanex01c.na.qualcomm.com ([10.47.97.222]) by ironmsg09-lv.qualcomm.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2022 15:06:23 -0700 Received: from nalasex01a.na.qualcomm.com (10.47.209.196) by nasanex01c.na.qualcomm.com (10.47.97.222) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.22; Wed, 20 Apr 2022 15:06:22 -0700 Received: from [10.111.175.210] (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.22; Wed, 20 Apr 2022 15:06:20 -0700 Message-ID: <4e1b4479-8771-6ad9-8a6d-f53bdb5ae13e@quicinc.com> Date: Wed, 20 Apr 2022 15:06:18 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.6.2 Subject: Re: [Freedreno] [PATCH v2 10/17] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder Content-Language: en-US From: Abhinav Kumar To: Dmitry Baryshkov References: <1650419169-13760-1-git-send-email-quic_abhinavk@quicinc.com> <1650419169-13760-11-git-send-email-quic_abhinavk@quicinc.com> <05c75f88-8b76-f931-7162-850827de5baf@quicinc.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: markyacoub@chromium.org, liviu.dudau@arm.com, dri-devel@lists.freedesktop.org, swboyd@chromium.org, seanpaul@chromium.org, laurent.pinchart@ideasonboard.com, quic_jesszhan@quicinc.com, quic_aravindh@quicinc.com, freedreno@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Dmitry On 4/20/2022 11:46 AM, Abhinav Kumar wrote: > > > On 4/20/2022 11:37 AM, Dmitry Baryshkov wrote: >> On Wed, 20 Apr 2022 at 20:41, Abhinav Kumar >> wrote: >>> >>> >>> >>> On 4/20/2022 12:44 AM, Dmitry Baryshkov wrote: >>>> On 20/04/2022 04:46, Abhinav Kumar wrote: >>>>> Make changes to dpu_encoder to support virtual encoder needed >>>>> to support writeback for dpu. >>>>> >>>>> changes in v2: >>>>>      - add the writeback parts to dpu_encoder_helper_phys_cleanup >>>>>      - rebase on tip of msm-next and fix related dependencies >>>>>      - get the writeback blocks directly from RM >>>>> >>>>> Signed-off-by: Abhinav Kumar >>>>> --- >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 71 >>>>> +++++++++++++++++------- >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  3 + >>>>>    2 files changed, 54 insertions(+), 20 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> index 0e31ad3..06b8631 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>> @@ -928,6 +928,7 @@ static void >>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >>>>>        struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; >>>>>        int num_lm, num_ctl, num_pp, num_dsc; >>>>>        unsigned int dsc_mask = 0; >>>>> +    enum dpu_hw_blk_type blk_type; >>>>>        int i; >>>>>        if (!drm_enc) { >>>>> @@ -1009,12 +1010,21 @@ static void >>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >>>>>            phys->hw_pp = dpu_enc->hw_pp[i]; >>>>>            phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); >>>>> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) >>>>> -            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >>>>> phys->intf_idx); >>>>> +        if (dpu_encoder_get_intf_mode(&dpu_enc->base) == >>>>> INTF_MODE_WB_LINE) >>>>> +            blk_type = DPU_HW_BLK_WB; >>>>> +        else >>>>> +            blk_type = DPU_HW_BLK_INTF; >>>>> + >>>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) { >>>>> +            if (blk_type == DPU_HW_BLK_INTF) >>>>> +                phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >>>>> phys->intf_idx); >>>>> +            else if (blk_type == DPU_HW_BLK_WB) >>>>> +                phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, >>>>> phys->intf_idx); >>>>> +        } >>>>> -        if (!phys->hw_intf) { >>>>> +        if (!phys->hw_intf && !phys->hw_wb) { >>>>>                DPU_ERROR_ENC(dpu_enc, >>>>> -                      "no intf block assigned at idx: %d\n", i); >>>>> +                      "no intf ow wb block assigned at idx: %d\n", >>>>> i); >>>> >>>> or wb >>> ack >>>> >>>>>                return; >>>>>            } >>>>> @@ -1157,15 +1167,22 @@ static void dpu_encoder_virt_disable(struct >>>>> drm_encoder *drm_enc) >>>>>        mutex_unlock(&dpu_enc->enc_lock); >>>>>    } >>>>> -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg >>>>> *catalog, >>>>> +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg >>>>> *catalog, >>>>>            enum dpu_intf_type type, u32 controller_id) >>>>>    { >>>>>        int i = 0; >>>>> -    for (i = 0; i < catalog->intf_count; i++) { >>>>> -        if (catalog->intf[i].type == type >>>>> -            && catalog->intf[i].controller_id == controller_id) { >>>>> -            return catalog->intf[i].id; >>>>> +    if (type != INTF_WB) { >>>>> +        for (i = 0; i < catalog->intf_count; i++) { >>>>> +            if (catalog->intf[i].type == type >>>>> +                && catalog->intf[i].controller_id == controller_id) { >>>>> +                return catalog->intf[i].id; >>>>> +            } >>>>> +        } >>>>> +    } else { >>>>> +        for (i = 0; i < catalog->wb_count; i++) { >>>>> +            if (catalog->wb[i].id == controller_id) >>>>> +                return catalog->wb[i].id; >>>>>            } >>>>>        } >>>>> @@ -1886,16 +1903,27 @@ void dpu_encoder_helper_phys_cleanup(struct >>>>> dpu_encoder_phys *phys_enc) >>>>>        dpu_encoder_helper_reset_mixers(phys_enc); >>>>> -    for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>>>> -        if (dpu_enc->phys_encs[i] && >>>>> phys_enc->hw_intf->ops.bind_pingpong_blk) >>>>> -            phys_enc->hw_intf->ops.bind_pingpong_blk( >>>>> -                    dpu_enc->phys_encs[i]->hw_intf, false, >>>>> -                    dpu_enc->phys_encs[i]->hw_pp->idx); >>>>> +    if (phys_enc->hw_wb) { >>>> >>>> I think this adds a hidden knowledge here. That there is always just a >>>> single phys_enc for the WB encoder. I'd still do this cleanup in a loop >>>> together with the INTF cleanup. >>> alright, I can make this change. I dug into the history of dpu_encoder_helper_phys_cleanup() API more in downstream. So this API seems to be a bit poorly designed in downstream too. This gets called from phys->ops.disable() today which is anyway called from within a loop. for (i = 0; i < dpu_enc->num_phys_encs; i++) { struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; if (phys->ops.disable) phys->ops.disable(phys); } So technically we dont need a loop even for the intf cases because of that. But, while calling for interfaces, downstream makes sure to call this just once for the master and skip for the same. Here the loop is needed just to set the flush bits even for the slave and let the master do the actual flush. Since today, we are not calling this for interface yet, I think i can get rid of the loop even for the interface case for uniformity. When I extend this API to be called for interface cases, we can decide how to handle master/slave cases. Let me know what you think. >>>> >>>>> +        /* disable the PP block */ >>>>> +        if (phys_enc->hw_wb->ops.bind_pingpong_blk) >>>>> +            phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, >>>>> false, >>>>> +                    phys_enc->hw_pp->idx); >>>>> -        /* mark INTF flush as pending */ >>>>> -        if (phys_enc->hw_ctl->ops.update_pending_flush_intf) >>>>> - >>>>> phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, >>>>> -                    dpu_enc->phys_encs[i]->hw_intf->idx); >>>>> +        /* mark WB flush as pending */ >>>>> +        if (phys_enc->hw_ctl->ops.update_pending_flush_wb) >>>>> +            phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, >>>>> phys_enc->hw_wb->idx); >>>>> +    } else { >>>>> +        for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>>>> +            if (dpu_enc->phys_encs[i] && >>>>> phys_enc->hw_intf->ops.bind_pingpong_blk) >>>>> +                phys_enc->hw_intf->ops.bind_pingpong_blk( >>>>> +                        dpu_enc->phys_encs[i]->hw_intf, false, >>>>> +                        dpu_enc->phys_encs[i]->hw_pp->idx); >>>>> + >>>>> +            /* mark INTF flush as pending */ >>>>> +            if (phys_enc->hw_ctl->ops.update_pending_flush_intf) >>>>> + >>>>> phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, >>>>> +                        dpu_enc->phys_encs[i]->hw_intf->idx); >>>>> +        } >>>>>        } >>>>>        /* reset the merge 3D HW block */ >>>>> @@ -2111,6 +2139,9 @@ static int dpu_encoder_setup_display(struct >>>>> dpu_encoder_virt *dpu_enc, >>>>>        case DRM_MODE_ENCODER_TMDS: >>>>>            intf_type = INTF_DP; >>>>>            break; >>>>> +    case DRM_MODE_ENCODER_VIRTUAL: >>>>> +        intf_type = INTF_WB; >>>>> +        break; >>>>>        } >>>>>        WARN_ON(disp_info->num_of_h_tiles < 1); >>>>> @@ -2145,11 +2176,11 @@ static int dpu_encoder_setup_display(struct >>>>> dpu_encoder_virt *dpu_enc, >>>>>            DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n", >>>>>                    i, controller_id, phys_params.split_role); >>>>> -        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >>>>> +        phys_params.intf_idx = >>>>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog, >>>>>                                                        intf_type, >>>>>                                                        controller_id); >>>> >>>> I'd prefer to be more explicit here. intf_idx is of type enum dpu_intf, >>>> while WBs are enumerated with enum dpu_wb. >>>> >>>> I's suggest adding a separate phys_params.wb_idx and a >>>> dpu_encoder_get_wb() and calling one here depending on intf_type. >>>> >>>> Then at the dpu_encoder_virt_atomic_mode_set() you don't need to check >>>> intf_mode, but get both intf and wb and report an error if none was >>>> provided. >>> >>> Your suggestion is valid and I also thought about it. >>> >>> Let me explain what prevented me from making the change here and put it >>> in a to-do bucket. >>> >>> 1) This needs a slighly bigger cleanup including the traces, debug >>> prints and some helpers as none of them are aware of the wb_idx >>> >>> 2) Some of the checks need to be adjusted like this one >>> >>> if (phys_params.intf_idx == INTF_MAX) { >>>               DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, >>> id %d\n", >>>                             intf_type, controller_id); >>>               ret = -EINVAL; >>>           } >>> >>> So, when we have a separate intf_idx and a wb_idx, having either one of >>> them is enough . Consider a case where a board has no physical display. >>> That would have no intf_idx but wb_idx can be valid. >>> >>> So i think this needs a little careful cleanup. >>> >>> Considering that I need to test that out more, I decided that for a >>> short-term we can live with the fact that wb_idx is of type enum >>> dpu_intf because dpu_encoder layer understands only that. >>> >>> Let me know if you agree on this separation to go as a follow-up. >> >> Actually both your points vote for the immediate separation rather >> than doing it in a follow-up. >> For example, if you see an error reported against INTF_2 on a board >> where INTF_2 is actually used, you can not determine whether it is an >> issue with INTF_2 or with WB_2. >> >> In fact even reporting an error (or a trace) against INTF_NONE (or >> INTF_MAX) might be a better option. It would clearly denote that the >> issue is related to the non-INTF. >> >> I think we can merge the patch as is, but I'd strongly prefer either >> to see an update or a (nearly) immediate followup. >> >> What do you think about an interim solution? We split the >> intf_idx/wb_idx in these series, but all the debugging can be updated >> later. This way once we see the report against INTF_NONE, we can >> deduce that it's WB. > > Thank you for your consideration. > > I will post a follow-up change to fix this no later than next week. > I will even try doing it this week itself. > > I will fix everything together so that it looks like a separate series > of "separating intf_idx and wb_idx" and there is no intermediate mismatch. > > Will leave a FIXME here as well. > >> >>>> >>>>>            if (phys_params.intf_idx == INTF_MAX) { >>>>> -            DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id >>>>> %d\n", >>>>> +            DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type >>>>> %d, id %d\n", >>>>>                              intf_type, controller_id); >>>>>                ret = -EINVAL; >>>>>            } >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>>> index 544a9a4..0b80af4 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>>> @@ -11,6 +11,7 @@ >>>>>    #include "dpu_kms.h" >>>>>    #include "dpu_hw_intf.h" >>>>> +#include "dpu_hw_wb.h" >>>>>    #include "dpu_hw_pingpong.h" >>>>>    #include "dpu_hw_ctl.h" >>>>>    #include "dpu_hw_top.h" >>>>> @@ -165,6 +166,7 @@ enum dpu_intr_idx { >>>>>     * @hw_ctl:        Hardware interface to the ctl registers >>>>>     * @hw_pp:        Hardware interface to the ping pong registers >>>>>     * @hw_intf:        Hardware interface to the intf registers >>>>> + * @hw_wb:        Hardware interface to the wb registers >>>>>     * @dpu_kms:        Pointer to the dpu_kms top level >>>>>     * @cached_mode:    DRM mode cached at mode_set time, acted on in >>>>> enable >>>>>     * @enabled:        Whether the encoder has enabled and running >>>>> a mode >>>>> @@ -193,6 +195,7 @@ struct dpu_encoder_phys { >>>>>        struct dpu_hw_ctl *hw_ctl; >>>>>        struct dpu_hw_pingpong *hw_pp; >>>>>        struct dpu_hw_intf *hw_intf; >>>>> +    struct dpu_hw_wb *hw_wb; >>>>>        struct dpu_kms *dpu_kms; >>>>>        struct drm_display_mode cached_mode; >>>>>        enum dpu_enc_split_role split_role; >>>> >>>> >> >> >>