From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 4/8] drm/i915/hdmi: Change the write_infoframe vfunc to take a buffer and a type Date: Mon, 5 Aug 2013 20:40:42 +0300 Message-ID: <20130805174042.GY5004@intel.com> References: <1375464180-7259-1-git-send-email-damien.lespiau@intel.com> <1375464180-7259-5-git-send-email-damien.lespiau@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 482E5E740C for ; Mon, 5 Aug 2013 10:40:46 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1375464180-7259-5-git-send-email-damien.lespiau@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Aug 02, 2013 at 06:22:56PM +0100, Damien Lespiau wrote: > First step in the move to the shared infoframe infrastructure, let's > move the different infoframe helpers and the write_infoframe() vfunc to > a type (enum hdmi_infoframe_type) and a buffer + len instead of using > our struct dip_infoframe. > = > Signed-off-by: Damien Lespiau > Signed-off-by: Paulo Zanoni > Signed-off-by: Thierry Reding > --- > drivers/gpu/drm/i915/intel_drv.h | 4 +- > drivers/gpu/drm/i915/intel_hdmi.c | 103 +++++++++++++++++++++-----------= ------ > 2 files changed, 59 insertions(+), 48 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_drv.h > index 474797b..f8c21ac 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -26,6 +26,7 @@ > #define __INTEL_DRV_H__ > = > #include > +#include > #include > #include "i915_drv.h" > #include > @@ -463,7 +464,8 @@ struct intel_hdmi { > enum hdmi_force_audio force_audio; > bool rgb_quant_range_selectable; > void (*write_infoframe)(struct drm_encoder *encoder, > - struct dip_infoframe *frame); > + enum hdmi_infoframe_type type, > + uint8_t *frame, ssize_t len); While at it, could we sprinkle a bit of constness around the infoframe code and make this 'const uint8_t *frame'? > void (*set_infoframes)(struct drm_encoder *encoder, > struct drm_display_mode *adjusted_mode); > }; > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/int= el_hdmi.c > index e82cd81..d3225df 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -81,74 +82,75 @@ void intel_dip_infoframe_csum(struct dip_infoframe *f= rame) > frame->checksum =3D 0x100 - sum; > } > = > -static u32 g4x_infoframe_index(struct dip_infoframe *frame) > +static u32 g4x_infoframe_index(enum hdmi_infoframe_type type) > { > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return VIDEO_DIP_SELECT_AVI; > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return VIDEO_DIP_SELECT_SPD; > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > = > -static u32 g4x_infoframe_enable(struct dip_infoframe *frame) > +static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type) > { > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return VIDEO_DIP_ENABLE_AVI; > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return VIDEO_DIP_ENABLE_SPD; > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > = > -static u32 hsw_infoframe_enable(struct dip_infoframe *frame) > +static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type) > { > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return VIDEO_DIP_ENABLE_AVI_HSW; > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return VIDEO_DIP_ENABLE_SPD_HSW; > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > = > -static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, > +static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, > enum transcoder cpu_transcoder) > { > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder); > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return HSW_TVIDEO_DIP_SPD_DATA(cpu_transcoder); > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > = > static void g4x_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > + enum hdmi_infoframe_type type, > + uint8_t *frame, ssize_t len) > { > uint32_t *data =3D (uint32_t *)frame; > struct drm_device *dev =3D encoder->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > u32 val =3D I915_READ(VIDEO_DIP_CTL); > - unsigned i, len =3D DIP_HEADER_SIZE + frame->len; > + unsigned i; 'len' is now signed, but before it was unsigned, and 'i' is still unsigned. A bit of consistency with the types might be nice. > = > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > = > val &=3D ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |=3D g4x_infoframe_index(frame); > + val |=3D g4x_infoframe_index(type); > = > - val &=3D ~g4x_infoframe_enable(frame); > + val &=3D ~g4x_infoframe_enable(type); > = > I915_WRITE(VIDEO_DIP_CTL, val); > = > @@ -162,7 +164,7 @@ static void g4x_write_infoframe(struct drm_encoder *e= ncoder, > I915_WRITE(VIDEO_DIP_DATA, 0); > mmiowb(); > = > - val |=3D g4x_infoframe_enable(frame); > + val |=3D g4x_infoframe_enable(type); > val &=3D ~VIDEO_DIP_FREQ_MASK; > val |=3D VIDEO_DIP_FREQ_VSYNC; > = > @@ -171,22 +173,23 @@ static void g4x_write_infoframe(struct drm_encoder = *encoder, > } > = > static void ibx_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > + enum hdmi_infoframe_type type, > + uint8_t *frame, ssize_t len) > { > uint32_t *data =3D (uint32_t *)frame; > struct drm_device *dev =3D encoder->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(encoder->crtc); > int reg =3D TVIDEO_DIP_CTL(intel_crtc->pipe); > - unsigned i, len =3D DIP_HEADER_SIZE + frame->len; > + unsigned i; > u32 val =3D I915_READ(reg); > = > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > = > val &=3D ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |=3D g4x_infoframe_index(frame); > + val |=3D g4x_infoframe_index(type); > = > - val &=3D ~g4x_infoframe_enable(frame); > + val &=3D ~g4x_infoframe_enable(type); > = > I915_WRITE(reg, val); > = > @@ -200,7 +203,7 @@ static void ibx_write_infoframe(struct drm_encoder *e= ncoder, > I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); > mmiowb(); > = > - val |=3D g4x_infoframe_enable(frame); > + val |=3D g4x_infoframe_enable(type); > val &=3D ~VIDEO_DIP_FREQ_MASK; > val |=3D VIDEO_DIP_FREQ_VSYNC; > = > @@ -209,25 +212,26 @@ static void ibx_write_infoframe(struct drm_encoder = *encoder, > } > = > static void cpt_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > + enum hdmi_infoframe_type type, > + uint8_t *frame, ssize_t len) > { > uint32_t *data =3D (uint32_t *)frame; > struct drm_device *dev =3D encoder->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(encoder->crtc); > int reg =3D TVIDEO_DIP_CTL(intel_crtc->pipe); > - unsigned i, len =3D DIP_HEADER_SIZE + frame->len; > + unsigned i; > u32 val =3D I915_READ(reg); > = > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > = > val &=3D ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |=3D g4x_infoframe_index(frame); > + val |=3D g4x_infoframe_index(type); > = > /* The DIP control register spec says that we need to update the AVI > * infoframe without clearing its enable bit */ > - if (frame->type !=3D DIP_TYPE_AVI) > - val &=3D ~g4x_infoframe_enable(frame); > + if (type !=3D HDMI_INFOFRAME_TYPE_AVI) > + val &=3D ~g4x_infoframe_enable(type); > = > I915_WRITE(reg, val); > = > @@ -241,7 +245,7 @@ static void cpt_write_infoframe(struct drm_encoder *e= ncoder, > I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); > mmiowb(); > = > - val |=3D g4x_infoframe_enable(frame); > + val |=3D g4x_infoframe_enable(type); > val &=3D ~VIDEO_DIP_FREQ_MASK; > val |=3D VIDEO_DIP_FREQ_VSYNC; > = > @@ -250,22 +254,23 @@ static void cpt_write_infoframe(struct drm_encoder = *encoder, > } > = > static void vlv_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > + enum hdmi_infoframe_type type, > + uint8_t *frame, ssize_t len) > { > uint32_t *data =3D (uint32_t *)frame; > struct drm_device *dev =3D encoder->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(encoder->crtc); > int reg =3D VLV_TVIDEO_DIP_CTL(intel_crtc->pipe); > - unsigned i, len =3D DIP_HEADER_SIZE + frame->len; > + unsigned i; > u32 val =3D I915_READ(reg); > = > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > = > val &=3D ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |=3D g4x_infoframe_index(frame); > + val |=3D g4x_infoframe_index(type); > = > - val &=3D ~g4x_infoframe_enable(frame); > + val &=3D ~g4x_infoframe_enable(type); > = > I915_WRITE(reg, val); > = > @@ -279,7 +284,7 @@ static void vlv_write_infoframe(struct drm_encoder *e= ncoder, > I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0); > mmiowb(); > = > - val |=3D g4x_infoframe_enable(frame); > + val |=3D g4x_infoframe_enable(type); > val &=3D ~VIDEO_DIP_FREQ_MASK; > val |=3D VIDEO_DIP_FREQ_VSYNC; > = > @@ -288,21 +293,24 @@ static void vlv_write_infoframe(struct drm_encoder = *encoder, > } > = > static void hsw_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > + enum hdmi_infoframe_type type, > + uint8_t *frame, ssize_t len) > { > uint32_t *data =3D (uint32_t *)frame; > struct drm_device *dev =3D encoder->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(encoder->crtc); > u32 ctl_reg =3D HSW_TVIDEO_DIP_CTL(intel_crtc->config.cpu_transcoder); > - u32 data_reg =3D hsw_infoframe_data_reg(frame, intel_crtc->config.cpu_t= ranscoder); > - unsigned int i, len =3D DIP_HEADER_SIZE + frame->len; > + u32 data_reg; > + unsigned int i; > u32 val =3D I915_READ(ctl_reg); > = > + data_reg =3D hsw_infoframe_data_reg(type, > + intel_crtc->config.cpu_transcoder); > if (data_reg =3D=3D 0) > return; > = > - val &=3D ~hsw_infoframe_enable(frame); > + val &=3D ~hsw_infoframe_enable(type); > I915_WRITE(ctl_reg, val); > = > mmiowb(); > @@ -315,7 +323,7 @@ static void hsw_write_infoframe(struct drm_encoder *e= ncoder, > I915_WRITE(data_reg + i, 0); > mmiowb(); > = > - val |=3D hsw_infoframe_enable(frame); > + val |=3D hsw_infoframe_enable(type); > I915_WRITE(ctl_reg, val); > POSTING_READ(ctl_reg); > } > @@ -326,7 +334,8 @@ static void intel_set_infoframe(struct drm_encoder *e= ncoder, > struct intel_hdmi *intel_hdmi =3D enc_to_intel_hdmi(encoder); > = > intel_dip_infoframe_csum(frame); > - intel_hdmi->write_infoframe(encoder, frame); > + intel_hdmi->write_infoframe(encoder, frame->type, (uint8_t *)frame, > + DIP_HEADER_SIZE + frame->len); > } > = > static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, > -- = > 1.8.3.1 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC