* [PATCH v2] drm: mediatek: change the variable type of rdma threshold
@ 2017-05-19 9:57 Bibby Hsieh
2017-05-22 5:46 ` CK Hu
2017-06-21 21:14 ` [v2] " Guenter Roeck
0 siblings, 2 replies; 5+ messages in thread
From: Bibby Hsieh @ 2017-05-19 9:57 UTC (permalink / raw)
To: linux-arm-kernel
For some greater resolution, the rdma threshold
variable will overflow.
Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
---
drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
index 0df05f9..9afdcd7 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
@@ -37,7 +37,7 @@
#define DISP_REG_RDMA_FIFO_CON 0x0040
#define RDMA_FIFO_UNDERFLOW_EN BIT(31)
#define RDMA_FIFO_PSEUDO_SIZE(bytes) (((bytes) / 16) << 16)
-#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) ((bytes) / 16)
+#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) (((bytes) / 16) & 0x3ff)
/**
* struct mtk_disp_rdma - DISP_RDMA driver structure
@@ -109,7 +109,7 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
unsigned int height, unsigned int vrefresh,
unsigned int bpc)
{
- unsigned int threshold;
+ unsigned long long threshold;
unsigned int reg;
rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_0, 0xfff, width);
@@ -121,7 +121,8 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
* output threshold to 6 microseconds with 7/6 overhead to
* account for blanking, and with a pixel depth of 4 bytes:
*/
- threshold = width * height * vrefresh * 4 * 7 / 1000000;
+ threshold = (unsigned long long)width * height * vrefresh *
+ 4 * 7 / 1000000;
reg = RDMA_FIFO_UNDERFLOW_EN |
RDMA_FIFO_PSEUDO_SIZE(SZ_8K) |
RDMA_OUTPUT_VALID_FIFO_THRESHOLD(threshold);
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] drm: mediatek: change the variable type of rdma threshold
2017-05-19 9:57 [PATCH v2] drm: mediatek: change the variable type of rdma threshold Bibby Hsieh
@ 2017-05-22 5:46 ` CK Hu
2017-06-22 2:21 ` Bibby Hsieh
2017-06-21 21:14 ` [v2] " Guenter Roeck
1 sibling, 1 reply; 5+ messages in thread
From: CK Hu @ 2017-05-22 5:46 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Bibby:
One comment inline.
On Fri, 2017-05-19 at 17:57 +0800, Bibby Hsieh wrote:
> For some greater resolution, the rdma threshold
> variable will overflow.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> index 0df05f9..9afdcd7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> @@ -37,7 +37,7 @@
> #define DISP_REG_RDMA_FIFO_CON 0x0040
> #define RDMA_FIFO_UNDERFLOW_EN BIT(31)
> #define RDMA_FIFO_PSEUDO_SIZE(bytes) (((bytes) / 16) << 16)
> -#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) ((bytes) / 16)
> +#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) (((bytes) / 16) & 0x3ff)
I think it's not necessary to do this mask operation. Before calling
RDMA_OUTPUT_VALID_FIFO_THRESHOLD(), you should make sure that width,
height, and vrefresh matches the HW spec, so the result of threshold
likely does not exceed 0x3ff. If width, height, and vrefresh matches the
HW spec but threshold exceed 0x3ff, maybe you should limited it to 0x3ff
rather than truncating it.
Regards,
CK
>
> /**
> * struct mtk_disp_rdma - DISP_RDMA driver structure
> @@ -109,7 +109,7 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> unsigned int height, unsigned int vrefresh,
> unsigned int bpc)
> {
> - unsigned int threshold;
> + unsigned long long threshold;
> unsigned int reg;
>
> rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_0, 0xfff, width);
> @@ -121,7 +121,8 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> * output threshold to 6 microseconds with 7/6 overhead to
> * account for blanking, and with a pixel depth of 4 bytes:
> */
> - threshold = width * height * vrefresh * 4 * 7 / 1000000;
> + threshold = (unsigned long long)width * height * vrefresh *
> + 4 * 7 / 1000000;
> reg = RDMA_FIFO_UNDERFLOW_EN |
> RDMA_FIFO_PSEUDO_SIZE(SZ_8K) |
> RDMA_OUTPUT_VALID_FIFO_THRESHOLD(threshold);
^ permalink raw reply [flat|nested] 5+ messages in thread
* [v2] drm: mediatek: change the variable type of rdma threshold
2017-05-19 9:57 [PATCH v2] drm: mediatek: change the variable type of rdma threshold Bibby Hsieh
2017-05-22 5:46 ` CK Hu
@ 2017-06-21 21:14 ` Guenter Roeck
2017-06-22 2:25 ` Bibby Hsieh
1 sibling, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2017-06-21 21:14 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, May 19, 2017 at 05:57:23PM +0800, Bibby Hsieh wrote:
> For some greater resolution, the rdma threshold
> variable will overflow.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> index 0df05f9..9afdcd7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> @@ -37,7 +37,7 @@
> #define DISP_REG_RDMA_FIFO_CON 0x0040
> #define RDMA_FIFO_UNDERFLOW_EN BIT(31)
> #define RDMA_FIFO_PSEUDO_SIZE(bytes) (((bytes) / 16) << 16)
> -#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) ((bytes) / 16)
> +#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) (((bytes) / 16) & 0x3ff)
I agree with the earlier comment; clamp_val() might be more appropriate here
and would avoid unexpected results.
>
> /**
> * struct mtk_disp_rdma - DISP_RDMA driver structure
> @@ -109,7 +109,7 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> unsigned int height, unsigned int vrefresh,
> unsigned int bpc)
> {
> - unsigned int threshold;
> + unsigned long long threshold;
> unsigned int reg;
>
> rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_0, 0xfff, width);
> @@ -121,7 +121,8 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> * output threshold to 6 microseconds with 7/6 overhead to
> * account for blanking, and with a pixel depth of 4 bytes:
> */
> - threshold = width * height * vrefresh * 4 * 7 / 1000000;
> + threshold = (unsigned long long)width * height * vrefresh *
> + 4 * 7 / 1000000;
This is a 64 bit divide operation. It will result in a build failure
if compiled for a 32 bit kernel (eg arm:allmodconfig).
ERROR: "__aeabi_uldivmod" [drivers/gpu/drm/mediatek/mediatek-drm.ko] undefined!
Guenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] drm: mediatek: change the variable type of rdma threshold
2017-05-22 5:46 ` CK Hu
@ 2017-06-22 2:21 ` Bibby Hsieh
0 siblings, 0 replies; 5+ messages in thread
From: Bibby Hsieh @ 2017-06-22 2:21 UTC (permalink / raw)
To: linux-arm-kernel
Hi, CK,
Thanks for your review and comment.
On Mon, 2017-05-22 at 13:46 +0800, CK Hu wrote:
> Hi, Bibby:
>
> One comment inline.
>
> On Fri, 2017-05-19 at 17:57 +0800, Bibby Hsieh wrote:
> > For some greater resolution, the rdma threshold
> > variable will overflow.
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > index 0df05f9..9afdcd7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > @@ -37,7 +37,7 @@
> > #define DISP_REG_RDMA_FIFO_CON 0x0040
> > #define RDMA_FIFO_UNDERFLOW_EN BIT(31)
> > #define RDMA_FIFO_PSEUDO_SIZE(bytes) (((bytes) / 16) << 16)
> > -#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) ((bytes) / 16)
> > +#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) (((bytes) / 16) & 0x3ff)
>
> I think it's not necessary to do this mask operation. Before calling
> RDMA_OUTPUT_VALID_FIFO_THRESHOLD(), you should make sure that width,
> height, and vrefresh matches the HW spec, so the result of threshold
> likely does not exceed 0x3ff. If width, height, and vrefresh matches the
> HW spec but threshold exceed 0x3ff, maybe you should limited it to 0x3ff
> rather than truncating it.
>
Ok, It seems to me that I will refer Guenter's comment, add clamp_val()
to avoid unexpected value. Thanks.
Bibby
> Regards,
> CK
>
> >
> > /**
> > * struct mtk_disp_rdma - DISP_RDMA driver structure
> > @@ -109,7 +109,7 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> > unsigned int height, unsigned int vrefresh,
> > unsigned int bpc)
> > {
> > - unsigned int threshold;
> > + unsigned long long threshold;
> > unsigned int reg;
> >
> > rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_0, 0xfff, width);
> > @@ -121,7 +121,8 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> > * output threshold to 6 microseconds with 7/6 overhead to
> > * account for blanking, and with a pixel depth of 4 bytes:
> > */
> > - threshold = width * height * vrefresh * 4 * 7 / 1000000;
> > + threshold = (unsigned long long)width * height * vrefresh *
> > + 4 * 7 / 1000000;
> > reg = RDMA_FIFO_UNDERFLOW_EN |
> > RDMA_FIFO_PSEUDO_SIZE(SZ_8K) |
> > RDMA_OUTPUT_VALID_FIFO_THRESHOLD(threshold);
>
>
--
Bibby
^ permalink raw reply [flat|nested] 5+ messages in thread
* [v2] drm: mediatek: change the variable type of rdma threshold
2017-06-21 21:14 ` [v2] " Guenter Roeck
@ 2017-06-22 2:25 ` Bibby Hsieh
0 siblings, 0 replies; 5+ messages in thread
From: Bibby Hsieh @ 2017-06-22 2:25 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Guenter,
Thanks for your test and comment.
On Wed, 2017-06-21 at 14:14 -0700, Guenter Roeck wrote:
> On Fri, May 19, 2017 at 05:57:23PM +0800, Bibby Hsieh wrote:
> > For some greater resolution, the rdma threshold
> > variable will overflow.
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > index 0df05f9..9afdcd7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> > @@ -37,7 +37,7 @@
> > #define DISP_REG_RDMA_FIFO_CON 0x0040
> > #define RDMA_FIFO_UNDERFLOW_EN BIT(31)
> > #define RDMA_FIFO_PSEUDO_SIZE(bytes) (((bytes) / 16) << 16)
> > -#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) ((bytes) / 16)
> > +#define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes) (((bytes) / 16) & 0x3ff)
>
> I agree with the earlier comment; clamp_val() might be more appropriate here
> and would avoid unexpected results.
>
Yep, It's a good suggestion to me, I will add clamp_val() to avoid
unexpected value, thanks.
> >
> > /**
> > * struct mtk_disp_rdma - DISP_RDMA driver structure
> > @@ -109,7 +109,7 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> > unsigned int height, unsigned int vrefresh,
> > unsigned int bpc)
> > {
> > - unsigned int threshold;
> > + unsigned long long threshold;
> > unsigned int reg;
> >
> > rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_0, 0xfff, width);
> > @@ -121,7 +121,8 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
> > * output threshold to 6 microseconds with 7/6 overhead to
> > * account for blanking, and with a pixel depth of 4 bytes:
> > */
> > - threshold = width * height * vrefresh * 4 * 7 / 1000000;
> > + threshold = (unsigned long long)width * height * vrefresh *
> > + 4 * 7 / 1000000;
>
> This is a 64 bit divide operation. It will result in a build failure
> if compiled for a 32 bit kernel (eg arm:allmodconfig).
>
> ERROR: "__aeabi_uldivmod" [drivers/gpu/drm/mediatek/mediatek-drm.ko] undefined!
>
Sorry about the lack of tests, I will change the divide operation to
div_u64 function, thanks for your suggestions.
> Guenter
--
Bibby
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-22 2:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-19 9:57 [PATCH v2] drm: mediatek: change the variable type of rdma threshold Bibby Hsieh
2017-05-22 5:46 ` CK Hu
2017-06-22 2:21 ` Bibby Hsieh
2017-06-21 21:14 ` [v2] " Guenter Roeck
2017-06-22 2:25 ` Bibby Hsieh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).