Re: [PATCH RFC v4 4/6] drm/sprd: add Unisoc's drm display controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Emil Velikov <emil.l.velikov@xxxxxxxxx> 于2020年3月7日周六 上午1:14写道:
>
> On Thu, 5 Mar 2020 at 13:15, tang pengchuan <kevin3.tang@xxxxxxxxx> wrote:
> > On Tue, Mar 3, 2020 at 2:29 AM Emil Velikov <emil.l.velikov@xxxxxxxxx> 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.
In drm atomic commit codepath:
drm_atomic_commit-->drm_atomic_plane_check--->drm_plane_check_pixel_format
already helped us check DRM_FORMAT_XXX, so default laber is dead code.
Is just a waste of time and increases the complexity of the code for no reason.

We can't use "return" replace "break"
Because "switch(format)" and "switch(blending)"exist at the same
funtion, and I don't think it's a good idea to split them.
I think there are two solutions:
1. Remove default label completely
2. Add "default: // Do nothing", Eg:
int flag = value > 1000 ? 1 : 0;
swich(flag) {
    case 0:
        // do something
        break;
    case 1:
        // do something
        break;
    default: // do nothing
         break;
}

>
>
>
> >> 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.
Your are right, switch(format) and switch(blending) will never reach
default label, so it's dead code.
As for rotation, we will ensure it is correct at the user layer.
>
>
> >> > +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.
I got it
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux