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