From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH 05/23] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer Date: Mon, 9 May 2016 18:15:10 +0300 Message-ID: <5730A97E.6020303@ti.com> References: <1461702945-14185-1-git-send-email-laurent.pinchart@ideasonboard.com> <1461702945-14185-6-git-send-email-laurent.pinchart@ideasonboard.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0633593135==" Return-path: Received: from bear.ext.ti.com (bear.ext.ti.com [192.94.94.41]) by gabe.freedesktop.org (Postfix) with ESMTPS id B2B146E05F for ; Mon, 9 May 2016 15:15:16 +0000 (UTC) In-Reply-To: <1461702945-14185-6-git-send-email-laurent.pinchart@ideasonboard.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0633593135== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="1ojO2KH9TqP7Emh06Mr8JnUT5uuq5PVaN" --1ojO2KH9TqP7Emh06Mr8JnUT5uuq5PVaN Content-Type: multipart/mixed; boundary="0IQlonRdXQVRtnDrf5GcCjX67EtN8w5cd" From: Tomi Valkeinen To: Laurent Pinchart , dri-devel@lists.freedesktop.org Cc: Rob Clark Message-ID: <5730A97E.6020303@ti.com> Subject: Re: [PATCH 05/23] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer References: <1461702945-14185-1-git-send-email-laurent.pinchart@ideasonboard.com> <1461702945-14185-6-git-send-email-laurent.pinchart@ideasonboard.com> In-Reply-To: <1461702945-14185-6-git-send-email-laurent.pinchart@ideasonboard.com> --0IQlonRdXQVRtnDrf5GcCjX67EtN8w5cd Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 26/04/16 23:35, Laurent Pinchart wrote: > Checks can be simplified based on the requirement that pitches must be > identical for all planes. Your code also presumes there are only 1 or 2 planes, I think that should be mentioned too. > Signed-off-by: Laurent Pinchart > --- > drivers/gpu/drm/omapdrm/omap_fb.c | 51 ++++++++++++++++++++-----------= -------- > 1 file changed, 26 insertions(+), 25 deletions(-) >=20 > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdr= m/omap_fb.c > index 015b6a50c581..8629ba6ff9d7 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fb.c > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c > @@ -407,6 +407,7 @@ struct drm_framebuffer *omap_framebuffer_init(struc= t drm_device *dev, > struct omap_framebuffer *omap_fb =3D NULL; > struct drm_framebuffer *fb =3D NULL; > const struct format *format =3D NULL; > + unsigned int pitch =3D mode_cmd->pitches[0]; > int ret, i; > =20 > DBG("create framebuffer: dev=3D%p, mode_cmd=3D%p (%dx%d@%4.4s)", > @@ -437,38 +438,38 @@ struct drm_framebuffer *omap_framebuffer_init(str= uct drm_device *dev, > omap_fb->format =3D format; > mutex_init(&omap_fb->lock); > =20 > - for (i =3D 0; i < format->num_planes; i++) { > - struct plane *plane =3D &omap_fb->planes[i]; > - int size, pitch =3D mode_cmd->pitches[i]; > + if (format->num_planes =3D=3D 2 && pitch !=3D mode_cmd->pitches[1]) {= > + dev_err(dev->dev, "pitches differ between planes 0 and 1\n"); > + ret =3D -EINVAL; > + goto fail; > + } > =20 > - if (pitch < (mode_cmd->width * format->stride_bpp)) { > - dev_err(dev->dev, "provided buffer pitch is too small! %d < %d\n", > - pitch, mode_cmd->width * format->stride_bpp); > - ret =3D -EINVAL; > - goto fail; > - } > + if (pitch < mode_cmd->width * format->stride_bpp) { > + dev_err(dev->dev, > + "provided buffer pitch is too small! %u < %u\n", > + pitch, mode_cmd->width * format->stride_bpp); > + ret =3D -EINVAL; > + goto fail; > + } > =20 > - if (pitch % format->stride_bpp !=3D 0) { > - dev_err(dev->dev, > - "buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes= )\n", > - pitch, format->stride_bpp); > - ret =3D -EINVAL; > - goto fail; > - } > + if (pitch % format->stride_bpp !=3D 0) { > + dev_err(dev->dev, > + "buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)= \n", > + pitch, format->stride_bpp); > + ret =3D -EINVAL; > + goto fail; > + } > + > + for (i =3D 0; i < format->num_planes; i++) { > + struct plane *plane =3D &omap_fb->planes[i]; > + unsigned int size; > =20 > size =3D pitch * mode_cmd->height / format->sub_y[i]; > =20 > if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) { > - dev_err(dev->dev, "provided buffer object is too small! %d < %d\n",= > - bos[i]->size - mode_cmd->offsets[i], size); > - ret =3D -EINVAL; > - goto fail; > - } > - > - if (i > 0 && pitch !=3D mode_cmd->pitches[i - 1]) { > dev_err(dev->dev, > - "pitches are not the same between framebuffer planes %d !=3D %d\n"= , > - pitch, mode_cmd->pitches[i - 1]); > + "provided buffer object is too small! %d < %d\n", > + bos[i]->size - mode_cmd->offsets[i], size); > ret =3D -EINVAL; > goto fail; > } So, hmm... I think the current code is actually not right, even if it works right: I think the DSS's requirement is actually that the width in pixels is the same between planes, not stride. At the moment the only two plane format, NV12, has the same pixel size for both planes, but an upcoming DSS might have a format that has a separate 8 byte A plane. I don't know if that ever realizes or if we want to support the mode, but after thinking about this, it makes more sense that the pixel width has to be the same between planes, not the byte width. Tomi --0IQlonRdXQVRtnDrf5GcCjX67EtN8w5cd-- --1ojO2KH9TqP7Emh06Mr8JnUT5uuq5PVaN Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXMKl+AAoJEPo9qoy8lh71Ay8P/AvYQObzPwibaQTXkA6U8DY2 Yd4D+8jJLfFhsJ279OILnbJlmktXHdC3jI2vQMz8mVYipqa64m9jXaiR+mx/GnLS ud6oZq+9I1Lx7KpInl296Xj2SFfOEtfz3GZ/1iHphHP/GoPPU5DqcAmY9ej++fXw WGzuuPcKrOAr+BCVQYR8n7GkTWbDIeFCoRktyzw9P8CfOVqSqG77oxw1kl+SfVd9 TJ5jUYoQVhQ3sTV3+HLs0iPPdMuAlPBNpwSx1ZfHhVv2lKqntdgrUDhH1Qb/jMgd W1rQD/wU8PtGQLS0d8dDy/EKbyW307uFJowj1rzL3CN68hyX845fU27UCAUngJqp Z0DhiolmIw85yS19DmHOrLEkZkkS9TqTHuzr+jiLbLN0+R/FpdTNtyCPIQEqaMxH dvtgE8aHxoqIr5LFBlBZksAwdeeAvg3bdUhiG/HILXL8IFEbcuWj6yVeAbMnUOQO TqlhYhVj0ipzNOcwY0ndOCLFo+KNCKwSAHf9TIcMSnlkiy/EPZqVN7N16/sxjjwG Rd5JRplKEwsf7z5BGnBHmDClvS8gVOUvIZQmLIXSSD5NlEUEeQRPFxZXWjqh6dW/ FtoJq5fa6aPt9C39FfDlVo+jGAsnnVLnN5MNvyNjwcgIhfiRxbJCeVRnZyGtpUli dNgbO9B/oJ9SmX269WGd =gKSx -----END PGP SIGNATURE----- --1ojO2KH9TqP7Emh06Mr8JnUT5uuq5PVaN-- --===============0633593135== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0633593135==--