On Fri, Jun 10, 2016 at 02:51:45PM +0300, Tomi Valkeinen wrote:
>
>
> On 09/06/16 02:32, Laurent Pinchart wrote:
> > The driver needs the number of bytes per pixel, not the bpp and depth
> > info meant for fbdev compatibility. Use the right API.
> >
> > In the tilcdc_crtc_mode_set() function compute the hardware register
> > value directly from the pixel format instead of computing the number of
> > bits per pixels first.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 23 ++++++++++-------------
> > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > Cc: Jyri Sarha <jsarha at ti.com>
> >
> > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > index 79027b1c64d3..d63c7363dabc 100644
> > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > @@ -65,15 +65,13 @@ static void set_scanout(struct drm_crtc *crtc, struct
> > drm_framebuffer *fb)
> > struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> > struct drm_device *dev = crtc->dev;
> > struct drm_gem_cma_object *gem;
> > - unsigned int depth, bpp;
> > dma_addr_t start, end;
> >
> > - drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> > gem = drm_fb_cma_get_gem_obj(fb, 0);
> >
> > start = gem->paddr + fb->offsets[0] +
> > crtc->y * fb->pitches[0] +
> > - crtc->x * bpp / 8;
> > + crtc->x * drm_format_plane_cpp(fb->pixel_format, 0);
> >
> > end = start + (crtc->mode.vdisplay * fb->pitches[0]);
> >
> > @@ -132,11 +130,12 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
> > static int tilcdc_verify_fb(struct drm_crtc *crtc, struct drm_framebuffer
> > *fb)
> > {
> > struct drm_device *dev = crtc->dev;
> > - unsigned int depth, bpp;
> > + unsigned int min_pitch;
> >
> > - drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> > + min_pitch = crtc->mode.hdisplay
> > + * drm_format_plane_cpp(fb->pixel_format, 0);
>
> Why 'min_pitch'? Plain 'pitch' should be fine here.
>
> >
> > - if (fb->pitches[0] != crtc->mode.hdisplay * bpp / 8) {
> > + if (fb->pitches[0] != min_pitch) {
> > dev_err(dev->dev,
> > "Invalid pitch: fb and crtc widths must be the same");
> > return -EINVAL;
> > @@ -401,16 +400,14 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
> > if (info->tft_alt_mode)
> > reg |= LCDC_TFT_ALT_ENABLE;
> > if (priv->rev == 2) {
> > - unsigned int depth, bpp;
> > -
> > - drm_fb_get_bpp_depth(crtc->primary->fb->pixel_format, &depth,
> > &bpp);
> > - switch (bpp) {
> > - case 16:
> > + switch (crtc->primary->fb->pixel_format) {
> > + case DRM_FORMAT_RGB565:
> > break;
> > - case 32:
> > + case DRM_FORMAT_XRGB8888:
> > + case DRM_FORMAT_ARGB8888:
>
> I'm not sure what's the common way, but tilcdc doesn't support alpha.
> ARGB works, of course, by ignoring A, but... If an userspace app creates
> ARGB buffer, does the app expect alpha to work?
I think what we decided a while ago (at least for i915, but would be good
to use the same convention everywhere) was that ARGB will be assumed to be
pre-multiplied and will enable blending using 1.0*sc+(1.0-sa)*dc as the
function. There have been some efforts at defining some new properties to
control the blend equation, but I guess those got bogged down again.
--
Ville Syrjälä
Intel OTC
