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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 12 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

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