Emil Velikov emil.l.velikov@gmail.com 于2020年3月7日周六 上午1:14写道：
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.
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