Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1582710377-15489-1-git-send-email-kevin3.tang@gmail.com>
 <1582710377-15489-5-git-send-email-kevin3.tang@gmail.com> <CACvgo53dME1ioYebimSzdOMvjAudtmzpz_-5Q7rNqQnZoBpaqA@mail.gmail.com>
 <CAFPSGXYgY7=vgX6ZPWRgfxfZfBeVRj7=gUOwrcTyYpkYE1C1cA@mail.gmail.com>
In-Reply-To: <CAFPSGXYgY7=vgX6ZPWRgfxfZfBeVRj7=gUOwrcTyYpkYE1C1cA@mail.gmail.com>
From:   Emil Velikov <emil.l.velikov@gmail.com>
Date:   Fri, 6 Mar 2020 17:14:14 +0000
Message-ID: <CACvgo51ShmP+HvLHzxbpzFg2gNs-cD0iey=nM29prDhZsN7fhQ@mail.gmail.com>
Subject: Re: [PATCH RFC v4 4/6] drm/sprd: add Unisoc's drm display controller driver
To:     tang pengchuan <kevin3.tang@gmail.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Sean Paul <sean@poorly.run>, Dave Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        ML dri-devel <dri-devel@lists.freedesktop.org>,
        "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        Baolin Wang <baolin.wang@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 5 Mar 2020 at 13:15, tang pengchuan <kevin3.tang@gmail.com> wrote:
> On Tue, Mar 3, 2020 at 2:29 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:

>> Have you seen a case where the 0 or default case are reached? AFAICT they will
>> never trigger. So one might as well use:
>>
>>     switch (angle) {
>>     case DRM_MODE_FOO:
>>         return DPU_LAYER_ROTATION_FOO;
>>     ...
>>     case DRM_MODE_BAR:
>>         return DPU_LAYER_ROTATION_BAR;
>>     }
>>
> Yeah, the 0 maybe unused code, i will remove it.
> But i think default is need, because userspace could give an incorrect value .
> So we need to setup a default value and doing error check.

As mentioned in the documentation [0] input (userspace) validation
should happen in atomic_check. This function here is called during
atomic_flush which is _not_ allowed to fail.


>> The default case here should be unreachable. Either it is or the upper layer (or
>> earlier code) should ensure that.
>
> There will be some differences in the formats supported by different chips, but userspace will only have one set of code.
> So it is necessary to check whether the parameters passed by the user layer are wrong. I think it is necessary

As said above - this type of issues should be checked _before_
reaching atomic_flush - aka in atomic_check.


>> > +static struct drm_plane *sprd_plane_init(struct drm_device *drm,
>> > +                                       struct sprd_dpu *dpu)
>> > +{
>> > +       struct drm_plane *primary = NULL;
>> > +       struct sprd_plane *p = NULL;
>> > +       struct dpu_capability cap = {};
>> > +       int err, i;
>> > +
>> > +       if (dpu->core && dpu->core->capability)
>> As mentioned before - this always evaluates to true, so drop the check.
>> Same applies for the other dpu->core->foo checks.
>>
>> Still not a huge fan of the abstraction layer, but I guess you're hesitant on
>> removing it.
>
> Sometimes,  some "dpu->core->foo" maybe always need, compatibility will be better.
> eg:
>
>     if (dpu->glb && dpu->glb->power)
>         dpu->glb->power(ctx, true);
>     if (dpu->glb && dpu->glb->enable)
>         dpu->glb->enable(ctx);
>
>     if (ctx->is_stopped && dpu->glb && dpu->glb->reset)
>         dpu->glb->reset(ctx);
>
>     if (dpu->clk && dpu->clk->init)
>         dpu->clk->init(ctx);
>     if (dpu->clk && dpu->clk->enable)
>         dpu->clk->enable(ctx);
>
>     if (dpu->core && dpu->core->init)
>         dpu->core->init(ctx);
>     if (dpu->core && dpu->core->ifconfig)
>         dpu->core->ifconfig(ctx);
>

If there are no hooks, then the whole thing is dead code. As such it
should not be included.


> >
> > Note: Custom properties should be separate patches. This includes documentation
> > why they are needed and references to open-source userspace.
> This only need for our chips, what documentation do we need to provide?
>

KMS properties should be generic. Reason being is that divergence
causes substantial overhead, and fragility, to each and every
userspace consumer. The documentation has some general notes on the
topic [1]. Don't forget the "Testing and validation" section ;-)

Although I've tried to catch everything, I might have missed a comment
or two due the HTML formatting. Please toggle to plain text [2] for
the future.

Thanks
-Emil

[0] https://www.kernel.org/doc/html/v5.5/gpu/drm-kms.html
[1] Documentation/gpu/drm-uapi.rst in particular "Open-Source
Userspace Requirements"
[2] https://smallbusiness.chron.com/reply-inline-gmail-40679.html