On Wed, 12 Oct 2022, Ville Syrjälä <[email protected]> wrote: > On Wed, Oct 12, 2022 at 05:28:27PM +0300, Jani Nikula wrote: >> On Tue, 11 Oct 2022, Ville Syrjala <[email protected]> wrote: >> > From: Ville Syrjälä <[email protected]> >> > >> > Currently we only write as many dwords into the hardware >> > ELD buffers as drm_eld_size() tells us. That could mean the >> > remainder of the hardware buffer is left with whatever >> > stale garbage it had before, which doesn't seem entirely >> > great. Let's zero out the remainder of the buffer in case >> > the provided ELD doesn't fill it fully. >> > >> > We can also sanity check out idea of the hardware ELD buffer's >> > size by making sure the address wrapped back to zero once >> > we wrote the entire buffer. >> > >> > Cc: Chaitanya Kumar Borah <[email protected]> >> > Cc: Kai Vehmanen <[email protected]> >> > Cc: Takashi Iwai <[email protected]> >> > Signed-off-by: Ville Syrjälä <[email protected]> >> > --- >> > drivers/gpu/drm/i915/display/intel_audio.c | 34 ++++++++++++++++------ >> > 1 file changed, 25 insertions(+), 9 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c >> > b/drivers/gpu/drm/i915/display/intel_audio.c >> > index abca5f23673a..d2f9c4c29061 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_audio.c >> > +++ b/drivers/gpu/drm/i915/display/intel_audio.c >> > @@ -333,19 +333,24 @@ static void g4x_audio_codec_enable(struct >> > intel_encoder *encoder, >> > struct drm_i915_private *i915 = to_i915(encoder->base.dev); >> > struct drm_connector *connector = conn_state->connector; >> > const u8 *eld = connector->eld; >> > + int eld_buffer_size, len, i; >> > u32 tmp; >> > - int len, i; >> > >> > tmp = intel_de_read(i915, G4X_AUD_CNTL_ST); >> > tmp &= ~(G4X_ELD_VALID | G4X_ELD_ADDRESS_MASK); >> > intel_de_write(i915, G4X_AUD_CNTL_ST, tmp); >> > >> > - len = g4x_eld_buffer_size(i915); >> > - len = min(drm_eld_size(eld) / 4, len); >> > + eld_buffer_size = g4x_eld_buffer_size(i915); >> > + len = min(drm_eld_size(eld) / 4, eld_buffer_size); >> > >> > for (i = 0; i < len; i++) >> > intel_de_write(i915, G4X_HDMIW_HDMIEDID, >> > *((const u32 *)eld + i)); >> > + for (; i < eld_buffer_size; i++) >> > + intel_de_write(i915, G4X_HDMIW_HDMIEDID, 0); >> >> I think I'd personally write this along the lines of: >> >> eld_buffer_size = g4x_eld_buffer_size(i915); >> len = drm_eld_size(eld) / 4; >> >> for (i = 0; i < eld_buffer_size; i++) { >> u32 val = i < len ? *((const u32 *)eld + i)) : 0; >> intel_de_write(i915, G4X_HDMIW_HDMIEDID, val); >> } >> >> Get rid of two loops, the loop variable "leaking" from one to the next, >> the min() around len calculation, and multiple reg writes. Seems cleaner >> to me. > > I suppose. Though the double loop is what we already use > in the infoframe code. So should probably change all of it > at once if we decide that a single loop is the way to go.
IMO it would be nicer, but it's not a super important thing. Can go with this now, and decide later. As said, R-b on this as-is. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
