| Message ID | 20240926074755.22176-3-Hermes.Wu@ite.com.tw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix HDCP CTS fail items and add MCCS support | expand |
On Thu, Sep 26, 2024 at 03:47:52PM GMT, Hermes Wu wrote: > From: Hermes Wu <Hermes.wu@ite.com.tw> > > The EDID read operation can reach the maximum size of the AUX FIFO buffer. And? Commit message should describe why the change is necessary and what is being done. Just providing a statement is not enough. > > Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") > Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw> > --- > drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c > index 28a8043229d3..b451d3c2ac1d 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -1078,8 +1078,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, > int i, ret_size, ret = 0, request_size; > > mutex_lock(&it6505->aux_lock); > - for (i = 0; i < size; i += 4) { > - request_size = min((int)size - i, 4); > + for (i = 0; i < size; ) { > + if (cmd == CMD_AUX_I2C_EDID_READ) > + request_size = min_t(int, (int)size - i, AUX_FIFO_MAX_SIZE); > + else > + request_size = min_t(int, (int)size - i, 4); > ret_size = it6505_aux_operation(it6505, cmd, address + i, > buffer + i, request_size, > reply); > @@ -1088,6 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, > goto aux_op_err; > } > > + i += request_size; > ret += ret_size; > } > > -- > 2.34.1 >
>On Thu, Sep 26, 2024 at 03:47:52PM GMT, Hermes Wu wrote: >> From: Hermes Wu <Hermes.wu@ite.com.tw> >> >> The EDID read operation can reach the maximum size of the AUX FIFO buffer. > >And? Commit message should describe why the change is necessary and what is being done. Just providing a statement is not enough. > The original AUX operation treat all reads by using data registers will be limited at 4 bytes. AUX operation command CMD_AUX_I2C_EDID_READ using AUX FIFO is capable of reads data by 16 bytes each time. It can improve speed of read EDID. >> >> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw> >> --- >> drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c >> b/drivers/gpu/drm/bridge/ite-it6505.c >> index 28a8043229d3..b451d3c2ac1d 100644 >> --- a/drivers/gpu/drm/bridge/ite-it6505.c >> +++ b/drivers/gpu/drm/bridge/ite-it6505.c >> @@ -1078,8 +1078,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, >> int i, ret_size, ret = 0, request_size; >> >> mutex_lock(&it6505->aux_lock); >> - for (i = 0; i < size; i += 4) { >> - request_size = min((int)size - i, 4); >> + for (i = 0; i < size; ) { >> + if (cmd == CMD_AUX_I2C_EDID_READ) >> + request_size = min_t(int, (int)size - i, AUX_FIFO_MAX_SIZE); >> + else >> + request_size = min_t(int, (int)size - i, 4); >> ret_size = it6505_aux_operation(it6505, cmd, address + i, >> buffer + i, request_size, >> reply); >> @@ -1088,6 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, >> goto aux_op_err; >> } >> >> + i += request_size; >> ret += ret_size; >> } >> >> -- >> 2.34.1 >> > >-- >With best wishes >Dmitry > BR, Hermes
On Thu, 26 Sept 2024 at 10:04, <Hermes.Wu@ite.com.tw> wrote: > > >On Thu, Sep 26, 2024 at 03:47:52PM GMT, Hermes Wu wrote: > >> From: Hermes Wu <Hermes.wu@ite.com.tw> > >> > >> The EDID read operation can reach the maximum size of the AUX FIFO buffer. > > > >And? Commit message should describe why the change is necessary and what is being done. Just providing a statement is not enough. > > > > The original AUX operation treat all reads by using data registers will be limited at 4 bytes. > AUX operation command CMD_AUX_I2C_EDID_READ using AUX FIFO is capable of reads data by 16 bytes each time. > It can improve speed of read EDID. improves, rather than "can improve". Please add this to the commit message. > > > >> > >> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") > >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw> > >> --- > >> drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > >> b/drivers/gpu/drm/bridge/ite-it6505.c > >> index 28a8043229d3..b451d3c2ac1d 100644 > >> --- a/drivers/gpu/drm/bridge/ite-it6505.c > >> +++ b/drivers/gpu/drm/bridge/ite-it6505.c > >> @@ -1078,8 +1078,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, > >> int i, ret_size, ret = 0, request_size; > >> > >> mutex_lock(&it6505->aux_lock); > >> - for (i = 0; i < size; i += 4) { > >> - request_size = min((int)size - i, 4); > >> + for (i = 0; i < size; ) { > >> + if (cmd == CMD_AUX_I2C_EDID_READ) > >> + request_size = min_t(int, (int)size - i, AUX_FIFO_MAX_SIZE); > >> + else > >> + request_size = min_t(int, (int)size - i, 4); > >> ret_size = it6505_aux_operation(it6505, cmd, address + i, > >> buffer + i, request_size, > >> reply); > >> @@ -1088,6 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, > >> goto aux_op_err; > >> } > >> > >> + i += request_size; > >> ret += ret_size; > >> } > >> > >> -- > >> 2.34.1 > >> > > > >-- > >With best wishes > >Dmitry > > > > BR, > Hermes >
Il 26/09/24 09:47, Hermes Wu ha scritto: > From: Hermes Wu <Hermes.wu@ite.com.tw> > > The EDID read operation can reach the maximum size of the AUX FIFO buffer. > > Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") > Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw> > --- > drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c > index 28a8043229d3..b451d3c2ac1d 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -1078,8 +1078,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, > int i, ret_size, ret = 0, request_size; int fifo_max_sz = (cmd == CMD_AUX_I2C_EDID_READ) ? AUX_FIFO_MAX_SIZE : 4; which later becomes int fifo_max_sz = (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) ? AUX_FIFO_MAX_SIZE : 4; otherwise you can do it "the long way"... if (cmd == .....) fifo_max_sz = AUX_FIFO_MAX_SIZE; else fifo_max_sz = 4; The point is, cmd won't change during iterations, so it's useless to put that if branch inside of that loop below. > > mutex_lock(&it6505->aux_lock); > - for (i = 0; i < size; i += 4) { > - request_size = min((int)size - i, 4); request_size = min_t(int, (int)size - i, fifo_max_sz); P.S.: Also, I'd consider changing this to a do-while instead... Cheers, Angelo > + for (i = 0; i < size; ) { > + if (cmd == CMD_AUX_I2C_EDID_READ) > + request_size = min_t(int, (int)size - i, AUX_FIFO_MAX_SIZE); > + else > + request_size = min_t(int, (int)size - i, 4); > ret_size = it6505_aux_operation(it6505, cmd, address + i, > buffer + i, request_size, > reply); > @@ -1088,6 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, > goto aux_op_err; > } > > + i += request_size; > ret += ret_size; > } >
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 28a8043229d3..b451d3c2ac1d 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -1078,8 +1078,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, int i, ret_size, ret = 0, request_size; mutex_lock(&it6505->aux_lock); - for (i = 0; i < size; i += 4) { - request_size = min((int)size - i, 4); + for (i = 0; i < size; ) { + if (cmd == CMD_AUX_I2C_EDID_READ) + request_size = min_t(int, (int)size - i, AUX_FIFO_MAX_SIZE); + else + request_size = min_t(int, (int)size - i, 4); ret_size = it6505_aux_operation(it6505, cmd, address + i, buffer + i, request_size, reply); @@ -1088,6 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505, goto aux_op_err; } + i += request_size; ret += ret_size; }