On Mon, May 27, 2019 at 06:51:18AM +0000, james qian wang (Arm Technology China) wrote:
On Fri, May 24, 2019 at 03:12:26PM +0300, Ville Syrjälä wrote:
On Fri, May 24, 2019 at 11:10:09AM +0000, Brian Starkey wrote:
Hi,
On Tue, May 21, 2019 at 09:45:58AM +0100, james qian wang (Arm Technology China) wrote:
On Thu, May 16, 2019 at 09:57:49PM +0800, Ayan Halder wrote:
On Thu, Apr 04, 2019 at 12:06:14PM +0100, james qian wang (Arm Technology China) wrote:
+static int +komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
const struct drm_mode_fb_cmd2 *mode_cmd)+{
- struct drm_framebuffer *fb = &kfb->base;
- const struct drm_format_info *info = fb->format;
- struct drm_gem_object *obj;
- u32 alignment_w = 0, alignment_h = 0, alignment_header;
- u32 n_blocks = 0, min_size = 0;
- obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
- if (!obj) {
DRM_DEBUG_KMS("Failed to lookup GEM object\n");return -ENOENT;- }
- switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
- case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
alignment_w = 32;alignment_h = 8;break;- case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
alignment_w = 16;alignment_h = 16;break;- default:
Can we have something like a warn here ?
will add a WARN here.
I think it's better not to. fb->modifier comes from userspace, so a malicious app could spam us with WARNs, effectively dos-ing the system. -EINVAL should be sufficient.
Should probably check that the entire modifier+format is actually valid. Otherwise you risk passing on a bogus modifier deeper into the driver which may trigger interesting bugs.
Also theoretically (however unlikely) some broken userspace might start to depend on the ability to create framebuffers with crap modifiers, which could later break if you change the way you handle the modifiers. Then you're stuck between the rock and hard place because you can't break existing userspace but you still want to change the way modifiers are handled in the kernel.
Best not give userspace too much rope IMO. Two ways to go about that:
- drm_any_plane_has_format() (assumes your .format_mod_supported() does its job properly)
- roll your own
-- Ville Syrjälä Intel
Hi Brian & Ville:
komed has a format+modifier check before the fb size check. and for komeda_fb_create, the first step is do the format+modifier check, the size check is the furthur check after the such format valid check. and the detailed fb_create is like:
struct drm_framebuffer * komeda_fb_create(struct drm_device *dev, struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd) { ... /* Step 1: format+modifier valid check, if it can not be support, * get_format_caps will return a NULL ptr. */ kfb->format_caps = komeda_get_format_caps(&mdev->fmt_tbl, mode_cmd->pixel_format, mode_cmd->modifier[0]); if (!kfb->format_caps) { DRM_DEBUG_KMS("FMT %x is not supported.\n", mode_cmd->pixel_format); kfree(kfb); return ERR_PTR(-EINVAL); }
drm_helper_mode_fill_fb_struct(dev, &kfb->base, mode_cmd);
/* step 2, do the size check */if (kfb->base.modifier) ret = komeda_fb_afbc_size_check(kfb, file, mode_cmd); else ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd); if (ret < 0) goto err_cleanup;
...}
So theoretically, the WARN in step2 is redundant if get_format_caps function has no problem. :). the WARN here is only for reporting the kernel BUG or code inconsitent with format caps check and the fb size check. And I agree, basically it will not happene. @Brian, I'm Ok to remove it. :)
@Ville: Basically komeda follow the way-1, but a little improvement for matching komeda's requirement. for komeda which will do two level's format+modifier check. 1). In fb_create, A roughly check to see if the format+modifier can be supported by current HW.
Yeah, looks like it shouldn't allow any unspecfied modifiers to sneak through. So should be good.
2). In plane_atomic_check: to see if the format+modifier can be supported for a specific layer and with a specific configuration (ROT)
This is a format valid check example for plane_check. https://patchwork.freedesktop.org/patch/301140/?series=59747&rev=2
James