| Message ID | 1479399271-31991-15-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ville, Thank you for the patch. On Thursday 17 Nov 2016 18:14:13 ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Stuff something semi-reasonable into fb->pixel_format. I had to guess > as to which formats we should pick. Did I guess correctly? > > We can't quite use drm_mode_legacy_fb_format() due to the ARGB1555 > vs. XRGB155 mess. However use of 'A' formats should imply per-pixel > alpha blending as far as KMS is concerned so I'm not at all sure we > want to have any 'A' formats exposed as opposed to just 'X' formats. > OTOH vmvgfx doesn't do planes, and so the core won't enforce that > these formats match any list of supported formats, so the choice > shouldn't be super important at this point. The vmgfx driver should really be converted to using struct drm_mode_fb_cmd2 through the whole code instead of converting struct drm_mode_fb_cmd2 to struct drm_mode_fb_cmd in vmw_kms_fb_create(). I gave it a go a few months back but the structure is passed around so many functions that conversion requires more knowledge about the driver than I have. I don't expect you to fix that, but it would be nice if Sinclair or Thomas could give it a go. > Cc: linux-graphics-maintainer@vmware.com > Cc: Sinclair Yeh <syeh@vmware.com> > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 7d92ab56961b..5788913ca8f9 > 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -524,6 +524,7 @@ static int vmw_kms_new_framebuffer_surface(struct > vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev; > struct vmw_framebuffer_surface *vfbs; > enum SVGA3dSurfaceFormat format; > + u32 pixel_format; > int ret; > > /* 3D is only supported on HWv8 and newer hosts */ > @@ -548,17 +549,22 @@ static int vmw_kms_new_framebuffer_surface(struct > vmw_private *dev_priv, return -EINVAL; > } > > + /* FIXME 'A' format implies per-pixel alpha blending for KMS */ > switch (mode_cmd->depth) { > case 32: > + pixel_format = DRM_FORMAT_ARGB8888; > format = SVGA3D_A8R8G8B8; > break; > case 24: > + pixel_format = DRM_FORMAT_XRGB8888; > format = SVGA3D_X8R8G8B8; > break; > case 16: > + pixel_format = DRM_FORMAT_RGB565; > format = SVGA3D_R5G6B5; > break; > case 15: > + pixel_format = DRM_FORMAT_ARGB1555; > format = SVGA3D_A1R5G5B5; > break; > default: > @@ -582,7 +588,8 @@ static int vmw_kms_new_framebuffer_surface(struct > vmw_private *dev_priv, } > > vfbs->base.base.dev = dev; > - /* XXX get the first 3 from the surface info */ > + /* XXX get the first 4 from the surface info */ > + vfbs->base.base.pixel_format = pixel_format; > vfbs->base.base.bits_per_pixel = mode_cmd->bpp; > vfbs->base.base.pitches[0] = mode_cmd->pitch; > vfbs->base.base.depth = mode_cmd->depth; > @@ -834,6 +841,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev; > struct vmw_framebuffer_dmabuf *vfbd; > unsigned int requested_size; > + u32 pixel_format; > int ret; > > requested_size = mode_cmd->height * mode_cmd->pitch; > @@ -852,6 +860,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > vmw_private *dev_priv, if (mode_cmd->bpp == 32) > break; > > + /* FIXME 'A' format implies per-pixel alpha blending for KMS */ > + if (mode_cmd->depth == 32) > + pixel_format = DRM_FORMAT_ARGB8888; > + else > + pixel_format = DRM_FORMAT_XRGB8888; > + > DRM_ERROR("Invalid color depth/bbp: %d %d\n", > mode_cmd->depth, mode_cmd->bpp); > return -EINVAL; > @@ -861,6 +875,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > vmw_private *dev_priv, if (mode_cmd->bpp == 16) > break; > > + /* FIXME 'A' format implies per-pixel alpha blending for KMS */ > + if (mode_cmd->depth == 16) > + pixel_format = DRM_FORMAT_RGB565; > + else > + pixel_format = DRM_FORMAT_ARGB1555; > + > DRM_ERROR("Invalid color depth/bbp: %d %d\n", > mode_cmd->depth, mode_cmd->bpp); > return -EINVAL; > @@ -877,6 +897,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > vmw_private *dev_priv, } > > vfbd->base.base.dev = dev; > + vfbd->base.base.pixel_format = pixel_format; > vfbd->base.base.bits_per_pixel = mode_cmd->bpp; > vfbd->base.base.pitches[0] = mode_cmd->pitch; > vfbd->base.base.depth = mode_cmd->depth;
On Thu, Nov 17, 2016 at 07:52:23PM +0200, Laurent Pinchart wrote: > Hi Ville, > > Thank you for the patch. > > On Thursday 17 Nov 2016 18:14:13 ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Stuff something semi-reasonable into fb->pixel_format. I had to guess > > as to which formats we should pick. Did I guess correctly? > > > > We can't quite use drm_mode_legacy_fb_format() due to the ARGB1555 > > vs. XRGB155 mess. However use of 'A' formats should imply per-pixel > > alpha blending as far as KMS is concerned so I'm not at all sure we > > want to have any 'A' formats exposed as opposed to just 'X' formats. > > OTOH vmvgfx doesn't do planes, and so the core won't enforce that > > these formats match any list of supported formats, so the choice > > shouldn't be super important at this point. > > The vmgfx driver should really be converted to using struct drm_mode_fb_cmd2 > through the whole code instead of converting struct drm_mode_fb_cmd2 to struct > drm_mode_fb_cmd in vmw_kms_fb_create(). I gave it a go a few months back but > the structure is passed around so many functions that conversion requires more > knowledge about the driver than I have. I don't expect you to fix that, but it > would be nice if Sinclair or Thomas could give it a go. Indeed. Would be nice to bring all drivers into the fold. I didn't want to start down this particular hole with the series already too big as is. > > > Cc: linux-graphics-maintainer@vmware.com > > Cc: Sinclair Yeh <syeh@vmware.com> > > Cc: Thomas Hellstrom <thellstrom@vmware.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 ++++++++++++++++++++++- > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 7d92ab56961b..5788913ca8f9 > > 100644 > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > > @@ -524,6 +524,7 @@ static int vmw_kms_new_framebuffer_surface(struct > > vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev; > > struct vmw_framebuffer_surface *vfbs; > > enum SVGA3dSurfaceFormat format; > > + u32 pixel_format; > > int ret; > > > > /* 3D is only supported on HWv8 and newer hosts */ > > @@ -548,17 +549,22 @@ static int vmw_kms_new_framebuffer_surface(struct > > vmw_private *dev_priv, return -EINVAL; > > } > > > > + /* FIXME 'A' format implies per-pixel alpha blending for KMS */ > > switch (mode_cmd->depth) { > > case 32: > > + pixel_format = DRM_FORMAT_ARGB8888; > > format = SVGA3D_A8R8G8B8; > > break; > > case 24: > > + pixel_format = DRM_FORMAT_XRGB8888; > > format = SVGA3D_X8R8G8B8; > > break; > > case 16: > > + pixel_format = DRM_FORMAT_RGB565; > > format = SVGA3D_R5G6B5; > > break; > > case 15: > > + pixel_format = DRM_FORMAT_ARGB1555; > > format = SVGA3D_A1R5G5B5; > > break; > > default: > > @@ -582,7 +588,8 @@ static int vmw_kms_new_framebuffer_surface(struct > > vmw_private *dev_priv, } > > > > vfbs->base.base.dev = dev; > > - /* XXX get the first 3 from the surface info */ > > + /* XXX get the first 4 from the surface info */ > > + vfbs->base.base.pixel_format = pixel_format; > > vfbs->base.base.bits_per_pixel = mode_cmd->bpp; > > vfbs->base.base.pitches[0] = mode_cmd->pitch; > > vfbs->base.base.depth = mode_cmd->depth; > > @@ -834,6 +841,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > > vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev; > > struct vmw_framebuffer_dmabuf *vfbd; > > unsigned int requested_size; > > + u32 pixel_format; > > int ret; > > > > requested_size = mode_cmd->height * mode_cmd->pitch; > > @@ -852,6 +860,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > > vmw_private *dev_priv, if (mode_cmd->bpp == 32) > > break; > > > > + /* FIXME 'A' format implies per-pixel alpha blending > for KMS */ > > + if (mode_cmd->depth == 32) > > + pixel_format = DRM_FORMAT_ARGB8888; > > + else > > + pixel_format = DRM_FORMAT_XRGB8888; > > + > > DRM_ERROR("Invalid color depth/bbp: %d %d\n", > > mode_cmd->depth, mode_cmd->bpp); > > return -EINVAL; > > @@ -861,6 +875,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > > vmw_private *dev_priv, if (mode_cmd->bpp == 16) > > break; > > > > + /* FIXME 'A' format implies per-pixel alpha blending > for KMS */ > > + if (mode_cmd->depth == 16) > > + pixel_format = DRM_FORMAT_RGB565; > > + else > > + pixel_format = DRM_FORMAT_ARGB1555; > > + > > DRM_ERROR("Invalid color depth/bbp: %d %d\n", > > mode_cmd->depth, mode_cmd->bpp); > > return -EINVAL; > > @@ -877,6 +897,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct > > vmw_private *dev_priv, } > > > > vfbd->base.base.dev = dev; > > + vfbd->base.base.pixel_format = pixel_format; > > vfbd->base.base.bits_per_pixel = mode_cmd->bpp; > > vfbd->base.base.pitches[0] = mode_cmd->pitch; > > vfbd->base.base.depth = mode_cmd->depth; > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 7d92ab56961b..5788913ca8f9 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -524,6 +524,7 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev; struct vmw_framebuffer_surface *vfbs; enum SVGA3dSurfaceFormat format; + u32 pixel_format; int ret; /* 3D is only supported on HWv8 and newer hosts */ @@ -548,17 +549,22 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, return -EINVAL; } + /* FIXME 'A' format implies per-pixel alpha blending for KMS */ switch (mode_cmd->depth) { case 32: + pixel_format = DRM_FORMAT_ARGB8888; format = SVGA3D_A8R8G8B8; break; case 24: + pixel_format = DRM_FORMAT_XRGB8888; format = SVGA3D_X8R8G8B8; break; case 16: + pixel_format = DRM_FORMAT_RGB565; format = SVGA3D_R5G6B5; break; case 15: + pixel_format = DRM_FORMAT_ARGB1555; format = SVGA3D_A1R5G5B5; break; default: @@ -582,7 +588,8 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv, } vfbs->base.base.dev = dev; - /* XXX get the first 3 from the surface info */ + /* XXX get the first 4 from the surface info */ + vfbs->base.base.pixel_format = pixel_format; vfbs->base.base.bits_per_pixel = mode_cmd->bpp; vfbs->base.base.pitches[0] = mode_cmd->pitch; vfbs->base.base.depth = mode_cmd->depth; @@ -834,6 +841,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev; struct vmw_framebuffer_dmabuf *vfbd; unsigned int requested_size; + u32 pixel_format; int ret; requested_size = mode_cmd->height * mode_cmd->pitch; @@ -852,6 +860,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, if (mode_cmd->bpp == 32) break; + /* FIXME 'A' format implies per-pixel alpha blending for KMS */ + if (mode_cmd->depth == 32) + pixel_format = DRM_FORMAT_ARGB8888; + else + pixel_format = DRM_FORMAT_XRGB8888; + DRM_ERROR("Invalid color depth/bbp: %d %d\n", mode_cmd->depth, mode_cmd->bpp); return -EINVAL; @@ -861,6 +875,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, if (mode_cmd->bpp == 16) break; + /* FIXME 'A' format implies per-pixel alpha blending for KMS */ + if (mode_cmd->depth == 16) + pixel_format = DRM_FORMAT_RGB565; + else + pixel_format = DRM_FORMAT_ARGB1555; + DRM_ERROR("Invalid color depth/bbp: %d %d\n", mode_cmd->depth, mode_cmd->bpp); return -EINVAL; @@ -877,6 +897,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv, } vfbd->base.base.dev = dev; + vfbd->base.base.pixel_format = pixel_format; vfbd->base.base.bits_per_pixel = mode_cmd->bpp; vfbd->base.base.pitches[0] = mode_cmd->pitch; vfbd->base.base.depth = mode_cmd->depth;