Re: [Intel-gfx] [PATCH 10/22] drm/i915/audio: Make sure we write the whole ELD buffer

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 12, 2022 at 05:28:27PM +0300, Jani Nikula wrote:
> On Tue, 11 Oct 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 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 <chaitanya.kumar.borah@intel.com>
> > Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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.

-- 
Ville Syrjälä
Intel