From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] i915: enable AVI infoframe for intel_hdmi.c [v2] Date: Thu, 09 Sep 2010 22:58:00 +0100 Message-ID: <8u3s8d$jhdi8u@orsmga001.jf.intel.com> References: <20100909210001.11985.1244.stgit@localhost.localdomain> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0513885244==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 7BB639EF7D for ; Thu, 9 Sep 2010 14:58:04 -0700 (PDT) In-Reply-To: <20100909210001.11985.1244.stgit@localhost.localdomain> 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: intel-gfx@lists.freedesktop.org Cc: jesse.barnes@intel.com List-Id: intel-gfx@lists.freedesktop.org --===============0513885244== Content-Type: text/plain On Thu, 09 Sep 2010 23:00:01 +0200, David Härdeman wrote: > This is the second version which merges the infoframe code from > intel_hdmi.c and intel_sdvo.c, I hope this is something along the lines > Chris Wilson had in mind. Note that I'm assuming that the sdvo hardware > also stores a header ECC byte in the MSB of the first dword (haven't found > any documentation for the sdvo). Pretty close. Using a callback function for writing the data is overkill, just call the common function to compute the avi csum, then pass the avi to the sdvo or hdmi handlers. It just makes following the logic that little bit easier. drm-intel-next has changed slightly since you wrote the patch, so can you respin it - just a couple of the macro names have changed to be consistent. > --- > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index ccd4c97..4fc323c 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -48,6 +48,79 @@ static struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder) > return container_of(enc_to_intel_encoder(encoder), struct intel_hdmi, base); > } > > +static void intel_hdmi_calc_infoframe_csum(struct dip_infoframe *avi_if) void intel_dip_infoframe_csum(struct dip_infoframe *avi_if) > +{ > + uint8_t *data = (uint8_t *)avi_if; > + uint8_t sum = 0; > + unsigned i; > + > + avi_if->checksum = 0; > + avi_if->ecc = 0; > + avi_if->padding = 0; > + > + for (i = 0; i < sizeof(*avi_if); i++) > + sum += data[i]; > + > + avi_if->checksum = 0x100 - sum; > +} > + > +static bool intel_hdmi_write_avi_infoframe(struct drm_encoder *encoder, > + uint8_t *data) > +{ > + struct drm_device *dev = encoder->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + I915_WRITE(VIDEO_DIP_DATA, *((u32 *)data)); > + return true; > +} > + > +bool intel_hdmi_proc_infoframe(struct drm_encoder *encoder, > + struct dip_infoframe *avi_if, > + bool (*write_func)(struct drm_encoder *, uint8_t *)) > +{ > + unsigned size; > + uint8_t *data = (uint8_t *)avi_if; > + > + intel_hdmi_calc_infoframe_csum(avi_if); > + for (size = sizeof(*avi_if); size > 0; size -= 4) { > + if (!write_func(encoder, data)) > + return false; > + data += 4; > + } > + return true; > +} This I think is unnecessary, especially as the current SDVO code handles it in 8 byte chunks. > + > +static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder) > +{ > + struct dip_infoframe avi_if = { > + .type = DIP_TYPE_AVI, > + .ver = DIP_VERSION_AVI, > + .len = DIP_LEN_AVI, > + .Y_A_B_S = 0x02, > + .C_M_R = 0x28, > + }; > + struct drm_device *dev = encoder->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); > + u32 port; > + > + if (!intel_hdmi->has_hdmi_sink) > + return; > + > + if (intel_hdmi->sdvox_reg == SDVOB) > + port = VIDEO_DIP_PORT_B; > + else if (intel_hdmi->sdvox_reg == SDVOC) > + port = VIDEO_DIP_PORT_C; > + else > + return; > + > + I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | port | VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC); Split this up over multiple lines to fit within 80-columns. > + POSTING_READ(VIDEO_DIP_ENABLE); Shouldn't need to flush the write here. > + intel_hdmi_proc_infoframe(encoder, &avi_if, intel_hdmi_write_avi_infoframe); I think this works better as: intel_dip_infofame_csum(&avi_if); intel_hdmi_write_dip_infoframe(encoder, &avi_if); > + I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | port | VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC); > + POSTING_READ(VIDEO_DIP_ENABLE); Same as above. Thanks, adding a second copy of that data structure would have been too ugly. -Chris -- Chris Wilson, Intel Open Source Technology Centre --===============0513885244== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0513885244==--