Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754746AbbBTP0x (ORCPT ); Fri, 20 Feb 2015 10:26:53 -0500 Received: from mail-ig0-f177.google.com ([209.85.213.177]:60557 "EHLO mail-ig0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754105AbbBTP0w (ORCPT ); Fri, 20 Feb 2015 10:26:52 -0500 MIME-Version: 1.0 In-Reply-To: <1424443346-9935-2-git-send-email-gbeeresh@codeaurora.org> References: <1424443346-9935-1-git-send-email-gbeeresh@codeaurora.org> <1424443346-9935-2-git-send-email-gbeeresh@codeaurora.org> Date: Fri, 20 Feb 2015 10:26:51 -0500 Message-ID: Subject: Re: [PATCH 2/2] drm/msm: Support NV12MT format in mdp4 From: Rob Clark To: Beeresh Gopal Cc: "dri-devel@lists.freedesktop.org" , Linux Kernel Mailing List , David Airlie , Stephane Viau , Hai Li Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8154 Lines: 200 On Fri, Feb 20, 2015 at 9:42 AM, Beeresh Gopal wrote: > Using fb modifier flag, support NV12MT format > in MDP4 > > Change-Id: I3de92b0c3b6d0cb56647aed6e4e35e56eff7adab > Signed-off-by: Beeresh Gopal > Signed-off-by: Stephane Viau on small comment below, and one open question at the end, but other than that it looks good. oh, and you should drop the Change-Id > --- > drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 9 +++++++++ > drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 22 ++++++++++++++++++++++ > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 5 +++++ > drivers/gpu/drm/msm/msm_drv.c | 2 ++ > drivers/gpu/drm/msm/msm_kms.h | 1 + > include/uapi/drm/drm_fourcc.h | 14 ++++++++++++++ > 6 files changed, 53 insertions(+) > > diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c > index d81e19d..24c38d4 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c > +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c > @@ -157,6 +157,14 @@ static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s > mdp4_disable(mdp4_kms); > } > > +static void mdp4_set_mode_config(struct msm_kms *kms) > +{ > + struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms)); > + struct drm_device *dev = mdp4_kms->dev; > + > + dev->mode_config.allow_fb_modifiers = true; why not just add this in mdp4_kms_init() or hw_init()? That would avoid having to add a new vfunc and keep things a bit simpler > +} > + > static long mdp4_round_pixclk(struct msm_kms *kms, unsigned long rate, > struct drm_encoder *encoder) > { > @@ -197,6 +205,7 @@ static const struct mdp_kms_funcs kms_funcs = { > .complete_commit = mdp4_complete_commit, > .get_format = mdp_get_format, > .round_pixclk = mdp4_round_pixclk, > + .set_mode_config = mdp4_set_mode_config, > .preclose = mdp4_preclose, > .destroy = mdp4_destroy, > }, > diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c > index cde2500..2c2d6a5 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c > +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c > @@ -33,6 +33,21 @@ struct mdp4_plane { > }; > #define to_mdp4_plane(x) container_of(x, struct mdp4_plane, base) > > +/* MDP format helper functions */ > +static inline > +enum mdp4_frame_format mdp4_get_frame_format(struct drm_framebuffer *fb) > +{ > + bool is_tile = false; > + > + if (fb->modifier[1] == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE) > + is_tile = true; > + > + if (fb->pixel_format == DRM_FORMAT_NV12 && is_tile) > + return FRAME_TILE_YCBCR_420; > + > + return FRAME_LINEAR; > +} > + > static void mdp4_plane_set_scanout(struct drm_plane *plane, > struct drm_framebuffer *fb); > static int mdp4_plane_mode_set(struct drm_plane *plane, > @@ -203,6 +218,7 @@ static int mdp4_plane_mode_set(struct drm_plane *plane, > uint32_t op_mode = 0; > uint32_t phasex_step = MDP4_VG_PHASE_STEP_DEFAULT; > uint32_t phasey_step = MDP4_VG_PHASE_STEP_DEFAULT; > + enum mdp4_frame_format frame_type = mdp4_get_frame_format(fb); > > if (!(crtc && fb)) { > DBG("%s: disabled!", mdp4_plane->name); > @@ -302,6 +318,7 @@ static int mdp4_plane_mode_set(struct drm_plane *plane, > MDP4_PIPE_SRC_FORMAT_UNPACK_COUNT(format->unpack_count - 1) | > MDP4_PIPE_SRC_FORMAT_FETCH_PLANES(format->fetch_type) | > MDP4_PIPE_SRC_FORMAT_CHROMA_SAMP(format->chroma_sample) | > + MDP4_PIPE_SRC_FORMAT_FRAME_FORMAT(frame_type) | > COND(format->unpack_tight, MDP4_PIPE_SRC_FORMAT_UNPACK_TIGHT)); > > mdp4_write(mdp4_kms, REG_MDP4_PIPE_SRC_UNPACK(pipe), > @@ -322,6 +339,11 @@ static int mdp4_plane_mode_set(struct drm_plane *plane, > mdp4_write(mdp4_kms, REG_MDP4_PIPE_PHASEX_STEP(pipe), phasex_step); > mdp4_write(mdp4_kms, REG_MDP4_PIPE_PHASEY_STEP(pipe), phasey_step); > > + if (frame_type != FRAME_LINEAR) > + mdp4_write(mdp4_kms, REG_MDP4_PIPE_SSTILE_FRAME_SIZE(pipe), > + MDP4_PIPE_SSTILE_FRAME_SIZE_WIDTH(src_w) | > + MDP4_PIPE_SSTILE_FRAME_SIZE_HEIGHT(src_h)); > + > return 0; > } > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c > index 92b61db..9cb5a5d 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c > @@ -80,6 +80,10 @@ static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s > mdp5_disable(mdp5_kms); > } > > +static void mdp5_set_mode_config(struct msm_kms *kms) > +{ > +} > + > static long mdp5_round_pixclk(struct msm_kms *kms, unsigned long rate, > struct drm_encoder *encoder) > { > @@ -131,6 +135,7 @@ static const struct mdp_kms_funcs kms_funcs = { > .complete_commit = mdp5_complete_commit, > .get_format = mdp_get_format, > .round_pixclk = mdp5_round_pixclk, > + .set_mode_config = mdp5_set_mode_config, > .preclose = mdp5_preclose, > .destroy = mdp5_destroy, > }, > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index dfd583f..a72ed0a 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -281,6 +281,8 @@ static int msm_load(struct drm_device *dev, unsigned long flags) > dev_err(dev->dev, "kms hw init failed: %d\n", ret); > goto fail; > } > + > + kms->funcs->set_mode_config(kms); > } > > dev->mode_config.min_width = 0; > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h > index 3a78cb4..d6e3bb4 100644 > --- a/drivers/gpu/drm/msm/msm_kms.h > +++ b/drivers/gpu/drm/msm/msm_kms.h > @@ -47,6 +47,7 @@ struct msm_kms_funcs { > const struct msm_format *(*get_format)(struct msm_kms *kms, uint32_t format); > long (*round_pixclk)(struct msm_kms *kms, unsigned long rate, > struct drm_encoder *encoder); > + void (*set_mode_config)(struct msm_kms *kms); > /* cleanup: */ > void (*preclose)(struct msm_kms *kms, struct drm_file *file); > void (*destroy)(struct msm_kms *kms); > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index 188e61f..1546302 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -161,4 +161,18 @@ > * authoritative source for all of these. > */ > > +/* Samsung framebuffer modifiers */ > + > +/* > + * NV12 64x32 Tiled > + * > + * 2 planes Y and CbCr, grouped into 64x32 macro tiles, > + * with a non-standard order in memory (Z-shape). > + * > + * Pixel layout identical to DRM_FORMAT_NV21 format: > + * index 0 = Y plane, [7:0] Y > + * index 1 = Cb:Cr plane, [15:0] Cb:Cr little endian > + */ > +#define DRM_FORMAT_MOD_SAMSUNG_64_32_TILE fourcc_mod_code(SAMSUNG, 1) > + so, the one thing I couldn't quite figure out... I'm assuming the untiling is before un-sub-sampling the UV, meaning that the pattern in the UV plane is 32x16 pairs of pixels (so 64x16). I found some gstreamer code that untile a buffer in this layout, but there were too many levels of indirection ;-) I'm not sure if we should then define separate modifiers for Y and UV. Or maybe it is simpler the define that the layout depends on the plane? > #endif /* DRM_FOURCC_H */ > -- > 1.8.2.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/